Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > There are several drivers which attempt to upgrading the commit to the
> > > full mode set from their per-object atomic_check() callbacks without
> > > calling the drm_atomic_helper_check_modeset() or
> > > drm_atomic_helper_check() again (as requested by those functions).
> > 
> > I don't really understand why any of that is supposedly necessary.
> > drm_atomic_helper_check_modeset() is really all about the
> > connector routing stuff, so if none of that is changing then there
> > is no point in calling it again. Eg. in i915 we call it just at
> > the start, and later on we flag internal modesets all over the
> > place, but none of those need drm_atomic_helper_check_modeset()
> > because nothing routing related will have changed.
> 
> i915 doesn't need this because as you say, it doesn't rely on the atomic
> helper modeset tracking much at all, but it's all internal. This is for
> drivers which rely more or less entirely on
> drm_atomic_crtc_needs_modeset().
> 
> Also note that it's not just about connector routing, but about adding all
> the necessary additional states with
> drm_atomic_add_affected_connectors/planes and re-running all the various
> state computation hooks for those. Again i915 hand-rolls that all.

IIRC it only runs the connectors' atomic_check(),
nothing else really. But maybe that's changed since I last
looked at it.

Anyways it feels like we're throwing everything and the
kitchen sink into a single function here. Maybe it should be
split into two or more functions with clear responsibilities?

> 
> So yeah i915 doesn't need this.
> -Sima
> 
> > 
> > > 
> > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > specify that for whatever reasons corresponding CRTC should undergo a
> > > full modeset. The helpers will call these callbacks in a proper place,
> > > adding affected objects and calling required functions as required.
> > > 
> > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > into msm-next and merge remaining msm-specific patches through the
> > > msm-next tree.
> > > 
> > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > Dmitry Baryshkov (6):
> > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > >       drm/msm: drop msm_atomic_check wrapper
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > >  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            | 29 ---------
> > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > ---
> > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > 
> > > Best regards,
> > > -- 
> > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel