Re: [PATCH v7 11/30] drm/display: bridge_connector: Wire up HDMI 2.0 scrambler callbacks

[Maxime Ripard <mripard@kernel.org>]

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

On Fri, Jun 12, 2026 at 11:42:06PM +0300, Cristian Ciocaltea wrote:
> On 6/12/26 11:52 AM, Maxime Ripard wrote:
> > On Tue, Jun 02, 2026 at 01:44:11AM +0300, Cristian Ciocaltea wrote:
> >> Connect the bridge connector's .scrambler_{enable|disable} callbacks to
> >> the underlying bridge's .hdmi_scrambler_{enable|disable} funcs when
> >> DRM_BRIDGE_OP_HDMI_SCRAMBLER is advertised.
> >>
> >> This completes the bridge connector plumbing so that the SCDC
> >> scrambling helpers can control source-side scrambling through the
> >> bridge chain.
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  drivers/gpu/drm/display/drm_bridge_connector.c | 41 +++++++++++++++++++++++++-
> >>  1 file changed, 40 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> >> index 9d21b1b57b0d..d048ab49eade 100644
> >> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> >> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> >> @@ -555,6 +555,32 @@ static int drm_bridge_connector_write_spd_infoframe(struct drm_connector *connec
> >>  	return bridge->funcs->hdmi_write_spd_infoframe(bridge, buffer, len);
> >>  }
> >>  
> >> +static int drm_bridge_connector_scrambler_enable(struct drm_connector *connector)
> >> +{
> >> +	struct drm_bridge_connector *bridge_connector =
> >> +		to_drm_bridge_connector(connector);
> >> +	struct drm_bridge *bridge;
> >> +
> >> +	bridge = bridge_connector->bridge_hdmi;
> >> +	if (!bridge)
> >> +		return -EINVAL;
> >> +
> >> +	return bridge->funcs->hdmi_scrambler_enable(bridge);
> >> +}
> >> +
> >> +static int drm_bridge_connector_scrambler_disable(struct drm_connector *connector)
> >> +{
> >> +	struct drm_bridge_connector *bridge_connector =
> >> +		to_drm_bridge_connector(connector);
> >> +	struct drm_bridge *bridge;
> >> +
> >> +	bridge = bridge_connector->bridge_hdmi;
> >> +	if (!bridge)
> >> +		return -EINVAL;
> >> +
> >> +	return bridge->funcs->hdmi_scrambler_disable(bridge);
> >> +}
> >> +
> >>  static const struct drm_edid *
> >>  drm_bridge_connector_read_edid(struct drm_connector *connector)
> >>  {
> >> @@ -580,7 +606,7 @@ static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
> >>  		.clear_infoframe = drm_bridge_connector_clear_hdmi_infoframe,
> >>  		.write_infoframe = drm_bridge_connector_write_hdmi_infoframe,
> >>  	},
> >> -	/* audio, hdr_drm and spd are set dynamically during init */
> >> +	/* scrambler, audio, hdr_drm and spd are set dynamically during init */
> >>  };
> >>  
> >>  static const struct drm_connector_infoframe_funcs drm_bridge_connector_hdmi_audio_infoframe = {
> >> @@ -886,6 +912,11 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >>  			     !bridge->funcs->hdmi_clear_spd_infoframe))
> >>  				return ERR_PTR(-EINVAL);
> >>  
> >> +			if (bridge->ops & DRM_BRIDGE_OP_HDMI_SCRAMBLER &&
> >> +			    (!bridge->funcs->hdmi_scrambler_enable ||
> >> +			     !bridge->funcs->hdmi_scrambler_disable))
> >> +				return ERR_PTR(-EINVAL);
> >> +
> >>  			bridge_connector->bridge_hdmi = drm_bridge_get(bridge);
> >>  
> >>  			if (bridge->supported_formats)
> >> @@ -990,6 +1021,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >>  			bridge_connector->hdmi_funcs.spd =
> >>  				drm_bridge_connector_hdmi_spd_infoframe;
> >>  
> >> +		if (bridge_connector->bridge_hdmi->ops & DRM_BRIDGE_OP_HDMI_SCRAMBLER) {
> >> +			bridge_connector->hdmi_funcs.scrambler_enable =
> >> +				drm_bridge_connector_scrambler_enable;
> >> +			bridge_connector->hdmi_funcs.scrambler_disable =
> >> +				drm_bridge_connector_scrambler_disable;
> >> +			connector->hdmi.scrambler_supported = true;
> >> +		}
> >> +
> > 
> > I think we're taking this backwards. The scrambler support isn't
> > optional: either the controller supports HDMI < 2.0, and then it doesn't
> > exist, or it supports >= 2.0 and then it's mandatory.
> > 
> > You're considering it optional here, when it's never actually optional
> > (unlike YUV420 for example)
> > 
> > I still think we should list, somehow, the capabilities of the
> > controller to the helpers, like max tmds rate supported, formats, etc.
> > We've so far put everything as an argument to drmm_connector_hdmi_init
> > but it becomes a bit overloaded, and I wonder if introducing a callback
> > wouldn't solve this, kind of like what we have for planes and formats.
> 
> 
> What about introducing something like:
> 
> struct drm_connector_hdmi_caps {
>     ...
>     unsigned int supported_formats;
>     enum hdmi_version supported_hdmi_ver;
> };
> 
> struct drm_connector_hdmi_funcs {
>     ...
>     int (*get_caps)(struct drm_connector *connector,
>                     struct drm_connector_hdmi_caps *caps);
>     ...
> };
> 
> int drmm_connector_hdmi_init(struct drm_device *dev, ...)
> {
>     ...
>     if (hdmi_funcs->get_caps) {
>         struct drm_connector_hdmi_caps caps = { };
> 
>         ret = hdmi_funcs->get_caps(connector, &caps);
>         if (ret)
>             return ret;
> 
>         connector->hdmi.supported_formats  = caps.supported_formats;
>         ...
>         if (caps.supported_hdmi_ver > HDMI_2_0)
>             connector->hdmi.frl_supported = true;
>         else if (caps.supported_hdmi_ver == HDMI_2_0)
>             connector->hdmi.scrambler_supported = true;
>     }
>     ...
> }

That's what I initially had in mind, but it feels a bit over-the-top
when looking at it. I think I'd really like something that drivers
cannot forget about, screw up and/or mess with, so for example report
HDMI 2.1 but force disable FRL support, or report HDMI 1.4 but support
YUV420.

Adding more arguments to drm_connector_hdmi_init could work I guess, but
it won't scale to everything we need. Expecting the callers to fill
drm_connector_hdmi won't work either. So I somewhat think a get_caps
like we discussed is the less bad solution, but I'm definitely open to
suggestions.

> Not sure if max_tmds_char_rate should be listed as a capability, as we already
> have the .tmds_char_rate_valid() callback.

I guess it would make sense to move it there and consolidate
atomic_check / mode_valid checks, but I don't think it should be a
prerequisite for this patch either.

Maxime

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