Re: [PATCH 5/8] drm: Pass supported color formats straight onto drm_bridge

[Marius Vlad <marius.vlad@collabora.com>]

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

On Thu, Sep 11, 2025 at 04:55:29PM +0300, Dmitry Baryshkov wrote:
> On Thu, Sep 11, 2025 at 04:07:36PM +0300, Marius Vlad wrote:
> > Initialize drm_brige with advertised colors formats straight on.
> > 
> > Drivers that make use of DRM helpers would check the
> > drm_brige::supported_formats bit-field list and refuse to use the color
> > format passed. Drivers that make use of drm_bridge can pass the
> > supported color formats in the bridge as well as supported color format
> > for the DRM color format property.
> 
> Your commit message doesn't match patch contents. You are pushing format
> selection to the instance creating drm_bridge_connector, which
> frequently has no idea about the other end of the chain - the bridges
> which actually send pixel data to the monitor.

None of these changes in this patch actually perform a functional
change, it will just explicitly expose the fact that BIT(HDMI_COLORSPACE_RGB)
was embedded in drm_bridge_connector_init(). 

Commit description is a bit forthcoming explaining the rationale behind
the change. This actually allows the ability to pass a list of supported
formats the bridge itself, similar how do we do it with the connector.

If any of these are wrong, they were prior to me touching them.

> 
> We have drm_bridge::ycbcr_420_allowed with clearly defined meaning. I
> think it would be wise to start from that and to describe why such a
> field doesn't fulfill your needs.
Alright, I'll be looking into this. 
> 
> > 
> > This includes a fallback to RGB when Auto has been selected.
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > ---
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c      | 2 +-
> >  drivers/gpu/drm/bridge/ite-it6263.c               | 2 +-
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c             | 3 ++-
> >  drivers/gpu/drm/display/drm_bridge_connector.c    | 4 ++--
> >  drivers/gpu/drm/imx/dcss/dcss-kms.c               | 2 +-
> >  drivers/gpu/drm/mediatek/mtk_dpi.c                | 2 +-
> >  drivers/gpu/drm/mediatek/mtk_dsi.c                | 2 +-
> >  drivers/gpu/drm/meson/meson_encoder_cvbs.c        | 3 ++-
> >  drivers/gpu/drm/meson/meson_encoder_hdmi.c        | 4 ++--
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c          | 2 +-
> >  drivers/gpu/drm/msm/dp/dp_drm.c                   | 3 ++-
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c             | 2 +-
> >  drivers/gpu/drm/msm/hdmi/hdmi.c                   | 2 +-
> >  drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c | 2 +-
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  | 2 +-
> >  drivers/gpu/drm/rockchip/cdn-dp-core.c            | 2 +-
> >  drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 2 +-
> >  drivers/gpu/drm/rockchip/rockchip_lvds.c          | 2 +-
> >  drivers/gpu/drm/tegra/hdmi.c                      | 2 +-
> >  drivers/gpu/drm/tegra/rgb.c                       | 2 +-
> >  drivers/gpu/drm/tidss/tidss_encoder.c             | 2 +-
> >  include/drm/drm_bridge_connector.h                | 3 ++-
> >  22 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 9a461ab2f32f..8d5299849be6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -368,7 +368,8 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
> >  {
> >  	struct drm_connector *connector = NULL;
> >  
> > -	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
> > +	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder,
> > +					      BIT(HDMI_COLORSPACE_RGB));
> 
> Just to point out: this is wrong.
Yeah, I understand why you're saying that just that I haven't really
modified these.
> 
> >  	if (IS_ERR(connector))
> >  		return connector;
> >  
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index ca400924d4ee..4b87f4f78d38 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -479,7 +479,7 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
> >  	if (ret)
> >  		return ret;
> >  
> > -	connector = drm_bridge_connector_init(dev, encoder);
> > +	connector = drm_bridge_connector_init(dev, encoder, BIT(HDMI_COLORSPACE_RGB));
> 
> And this totally depends on the bridge chain. If we have a DSI-to-HDMI
> bridge somewhere in the middle, we are able to output YUV data to the
> HDMI connector.
That's actually the usecase for this patch: to allow passing other color
formats, but this patch is a transitory patch to further expose the fact
drm_bridge_connector_init was embedding BIT(HDMI_COLORSPACE_RGB) for the
format. See rockchip implementation for this bit, the last patch in this
series.
> 
> >  	if (IS_ERR(connector)) {
> >  		DRM_ERROR("Unable to create bridge connector\n");
> >  		return PTR_ERR(connector);
> 
> -- 
> With best wishes
> Dmitry

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