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: 4179 bytes --]

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.

Maxime

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