On Wed, Jan 08, 2025 at 06:27:13PM -0800, Abhinav Kumar wrote:
>
>
> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> > The MSM driver uses drm_atomic_helper_check() which mandates that none
> > of the atomic_check() callbacks toggles crtc_state->mode_changed.
> > Perform corresponding check before calling the drm_atomic_helper_check()
> > function.
> >
> > Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
> > Reported-by: Simona Vetter <simona.vetter@xxxxxxxx>
> > Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26 +++++++++++++++++++++++
> > drivers/gpu/drm/msm/msm_atomic.c | 13 +++++++++++-
> > drivers/gpu/drm/msm/msm_kms.h | 7 +++++++
> > 5 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -753,6 +753,34 @@ static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms,
> > cstate->num_mixers = num_lm;
> > }
> > +/**
> > + * dpu_encoder_virt_check_mode_changed: check if full modeset is required
> > + * @drm_enc: Pointer to drm encoder structure
> > + * @crtc_state: Corresponding CRTC state to be checked
> > + * @conn_state: Corresponding Connector's state to be checked
> > + *
> > + * Check if the changes in the object properties demand full mode set.
> > + */
> > +int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > + struct msm_display_topology topology;
> > +
> > + DPU_DEBUG_ENC(dpu_enc, "\n");
> > +
> > + /* Using mode instead of adjusted_mode as it wasn't computed yet */
> > + topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode, crtc_state, conn_state);
> > +
> > + if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> > + crtc_state->mode_changed = true;
> > + else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > + crtc_state->mode_changed = true;
> > +
> > + return 0;
> > +}
>
> How will this work exactly?
>
> needs_cdm is set in the encoder's atomic_check which is called inside
> drm_atomic_helper_check(). But this function is called before that.
>
> So needs_cdm will never hit.
>
Please refer to the previous patch, it should answer your question.
>
> > +
> > static int dpu_encoder_virt_atomic_check(
> > struct drm_encoder *drm_enc,
> > struct drm_crtc_state *crtc_state,
> > @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
> > topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
> > - if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> > - crtc_state->mode_changed = true;
> > - else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > - crtc_state->mode_changed = true;
> > /*
> > * Release and Allocate resources on every modeset
> > */
--
With best wishes
Dmitry
