Message-ID: <fb95b645-acf7-49a1-b938-a749b20003d4@ti.com>
Date: Wed, 26 Jun 2024 14:32:16 +0530
From: Aradhya Bhatia <a-bhatia1@...com>
To: <neil.armstrong@...aro.org>,
        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/24 19:46, Neil Armstrong wrote:
> 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.
> 

Thanks Neil!

I haven't been able to completely understand your concern above, but
here is what I was able to catch. Let me know if I missed something.

Are you concerned that since dw-hdmi (drm/bridge/synopsis/dw-hdmi.c) has
10+ output formats to offer, all of them won't be negotiated against the
dw-hdmi's get_input_fmt(), but just the first output_fmt?
Because the propagate_bus_fmt() API can only pass one format at a time?


> a bridge before depends on the display support,

Also, could you help me with what you mean here by 'display support'?


Regards
Aradhya

Powered by blists - more mailing lists