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

From: Sam Ravnborg <sam@ravnborg.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 18:53:02 +0200	[thread overview]
Message-ID: <YXLsbp+IwiRYfY2a@ravnborg.org> (raw)
In-Reply-To: <YXLHlaLfPD4wI7R7@pendragon.ideasonboard.com>

Hi Laurent,

> > @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> >  	.attach = ps8640_bridge_attach,
> >  	.detach = ps8640_bridge_detach,
> >  	.get_edid = ps8640_bridge_get_edid,
> > -	.post_disable = ps8640_post_disable,
> > -	.pre_enable = ps8640_pre_enable,
> > +	.atomic_post_disable = ps8640_atomic_post_disable,
> > +	.atomic_pre_enable = ps8640_atomic_pre_enable,
> 
> Don't you also need to implement .atomic_duplicate_state(),
> .atomic_destroy_state() and .atomic_reset() to use the atomic API ?

What I think happens is that the atomic drivers uses the bridges and
the implementation in drm_bridge has fall-backs to the non-atomic
variants.

So for example drm_atomic_bridge_chain_pre_enable() will call
atomic_pre_enable() if it exists, and if not it will call pre_enable()
if it exists.

With this patch-set I show that the non-atomic versions of several of
the drm_bridge helper functions are almost not used and easy to drop.
So based on this I concluded that the bridge drivers were used
in a way so we only would need to implement the atomic variants
of the functions.

That said I did not consider .atomic_duplicate_state(),
.atomic_destroy_state(), or .atomic_reset().

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,

And new bridges (or bridges we convert to use the atomic operations)
would not have to specify them.

I would prefer implementing the fallback - rather than adding the above
to this driver.

	Sam

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