Message-ID: <c6f4233f-4a66-44c2-b962-9c80352bb7e1@collabora.com>
Date: Tue, 11 Mar 2025 20:59:00 +0200
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, kernel@...labora.com,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/7] drm/connector: hdmi: Use YUV420 output format as
 an RGB fallback

Hi Maxime,

On 3/11/25 5:55 PM, Maxime Ripard wrote:
> Hi,
> 
> I think the first thing we need to address is that we will need to
> differentiate between HDMI 1.4 devices and HDMI 2.0.
> 
> It applies to YUV420, which is HDMI 2.0-only, and I guess your patches
> are good enough if you consider YUV420 support only

Yes, my intention was to get the very basic support for now.

, but scrambler setup
> for example is a thing we want to support in that infrastructure
> eventually, and is conditioned on HDMI 2.0 as well.

Right, the scrambler setup is actually among the next tasks I'll focus on,
e.g. this is still missing on dw-hdmi-qp side and I got it reworked a bit
according to your initial review.  It would probably make sense for me to
submit that and get some feedback before attempting to go for a generic
approach (still need to do a few more checks/improvements before).

> On Tue, Mar 11, 2025 at 12:57:36PM +0200, Cristian Ciocaltea wrote:
>> Try to make use of YUV420 when computing the best output format and
>> RGB cannot be supported for any of the available color depths.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 69 +++++++++++++------------
>>  1 file changed, 35 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index a70e204a8df3ac1c2d7318e81cde87a83267dd21..f2052781b797dd09b41127e33d98fe25408a9b23 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -287,8 +287,9 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
>>  	struct drm_device *dev = connector->dev;
>>  	int ret;
>>  
>> -	drm_dbg_kms(dev, "Trying %s output format\n",
>> -		    drm_hdmi_connector_get_output_format_name(fmt));
>> +	drm_dbg_kms(dev, "Trying %s output format with %u bpc\n",
>> +		    drm_hdmi_connector_get_output_format_name(fmt),
>> +		    bpc);
> 
> That part should be in a separate patch, it's independant of the rest.

Ack.

> 
>>  	if (!sink_supports_format_bpc(connector, info, mode, fmt, bpc)) {
>>  		drm_dbg_kms(dev, "%s output format not supported with %u bpc\n",
>> @@ -313,47 +314,22 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
>>  }
>>  
>>  static int
>> -hdmi_compute_format(const struct drm_connector *connector,
>> -		    struct drm_connector_state *conn_state,
>> -		    const struct drm_display_mode *mode,
>> -		    unsigned int bpc)
>> -{
>> -	struct drm_device *dev = connector->dev;
>> -
>> -	/*
>> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
>> -	 * devices, for modes that only support YCbCr420.
>> -	 */
>> -	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) {
>> -		conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
>> -		return 0;
>> -	}
>> -
>> -	drm_dbg_kms(dev, "Failed. No Format Supported for that bpc count.\n");
>> -
>> -	return -EINVAL;
>> -}
>> -
>> -static int
>> -hdmi_compute_config(const struct drm_connector *connector,
>> -		    struct drm_connector_state *conn_state,
>> -		    const struct drm_display_mode *mode)
>> +hdmi_try_format(const struct drm_connector *connector,
>> +		struct drm_connector_state *conn_state,
>> +		const struct drm_display_mode *mode,
>> +		unsigned int max_bpc, enum hdmi_colorspace fmt)
>>  {
>>  	struct drm_device *dev = connector->dev;
>> -	unsigned int max_bpc = clamp_t(unsigned int,
>> -				       conn_state->max_bpc,
>> -				       8, connector->max_bpc);
>>  	unsigned int bpc;
>>  	int ret;
>>  
>>  	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
>> -		drm_dbg_kms(dev, "Trying with a %d bpc output\n", bpc);
>> -
>> -		ret = hdmi_compute_format(connector, conn_state, mode, bpc);
>> -		if (ret)
>> +		ret = hdmi_try_format_bpc(connector, conn_state, mode, bpc, fmt);
>> +		if (!ret)
>>  			continue;
>>  
>>  		conn_state->hdmi.output_bpc = bpc;
>> +		conn_state->hdmi.output_format = fmt;
> 
> I guess it's a matter of semantics, but if it sets the value in the
> state, it doesn't try. Maybe the function should be named
> hdmi_compute_format_bpc then?

Good point!

> 
> That renaming should be in a separate patch too (possibly several).

Yes, I'll move all these preparatory changes into separate patch(es).

> 
>>  		drm_dbg_kms(dev,
>>  			    "Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n",
>> @@ -368,6 +344,31 @@ hdmi_compute_config(const struct drm_connector *connector,
>>  	return -EINVAL;
>>  }
>>  
>> +static int
>> +hdmi_compute_config(const struct drm_connector *connector,
>> +		    struct drm_connector_state *conn_state,
>> +		    const struct drm_display_mode *mode)
>> +{
>> +	unsigned int max_bpc = clamp_t(unsigned int,
>> +				       conn_state->max_bpc,
>> +				       8, connector->max_bpc);
>> +	int ret;
>> +
>> +	ret = hdmi_try_format(connector, conn_state, mode, max_bpc,
>> +			      HDMI_COLORSPACE_RGB);
>> +	if (!ret)
>> +		return 0;
>> +
>> +	if (connector->ycbcr_420_allowed)
>> +		ret = hdmi_try_format(connector, conn_state, mode, max_bpc,
>> +				      HDMI_COLORSPACE_YUV420);
> 
> I think that's conditioned on a few more things:

I've actually expected this! :-)

You've already raised some points during v1, but I preferred to restart the
discussion on updated code instead - sorry for taking so long to respin the
series.  In particular, I worked on [1] to improve handling of
ycbcr_420_allowed flag and fix some consistency issues with
HDMI_COLORSPACE_YUV420 advertised in drm_bridge->supported_formats.  Hence
I assumed it's now safe to rely exclusively on this flag to indicate the
connector is YUV420 capable, without doing any additional checks.

>   - That the driver supports HDMI 2.0

Probably I'm missing something obvious here, but is this necessary to
actually double-check ycbcr_420_allowed has been set correctly?

E.g. for bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops, the
framework does already adjust ycbcr_420_allowed, hence any additional
verification would be redundant.  When not making use of the framework,
drivers are not expected to set the flag if they are not HDMI 2.0 compliant
or not supporting YUV420, right? Are there any other use cases we need to
handle?

>   - That the display is an HDMI output

I think this should be handled by sink_supports_format_bpc() via:

    if (!info->is_hdmi &&
        (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
            drm_dbg_kms(dev, "DVI Monitors require an RGB output at 8 bpc\n");
            return false;
    }

>   - That the mode is allowed YUV420 by the sink EDIDs

And that would be handled via the changes introduced by "drm/connector:
hdmi: Add support for YUV420 format verification".

>> +	else
>> +		drm_dbg_kms(connector->dev,
>> +			    "%s output format not allowed for connector\n",
>> +			    drm_hdmi_connector_get_output_format_name(HDMI_COLORSPACE_YUV420));
> 
> And I think we should keep the catch-all failure message we had.

IIRC, the rational for the change was to get rid of some redundancy, but
I'll recheck and make sure to keep that message in place.

Thanks,
Cristian

[1] https://patchwork.freedesktop.org/series/142679/

Powered by blists - more mailing lists