Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek