Hi Sam, On Fri, Oct 22, 2021 at 09:32:08PM +0200, Sam Ravnborg wrote: > On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote: > > On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote: > > > Hi Laurent, > > > > > > > From a quick look only cadence/cdns-mhdp8546 subclass > > > > drm_bridge_state and I wonder if the right thing to do would be to > > > > implement fallback to the helpers if the bridge driver do not set > > > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset(). > > > > > > > > That would drop the following from a few bridges: > > > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > > > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > > > .atomic_reset = drm_atomic_helper_bridge_reset, > > > > > > To answer myself here. This would create a dependency from the core to > > > the helpers which is not OK so idea dropped again. > > > > I agree it would be nicer, but the dependency is likely a problem. That > > being said, we have multiple types of helpers. The first set is the > > modeset helpers, which were designed as one implementation of KMS > > operations, with an opt-in API for drivers. The core should not depend > > on those. There are however other helpers that are only default > > implementations of some operations, without any dependency on other > > components. The atomic state helpers fall in this category, they > > implement .atomic_* operations of the drm_*_funcs structures, not > > drm_*_helper_funcs. It could make sense to move them to the DRM core. > > For now I went with a simple macro: > > +/** > + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs > + * > + * Bridge driver that do not subclass &drm_bridge_state can use the helpers > + * for reset, duplicate, and destroy. This macro provides a shortcut for > + * setting the helpers in the &drm_bridge_funcs structure. > + */ > +#define DRM_BRIDGE_STATE_OPS \ > + .atomic_reset = drm_atomic_helper_bridge_reset, \ > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state > + > > Thomas Z. is trying to make the core smaller so pulling in these helpers > would be counterproductive to that. So I took the simpler approach here > which we have already done in several places. Those helpers are in the same file as the other state helpers, which are used by all atomic drivers as far as I can tell, so I'm not sure we can really make anything smaller (except if we moved the bridge helpers to a separate file, but I don't think it would be worth it). > It will be part of v3 when I post it. > > Drop a note if you (or any other reader) have better ideas. -- Regards, Laurent Pinchart
