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

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

On Thu, 22 Aug 2019 03:55:56 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 05:11:45PM +0200, 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,  
> 
> s/reserve/reverse/
> 
> > 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;
> > +	} else if (a->num_supported_fmts > 1 && a->supported_fmts) {
> > +		*selected_bus_fmt = a->supported_fmts[0];
> > +		return 0;
> > +	} 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);
> > +
> > +/**
> > + * drm_atomic_bridge_choose_input_bus_cfg() - Choose bus config for the input
> > + *					      end
> > + * @bridge_state: bridge state
> > + * @crtc_state: CRTC state
> > + * @conn_state: connector state
> > + *
> > + * Choose a bus format for the input side of a bridge based on what the
> > + * previous bridge in the chain and this bridge support. Can be called from
> > + * bridge drivers' &drm_bridge_funcs.atomic_check() implementation.
> > + *
> > + * RETURNS:
> > + * 0 if a matching format was found, a negative error code otherwise
> > + */
> > +int
> > +drm_atomic_bridge_choose_input_bus_cfg(struct drm_bridge_state *bridge_state,
> > +				       struct drm_crtc_state *crtc_state,
> > +				       struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_bridge *self = bridge_state->bridge;
> > +	struct drm_bus_caps *prev_output_bus_caps;
> > +	struct drm_bridge *prev;
> > +	int ret;
> > +
> > +	prev = drm_bridge_chain_get_prev_bridge(self);
> > +	if (!prev)
> > +		prev_output_bus_caps = &self->encoder->output_bus_caps;
> > +	else
> > +		prev_output_bus_caps = &prev->output_bus_caps;
> > +
> > +	ret = drm_find_best_bus_format(prev_output_bus_caps,
> > +				       &self->input_bus_caps, &crtc_state->mode,
> > +				       &bridge_state->input_bus_cfg.fmt);
> > +	if (ret)
> > +		return ret;  
> 
> I think most bridges that support multiple input and output formats will
> not necessarily be able to transcode, meaning that for a particular
> output format a particular corresponding input format will need to be
> picked (or possibly one of a subset of input formats). I'm thus not sure
> how useful this helper will be in practice.
> 
> I also fear that this negotiation mechanism is too simple and will lock
> is with support for very simple systems only :-S Let's assume a system
> with a display device (D), two bridges (B0 and B1) and a connector (C).
> Let's further assume that the bus format required by the connector is
> fixed. With this patch series, B1 will pick an output format identical
> to the format on the connector, and will then pick an input format among
> the ones supported by both B0 and B1. Let's assume there are multiple
> options, Fa and Fb, and drm_find_best_bus_format() will pick one of
> them, say Fa. Then B0 will configure its output with that format, and
> will proceed with finding an input format supported by D. It could be
> that the output format Fa for B0 requires an input format that is not
> supported by D, while Fb would have produced a working pipeline.

Yes, I assumed all pipeline elements would be able to convert any of
their supported input formats to any of the output ones, which might
not be the case in practice. I could add a matrix to expose the
supported conversions and add a pass of 'format eviction' at bridge
attach time, but I'm not sure that's enough.

> 
> I don't think this example is really far-fetched, and while we don't
> need to support it immediately, we need an API that makes it possible to
> support it, as changing the API later would require reworking all the
> drivers that use it.
> 
> > +
> > +	/*
> > +	 * TODO:
> > +	 * Should we fill/check the ->flag field too?
> > +	 */
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_choose_input_bus_cfg);
> > +
> > +/**
> > + * drm_atomic_bridge_choose_output_bus_cfg() - Choose bus config for the output
> > + *					       end
> > + * @bridge_state: bridge state
> > + * @crtc_state: CRTC state
> > + * @conn_state: connector state
> > + *
> > + * Choose a bus format for the output side of a bridge based on what the next
> > + * bridge in the chain and this bridge support. Can be called from bridge
> > + * drivers' &drm_bridge_funcs.atomic_check() implementation.
> > + *
> > + * RETURNS:
> > + * 0 if a matching format was found, a negative error code otherwise
> > + */
> > +int
> > +drm_atomic_bridge_choose_output_bus_cfg(struct drm_bridge_state *bridge_state,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_atomic_state *state = crtc_state->state;
> > +	struct drm_bridge *self = bridge_state->bridge;
> > +	struct drm_bridge_state *next_bridge_state;
> > +	struct drm_bridge *next;
> > +	u32 fmt;
> > +
> > +	next = drm_bridge_chain_get_next_bridge(self);
> > +	if (!next) {
> > +		struct drm_connector *conn = conn_state->connector;
> > +		struct drm_display_info *di = &conn->display_info;
> > +
> > +		/*
> > +		 * FIXME:
> > +		 * We currently pick the first supported format
> > +		 * unconditionally. It seems to fit all current use cases but
> > +		 * might be too limited for panels/displays that can configure
> > +		 * the bus format dynamically.
> > +		 */
> > +		if (di->num_bus_formats && di->bus_formats)
> > +			bridge_state->output_bus_cfg.fmt = di->bus_formats[0];
> > +		else
> > +			bridge_state->output_bus_cfg.fmt = 0;  
> 
> What is the bridge driver supposed to do in this case ?

Pick a default value (or a something fixed by a FW property), as it used
to do before this patchset. There's no way we can convert all bridges at
once, so we need to support mixed chains formed of bridges that support
bus format negotiation and bridges that don't.

> 
> > +
> > +		bridge_state->output_bus_cfg.flags = di->bus_flags;
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * We expect output_bus_caps to contain at least one format. Note
> > +	 * that don't care about bus format negotiation can simply not
> > +	 * call this helper.  
> 
> I'm not sure to parse the second sentence correctly.

Oops.

"Note that drivers that don't care about ...".
I'm just trying to explain why EINVAL is returned when
->output_bus_caps is not populated.

> 
> > +	 */
> > +	if (!self->output_bus_caps.num_supported_fmts ||!  
> 
> Extra ! at the end of the line ?
> 
> > +	    !self->output_bus_caps.supported_fmts)
> > +		return -EINVAL;
> > +
> > +	next_bridge_state = drm_atomic_get_new_bridge_state(state, next);
> > +
> > +	/*
> > +	 * The next bridge is expected to have called
> > +	 * &drm_atomic_bridge_choose_input_bus_cfg() as part of its
> > +	 * &drm_bridge_funcs.atomic_check() implementation, but this hook is
> > +	 * optional, and even if it's implemented, calling
> > +	 * &drm_atomic_bridge_choose_input_bus_cfg() is not mandated.
> > +	 * If fmt is zero, that means the next element in the chain doesn't
> > +	 * care about bus format negotiation (probably because there's only
> > +	 * one possible setting). In that case, we still have to select one
> > +	 * bus format for the output port of our bridge, and this is only
> > +	 * possible if the bridge supports only one format.  
> 
> Theoritically we could also just pick the first one (or actually any of
> the supported formats), couldn't we ?

Hm, not really. I mean, if the bridge supports several output formats,
there's no way we can tell which one is the right one for this
specific setup. The idea being that drivers are responsible for picking
an appropriate output bus-format based on something else (DT prop?)
when drm_atomic_bridge_choose_output_bus_cfg() returns an error.

> 
> 
> > +	 */
> > +	fmt = next_bridge_state->input_bus_cfg.fmt;
> > +	if (fmt) {
> > +		unsigned int i;
> > +
> > +		for (i = 0; i < self->output_bus_caps.num_supported_fmts; i++) {
> > +			if (self->output_bus_caps.supported_fmts[i] == fmt)
> > +				break;
> > +		}
> > +
> > +		if (i == self->output_bus_caps.num_supported_fmts)
> > +			return -ENOTSUPP;
> > +	} else if (self->output_bus_caps.num_supported_fmts == 1) {
> > +		fmt = self->output_bus_caps.supported_fmts[0];
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * TODO:
> > +	 * Should we fill/check the ->flag field too?
> > +	 */
> > +	bridge_state->output_bus_cfg.fmt = fmt;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_choose_output_bus_cfg);
> > +
> >  static int drm_atomic_bridge_check(struct drm_bridge *bridge,
> >  				   struct drm_crtc_state *crtc_state,
> >  				   struct drm_connector_state *conn_state)
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 2f69adb7b0f3..c578800a05e0 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -33,15 +33,45 @@ 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_bus_caps - bus capabilities
> > + * @supported_fmts: array of MEDIA_BUS_FMT_ formats
> > + * @num_supported_fmts: size of the supported_fmts array
> > + *
> > + * Used by the core to negotiate the bus format at runtime.
> > + */
> > +struct drm_bus_caps {
> > +	const u32 *supported_fmts;
> > +	unsigned int num_supported_fmts;
> > +};
> > +
> >  /**
> >   * 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  
> 
> I would make this an array, to prepare for bridges that will have more
> than one input or one output.

Hm, not sure how you'd represent that. I guess you want to support
bridges that can activate several inputs+outputs in parallel. Do we
even support that right now?
Also, how do we link the input to the output in that case? By their
position in the array? And we'd need an extra field in bus_caps to
point to the corresponding bridge input/output port.

I'm glad we can discuss all those problems, but I'd like to focus on
what's needed right now, not what could potentially be needed in the
future. If we can design things to be more future-proof that's great,
but in my experience, trying to solve problems that do not exist yet
often leads to complex designs which prove to be wrong when we try to
apply them to real situations when they finally occur.

> 
> >   */
> >  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 *
> > @@ -465,6 +495,9 @@ struct drm_bridge {
> >  	const struct drm_bridge_funcs *funcs;
> >  	/** @driver_private: pointer to the bridge driver's internal context */
> >  	void *driver_private;
> > +
> > +	struct drm_bus_caps input_bus_caps;
> > +	struct drm_bus_caps output_bus_caps;
> >  };
> >  
> >  static inline struct drm_bridge *
> > @@ -479,6 +512,14 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> >  
> > +int
> > +drm_atomic_bridge_choose_input_bus_cfg(struct drm_bridge_state *bridge_state,
> > +				       struct drm_crtc_state *crtc_state,
> > +				       struct drm_connector_state *conn_state);
> > +int
> > +drm_atomic_bridge_choose_output_bus_cfg(struct drm_bridge_state *bridge_state,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state);
> >  struct drm_bridge *
> >  drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder);
> >  struct drm_bridge *
> > @@ -562,6 +603,11 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
> >  	return drm_priv_to_bridge_state(obj_state);
> >  }
> >  
> > +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);
> > +
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >  					u32 connector_type);
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index 0899f7f34e3a..e891921b3aed 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -25,6 +25,7 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/ctype.h>
> > +#include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_mode_object.h>
> > @@ -184,6 +185,11 @@ struct drm_encoder {
> >  	 * functions as part of its encoder enable/disable handling.
> >  	 */
> >  	uint32_t custom_bridge_enable_disable_seq : 1;
> > +
> > +	/**
> > +	 * @output_bus_caps: Supported output bus caps
> > +	 */
> > +	struct drm_bus_caps output_bus_caps;
> >  };
> >  
> >  #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
> >   
> 

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