Re: [PATCH v2 4/7] drm/connector: hdmi: Use YUV420 output format as an RGB fallback

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5047 bytes --]

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, 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.

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@collabora.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.

>  	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?

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

>  		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:
  - That the driver supports HDMI 2.0
  - That the display is an HDMI output
  - That the mode is allowed YUV420 by the sink EDIDs

> +	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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]