Re: [PATCH v2 16/21] drm/bridge: Add the necessary bits to support bus format negotiation

On 26/08/2019 17:26, Boris Brezillon wrote:

This takes the form of various helpers, plus the addition of 2 new structs:

• drm_bus_caps: describe the bus capabilities of a bridge/encoder. For bridges we have one for the input port and one for the output port. Encoders just have one output port.
• drm_bus_cfg: added to the drm_bridge_state. It's supposed to store bus configuration information. For now we just have format and flags but more fields might be added in the future. Again, each drm_bridge_state contains 2 drm_bus_cfg elements: one for the output port and one for the input port

Helpers can be used from bridge drivers' ->atomic_check() implementation to negotiate the bus format to use. Negotiation happens in reserve order, starting from the last element of the bridge chain (the one directly connected to the display) to the first element of the chain (the one connected to the encoder).

Negotiation usually takes place in 2 steps:

• make sure the input end of the next element in the chain picked a format that's supported by the output end of our bridge. That's done with drm_atomic_bridge_choose_output_bus_cfg().
• choose a format for the input end of our bridge based on what's supported by the previous bridge in the chain. This is achieved by calling drm_atomic_bridge_choose_input_bus_cfg()

Note that those helpers are a bit lax when it comes to uninitialized bus format validation. That's intentional. If we don't do that we'll basically break things if we start adding support for bus format negotiation to some elements of the pipeline leaving others on the side, and that's likely to happen for all external bridges since we can't necessarily tell where they are used.

I'll wait Laurent's feedback on this one, but I implemented the DW-HDMI negotiation with it, and it works fine, see [1]

Tested-by: Neil Armstrong narmstrong@baylibre.com

[1] https://patchwork.freedesktop.org/patch/msgid/20190827081425.15011-4-narmstr...

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v2:

• Rework things to support more complex use cases

---

drivers/gpu/drm/drm_bridge.c | 179 ++++++++++++++++++++++++++++++++++- include/drm/drm_bridge.h | 82 ++++++++++++++++ 2 files changed, 260 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 9c74b285da9d..b53732ac997d 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -624,13 +624,184 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge, return 0; }

+static int select_bus_fmt_recursive(struct drm_bridge *first,

• 		    struct drm_bridge *cur,
  

• 		    struct drm_crtc_state *crtc_state,
  

• 		    struct drm_connector_state *conn_state,
  

• 		    u32 out_bus_fmt)
  

