| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1de36429-a27a-4244-8e39-4cb0b09b2689@linaro.org>
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