On Thu, Feb 03, 2022 at 10:07:22PM +0200, Ville Syrjälä wrote:
> 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.

Indeed I haven't been able to find anything in the spec, so I'll just
drop it, if only to remain consistent across drivers.

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

Our use-case is slightly different, but the basic idea is the same:
since we support from 8 to 12 bpc, an output to YUV422 output can prove
useful if we are reading a 12 bpc content and we don't have the
bandwidth to use RGB.

Thus, when we have a max_bpc of 12, we try RGB and YUV422, if none of
them work we try RGB in 10 and 8 bpc.

This is indeed a bit different than i915, but it doesn't seem
fundamentally different to me.

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

This seems to be stalled entirely though, and this approach isn't
incompatible with whatever that would involve.

Maxime