On 12/20/24 3:05 AM, Dmitry Baryshkov wrote: > 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@xxxxxxxxxxxxx> >>>> --- >>>> 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. Split done in v4: https://lore.kernel.org/lkml/20241224-bridge-conn-fmt-prio-v4-2-a9ceb5671379@xxxxxxxxxxxxx/ Thanks, Cristian