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

Maxime Ripard
2025-03-11T15:55:20Z

[-- 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 --]