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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: dri-devel@lists.freedesktop.org,
	Andrzej Hajda <andrzej.hajda@intel.com>,
	Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Maxime Ripard <mripard@kernel.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Philip Chen <philipchen@chromium.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Robert Foss <robert.foss@linaro.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Maxime Ripard <maxime@cerno.tech>,
	Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs
Date: Fri, 22 Oct 2021 22:46:20 +0300	[thread overview]
Message-ID: <YXMVDOOJER1VF2JC@pendragon.ideasonboard.com> (raw)
In-Reply-To: <YXMRuKkWyF9tGhG3@ravnborg.org>

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel