Message-ID: <aacca40a-0e3d-9c44-5441-f16a33d52bb7@baylibre.com>
Date:   Fri, 7 Jun 2019 16:11:24 +0200
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     a.hajda@...sung.com, jernej.skrabec@...l.net,
        maxime.ripard@...tlin.com, jonas@...boo.se,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        hverkuil@...all.nl, linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH 2/5] drm/bridge: add encoder support to specify bridge
 input format

On 07/06/2019 15:38, Laurent Pinchart wrote:
> Hi Neil,
> 
> Thank you for the patch.
> 
> On Mon, May 20, 2019 at 03:37:50PM +0200, Neil Armstrong wrote:
>> This patch adds a new format_set() callback to the bridge ops permitting
>> the encoder to specify the new input format and encoding.
>>
>> This allows supporting the very specific HDMI2.0 YUV420 output mode
>> when the bridge cannot convert from RGB or YUV444 to YUV420.
>>
>> In this case, the encode must downsample before the bridge and must
>> specify the bridge the new input bus format differs.
>>
>> This will also help supporting the YUV420 mode where the bridge cannot
>> downsample, and also support 10bit, 12bit and 16bit output modes
>> when the bridge cannot convert between different bit depths.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 35 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_bridge.h     | 19 +++++++++++++++++++
>>  2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 138b2711d389..33be74a977f7 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -307,6 +307,41 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>>  }
>>  EXPORT_SYMBOL(drm_bridge_mode_set);
>>  
>> +/**
>> + * drm_bridge_format_set - setup with proposed input format and encoding for
>> + *			   all bridges in the encoder chain
>> + * @bridge: bridge control structure
>> + * @input_bus_format: proposed input bus format for the bridge
>> + * @input_encoding: proposed input encoding for this bridge
>> + *
>> + * Calls &drm_bridge_funcs.format_set op for all the bridges in the
>> + * encoder chain, starting from the first bridge to the last.
>> + *
>> + * Note: the bridge passed should be the one closest to the encoder
>> + *
>> + * RETURNS:
>> + * true on success, false if one of the bridge cannot handle the format
> 
> I would return an int to propagate the failure reason upstream. It will
> reach the commit tail handler in any case, so will be dropped there, but
> could help debugging issues if we print it in the right place.

Indeed.

> 
>> + */
>> +bool drm_bridge_format_set(struct drm_bridge *bridge,
>> +			   const u32 input_bus_format,
>> +			   const u32 input_encoding)
> 
> You don't need a const here.

Noted

> 
>> +{
>> +	bool ret = true;
>> +
>> +	if (!bridge)
>> +		return true;
>> +
>> +	if (bridge->funcs->format_set)
>> +		ret = bridge->funcs->format_set(bridge, input_bus_format,
>> +						input_encoding);
>> +	if (!ret)
>> +		return ret;
>> +
>> +	return drm_bridge_format_set(bridge->next, input_bus_format,
>> +				     input_encoding);
> 
> I don't think this will scale. It's not that uncommon for bridges to
> change the format (most likely converting from YUV to RGB or the other
> way around, or reducing the number of bits per sample) and the encoding.
> We thus can't propagate it from bridge to bridge and expect that to
> work.
> 
> At the very least, the bridge should report its output bus format and
> encoding, to be applied to the next bridge, but this won't allow
> checking if the configuration can be applied ahead of time, resulting in
> possible failures of a commit tail handler. I wonder if this wouldn't be
> a good time to introduce bridge states...

Ok for chaining the format_set, I thought about it when writing it.

And what if a added a second call drm_bridge_format_check() to check
if a future format_set chaining would work ? then we could either do the
format_set, try another or fallback to a known default format setup.

Would that be sufficient ?

Bridge states would be interesting, but it's really out of the scope of
our current needs in term of bridge changes.

IMHO we can start with format_check/format_set and switch to bridges states
when we have a sufficient use case needing a finer handling of states.

Neil

> 
>> +}
>> +EXPORT_SYMBOL(drm_bridge_format_set);
>> +
>>  /**
>>   * drm_bridge_pre_enable - prepares for enabling all
>>   *			   bridges in the encoder chain
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index d4428913a4e1..7a79e61b7825 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -198,6 +198,22 @@ struct drm_bridge_funcs {
>>  	void (*mode_set)(struct drm_bridge *bridge,
>>  			 const struct drm_display_mode *mode,
>>  			 const struct drm_display_mode *adjusted_mode);
>> +
>> +	/**
>> +	 * @format_set:
>> +	 *
>> +	 * This callback should configure the bridge for the given input bus
>> +	 * format and encoding. It is called after the @format_set callback
>> +	 * for the preceding element in the display pipeline has been called
>> +	 * already. If the bridge is the first element then this would be
>> +	 * &drm_encoder_helper_funcs.format_set. The display pipe (i.e.
>> +	 * clocks and timing signals) is off when this function is called.
>> +	 *
>> +	 * @returns: true in success, false is a bridge refuses the format
>> +	 */
>> +	bool (*format_set)(struct drm_bridge *bridge,
>> +			   const u32 input_bus_format,
>> +			   const u32 input_encoding);
>>  	/**
>>  	 * @pre_enable:
>>  	 *
>> @@ -311,6 +327,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge);
>>  void drm_bridge_mode_set(struct drm_bridge *bridge,
>>  			 const struct drm_display_mode *mode,
>>  			 const struct drm_display_mode *adjusted_mode);
>> +bool drm_bridge_format_set(struct drm_bridge *bridge,
>> +			   const u32 input_bus_format,
>> +			   const u32 input_encoding);
>>  void drm_bridge_pre_enable(struct drm_bridge *bridge);
>>  void drm_bridge_enable(struct drm_bridge *bridge);
>>  
> 

Powered by blists - more mailing lists