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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

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;
    }
    ...
}

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

Cristian