Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
From: Neil Armstrong <narmstrong@baylibre.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: jernej.skrabec@siol.net, jonas@kwiboo.se,
	maxime.ripard@bootlin.com, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, hverkuil@xs4all.nl,
	a.hajda@samsung.com, linux-amlogic@lists.infradead.org
Subject: Re: [PATCH 2/5] drm/bridge: add encoder support to specify bridge input format
Date: Fri, 7 Jun 2019 16:11:24 +0200
Message-ID: <aacca40a-0e3d-9c44-5441-f16a33d52bb7@baylibre.com> (raw)
In-Reply-To: <20190607133818.GM7593@pendragon.ideasonboard.com>

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@baylibre.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);
>>  
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-20 13:37 [PATCH 0/5] drm/meson: Add support for HDMI2.0 YUV420 4k60 Neil Armstrong
2019-05-20 13:37 ` [PATCH 1/5] drm/bridge: dw-hdmi: allow ycbcr420 modes for >= 0x200a Neil Armstrong
2019-05-22  6:07   ` Andrzej Hajda
2019-06-05 12:59     ` Neil Armstrong
2019-05-20 13:37 ` [PATCH 2/5] drm/bridge: add encoder support to specify bridge input format Neil Armstrong
2019-05-27 15:29   ` Jernej Škrabec
2019-06-07 13:38   ` Laurent Pinchart
2019-06-07 14:11     ` Neil Armstrong [this message]
2019-05-20 13:37 ` [PATCH 3/5] drm/bridge: dw-hdmi: Add support for dynamic output format setup Neil Armstrong
2019-05-27 15:30   ` Jernej Škrabec
2019-05-20 13:37 ` [PATCH 4/5] drm/meson: Add YUV420 output support Neil Armstrong
2019-06-06 17:11   ` Kevin Hilman
2019-05-20 13:37 ` [PATCH 5/5] drm/meson: Output in YUV444 if sink supports it Neil Armstrong
2019-06-06 17:16   ` Kevin Hilman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aacca40a-0e3d-9c44-5441-f16a33d52bb7@baylibre.com \
    --to=narmstrong@baylibre.com \
    --cc=a.hajda@samsung.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hverkuil@xs4all.nl \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxime.ripard@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org linux-amlogic@archiver.kernel.org
	public-inbox-index linux-amlogic


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/ public-inbox