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