Re: [PATCH 1/7] drm/bridge: display-connector: implement bus fmts callbacks

Hi Neil,

code looks fine. A few improvement proposals to the comments. With the include order fixed and the comments considered: Reviewed-by: Sam Ravnborg sam@ravnborg.org

Sam

On Thu, Oct 14, 2021 at 05:26:00PM +0200, Neil Armstrong wrote:

Since this bridge is tied to the connector, it acts like a passthrough, so concerning the output & input bus formats, either pass the bus formats from the previous bridge or return fallback data like done in the bridge function: drm_atomic_bridge_chain_select_bus_fmts() & select_bus_fmt_recursive.

This permits avoiding skipping the negociation if the remaining bridge chain has all the bits in place.

Without this bus fmt negociation breaks on drm/meson HDMI pipeline when attaching dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR, because the last bridge of the display-connector doesn't implement buf fmt callbacks and MEDIA_BUS_FMT_FIXED is used leading to select an unsupported default bus format from dw-hdmi.

Signed-off-by: Neil Armstrong narmstrong@baylibre.com

drivers/gpu/drm/bridge/display-connector.c | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c index 05eb759da6fc..9697ac173157 100644 --- a/drivers/gpu/drm/bridge/display-connector.c +++ b/drivers/gpu/drm/bridge/display-connector.c @@ -14,6 +14,7 @@ #include <linux/regulator/consumer.h>

#include <drm/drm_bridge.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h>

Alphabetic order.

struct display_connector { @@ -87,10 +88,97 @@ static struct edid *display_connector_get_edid(struct drm_bridge *bridge, return drm_get_edid(connector, conn->bridge.ddc); }

+/*

• • Since this bridge is tied to the connector, it acts like a passthrough,
• • so concerning the output bus formats, either pass the bus formats from the
• • previous bridge or return fallback data like done in the bridge function:
• • drm_atomic_bridge_chain_select_bus_fmts().
• • This permits avoiding skipping the negociation if the bridge chain has all
• • bits in place.

negociation if => negotiation of

Consider the following wording: This supports negotiation if the bridge chain has all bits in place.

• */

+static u32 *display_connector_get_output_bus_fmts(struct drm_bridge *bridge,

• 			struct drm_bridge_state *bridge_state,
  

• 			struct drm_crtc_state *crtc_state,
  

• 			struct drm_connector_state *conn_state,
  

• 			unsigned int *num_output_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_output_bus_fmts) {

• struct drm_connector *conn = conn_state->connector;
  

• u32 *out_bus_fmts;
  

• *num_output_fmts = 1;
  

• out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL);
  

• if (!out_bus_fmts)
  

• 	return NULL;
  

• if (conn->display_info.num_bus_formats &&
  

•     conn->display_info.bus_formats)
  

• 	out_bus_fmts[0] = conn->display_info.bus_formats[0];
  

• else
  

• 	out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
  

• return out_bus_fmts;
  

• }
• prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,

• 					    prev_bridge);
  

• return prev_bridge->funcs->atomic_get_output_bus_fmts(prev_bridge, prev_bridge_state,

• 					      crtc_state, conn_state,
  

• 					      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 return fallback data like done in the bridge function:
• • select_bus_fmt_recursive() when atomic_get_input_bus_fmts is not supported.

Maybe use this this: from the previous bridge or MEDIA_BUS_FMT_FIXED (like select_bus_fmt_recursive())

• • This permits avoiding skipping the negociation if the bridge chain has all
• • bits in place.

Like above

• */

+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, .get_edid = display_connector_get_edid,

• .atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts,
• .atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts,
• .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,

};

static irqreturn_t display_connector_hpd_irq(int irq, void *arg)

2.25.1