| Message ID | 20240829-concurrent-wb-v1-13-502b16ae2ebb@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: Add Concurrent Writeback Support for DPU 10.x+ | expand |
On Thu, Aug 29, 2024 at 01:48:34PM -0700, Jessica Zhang wrote: > If the clone mode enabled status is changing, a modeset needs to happen > so that the resources can be reassigned > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 1b0cc899e8c1..99eaaca405a4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1306,6 +1306,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > int rc = 0; > > bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state); > + bool clone_mode_requested = drm_crtc_in_clone_mode(crtc->state); > + bool clone_mode_enabled = drm_crtc_in_clone_mode(crtc_state); > > /* there might be cases where encoder needs a modeset too */ > drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) { > @@ -1313,6 +1315,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > crtc_state->mode_changed = true; > } > > + if ((clone_mode_requested && !clone_mode_enabled) || > + (!clone_mode_requested && clone_mode_enabled)) > + crtc_state->mode_changed = true; So two things, and kinda about the overall patch series: - msm is confused about crtc_state->mode_changed, e.g. it sets it if crtc_state->active_changed, which is wrong. Or msm dpu code doesn't use drm_atomic_crtc_needs_modeset() correctly. - changing crtc_state->mode_changed from your crtc or plane callbacks means you cannot use drm_atomic_helper_check directly, but need to roll your own that calls drm_atomic_helper_check_modesets again as needed. See the kerneldoc comment for drm_atomic_helper_check_modesets() - the same holds if you set mode_changed from your encoder or bridge functions, but I think this doesn't apply here for this patch. I think it'd be really good to - review existing kerneldoc and please submit patches where it's not clear or detailed enough - add checks to the atomic helper code to catch this. I think the best way would be to check if drm_atomic_crtc_needs_modeset() changes outside of areas where the helper code allows it already (essentially connector functions setting ->connectors_changed), and set a new drm_atomic_state->dirty_needs_modeset. Which drm_atomic_helper_check_modeset would clear, and which would result in a WARN_ON in drm_atomic_check if it's not yet cleared when the driver returns with success. Otherwise there's just no way I think to make sure drivers get this right. Can I please sign you up for these patches? Thanks, Sima > + > if (drm_atomic_crtc_needs_modeset(crtc_state)) { > rc = dpu_crtc_assign_resources(crtc, crtc_state); > if (rc < 0) > > -- > 2.34.1 >
On Mon, Sep 02, 2024 at 03:36:11PM GMT, Daniel Vetter wrote: > On Thu, Aug 29, 2024 at 01:48:34PM -0700, Jessica Zhang wrote: > > If the clone mode enabled status is changing, a modeset needs to happen > > so that the resources can be reassigned > > > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 1b0cc899e8c1..99eaaca405a4 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -1306,6 +1306,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > int rc = 0; > > > > bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state); > > + bool clone_mode_requested = drm_crtc_in_clone_mode(crtc->state); > > + bool clone_mode_enabled = drm_crtc_in_clone_mode(crtc_state); > > > > /* there might be cases where encoder needs a modeset too */ > > drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) { > > @@ -1313,6 +1315,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > crtc_state->mode_changed = true; > > } > > > > + if ((clone_mode_requested && !clone_mode_enabled) || > > + (!clone_mode_requested && clone_mode_enabled)) > > + crtc_state->mode_changed = true; > > So two things, and kinda about the overall patch series: > > - msm is confused about crtc_state->mode_changed, e.g. it sets it if > crtc_state->active_changed, which is wrong. Or msm dpu code doesn't use > drm_atomic_crtc_needs_modeset() correctly. This seems to be a leftover from prehistoric times and it should be removed or reworked. I'll take a look during the next development cycle. > > - changing crtc_state->mode_changed from your crtc or plane callbacks > means you cannot use drm_atomic_helper_check directly, but need to roll > your own that calls drm_atomic_helper_check_modesets again as needed. > See the kerneldoc comment for drm_atomic_helper_check_modesets() > > - the same holds if you set mode_changed from your encoder or bridge > functions, but I think this doesn't apply here for this patch. Yes. But we have it already in dpu_encoder.c. And we didn't notice that. We have the code in dpu_encoder.c which makes sure that the hardware resources get reallocated if required. As we already have code in msm_atomic.c which sets mode_changed for this purpose (before calling drm_atomic_helper_check()) it might simply make sense to add pre-check callbacks to msm/dpu encoder. I will try working on that during or after the LPC if Abhinav doesn't pick up that issue. In the end we still have debt regarding the allow_modeset in msm_atomic_check(), I remember. > > I think it'd be really good to > > - review existing kerneldoc and please submit patches where it's not clear > or detailed enough > > - add checks to the atomic helper code to catch this. I think the best way > would be to check if drm_atomic_crtc_needs_modeset() changes outside of > areas where the helper code allows it already (essentially connector > functions setting ->connectors_changed), and set a new > drm_atomic_state->dirty_needs_modeset. Which > drm_atomic_helper_check_modeset would clear, and which would result in a > WARN_ON in drm_atomic_check if it's not yet cleared when the driver > returns with success. > > Otherwise there's just no way I think to make sure drivers get this > right. > > Can I please sign you up for these patches? Ack. Thanks a lot for pointing out the issue! > > Thanks, Sima > > > + > > if (drm_atomic_crtc_needs_modeset(crtc_state)) { > > rc = dpu_crtc_assign_resources(crtc, crtc_state); > > if (rc < 0) > > > > -- > > 2.34.1 > > > > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 1b0cc899e8c1..99eaaca405a4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1306,6 +1306,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, int rc = 0; bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state); + bool clone_mode_requested = drm_crtc_in_clone_mode(crtc->state); + bool clone_mode_enabled = drm_crtc_in_clone_mode(crtc_state); /* there might be cases where encoder needs a modeset too */ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) { @@ -1313,6 +1315,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, crtc_state->mode_changed = true; } + if ((clone_mode_requested && !clone_mode_enabled) || + (!clone_mode_requested && clone_mode_enabled)) + crtc_state->mode_changed = true; + if (drm_atomic_crtc_needs_modeset(crtc_state)) { rc = dpu_crtc_assign_resources(crtc, crtc_state); if (rc < 0)
If the clone mode enabled status is changing, a modeset needs to happen so that the resources can be reassigned Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)