| Message ID | 20241217-bridge-conn-fmt-prio-v3-1-3ecb3c8fc06f@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ycbcr_420_allowed inconsistency for HDMI bridges | expand |
On Tue, Dec 17, 2024 at 12:54:07AM +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. > > Handle the inconsistency by overwriting drm_bridge->ycbcr_420_allowed > for HDMI bridges according to drm_bridge->supported_formats, before > adding them to the global bridge list. > > 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> > --- > drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++-- > drivers/gpu/drm/drm_bridge.c | 4 ++++ > 2 files changed, 10 insertions(+), 2 deletions(-) I think the second patch in the series is enough, it ensures that connector's state is consistent, no matter if the drm_bridge_connector is being used or a normal drm_connector. Nevertheless, I'd leave the final decision to DRM maintainers.
On 12/17/24 1:27 AM, Dmitry Baryshkov wrote: > On Tue, Dec 17, 2024 at 12:54:07AM +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. >> >> Handle the inconsistency by overwriting drm_bridge->ycbcr_420_allowed >> for HDMI bridges according to drm_bridge->supported_formats, before >> adding them to the global bridge list. >> >> 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> >> --- >> drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++-- >> drivers/gpu/drm/drm_bridge.c | 4 ++++ >> 2 files changed, 10 insertions(+), 2 deletions(-) > > I think the second patch in the series is enough, it ensures that > connector's state is consistent, no matter if the drm_bridge_connector > is being used or a normal drm_connector. > > Nevertheless, I'd leave the final decision to DRM maintainers. This patch has 2 parts, maybe I should have put them into separate patches as they kind of relate to distinct problems. The 1st part makes sure that drm_bridge->ycbcr_420_allowed is automatically set when HDMI_COLORSPACE_YUV420 is provided in drm_bridge->supported_formats, to avoid the need of requiring redundant information on HDMI bridges initialization. This implicitly ensures the consistency needed to further allow relying on ->ycbcr_420_allowed internally. While the 1st part could be dropped (assuming redundancy & consistency is not really something we want/need to handle), the 2nd part I think is mandatory, i.e. we must adjust supported_formats before calling drmm_connector_hdmi_init() to ensure the presence of HDMI_COLORSPACE_YUV420 reflects the status of the computed connector->ycbcr_420_allowed, which might end up being different than what the HDMI bridge advertised, i.e. the case of having an HDMI bridge in the pipeline advertising YUV420 via supported_formats and a non-HDMI one that didn't enable ycbcr_420_allowed. Thanks, Cristian
On Tue, Dec 17, 2024 at 02:38:55AM +0200, Cristian Ciocaltea wrote: > On 12/17/24 1:27 AM, Dmitry Baryshkov wrote: > > On Tue, Dec 17, 2024 at 12:54:07AM +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. > >> > >> Handle the inconsistency by overwriting drm_bridge->ycbcr_420_allowed > >> for HDMI bridges according to drm_bridge->supported_formats, before > >> adding them to the global bridge list. > >> > >> 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> > >> --- > >> drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++-- > >> drivers/gpu/drm/drm_bridge.c | 4 ++++ > >> 2 files changed, 10 insertions(+), 2 deletions(-) > > > > I think the second patch in the series is enough, it ensures that > > connector's state is consistent, no matter if the drm_bridge_connector > > is being used or a normal drm_connector. > > > > Nevertheless, I'd leave the final decision to DRM maintainers. > > This patch has 2 parts, maybe I should have put them into separate patches > as they kind of relate to distinct problems. > > The 1st part makes sure that drm_bridge->ycbcr_420_allowed is automatically > set when HDMI_COLORSPACE_YUV420 is provided in drm_bridge->supported_formats, > to avoid the need of requiring redundant information on HDMI bridges > initialization. This implicitly ensures the consistency needed to further > allow relying on ->ycbcr_420_allowed internally. > > While the 1st part could be dropped (assuming redundancy & consistency is > not really something we want/need to handle), the 2nd part I think is > mandatory, i.e. we must adjust supported_formats before calling > drmm_connector_hdmi_init() to ensure the presence of HDMI_COLORSPACE_YUV420 > reflects the status of the computed connector->ycbcr_420_allowed, which > might end up being different than what the HDMI bridge advertised, i.e. the > case of having an HDMI bridge in the pipeline advertising YUV420 via > supported_formats and a non-HDMI one that didn't enable ycbcr_420_allowed. Please split it into two patches. I don't have a strong opinion upon the first one (I'd change it to dev_warn() maybe), while the second one (removing HDMI_COLORSPACE_YUV420 if connector->ycbcr_420_allowed is false) is definitely a correct change. > > Thanks, > Cristian
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c index 320c297008aaa8b6ef5b1f4c71928849b202e8ac..3a5a4f92c979accaa2a8f79ca0f15396dd579429 100644 --- a/drivers/gpu/drm/display/drm_bridge_connector.c +++ b/drivers/gpu/drm/display/drm_bridge_connector.c @@ -459,7 +459,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 +471,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); 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);
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 overwriting drm_bridge->ycbcr_420_allowed for HDMI bridges according to drm_bridge->supported_formats, before adding them to the global bridge list. 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> --- drivers/gpu/drm/display/drm_bridge_connector.c | 8 ++++++-- drivers/gpu/drm/drm_bridge.c | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-)