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