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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 14 Aug 2019 09:51:07 +0200
Neil Armstrong <narmstrong@baylibre.com> wrote:

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

Thanks for the report. Will be fixed in v2.

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel