linux-kernel - Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2i5vun3pabozzqxjnciylahfx7jljtdmowjo625ida44e37djm@2axmuodlaqtk>
Date: Thu, 9 Jan 2025 06:26:42 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Clark <robdclark@...il.com>, 
	Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	Chandan Uddaraju <chandanu@...eaurora.org>, Jeykumar Sankaran <jsanka@...eaurora.org>, 
	Jordan Crouse <jordan@...micpenguin.net>, Sravanthi Kollukuduru <skolluku@...eaurora.org>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Archit Taneja <architt@...eaurora.org>, Rajesh Yadav <ryadav@...eaurora.org>, 
	linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org, 
	Simona Vetter <simona.vetter@...ll.ch>
Subject: Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from
 atomic_check()

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@...ll.ch>
> > > Closes:
> > > https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > ---
> > >   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.

> 
> 
> > 
> > > +
> > >   static int dpu_encoder_virt_atomic_check(
> > >           struct drm_encoder *drm_enc,
> > >           struct drm_crtc_state *crtc_state,

-- 
With best wishes
Dmitry