Dmitry Baryshkov <dmitry.barysh...@oss.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....@linaro.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
>
