Re: [PATCH v5 6/6] drm/vc4: hdmi: Support HDMI YUV output

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jan 27, 2022 at 03:10:21PM +0100, Maxime Ripard wrote:
> +/*
> + * Conversion between Full Range RGB and Full Range YUV444 using the
> + * BT.709 Colorspace
> + *
> + * [ -0.117208 -0.394207  0.511416  128 ]
> + * [  0.511416 -0.464524 -0.046891  128 ]
> + * [  0.212639  0.715169  0.072192  0   ]
> + *
> + * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
> + */
> +static const u16 vc5_hdmi_csc_full_rgb_to_full_yuv444_bt709[3][4] = {

I think YCbCr output is supposed to be limited range. Or at least 
that was the case with DP. For HDMI/CTA IIRC the spec is super
unclear/confusing when it talks about the YCC quantizaton range
stuff. 

Currently i915 will only do limited range BT.709 YCbCr output.

> +	{ 0xfc41, 0xf364, 0x105e, 0x2000 },
> +	{ 0x105e, 0xf124, 0xfe81, 0x2000 },
> +	{ 0x06ce, 0x16e3, 0x024f, 0x0000 },
> +};
> +
>  static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
>  				    const u16 coeffs[3][4])
>  {
<snip>
> @@ -1323,11 +1534,56 @@ vc4_hdmi_encoder_compute_clock(const struct vc4_hdmi *vc4_hdmi,
>  	return 0;
>  }
>  
> +static int
> +vc4_hdmi_encoder_compute_format(const struct vc4_hdmi *vc4_hdmi,
> +				struct vc4_hdmi_connector_state *vc4_state,
> +				const struct drm_display_mode *mode,
> +				unsigned int bpc)
> +{
> +	struct drm_device *dev = vc4_hdmi->connector.dev;
> +	const struct drm_connector *connector = &vc4_hdmi->connector;
> +	const struct drm_display_info *info = &connector->display_info;
> +	unsigned int format;
> +
> +	drm_dbg(dev, "Trying with an RGB output\n");
> +
> +	format = VC4_HDMI_OUTPUT_RGB;
> +	if (vc4_hdmi_sink_supports_format_bpc(vc4_hdmi, info, mode, format, bpc)) {
> +		int ret;
> +
> +		ret = vc4_hdmi_encoder_compute_clock(vc4_hdmi, vc4_state,
> +						     mode, bpc, format);
> +		if (!ret) {
> +			vc4_state->output_format = format;
> +			return 0;
> +		}
> +	}

You seem to have the bpc vs. format selection in the opposite order to
i915. We try to exhaust all RGB color depths first, and only then fall
back to YCbCr 4:2:0. The automagic YCbCr 4:2:0 fallback is only
really there because without it you may not be able to light up the
display at all (at least if you want the higher resolutions).

With the current i915 logic 4:2:2 doesn't make any sense since it has 
the same requirements as 8bpc RGB. Hence we don't even implement
YCbCr 4:2:2 (also hw didn't have it until recently). There has
occasionally been some talk about introducing a new property
to give the user explicit control over this stuff. If/when that
happens I guess we might want to revisit the 4:2:2 situation for
i915.

> +
> +	drm_dbg(dev, "Failed, Trying with an YUV422 output\n");
> +
> +	format = VC4_HDMI_OUTPUT_YUV422;
> +	if (vc4_hdmi_sink_supports_format_bpc(vc4_hdmi, info, mode, format, bpc)) {
> +		int ret;
> +
> +		ret = vc4_hdmi_encoder_compute_clock(vc4_hdmi, vc4_state,
> +						     mode, bpc, format);
> +		if (!ret) {
> +			vc4_state->output_format = format;
> +			return 0;
> +		}
> +	}
> +
> +	drm_dbg(dev, "Failed. No Format Supported for that bpc count.\n");
> +
> +	return -EINVAL;
> +}
> +

-- 
Ville Syrjälä
Intel