On Wed, 14 Aug 2019 09:51:07 +0200
Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
