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. 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. Maxime
signature.asc
Description: PGP signature