+{

• struct drm_bridge_state *cur_state;
• unsigned int num_in_bus_fmts, i;
• struct drm_bridge *prev;
• u32 *in_bus_fmts;
• int ret;
• prev = drm_bridge_chain_get_prev_bridge(cur);
• cur_state = drm_atomic_get_new_bridge_state(crtc_state->state, cur);
• if (WARN_ON(!cur_state))

• return -EINVAL;
  

• /*

• * Bus format negotiation is not supported by this bridge, let's pass
  

• * MEDIA_BUS_FMT_FIXED to the previous bridge in the chain and hope
  

• * that it can handle this situation gracefully (by providing
  

• * appropriate default values).
  

• */
  

• if (!cur->funcs->atomic_get_input_bus_fmts) {

• if (cur != first) {
  

• 	ret = select_bus_fmt_recursive(first, prev, crtc_state,
  

• 				       conn_state,
  

• 				       MEDIA_BUS_FMT_FIXED);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• cur_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_FIXED;
  

• cur_state->output_bus_cfg.fmt = out_bus_fmt;
  

• return 0;
  

• }
• cur->funcs->atomic_get_input_bus_fmts(cur, cur_state, crtc_state,

• 			      conn_state, out_bus_fmt,
  

• 			      &num_in_bus_fmts, NULL);
  

• if (!num_in_bus_fmts)

• return -ENOTSUPP;
  

• in_bus_fmts = kcalloc(num_in_bus_fmts, sizeof(*in_bus_fmts),

• 	      GFP_KERNEL);
  

• if (!in_bus_fmts)

• return -ENOMEM;
  

• cur->funcs->atomic_get_input_bus_fmts(cur, cur_state, crtc_state,

• 			      conn_state, out_bus_fmt,
  

• 			      &num_in_bus_fmts, in_bus_fmts);
  

• if (first == cur) {

• cur_state->input_bus_cfg.fmt = in_bus_fmts[0];
  

• cur_state->output_bus_cfg.fmt = out_bus_fmt;
  

• kfree(in_bus_fmts);
  

• return 0;
  

• }
• for (i = 0; i < num_in_bus_fmts; i++) {

• ret = select_bus_fmt_recursive(first, prev, crtc_state,
  

• 			       conn_state, in_bus_fmts[i]);
  

• if (ret != -ENOTSUPP)
  

• 	break;
  

• }
• if (!ret) {

• cur_state->input_bus_cfg.fmt = in_bus_fmts[i];
  

• cur_state->output_bus_cfg.fmt = out_bus_fmt;
  

• }
• kfree(in_bus_fmts);
• return ret;

+}

+/*

• • This function is called by &drm_atomic_bridge_chain_check() just before
• • calling &drm_bridge_funcs.atomic_check() on all elements of the chain.
• • It's providing bus format negotiation between bridge elements. The
• • negotiation happens in reverse order, starting from the last element in
• • the chain up to @bridge.
• • Negotiation starts by retrieving supported output bus formats on the last
• • bridge element and testing them one by one. The test is recursive, meaning
• • that for each tested output format, the whole chain will be walked backward,
• • and each element will have to choose an input bus format that can be
• • transcoded to the requested output format. When a bridge element does not
• • support transcoding into a specific output format -ENOTSUPP is returned and
• • the next bridge element will have to try a different format. If none of the
• • combinations worked, -ENOTSUPP is returned and the atomic modeset will fail.
• • This implementation is relying on
• • &drm_bridge_funcs.atomic_get_output_bus_fmts() and
• • &drm_bridge_funcs.atomic_get_input_bus_fmts() to gather supported
• • input/output formats.
• • When &drm_bridge_funcs.atomic_get_output_bus_fmts() is not implemented by
• • the last element of the chain, &drm_atomic_bridge_chain_select_bus_fmts()
• • tries a single format: &drm_connector.display_info.bus_formats[0] if
• • available, MEDIA_BUS_FMT_FIXED otherwise.
• • When &drm_bridge_funcs.atomic_get_input_bus_fmts() is not implemented,
• • &drm_atomic_bridge_chain_select_bus_fmts() skips the negotiation on the
• • bridge element that lacks this hook and asks the previous element in the
• • chain to try MEDIA_BUS_FMT_FIXED. It's up to bridge drivers to decide what
• • to do in that case (fail if they want to enforce bus format negotiation, or
• • provide a reasonable default if they need to support pipelines where not
• • all elements support bus format negotiation).
• */

+static int +drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,

• 			struct drm_crtc_state *crtc_state,
  

• 			struct drm_connector_state *conn_state)
  

+{

• struct drm_connector *conn = conn_state->connector;
• struct drm_encoder *encoder = bridge->encoder;
• struct drm_bridge_state *last_bridge_state;
• unsigned int i, num_out_bus_fmts;
• struct drm_bridge *last_bridge;
• u32 *out_bus_fmts;
• int ret = 0;
• last_bridge = list_last_entry(&encoder->bridge_chain,

• 		      struct drm_bridge, chain_node);
  

• last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,

• 					    last_bridge);
  

• if (WARN_ON(!last_bridge_state))

• return -EINVAL;
  

• if (last_bridge->funcs->atomic_get_output_bus_fmts)

• last_bridge->funcs->atomic_get_output_bus_fmts(last_bridge,
  

• 					last_bridge_state,
  

• 					crtc_state,
  

• 					conn_state,
  

• 					&num_out_bus_fmts,
  

• 					NULL);
  

• else

• num_out_bus_fmts = 1;
  

• if (!num_out_bus_fmts)

• return -ENOTSUPP;
  

• out_bus_fmts = kcalloc(num_out_bus_fmts, sizeof(*out_bus_fmts),

• 	       GFP_KERNEL);
  

• if (!out_bus_fmts)

• return -ENOMEM;
  

• if (last_bridge->funcs->atomic_get_output_bus_fmts)

• last_bridge->funcs->atomic_get_output_bus_fmts(last_bridge,
  

• 					last_bridge_state,
  

• 					crtc_state,
  

• 					conn_state,
  

• 					&num_out_bus_fmts,
  

• 					out_bus_fmts);
  

• else 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;
  

• for (i = 0; i < num_out_bus_fmts; i++) {

• ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
  

• 			       conn_state, out_bus_fmts[i]);
  

• if (ret != -ENOTSUPP)
  

• 	break;
  

• }
• kfree(out_bus_fmts);
• return ret;

+}

