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.ciocal...@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
signature.asc
Description: PGP signature
