On Fri, Jan 10, 2025 at 02:30:55PM +0100, Thomas Zimmermann wrote:
> 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.
No, it is legal if I understand it correctly. The warning comes up if
the dirty_needs_modeset is set after the driver returns from its
atomic_check() callback, see drm_atomic_check_only() patched in the
second patch.
>
> 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@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)
> > >
>
> --
> --
> 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
