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

[Maxime Ripard <mripard@kernel.org>]

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

On Sat, Nov 30, 2024 at 01:56:34AM +0200, Cristian Ciocaltea wrote:
> Introduce the switch to YUV420 when computing the best output format and
> RGB cannot be supported for a given color depth.
> 
> While at it, add a minor improvement to the debug message indicating the
> supported format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 3a55881a544a519bb1254968db891c814f831a0f..b4e865e0680f35fd2d849536789f6c1f98a48258 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -304,7 +304,7 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
>  		return false;
>  	}
>  
> -	drm_dbg_kms(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n",
> +	drm_dbg_kms(dev, "%s output format supported with %u bpc (TMDS char rate: %llu Hz)\n",
>  		    drm_hdmi_connector_get_output_format_name(fmt),
>  		    bpc, conn_state->hdmi.tmds_char_rate);
>  
> @@ -319,15 +319,16 @@ hdmi_compute_format(const struct drm_connector *connector,
>  {
>  	struct drm_device *dev = connector->dev;
>  
> -	/*
> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
> -	 * devices, for modes that only support YCbCr420.
> -	 */

It's something that I had in the back of my mind for a while, but we're
at the point where we need to discuss this I guess :)

Not all HDMI controllers are HDMI2.0+ compliant, and we need to gatekeep
this to the fact the controller supports it.

This will also be useful for things like scrambling support. And
probably to provide some TMDS rate check based on the standard a given
controller supports, since most of the drivers have that check
duplicated everywhere.

I don't really have an opinion on how to do this, so I guess it's really
up for debate. The alternatives I could think of are either to add a new
parameter to the init function, or to create a new callback to query the
driver for its capabilities.

The former doesn't seem great since the parameters set is pretty
extensive already. The latter doesn't seem super idiomatic in KMS, but
it's a common pattern in the rest of the kernel, so maybe it's a good
idea still.

>  	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) {
>  		conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
>  		return 0;
>  	}
>  
> +	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_YUV420)) {
> +		conn_state->hdmi.output_format = HDMI_COLORSPACE_YUV420;
> +		return 0;
> +	}
> +

During our discussions when we merged this infrastructure, the goal was
to align our behaviour to Intel's. The discussion also pointed out that
we want to degrade the bpc before falling back to a YUV format.

So we need to first try RGB with any bpc, and then try YUV with any BPC
if it didn't work.

We also need plenty of tests based on whether the source supports
YUV420, the sink has YUV420-only modes, that the fallback occurs
properly, etc.

Maxime

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