On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote: > 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. It calls into connector/bridge/crtc callbacks related to modesets and mode validation. The thing is a few hundred lines in total if you include all the split out subfunctions. Like the kerneldoc pretty clearly spells out that it does a lot more than what you've listed here. Just i915 doesn't used most of that. > 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? I'm not sure you can split it up much, because modesetting is complicated. Like even if you'd want to split out just the routing update logic that's a pretty big mess with a bunch of callbacks so that we can pick the right encoders to add the right bridges. And then have a 2nd function that does the actual state computation/validation. Not sure that's worth it, since only benefit would be for drivers like i915 that almost entirely hand-roll their own atomic check flow and really only need the connector routing bits. I guess if you're bored you could give it a stab. -Sima > > > > > 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@xxxxxxxxxx/ > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxx> > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Simona Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
