On Thu, Jan 09, 2025 at 02:53:16PM +0100, Thomas Zimmermann wrote: > Hi > > > Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov: > > As pointed out by Simona, the drm_atomic_helper_check_modeset() and > > drm_atomic_helper_check() require the former function is rerun if the > > driver's callbacks modify crtc_state->mode_changed. MSM is one of the > > drivers which failed to follow this requirement. > > I'm concerned about the implications of this series. How does a driver > upgrade from simple pageflip to full modeset if necessary? The solution in > msm appears to be to run the related test before drm_atomic_helper_check(). > (Right?) > > My corner case is in mgag200, which has to reprogram the PLL if the color > mode changes. So it sets mode_changed to true in the primary plane's > atomic_check. [1] This works in practice because the plane checks run before > the CRTC checks. So the CRTC code will do the correct thing. Reprogramming > the PLL means to disable the display at some point. So it comes down to a > full modeset. > > You mention that drm_atomic_helper_check() needs to rerun if mode_changed > flips. Would it be possible to implement this instead within the helper? I think this should be a driver's decision. For MSM it was easier to move the mode_changed changes and to isolate that before calling into the drm_atomic_helper_check_modeset() code. Other drivers might prefer to rerun the helper. > > Best regards > Thomas > > [1] https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493 > > > > > As suggested by Simona, implement generic code to verify that the > > drivers abide to those requirement and rework MSM driver to follow that > > restrictions. > > > > There are no dependencies between core and MSM parts, so they can go > > separately via corresponding trees. > > > > Reported-by: Simona Vetter <simona.vetter@xxxxxxxx> > > Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/ > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > --- > > Dmitry Baryshkov (6): > > drm/atomic-helper: document drm_atomic_helper_check() restrictions > > drm/atomic: prepare to check that drivers follow restrictions for needs_modeset > > drm/msm/dpu: don't use active in atomic_check() > > drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology() > > drm/msm/dpu: simplify dpu_encoder_get_topology() interface > > drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check() > > > > drivers/gpu/drm/drm_atomic.c | 3 + > > drivers/gpu/drm/drm_atomic_helper.c | 86 ++++++++++++++++++++++++++--- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 -- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++---------- > > 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 +++ > > include/drm/drm_atomic.h | 10 ++++ > > 9 files changed, 192 insertions(+), 43 deletions(-) > > --- > > base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1 > > change-id: 20241222-drm-dirty-modeset-88079bd27ae6 > > > > Best regards, > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) > -- With best wishes Dmitry
