[PATCH RFC 14/19] drm/bridge: Add the necessary bits to support bus format negotiation

[Neil Armstrong]

On 08/08/2019 17:11, 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 unitialized
> 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.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 187 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  46 +++++++++
>  include/drm/drm_encoder.h    |   6 ++
>  3 files changed, 239 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 9efb27087e70..ef072df42422 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -602,6 +602,193 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder,
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>  
> +int drm_find_best_bus_format(const struct drm_bus_caps *a,
> +			     const struct drm_bus_caps *b,
> +			     const struct drm_display_mode *mode,
> +			     u32 *selected_bus_fmt)
> +{
> +	unsigned int i, j;
> +
> +	/*
> +	 * Some drm_bridge/drm_encoder don't care about the input/output bus
> +	 * format, let's set the format to zero in that case (this is only
> +	 * valid if both side of the link don't care).
> +	 */
> +	if (!a->num_supported_fmts && !b->num_supported_fmts) {
> +		*selected_bus_fmt = 0;
> +		return 0;
> +	} else if (b->num_supported_fmts > 1 && b->supported_fmts) {
> +		*selected_bus_fmt = b->supported_fmts[0];
> +		return 0;

Here, `!a->num_supported_fmts &&` is missing otherwise this code will
select b->supported_fmts[0] whatever the supported formats of a.

> +	} else if (a->num_supported_fmts > 1 && a->supported_fmts) {
> +		*selected_bus_fmt = a->supported_fmts[0];
> +		return 0;

Here, `!b->num_supported_fmts &&` is missing otherwise this code will
select a->supported_fmts[0] whatever the supported formats of b.

> +	} else if (!a->num_supported_fmts || !a->supported_fmts ||
> +		   !b->num_supported_fmts || !b->supported_fmts) {
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * TODO:
> +	 * Dummy algo picking the first match. We probably want something
> +	 * smarter where the narrowest format (in term of bus width) that
> +	 * does not incur data loss is picked, and if all possible formats
> +	 * are lossy, pick the one that's less lossy among all the choices
> +	 * we have. In order to do that we'd need to convert MEDIA_BUS_FMT_
> +	 * modes into something like drm_format_info.
> +	 */
> +	for (i = 0; i < a->num_supported_fmts; i++) {
> +		for (j = 0; j < b->num_supported_fmts; j++) {
> +			if (a->supported_fmts[i] == b->supported_fmts[j]) {
> +				*selected_bus_fmt = a->supported_fmts[i];
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_find_best_bus_format);

[...]

With that fixed, this works with basic negociation of the single MEDIA_BUS_FMT_YUV8_1X24
exposed by the meson hdmi encoder, otherwise it takes the first default MEDIA_BUS_FMT_RGB888_1X24
format exposed by dw-hdmi.

Neil