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

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