Hi,
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.
> 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.
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?
I made a quick test with the flag approach, below. It's not many lines,
but... Ugh, it does feel like a hack.
> 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);
>
Tomi
