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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi


Am 10.01.25 um 00:57 schrieb Dmitry Baryshkov:
> 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.

Is it legal to do something like

int atomic_check(state)
{
   ret = drm_atomic_helper_check(state)
   if (state->dirty_needs_modeset)
     ret = drm_atomic_helper_check(state)
   return ret;
}

within the driver ? It appears that the atomic helpers warn then.

Best regards
Thomas

>
>> 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)
>>

-- 
--
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)