On Wed, Nov 19, 2025 at 12:41:52PM +0200, Tomi Valkeinen wrote:
> On 19/11/2025 11:19, Maxime Ripard wrote:
> > On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
> >> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <[email protected]> wrote:
> >>> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> >>>> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> >>
> >>>>> +/**
> >>>>> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> >>>>
> >>>> Based on the function name, it feels that the nem commit tail and
> >>>> modeset enable/disable helpers reached a point where we may want to
> >>>> reconsider the design instead of adding new functions with small
> >>>> differences in behaviour that will end up confusing driver developers.
> >>>
> >>> Agreed, and I'd go even further than that: we don't want every odd order
> >>> in the core. And if some driver has to break the order we document in
> >>> some way it should be very obvious.
> >>
> >> Is this just a comment on this patch 3/3?
> >>
> >> Or do you mean that Mareks new callback
> >> drm_atomic_helper_commit_modeset_enables_crtc_early()
> >> from patch 1/2 should go straight into the R-Car driver as well
> >> and that
> >> drm_atomic_helper_commit_modeset_disables_crtc_late()
> >> patch 2/2 should also go into my driver, even if this
> >> is a comment on patch 3/3?
> >>
> >> Both patches 1 & 2 have a lot to do with ordering, this is
> >> why I ask.
> >
> > I mean, it applies to all your three patches and Marek's: helpers are
> > here to provide a default implementation. We shouldn't provide a default
> > implementation for a single user. All your patches enable to create
> > defaults for a single user.
>
> Two users so far: Renesas and ST-Ericsson.
Only MCDE uses the new drm_atomic_helper_commit_tail_crtc_early_late()
function, while the new
drm_atomic_helper_commit_modeset_enables_crtc_early() helper is used
directly by R-Car DU to implement its commit tail handler, and by
drm_atomic_helper_commit_tail_crtc_early_late().
> > So my point is that none of those functions should be helpers.
> >
> >> We already have
> >> drm_atomic_helper_commit_tail()
> >> drm_atomic_helper_commit_tail_rpm()
> >
> > The former has 5 users, the latter 13. And it's already confusing enough
> > and regression-prone as it is.
> >
> >> Does one more or less really matter? Maybe, I'm not sure,
> >> but if it's just this one patch that is the problem I can surely
> >> do it that way since we're only calling public functions.
> >>
> >> Pushing the first two patches would be more problematic,
> >> because they call a lot of functions that are local to the
> >> drm atomic helpers.
> >
> > I'm totally fine with making more internal functions public though.
>
> While I generally agree with that, I still wonder if an implementation
> in the core is better here. Perhaps a flag in struct drm_driver, instead
> of new set of helpers.
>
> Moving this to the driver would require (with a quick glance) exposing
> the following functions:
>
> crtc_enable
> crtc_disable
> crtc_set_mode
> encoder_bridge_pre_enable
> encoder_bridge_enable
> encoder_bridge_disable
> encoder_bridge_post_disable
>
> Not impossible to expose, but making a private function public does
> require work in validating the function for more general use, and adding
> kernel docs.
>
> Handling this in the core would act as documentation too, so instead of
> the driver doing things in a different way "hidden" inside the driver,
> it would be a standard quirk, clearly documented.
>
> Also, I'm also not sure how rare this quirk is. In fact, I feel we're
> missing ways to handle the enable/disable related issues in the core
> framework. In these patches we're talking about the case where the SoC's
> DSI host needs an incoming pclk to operate, and panels need to do
> configuration before the video stream is enabled. But the exact same
> problem could be present with an external DSI bridge, and then we can't
> fix it in the crtc driver.
>
> So the question becomes "does any component in the pipeline need the
> video stream's clock to operate". But then, it doesn't help if the crtc
> output is enabled early if any bridge in between does not also enable
> its output early. So it all gets a bit complex.
Are we getting close to a point where we all know the bridge model will
need to be reworked extensively, and everybody hopes someone else will
do it ? :-)
> And sometimes the clocks go backward: the entity on the downstream side
> provides a clock backwards, to the source entity...
>
> But I digress. I think initially we should just look for a clean fix for
> the platforms affected:
>
> - Add the implementation into the drivers?
> - Add helpers to the core?
> - Add a flag of some kind so the core can do the right thing?
drm_atomic_helper_commit_modeset_enables_crtc_early() would be more
cumbersome to implement manually in drivers as most of the functions it
calls are not exported. drm_atomic_helper_commit_tail_crtc_early_late()
shouldn't be difficult to implement in the MCDE driver.
> I made a quick test with the flag approach, below. It's not many lines,
> but... Ugh, it does feel like a hack.
Without seeing the code I can already imagine how this would feel like a
hack, so I agree not to go that way.
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index d5ebe6ea0acb..8225aae43e3b 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1341,9 +1341,13 @@ disable_outputs(struct drm_device *dev, struct
> > drm_atomic_state *state)
> > {
> > encoder_bridge_disable(dev, state);
> >
> > - crtc_disable(dev, state);
> > + if (!dev->driver->crtc_early_on)
> > + crtc_disable(dev, state);
> >
> > encoder_bridge_post_disable(dev, state);
> > +
> > + if (dev->driver->crtc_early_on)
> > + crtc_disable(dev, state);
> > }
> >
> > /**
> > @@ -1682,9 +1686,13 @@ encoder_bridge_enable(struct drm_device *dev, struct
> > drm_atomic_state *state)
> > void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > struct drm_atomic_state
> > *state)
> > {
> > + if (dev->driver->crtc_early_on)
> > + crtc_enable(dev, state);
> > +
> > encoder_bridge_pre_enable(dev, state);
> >
> > - crtc_enable(dev, state);
> > + if (!dev->driver->crtc_early_on)
> > + crtc_enable(dev, state);
> >
--
Regards,
Laurent Pinchart
