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

Neil Armstrong narmstrong at baylibre.com
Tue Aug 27 08:18:08 UTC 2019


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 at baylibre.com>

[1] https://patchwork.freedesktop.org/patch/msgid/20190827081425.15011-4-narmstrong@baylibre.com

> 
> Signed-off-by: Boris Brezillon <boris.brezillon at 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:
>  	 *
> 



More information about the dri-devel mailing list