Date: Tue, 25 Jun 2024 16:16:18 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Aradhya Bhatia <a-bhatia1@...com>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
 Andrzej Hajda <andrzej.hajda@...el.com>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 Sam Ravnborg <sam@...nborg.org>
Cc: DRI Development List <dri-devel@...ts.freedesktop.org>,
 Linux Kernel List <linux-kernel@...r.kernel.org>,
 Devarsh Thakkar <devarsht@...com>, Jayesh Choudhary <j-choudhary@...com>,
 Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH] drm/bridge: display-connector: Fix
 atomic_get_input_bus_fmt hook

On 25/06/2024 11:50, Aradhya Bhatia wrote:
> The display-connector acts as a pass-through bridge. To truly reflect
> that, this bridge should accept the same input format, as it expects to
> output. That in turn should be the same as what the preceding bridge has
> to output.
> 
> While the get_output_fmt hook does exactly that by calling the same hook
> of the previous bridge, the get_input_fmt hook should simply propagate
> the expected output format as its required input format.
> 
> Let's say bridge(n) converts YUV bus format to RGB before transmitting
> the video signals. B is supposed to be RGB and A is YUV. The
> get_input_fmt hook of bridge(n) should receive RGB as its expected
> output format for it to select YUV as its required input format.
> 
> Moreover, since the display-connector is a pass-through bridge, X and Y
> should both be RGB as well.
> 
>      +-------------+            +-------------+
> A   |             |   B    X   |             |   Y
> --->|  Bridge(n)  +--->    --->| Display     +--->
>      |             |            | Connector   |
>      |             |            |             |
>      +-------------+            +-------------+
> 
> But that's not what's happening at the moment.
> 
> The core will call get_output_fmt hook of display-connector, which will
> call the same hook of bridge(n). Y will get set to RGB because B is RGB.
> 
> Now the core will call get_input_fmt hook of display-connector with Y =
> RGB as its expected output format. This hook will in turn call the
> get_input_fmt hook of bridge(n), with expected output as RGB. This hook
> will then return YUV as its required input format, which will set X =
> YUV.
> 
> This is where things get off the track. The core will then call
> bridge(n)'s get_input_fmt hook but this time the expected output will
> have changed to X = YUV, instead of what ideally should have been X =
> RGB. We don't know how bridge(n)'s input format requirement will change
> now that its expected output format isn't RGB but YUV.
> 
> Ideally, formats Y, X, B need to be the same and the get_input_fmt hook
> for bridge(n) should be called with these as its expected output format.
> Calling that hook twice can potentially change the expected output
> format - which can then change the required input format again, or it
> might just throw an -ENOTSUPP error.
> 
> While many bridges don't utilize these APIs, or in a lot of cases A and
> B are same anyway, it is not the biggest problem, but one that should be
> fixed anyway.
> 
> Fix this.
> 
> Fixes: 7cd70656d128 ("drm/bridge: display-connector: implement bus fmts callbacks")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
> ---
>   drivers/gpu/drm/bridge/display-connector.c | 40 +---------------------
>   1 file changed, 1 insertion(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index ab8e00baf3f1..eebf1fbcdd23 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -131,50 +131,12 @@ static u32 *display_connector_get_output_bus_fmts(struct drm_bridge *bridge,
>   							      num_output_fmts);
>   }
>   
> -/*
> - * Since this bridge is tied to the connector, it acts like a passthrough,
> - * so concerning the input bus formats, either pass the bus formats from the
> - * previous bridge or MEDIA_BUS_FMT_FIXED (like select_bus_fmt_recursive())
> - * when atomic_get_input_bus_fmts is not supported.
> - * This supports negotiation if the bridge chain has all bits in place.
> - */
> -static u32 *display_connector_get_input_bus_fmts(struct drm_bridge *bridge,
> -					struct drm_bridge_state *bridge_state,
> -					struct drm_crtc_state *crtc_state,
> -					struct drm_connector_state *conn_state,
> -					u32 output_fmt,
> -					unsigned int *num_input_fmts)
> -{
> -	struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge);
> -	struct drm_bridge_state *prev_bridge_state;
> -
> -	if (!prev_bridge || !prev_bridge->funcs->atomic_get_input_bus_fmts) {
> -		u32 *in_bus_fmts;
> -
> -		*num_input_fmts = 1;
> -		in_bus_fmts = kmalloc(sizeof(*in_bus_fmts), GFP_KERNEL);
> -		if (!in_bus_fmts)
> -			return NULL;
> -
> -		in_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
> -
> -		return in_bus_fmts;
> -	}
> -
> -	prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> -							    prev_bridge);
> -
> -	return prev_bridge->funcs->atomic_get_input_bus_fmts(prev_bridge, prev_bridge_state,
> -							     crtc_state, conn_state, output_fmt,
> -							     num_input_fmts);
> -}
> -
>   static const struct drm_bridge_funcs display_connector_bridge_funcs = {
>   	.attach = display_connector_attach,
>   	.detect = display_connector_detect,
>   	.edid_read = display_connector_edid_read,
>   	.atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts,
> -	.atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts,
> +	.atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
>   	.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,
> 
> base-commit: 62c97045b8f720c2eac807a5f38e26c9ed512371

This will break dw-hdmi YUV output negociation because returning output_format
it won't even try to select something else than the connector output_fmt.

This is limitation of the bus_fmt negociation, it negociates in backwards, but
if the last one uses bridge_propagate_bus_fmt, and a bridge before depends on the
display support, it will be constrained by the first output_fmt.

Neil

Powered by blists - more mailing lists