On Fri, Dec 06, 2024 at 10:00:46PM +0200, Cristian Ciocaltea wrote:
> Bridges having the DRM_BRIDGE_OP_HDMI flag set in drm_bridge->ops are
> supposed to rely on drm_bridge->supported_formats bitmask to advertise
> the supported colorspaces, including HDMI_COLORSPACE_YUV420. Therefore,
> the newly introduced drm_bridge->ycbcr_420_allowed flag becomes
> redundant in this particular context.
>
> Moreover, when drm_bridge_connector gets initialised, only
> drm_bridge->ycbcr_420_allowed is considered in the process of adjusting
> the equivalent property of the base drm_connector, which effectively
> discards the formats advertised by the HDMI bridge.
I think this should be handled in a different way: during HDMI connector
init verify that HDMI_COLORSPACE_YUV420 matches the
drm_connector->ycbcr_420_allowed value and rejects incompatible
connectors.
> Handle the inconsistency by ignoring ycbcr_420_allowed for HDMI bridges
> and, instead, make use of the supported_formats bitmask when setting up
> the bridge connector.
>
> Additionally, ensure the YUV420 related bit is removed from the bitmask
> passed to drmm_connector_hdmi_init() when the final ycbcr_420_allowed
> flag for the connector ends up not being set (i.e. the case of having at
> least one non-HDMI bridge in the pipeline that didn't enable it).
>
> Fixes: 3ced1c687512 ("drm/display: bridge_connector: handle ycbcr_420_allowed")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Wrapped HDMI_COLORSPACE_YUV420 flag in the BIT() macro to properly
> check its presence in supported_formats
> - Ensured YUV420 gets removed from the bitmask passed to
> drmm_connector_hdmi_init() when ycbcr_420_allowed is not set
> - Link to v1: https://lore.kernel.org/r/20241130-bridge-conn-fmt-prio-v1-1-146b663f17f3@xxxxxxxxxxxxx
> ---
> drivers/gpu/drm/display/drm_bridge_connector.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 320c297008aaa8b6ef5b1f4c71928849b202e8ac..1f05278b8683a25a845f943720c76faeed24c2e2 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -414,7 +414,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> drm_for_each_bridge_in_chain(encoder, bridge) {
> if (!bridge->interlace_allowed)
> connector->interlace_allowed = false;
> - if (!bridge->ycbcr_420_allowed)
> + if (!bridge->ycbcr_420_allowed && !(bridge->ops & DRM_BRIDGE_OP_HDMI))
> connector->ycbcr_420_allowed = false;
>
> if (bridge->ops & DRM_BRIDGE_OP_EDID)
> @@ -436,6 +436,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>
> if (bridge->supported_formats)
> supported_formats = bridge->supported_formats;
> +
> + if (!(bridge->supported_formats & BIT(HDMI_COLORSPACE_YUV420)))
> + connector->ycbcr_420_allowed = false;
> +
> if (bridge->max_bpc)
> max_bpc = bridge->max_bpc;
> }
> @@ -459,7 +463,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> if (connector_type == DRM_MODE_CONNECTOR_Unknown)
> return ERR_PTR(-EINVAL);
>
> - if (bridge_connector->bridge_hdmi)
> + if (bridge_connector->bridge_hdmi) {
> + if (!connector->ycbcr_420_allowed)
> + supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
> +
> ret = drmm_connector_hdmi_init(drm, connector,
> bridge_connector->bridge_hdmi->vendor,
> bridge_connector->bridge_hdmi->product,
> @@ -468,10 +475,11 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> connector_type, ddc,
> supported_formats,
> max_bpc);
> - else
> + } else {
> ret = drmm_connector_init(drm, connector,
> &drm_bridge_connector_funcs,
> connector_type, ddc);
> + }
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> change-id: 20241130-bridge-conn-fmt-prio-c517c1407ed5
>
--
With best wishes
Dmitry
