Re: [PATCH v1 4/7] drm/bridge: lontium-lt9611: Use atomic variants of drm_bridge_funcs

From: Maxime Ripard <maxime@cerno.tech>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Jonas Karlman <jonas@kwiboo.se>, David Airlie <airlied@linux.ie>,
	Robert Foss <robert.foss@linaro.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	dri-devel@lists.freedesktop.org,
	Andrzej Hajda <a.hajda@samsung.com>,
	linux-mediatek@lists.infradead.org,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v1 4/7] drm/bridge: lontium-lt9611: Use atomic variants of drm_bridge_funcs
Date: Thu, 22 Jul 2021 09:42:00 +0200	[thread overview]
Message-ID: <20210722074200.3pdgt63vmex76oug@gilmour> (raw)
In-Reply-To: <20210722062246.2512666-5-sam@ravnborg.org>

Hi,

On Thu, Jul 22, 2021 at 08:22:43AM +0200, Sam Ravnborg wrote:
> The atomic variants of enable/disable in drm_bridge_funcs are the
> preferred operations - introduce these.
> 
> Use of mode_set is deprecated - merge the functionality with
> atomic_enable()
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/lontium-lt9611.c | 69 ++++++++++---------------
>  1 file changed, 27 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index 29b1ce2140ab..dfa7baefe2ab 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -700,9 +700,17 @@ lt9611_connector_mode_valid(struct drm_connector *connector,
>  }
>  
>  /* bridge funcs */
> -static void lt9611_bridge_enable(struct drm_bridge *bridge)
> +static void lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
> +					struct drm_bridge_state *old_bridge_state)
>  {
>  	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> +	const struct drm_display_mode *mode;
> +	const struct drm_crtc_state *crtc_state;
> +	struct hdmi_avi_infoframe avi_frame;
> +	int ret;
> +
> +	crtc_state = drm_bridge_new_crtc_state(bridge, old_bridge_state);
> +	mode = &crtc_state->mode;

So, yeah, it looks like you can't make the assumption that crtc_state is
going to be valid here.

I'm not entirely clear on how bridge states are allocated, but it looks
to me that they are through drm_atomic_add_encoder_bridges, which is
called for all the affected connectors in a commit here:

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L744

Then, the atomic_enable call is made through
drm_atomic_bridge_chain_enable(), which is called in
drm_atomic_helper_commit_modeset_enables only if the CRTC is active and
needs a modeset.

I guess this means that we won't have a null pointer for crtc_state
there, but wouldn't that cause some issues? I can imagine a property
like the bpc count or output format where it wouldn't imply a modeset
but would definitely affect the bridges in the chain?

Maxime