drivers/gpu/drm/display/drm_bridge_connector.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
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.
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@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
---
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-c517c1407ed5On 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@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
> ---
> 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
Hi Dmitry, On 12/10/24 3:12 AM, Dmitry Baryshkov wrote: > 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. I added a 2nd patch in v3 [1] to perform this verification on HDMI connector init, but I think we still need a proper handling of the inconsistency at bridge[-connector] level. I simplified a bit the initial approach, hopefully that would bring us closer to an acceptable solution. Thanks, Cristian [1] https://lore.kernel.org/lkml/20241217-bridge-conn-fmt-prio-v3-0-3ecb3c8fc06f@collabora.com/
On Tue, Dec 17, 2024 at 01:17:48AM +0200, Cristian Ciocaltea wrote: > Hi Dmitry, > > On 12/10/24 3:12 AM, Dmitry Baryshkov wrote: > > 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. > > I added a 2nd patch in v3 [1] to perform this verification on HDMI > connector init, but I think we still need a proper handling of the > inconsistency at bridge[-connector] level. I simplified a bit the initial > approach, hopefully that would bring us closer to an acceptable solution. I'm sorry, I first commented on the new patchseries. I think that the second patch is enough. > > Thanks, > Cristian > > [1] https://lore.kernel.org/lkml/20241217-bridge-conn-fmt-prio-v3-0-3ecb3c8fc06f@collabora.com/ -- With best wishes Dmitry
On 12/17/24 1:32 AM, Dmitry Baryshkov wrote: > On Tue, Dec 17, 2024 at 01:17:48AM +0200, Cristian Ciocaltea wrote: >> Hi Dmitry, >> >> On 12/10/24 3:12 AM, Dmitry Baryshkov wrote: >>> 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. >> >> I added a 2nd patch in v3 [1] to perform this verification on HDMI >> connector init, but I think we still need a proper handling of the >> inconsistency at bridge[-connector] level. I simplified a bit the initial >> approach, hopefully that would bring us closer to an acceptable solution. > > I'm sorry, I first commented on the new patchseries. I think that the > second patch is enough. No worries, thanks for your quick review! Let's move the discussion on the new series!
© 2016 - 2024 Red Hat, Inc.