On Wed, Jan 08, 2025 at 09:22:56PM -0800, Abhinav Kumar wrote:
>
>
> On 1/8/2025 8:26 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
> > >
> > >
> > > 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?
> >
> > I think dpu_encoder_virt_check_mode_changed() would transform into the
> > dpu_crtc_check_mode_changed() together with one of the patches that
> > moves resource allocation and refactors topology handling.
> >
>
> hmm we need the cur_master for cdm. That will not be accessible in
> dpu_crtc.c so we will end up with a separate dpu_crtc_check_mode_changed()
> for CWB from what I see. We will discuss it further when we re-post CWB.
>
> But overall, I think we can make CWB work on top of this.
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
>
> I do not know how important patch 2 is for this series and I would prefer
> not delaying CWB even more than what it already has been.
>
> If we cannot reach a conclusion on patch 2, can you break that one out of
> this series so that the rest of it is ready to land?
Yes, there is no dependency between patches 1-2 and 3-6.
>
> > >
> > >
> > > >
> > > > > +
> > > > > static int dpu_encoder_virt_atomic_check(
> > > > > struct drm_encoder *drm_enc,
> > > > > struct drm_crtc_state *crtc_state,
> >
--
With best wishes
Dmitry