/**

• drm_atomic_bridge_chain_check() - Do an atomic check on the bridge chain
• @bridge: bridge control structure
• @crtc_state: new CRTC state
• @conn_state: new connector state

• • Calls &drm_bridge_funcs.atomic_check() (falls back on

• • First trigger a bus format negotiation before calling
• • &drm_bridge_funcs.atomic_check() (falls back on
  • &drm_bridge_funcs.mode_fixup()) op for all the bridges in the encoder chain,
  • starting from the last bridge to the first. These are called before calling
  • &drm_encoder_helper_funcs.atomic_check()

@@ -644,6 +815,12 @@ int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, { struct drm_encoder *encoder = bridge->encoder; struct drm_bridge *iter;

• int ret;

• ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state,

• 				      conn_state);
  

• if (ret)

• return ret;
  
  

  list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { int ret;

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 95dc58c3a4e8..96df3bedfee4 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -33,15 +33,33 @@ struct drm_bridge; struct drm_bridge_timings; struct drm_panel;

+/**

• • struct drm_bus_cfg - bus configuration
• • @fmt: format used on this bus
• • @flags: DRM_BUS_ flags used on this bus
• • Encodes the bus format and bus flags used by one end of the bridge or
• • by the encoder output.
• */

+struct drm_bus_cfg {

• u32 fmt;
• u32 flags;

+};

/**

• struct drm_bridge_state - Atomic bridge state object
• @base: inherit from &drm_private_state
• @bridge: the bridge this state refers to

• • @input_bus_info: input bus information
• • @output_bus_info: output bus information
  */

struct drm_bridge_state { struct drm_private_state base;

struct drm_bridge *bridge;

• struct drm_bus_cfg input_bus_cfg;
• struct drm_bus_cfg output_bus_cfg;

};

static inline struct drm_bridge_state * @@ -391,6 +409,70 @@ struct drm_bridge_funcs { void (*atomic_destroy_state)(struct drm_bridge *bridge, struct drm_bridge_state *state);

• /**

• * @atomic_get_output_bus_fmts:
  

• *
  

• * Return the supported bus formats on the output end of a bridge.
  

• * This method is called twice. Once with output_fmts set NULL, in this
  

• * case the driver should set num_output_fmts to the number of
  

• * supported output bus formats such that the core can allocate an
  

• * array of the right dimension. The second time it's called with a
  

• * non-NULL output_fmts, and the driver is expected to fill the
  

• * output_fmts array. The output_fmts array passed to the driver is
  

• * guaranteed to be big enough to store the number of entries returned
  

• * on the first call (no need to check num_output_fmts).
  

• *
  

• * This method is only called on the last element of the bridge chain
  

• * as part of the bus format negotiation process that happens in
  

• * &drm_atomic_bridge_chain_select_bus_fmts().
  

• * This method is optional. When not implemented, the core will
  

• * fallback to &drm_connector.display_info.bus_formats[0] if
  

• * &drm_connector.display_info.num_bus_formats > 0,
  

• * MEDIA_BUS_FMT_FIXED otherwise.
  

• */
  

• void (*atomic_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,
  

• 			   u32 *output_fmts);
  

• /**

• * @atomic_get_input_bus_fmts:
  

• *
  

• * Return the supported bus formats on the input end of a bridge for
  

• * a specific output bus format.
  

• * This method is called twice. Once with input_fmts set NULL, in this
  

• * case the driver should set num_input_fmts to the number of
  

• * supported input bus formats such that the core can allocate an array
  

• * of the right dimension. The second time it's called with a non-NULL
  

• * input_fmts, and the driver is expected to fill the input_fmts array.
  

• * The input_fmts array passed to the driver is guaranteed to be big
  

• * enough to store the number of entries returned on the first call (no
  

• * need to check num_input_fmts).
  

• *
  

• * This method is called on all element of the bridge chain as part of
  

• * the bus format negotiation process that happens in
  

• * &drm_atomic_bridge_chain_select_bus_fmts().
  

• * This method is optional. When not implemented, the core will bypass
  

• * bus format negotiation on this element of the bridge without
  

• * failing, and the previous element in the chain will be passed
  

• * MEDIA_BUS_FMT_FIXED as its output bus format.
  

• *
  

• * Bridge drivers that need to support being linked to bridges that are
  

• * not supporting bus format negotiation should handle the
  

• * output_fmt == MEDIA_BUS_FMT_FIXED case appropriately, by selecting a
  

• * sensible default value or extracting this information from somewhere
  

• * else (FW property, &drm_display_mode, &drm_display_info, ...)
  

• */
  

• void (*atomic_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,
  

• 			  u32 *input_fmts);
  

• /**
  • @atomic_check: