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

[Maxime Ripard <mripard@kernel.org>]

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

On Tue, Mar 11, 2025 at 08:59:00PM +0200, Cristian Ciocaltea wrote:
> >> +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:
> 
> I've actually expected this! :-)
> 
> You've already raised some points during v1, but I preferred to restart the
> discussion on updated code instead - sorry for taking so long to respin the
> series.  In particular, I worked on [1] to improve handling of
> ycbcr_420_allowed flag and fix some consistency issues with
> HDMI_COLORSPACE_YUV420 advertised in drm_bridge->supported_formats.  Hence
> I assumed it's now safe to rely exclusively on this flag to indicate the
> connector is YUV420 capable, without doing any additional checks.
> 
> >   - That the driver supports HDMI 2.0
> 
> Probably I'm missing something obvious here, but is this necessary to
> actually double-check ycbcr_420_allowed has been set correctly?
>
> E.g. for bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops, the
> framework does already adjust ycbcr_420_allowed, hence any additional
> verification would be redundant.  When not making use of the framework,
> drivers are not expected to set the flag if they are not HDMI 2.0 compliant
> or not supporting YUV420, right? Are there any other use cases we need to
> handle?

That's what I answered Dmitry as well, we can definitely make
ycbcr_420_allowed conditional on HDMI 2.0 support being implemented.
We'd have to check that it's properly set through
drmm_connector_hdmi_init tests too I guess.

> >   - That the display is an HDMI output
> 
> I think this should be handled by sink_supports_format_bpc() via:
> 
>     if (!info->is_hdmi &&
>         (format != HDMI_COLORSPACE_RGB || bpc != 8)) {
>             drm_dbg_kms(dev, "DVI Monitors require an RGB output at 8 bpc\n");
>             return false;
>     }

Yeah, that makes sense.

> >   - That the mode is allowed YUV420 by the sink EDIDs
> 
> And that would be handled via the changes introduced by "drm/connector:
> hdmi: Add support for YUV420 format verification".

ACK

Maxime

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