Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 1/8/2025 6:27 PM, 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.

Sorry, my bad. after change (4) of this series needs_cdm is also 
populated within  dpu_encoder_get_topology().

To follow up on 
https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651

So is the plan for CWB to add a dpu_crtc_check_mode_changed() like 
dpu_encoder's and call it?


> > +
> >   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
> >        */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 
> > 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -88,4 +88,8 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder 
> > *drm_enc,
> >   bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
> > +int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
> > +                    struct drm_crtc_state *crtc_state,
> > +                    struct drm_connector_state *conn_state);
> > +
> >   #endif /* __DPU_ENCODER_H__ */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 
> > dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -446,6 +446,31 @@ static void dpu_kms_disable_commit(struct msm_kms 
> > *kms)
> >       pm_runtime_put_sync(&dpu_kms->pdev->dev);
> >   }
> > +static int dpu_kms_check_mode_changed(struct msm_kms *kms, struct 
> > drm_atomic_state *state)
> > +{
> > +    struct drm_crtc_state *new_crtc_state;
> > +    struct drm_connector *connector;
> > +    struct drm_connector_state *new_conn_state;
> > +    int i;
> > +
> > +    for_each_new_connector_in_state(state, connector, new_conn_state, 
> > i) {
> > +        struct drm_encoder *encoder;
> > +
> > +        WARN_ON(!!new_conn_state->best_encoder != 
> > !!new_conn_state->crtc);
> > +
> > +        if (!new_conn_state->crtc || !new_conn_state->best_encoder)
> > +            continue;
> > +
> > +        new_crtc_state = drm_atomic_get_new_crtc_state(state, 
> > new_conn_state->crtc);
> > +
> > +        encoder = new_conn_state->best_encoder;
> > +
> > +        dpu_encoder_virt_check_mode_changed(encoder, new_crtc_state, 
> > new_conn_state);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned 
> > crtc_mask)
> >   {
> >       struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> > @@ -1049,6 +1074,7 @@ static const struct msm_kms_funcs kms_funcs = {
> >       .irq             = dpu_core_irq,
> >       .enable_commit   = dpu_kms_enable_commit,
> >       .disable_commit  = dpu_kms_disable_commit,
> > +    .check_mode_changed = dpu_kms_check_mode_changed,
> >       .flush_commit    = dpu_kms_flush_commit,
> >       .wait_flush      = dpu_kms_wait_flush,
> >       .complete_commit = dpu_kms_complete_commit,
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index 
> > a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct 
> > drm_atomic_state *state)
> >   int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state 
> > *state)
> >   {
> > +    struct msm_drm_private *priv = dev->dev_private;
> > +    struct msm_kms *kms = priv->kms;
> >       struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> >       struct drm_crtc *crtc;
> > -    int i;
> > +    int i, ret = 0;
> > +    /*
> > +     * FIXME: stop setting allow_modeset and move this check to the DPU
> > +     * driver.
> > +     */
> >       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> >                         new_crtc_state, i) {
> >           if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
> > @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, 
> > struct drm_atomic_state *state)
> >           }
> >       }
> > +    if (kms && kms->funcs && kms->funcs->check_mode_changed)
> > +        ret = kms->funcs->check_mode_changed(kms, state);
> > +    if (ret)
> > +        return ret;
> > +
> >       return drm_atomic_helper_check(dev, state);
> >   }
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h 
> > b/drivers/gpu/drm/msm/msm_kms.h
> > index 
> > e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -59,6 +59,13 @@ struct msm_kms_funcs {
> >       void (*enable_commit)(struct msm_kms *kms);
> >       void (*disable_commit)(struct msm_kms *kms);
> > +    /**
> > +     * @check_mode_changed:
> > +     *
> > +     * Verify if the commit requires a full modeset on one of CRTCs.
> > +     */
> > +    int (*check_mode_changed)(struct msm_kms *kms, struct 
> > drm_atomic_state *state);
> > +
> >       /**
> >        * Prepare for atomic commit.  This is called after any previous
> >        * (async or otherwise) commit has completed.