linux-kernel - Re: [PATCH v9 01/14] drm/atomic-helper: Add crtc check before checking plane

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: <CABymUCMuiKQwtuYW-HX4bwyTR4awm4scwXTWNzVCEmyQsQ29aA@mail.gmail.com>
Date: Fri, 9 May 2025 11:08:30 +0800
From: Jun Nie <jun.nie@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Rob Clark <robdclark@...il.com>, Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 01/14] drm/atomic-helper: Add crtc check before
 checking plane

Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年5月8日周四 18:47写道：
>
> On Tue, May 06, 2025 at 11:47:31PM +0800, Jun Nie wrote:
> > Some display controller support flexible CRTC and DMA, such as the display
> > controllers in snapdragon SoCs. CRTC can be implemented with several mixers
> > in parallel, and plane fetching can be implemented with several DMA under
> > umberala of a virtual drm plane.
> >
> > The mixer number is decided per panel resolution and clock rate constrain
> > first, which happens in CRTC side. Then plane is split per mixer number
> > and configure DMA accordingly.
>
> Here you are describing a behaviour of one particular driver as a reason
> to change the framework.

Yeah, the specific driver requires a change in framework. Maybe the
comment is not
proper?
>
> >
> > To support such forthcoming usage case, CRTC checking shall happen before
> > checking plane. Add the checking in the drm_atomic_helper_check_modeset().
>
> So, now drivers will get two calls to atomic_check(), one coming in
> circumstances which were not expected by the drivers before. Are you
> sure that this won't break anything?

Yes, it is a concern. Is there any way to limit the change in
framework to specific
driver with a flag, such as DRM_FLAG_CHECK_CRTC_BEFORE_PLANE?
>
> >
> > Signed-off-by: Jun Nie <jun.nie@...aro.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 5302ab3248985d3e0a47e40fd3deb7ad0d9f775b..5bca4c9683838c38574c8cb7c0bc9d57960314fe 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -816,6 +816,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >                       return ret;
> >       }
> >
> > +     for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +             const struct drm_crtc_helper_funcs *funcs;
> > +
> > +             funcs = crtc->helper_private;
> > +
> > +             if (!funcs || !funcs->atomic_check)
> > +                     continue;
> > +
> > +             ret = funcs->atomic_check(crtc, state);
> > +             if (ret) {
> > +                     drm_dbg_atomic(crtc->dev,
> > +                                    "[CRTC:%d:%s] atomic driver check failed\n",
> > +                                    crtc->base.id, crtc->name);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +
> > +
>
> Too many empty lines. But the main quesiton is: why are you calling it
> before mode_valid()? According to your description a better place would
> be in drm_atomic_helper_check_planes().
>
Agree, that's the proper function. Will remove the empty line in next version.

> >       ret = mode_valid(state);
> >       if (ret)
> >               return ret;
> >
> > --
> > 2.34.1
> >
>
> --
> With best wishes
> Dmitry
>