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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22/08/2019 09:48, Boris Brezillon wrote:
> On Thu, 22 Aug 2019 03:55:56 +0300
> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@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;
>>> +	} 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.

I had a thought when implementing bus format negotiation for dw-hdmi.

Depending on the output bus-format, we could have different sets of possible
input drm_bus_caps.

For example, if output is MEDIA_BUS_FMT_RGB888_1X24,
the input bus formats could only be MEDIA_BUS_FMT_RGB888_1X24 or MEDIA_BUS_FMT_YUV8_1X24.
Same for MEDIA_BUS_FMT_RGB101010_1X30, MEDIA_BUS_FMT_RGB121212_1X36, MEDIA_BUS_FMT_RGB161616_1X48...
And the special MEDIA_BUS_FMT_UYYVYY8_0_5X24, where input must be output.

How could we handle this ? I mean, could we have a fixed
output drm_bus_caps and be able to dynamically change the input drm_bus_caps ?

Adding matrix in struct drm_bridge seems over-engineered, no ? Since it should take
in account the actual drm_display_mode.

Maybe by passing a dynamic input drm_bus_caps to drm_atomic_bridge_choose_input_bus_cfg()
and take the default struct drm_bridge one if not provided ?
This would really simplify the bridge atomic_check implementation by reusing
the drm_atomic_bridge_choose_*() functions.

Neil

> 
>>
>>>   */
>>>  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux