Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Thomas Zimmermann <tzimmermann@suse.de>]

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?

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@ffwll.ch>
> Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 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)