Hi, On 20/11/2025 18:19, Maxime Ripard wrote: > On Wed, Nov 19, 2025 at 12:41:52PM +0200, Tomi Valkeinen wrote: >> 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. > > Those are pretty trivial to document though, compared to document how > the new variants differ from drm_atomic_helper_commit_tail() and > drm_atomic_helper_commit_tail_rpm(), and then validating that it does > indeed stay that way.
I agree. >> 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. > > We've had the "let's not introduce helpers for a single user" rule for > like a decade at this point, because it simply doesn't scale. Plenty of > drivers have opted-out for very specific use-case already. I'm not sure > why we should create this precedent. Ok. >> 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... > > Yes, you're right, this is why it's so fragile. Do you want to create > the test suite to check that all combinations are properly tested before > reworking the whole thing? Yes, right after I finish rewriting V4L2 to fix the mistakes there! =) >> 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. > > Because it is. > > Really, I don't get it. I gave you a free pass to do whatever you wanted > in your driver. It doesn't add any maintenance burden on anyone. It > doesn't risk regressing other drivers in the process. It doesn't come > with any testing requirement. It doesn't even have to be reviewed by us, > really. > > Why do you argue for a more bothersome (for everyone) solution? I don't argue for it. I presented the easy-ish options I see to fix this. I think there are valid reasons to have this, in a way or another, in the core. But as I said, it feels like a hack so I'm not too happy with it. In any case, it makes sense to fix these in the respective drivers (with some core functions exported). It will get the issue sorted out for now, without needing elaborate reverts, and without hacking the core. Linus, Marek, is this ok for you? Tomi
