Re: [PATCH] drm/bridge: display-connector: Fix atomic_get_input_bus_fmt hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
---
  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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux