[PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[Cristian Ciocaltea]

Bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops are expected to
rely on drm_bridge->supported_formats to advertise the supported
colorspaces, including HDMI_COLORSPACE_YUV420.

However, when drm_bridge_connector gets initialised, only
drm_bridge->ycbcr_420_allowed is considered in the process of adjusting
the drm_connector->ycbcr_420_allowed, which effectively discards the
formats advertised by the HDMI bridge.

This patchset tries to address the issue by prioritizing
supported_formats over ycbcr_420_allowed.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Changes in v4:
- Moved the adjustment of bridge connector's supported_formats to a
  separate patch (Dmitry)
- Collected Dmitry's R-b tag for patch "drm/connector: hdmi: Validate
  supported_formats matches ycbcr_420_allowed"
- Rebased series onto drm-misc-next-2024-12-19
- Added new patch to extend HDMI connector output format tests (Maxime)
- Link to v3: https://lore.kernel.org/r/20241217-bridge-conn-fmt-prio-v3-0-3ecb3c8fc06f@collabora.com

Changes in v3:
- Simplified the inconsistency handling by overwriting ycbcr_420_allowed
  for HDMI bridges before adding them to the global bridge list
- Added a 2nd patch to check if supported_formats matches
  ycbcr_420_allowed on HDMI connector initialization (Dmitry)
- Link to v2: https://lore.kernel.org/r/20241206-bridge-conn-fmt-prio-v2-1-85c817529b88@collabora.com

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@collabora.com

---
Cristian Ciocaltea (4):
      drm/bridge: Prioritize supported_formats over ycbcr_420_allowed
      drm/bridge-connector: Sync supported_formats with computed ycbcr_420_allowed
      drm/connector: hdmi: Validate supported_formats matches ycbcr_420_allowed
      drm/tests: connector: Add ycbcr_420_allowed tests

 drivers/gpu/drm/display/drm_bridge_connector.c |  8 +++-
 drivers/gpu/drm/drm_bridge.c                   |  4 ++
 drivers/gpu/drm/drm_connector.c                |  3 ++
 drivers/gpu/drm/tests/drm_connector_test.c     | 60 ++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 2 deletions(-)
---
base-commit: 1e93f594285faef57651a0c89f61a7d976db7def
change-id: 20241130-bridge-conn-fmt-prio-c517c1407ed5

[PATCH v4 1/4] drm/bridge: Prioritize supported_formats over ycbcr_420_allowed

[Cristian Ciocaltea]

Bridges having DRM_BRIDGE_OP_HDMI set in their ->ops are supposed to
rely on the ->supported_formats bitmask to advertise the permitted
colorspaces, including HDMI_COLORSPACE_YUV420.

However, a new flag ->ycbcr_420_allowed has been recently introduced,
which brings the necessity to require redundant and potentially
inconsistent information to be provided on HDMI bridges initialization.

Adjust ->ycbcr_420_allowed for HDMI bridges according to
->supported_formats, right before adding them to the global bridge list.
This keeps the initialization process straightforward and unambiguous,
thereby preventing any further confusion.

Fixes: 3ced1c687512 ("drm/display: bridge_connector: handle ycbcr_420_allowed")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_bridge.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c6af46dd02bfa9e15b59e4c460debdd7fd84be44..241a384ebce39b4a3db58c208af27960904fc662 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -207,6 +207,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
 {
 	mutex_init(&bridge->hpd_mutex);
 
+	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
+		bridge->ycbcr_420_allowed = !!(bridge->supported_formats &
+					       BIT(HDMI_COLORSPACE_YUV420));
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);

-- 
2.47.0

[PATCH v4 2/4] drm/bridge-connector: Sync supported_formats with computed ycbcr_420_allowed

[Cristian Ciocaltea]

The case of having an HDMI bridge in the pipeline which advertises
YUV420 capability via its ->supported_formats and a non-HDMI one that
didn't enable ->ycbcr_420_allowed, is incorrectly handled because
supported_formats is passed as is to the helper initializing the HDMI
connector.

Ensure HDMI_COLORSPACE_YUV420 is removed from the bitmask passed to
drmm_connector_hdmi_init() when connector's ->ycbcr_420_allowed flag
ends up not being set.

Fixes: 3ced1c687512 ("drm/display: bridge_connector: handle ycbcr_420_allowed")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 512ced87ea18c74e182a558a686ddd83de891814..7d73bcee90f9a295ac8e6534ac0aa0bc46b1339c 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -473,7 +473,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,
@@ -482,10 +485,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);
 

-- 
2.47.0

[PATCH v4 3/4] drm/connector: hdmi: Validate supported_formats matches ycbcr_420_allowed

[Cristian Ciocaltea]

Ensure HDMI connector initialization fails when the presence of
HDMI_COLORSPACE_YUV420 in the given supported_formats bitmask doesn't
match the value of drm_connector->ycbcr_420_allowed.

Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_connector.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index ae6e71305f3000aff1cfe96b050061a300b9478e..46295176f39f91d69eaad716bb9315002f38e633 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -590,6 +590,9 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
 	if (!supported_formats || !(supported_formats & BIT(HDMI_COLORSPACE_RGB)))
 		return -EINVAL;
 
+	if (connector->ycbcr_420_allowed != !!(supported_formats & BIT(HDMI_COLORSPACE_YUV420)))
+		return -EINVAL;
+
 	if (!(max_bpc == 8 || max_bpc == 10 || max_bpc == 12))
 		return -EINVAL;
 

-- 
2.47.0

[PATCH v4 4/4] drm/tests: connector: Add ycbcr_420_allowed tests

[Cristian Ciocaltea]

Extend HDMI connector output format tests to verify its registration
succeeds only when the presence of YUV420 in the supported formats
matches the state of ycbcr_420_allowed flag.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_connector_test.c | 60 ++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
index 129e813cfd1b0eadc932878458855e15d2a5bceb..22e2d959eb31459f9981fef488228904d67cb6f9 100644
--- a/drivers/gpu/drm/tests/drm_connector_test.c
+++ b/drivers/gpu/drm/tests/drm_connector_test.c
@@ -1095,6 +1095,64 @@ static void drm_test_connector_hdmi_init_formats_no_rgb(struct kunit *test)
 	KUNIT_EXPECT_LT(test, ret, 0);
 }
 
+struct drm_connector_hdmi_init_formats_yuv420_allowed_test {
+	unsigned long supported_formats;
+	bool yuv420_allowed;
+	int expected_result;
+};
+
+#define YUV420_ALLOWED_TEST(_formats, _allowed, _result)			\
+	{									\
+		.supported_formats = BIT(HDMI_COLORSPACE_RGB) | (_formats),	\
+		.yuv420_allowed = _allowed,					\
+		.expected_result = _result,					\
+	}
+
+static const struct drm_connector_hdmi_init_formats_yuv420_allowed_test
+drm_connector_hdmi_init_formats_yuv420_allowed_tests[] = {
+	YUV420_ALLOWED_TEST(BIT(HDMI_COLORSPACE_YUV420), true, 0),
+	YUV420_ALLOWED_TEST(BIT(HDMI_COLORSPACE_YUV420), false, -EINVAL),
+	YUV420_ALLOWED_TEST(BIT(HDMI_COLORSPACE_YUV422), true, -EINVAL),
+	YUV420_ALLOWED_TEST(BIT(HDMI_COLORSPACE_YUV422), false, 0),
+};
+
+static void
+drm_connector_hdmi_init_formats_yuv420_allowed_desc(const struct drm_connector_hdmi_init_formats_yuv420_allowed_test *t,
+						    char *desc)
+{
+	sprintf(desc, "supported_formats=0x%lx yuv420_allowed=%d",
+		t->supported_formats, t->yuv420_allowed);
+}
+
+KUNIT_ARRAY_PARAM(drm_connector_hdmi_init_formats_yuv420_allowed,
+		  drm_connector_hdmi_init_formats_yuv420_allowed_tests,
+		  drm_connector_hdmi_init_formats_yuv420_allowed_desc);
+
+/*
+ * Test that the registration of an HDMI connector succeeds only when
+ * the presence of YUV420 in the supported formats matches the value
+ * of the ycbcr_420_allowed flag.
+ */
+static void drm_test_connector_hdmi_init_formats_yuv420_allowed(struct kunit *test)
+{
+	const struct drm_connector_hdmi_init_formats_yuv420_allowed_test *params;
+	struct drm_connector_init_priv *priv = test->priv;
+	int ret;
+
+	params = test->param_value;
+	priv->connector.ycbcr_420_allowed = params->yuv420_allowed;
+
+	ret = drmm_connector_hdmi_init(&priv->drm, &priv->connector,
+				       "Vendor", "Product",
+				       &dummy_funcs,
+				       &dummy_hdmi_funcs,
+				       DRM_MODE_CONNECTOR_HDMIA,
+				       &priv->ddc,
+				       params->supported_formats,
+				       8);
+	KUNIT_EXPECT_EQ(test, ret, params->expected_result);
+}
+
 /*
  * Test that the registration of an HDMI connector with an HDMI
  * connector type succeeds.
@@ -1186,6 +1244,8 @@ static struct kunit_case drmm_connector_hdmi_init_tests[] = {
 	KUNIT_CASE(drm_test_connector_hdmi_init_bpc_null),
 	KUNIT_CASE(drm_test_connector_hdmi_init_formats_empty),
 	KUNIT_CASE(drm_test_connector_hdmi_init_formats_no_rgb),
+	KUNIT_CASE_PARAM(drm_test_connector_hdmi_init_formats_yuv420_allowed,
+			 drm_connector_hdmi_init_formats_yuv420_allowed_gen_params),
 	KUNIT_CASE(drm_test_connector_hdmi_init_null_ddc),
 	KUNIT_CASE(drm_test_connector_hdmi_init_null_product),
 	KUNIT_CASE(drm_test_connector_hdmi_init_null_vendor),

-- 
2.47.0

Re: [PATCH v4 4/4] drm/tests: connector: Add ycbcr_420_allowed tests

[Dmitry Baryshkov]

On Tue, Dec 24, 2024 at 08:22:44PM +0200, Cristian Ciocaltea wrote:
> Extend HDMI connector output format tests to verify its registration
> succeeds only when the presence of YUV420 in the supported formats
> matches the state of ycbcr_420_allowed flag.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_connector_test.c | 60 ++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v4 2/4] drm/bridge-connector: Sync supported_formats with computed ycbcr_420_allowed

[Dmitry Baryshkov]

On Tue, Dec 24, 2024 at 08:22:42PM +0200, Cristian Ciocaltea wrote:
> The case of having an HDMI bridge in the pipeline which advertises
> YUV420 capability via its ->supported_formats and a non-HDMI one that
> didn't enable ->ycbcr_420_allowed, is incorrectly handled because
> supported_formats is passed as is to the helper initializing the HDMI
> connector.
> 
> Ensure HDMI_COLORSPACE_YUV420 is removed from the bitmask passed to
> drmm_connector_hdmi_init() when connector's ->ycbcr_420_allowed flag
> ends up not being set.
> 
> Fixes: 3ced1c687512 ("drm/display: bridge_connector: handle ycbcr_420_allowed")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/4] drm/bridge: Prioritize supported_formats over ycbcr_420_allowed

[Dmitry Baryshkov]

On Tue, Dec 24, 2024 at 08:22:41PM +0200, Cristian Ciocaltea wrote:
> Bridges having DRM_BRIDGE_OP_HDMI set in their ->ops are supposed to
> rely on the ->supported_formats bitmask to advertise the permitted
> colorspaces, including HDMI_COLORSPACE_YUV420.
> 
> However, a new flag ->ycbcr_420_allowed has been recently introduced,
> which brings the necessity to require redundant and potentially
> inconsistent information to be provided on HDMI bridges initialization.
> 
> Adjust ->ycbcr_420_allowed for HDMI bridges according to
> ->supported_formats, right before adding them to the global bridge list.
> This keeps the initialization process straightforward and unambiguous,
> thereby preventing any further confusion.
> 
> Fixes: 3ced1c687512 ("drm/display: bridge_connector: handle ycbcr_420_allowed")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 4 ++++
>  1 file changed, 4 insertions(+)

I'm still slightly torn between this patch and a simple
drm_WARN_ON(bridge->dev, bridge->ycbcr_420_allowed !=
(bridge->supported_formats & BIT(HDMI_COLORSPACE_YUV420)))

Nevertheless,

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c6af46dd02bfa9e15b59e4c460debdd7fd84be44..241a384ebce39b4a3db58c208af27960904fc662 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -207,6 +207,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  {
>  	mutex_init(&bridge->hpd_mutex);
>  
> +	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> +		bridge->ycbcr_420_allowed = !!(bridge->supported_formats &
> +					       BIT(HDMI_COLORSPACE_YUV420));
> +
>  	mutex_lock(&bridge_lock);
>  	list_add_tail(&bridge->list, &bridge_list);
>  	mutex_unlock(&bridge_lock);
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[Maxime Ripard]

On Tue, 24 Dec 2024 20:22:40 +0200, Cristian Ciocaltea wrote:
> Bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops are expected to
> rely on drm_bridge->supported_formats to advertise the supported
> colorspaces, including HDMI_COLORSPACE_YUV420.
> 
> However, when drm_bridge_connector gets initialised, only
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[Dmitry Baryshkov]

On Mon, Jan 06, 2025 at 02:55:52PM +0000, Maxime Ripard wrote:
> On Tue, 24 Dec 2024 20:22:40 +0200, Cristian Ciocaltea wrote:
> > Bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops are expected to
> > rely on drm_bridge->supported_formats to advertise the supported
> > colorspaces, including HDMI_COLORSPACE_YUV420.
> > 
> > However, when drm_bridge_connector gets initialised, only
> > 
> > [ ... ]
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!

I thought about picking up those patches, but then I deciced to ask you
first. I think first two patches should go to drm-next-fixes? Or just
drm-fixes? While the last two patches look kike a drm-next material.

-- 
With best wishes
Dmitry

Re: [PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[Maxime Ripard]

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

On Tue, Jan 07, 2025 at 01:36:59PM +0200, Dmitry Baryshkov wrote:
> On Mon, Jan 06, 2025 at 02:55:52PM +0000, Maxime Ripard wrote:
> > On Tue, 24 Dec 2024 20:22:40 +0200, Cristian Ciocaltea wrote:
> > > Bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops are expected to
> > > rely on drm_bridge->supported_formats to advertise the supported
> > > colorspaces, including HDMI_COLORSPACE_YUV420.
> > > 
> > > However, when drm_bridge_connector gets initialised, only
> > > 
> > > [ ... ]
> > 
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 
> Thanks!
> 
> I thought about picking up those patches, but then I deciced to ask you
> first. I think first two patches should go to drm-next-fixes? Or just
> drm-fixes? While the last two patches look kike a drm-next material.

Everything can go in drm-misc-fixes imo.

Maxime

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

Re: [PATCH v4 0/4] Fix ycbcr_420_allowed inconsistency for HDMI bridges

[Dmitry Baryshkov]

On Tue, 24 Dec 2024 20:22:40 +0200, Cristian Ciocaltea wrote:
> Bridges with DRM_BRIDGE_OP_HDMI set in drm_bridge->ops are expected to
> rely on drm_bridge->supported_formats to advertise the supported
> colorspaces, including HDMI_COLORSPACE_YUV420.
> 
> However, when drm_bridge_connector gets initialised, only
> drm_bridge->ycbcr_420_allowed is considered in the process of adjusting
> the drm_connector->ycbcr_420_allowed, which effectively discards the
> formats advertised by the HDMI bridge.
> 
> [...]

Applied to drm-misc-fixes, thanks!

[1/4] drm/bridge: Prioritize supported_formats over ycbcr_420_allowed
      commit: d102ac39fbe181a6f0c9e98bc26bd94018657808
[2/4] drm/bridge-connector: Sync supported_formats with computed ycbcr_420_allowed
      commit: 1bd1562d3522f7a846dad795359c31b371e6303b
[3/4] drm/connector: hdmi: Validate supported_formats matches ycbcr_420_allowed
      commit: f2f96619590f944f74f3c2b0b57a6dcc5d13cd9f
[4/4] drm/tests: connector: Add ycbcr_420_allowed tests
      commit: 93801b8c2d18546fdf807c8e3075e6df93960446

Best regards,
-- 
With best wishes
Dmitry