diff mbox series

drm/bridge: display-connector: Fix atomic_get_input_bus_fmt hook

Message ID 20240625095049.328461-1-a-bhatia1@ti.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: display-connector: Fix atomic_get_input_bus_fmt hook | expand

Commit Message

Aradhya Bhatia June 25, 2024, 9:50 a.m. UTC
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@ti.com>
---
 drivers/gpu/drm/bridge/display-connector.c | 40 +---------------------
 1 file changed, 1 insertion(+), 39 deletions(-)


base-commit: 62c97045b8f720c2eac807a5f38e26c9ed512371

Comments

Neil Armstrong June 25, 2024, 2:16 p.m. UTC | #1
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@ti.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
Aradhya Bhatia June 26, 2024, 9:02 a.m. UTC | #2
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@ti.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
diff mbox series

Patch

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,