Re: [PATCH v8 04/39] drm/connector: Add HDMI 2.0 scrambler infrastructure

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

On 7/3/26 5:34 PM, Dmitry Baryshkov wrote:
> On Thu, Jul 02, 2026 at 05:46:17PM +0300, Cristian Ciocaltea wrote:
>> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
>>
>> - A scrambler_supported flag to indicate whether the source supports the
>>   scrambling capability, in which case the newly introduced
>>   .scrambler_{enable|disable}() callbacks in drm_connector_hdmi_funcs
>>   are mandatory
> 
> Do we need a flag? What would it mean if the flag is set, but the
> callbacks are not? Can we drop the flag and use the presence of the
> callbacks as a way to identify that scrambler is enabled?

The flag is intended to be set only within drmm_connector_hdmi_init_with_caps()
when drivers advertise HDMI 2.x capability, in which case it also ensures the
callbacks are provided.  

We could drop the flag and instead have the init helper clear the callbacks if
they were provided for HDMI 1.x.  This might slightly reduce code readability,
as it relies on checking the presence of individual callbacks - especially since
we plan to extend this further with HDMI 2.1 support, providing four or five
additional FRL-specific callbacks.  

That said, I don’t have a strong opinion on this.

>> - A scrambler_needed flag to be managed by the hdmi state helpers based
>>   on the negotiated TMDS character rate and the source/sink scrambling
>>   capabilities
>> - A scrambler_enabled flag to track whether scrambling is currently
>>   active
>> - A delayed work item (scdc_work) with an associated callback (scdc_cb)
>>   to monitor sink-side scrambling status and retry the setup if the sink
>>   resets it
>>
>> These are intended to be used by SCDC scrambling helpers to coordinate
>> scrambling setup and teardown between the source driver and the DRM
>> core.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 26 ++++++++++++--
>>  include/drm/drm_connector.h     | 77 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 100 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index c7ce6b7bd8b0..deecfc582f09 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -220,6 +220,19 @@ void drm_connector_free_work_fn(struct work_struct *work)
>>  	}
>>  }
>>  
>> +static void drm_connector_hdmi_scdc_work(struct work_struct *work)
>> +{
>> +	struct drm_connector *connector;
>> +	struct drm_connector_hdmi *hdmi;
>> +
>> +	hdmi = container_of(to_delayed_work(work), struct drm_connector_hdmi,
>> +			    scdc_work);
>> +	connector = container_of(hdmi, struct drm_connector, hdmi);
>> +
>> +	if (hdmi->scdc_cb)
>> +		hdmi->scdc_cb(connector);
>> +}
>> +
>>  static int drm_connector_init_only(struct drm_device *dev,
>>  				   struct drm_connector *connector,
>>  				   const struct drm_connector_funcs *funcs,
>> @@ -285,6 +298,7 @@ static int drm_connector_init_only(struct drm_device *dev,
>>  	mutex_init(&connector->edid_override_mutex);
>>  	mutex_init(&connector->hdmi.infoframes.lock);
>>  	mutex_init(&connector->hdmi_audio.lock);
>> +	INIT_DELAYED_WORK(&connector->hdmi.scdc_work, drm_connector_hdmi_scdc_work);
>>  	connector->edid_blob_ptr = NULL;
>>  	connector->epoch_counter = 0;
>>  	connector->tile_blob_ptr = NULL;
>> @@ -624,12 +638,18 @@ int drmm_connector_hdmi_init_with_caps(struct drm_device *dev,
>>  	 * inferred limit with the actual controller capability. A value of
>>  	 * zero keeps the default limit inferred from supported_hdmi_ver.
>>  	 */
> 
> Here we need to check that the driver doesn't set only one callback of
> the pair.

Is anything missing from the check below?  I chose to ignore the presence of the
callbacks in HDMI 1.x cases.

> 
>> -	if (caps->supported_hdmi_ver >= HDMI_VERSION_2_0)
>> +	if (caps->supported_hdmi_ver >= HDMI_VERSION_2_0) {
>> +		if (!hdmi_funcs->scrambler_enable ||
>> +		    !hdmi_funcs->scrambler_disable)
>> +			return -EINVAL;
>> +
>> +		connector->hdmi.scrambler_supported = true;
>>  		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
>> -	else if (caps->supported_hdmi_ver >= HDMI_VERSION_1_3)
>> +	} else if (caps->supported_hdmi_ver >= HDMI_VERSION_1_3) {
>>  		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>> -	else if (caps->supported_hdmi_ver >= HDMI_VERSION_1_0)
>> +	} else if (caps->supported_hdmi_ver >= HDMI_VERSION_1_0) {
>>  		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>> +	}
>>  
>>  	if (caps->max_tmds_char_rate) {
>>  		if (caps->max_tmds_char_rate > connector->hdmi.max_tmds_char_rate)
>> +
>> +	/** @scdc_cb: Callback to be invoked as part of @scdc_work.
>> +	 *
>> +	 * Currently used to monitor sink-side scrambling status and retry
>> +	 * setup if the sink resets it.
>> +	 *
>> +	 * This is assigned by the framework when making use of
>> +	 * drm_connector_hdmi_enable_scrambling() helper.
> 
> No such helper. 

Right, I'll drop the reference to the helper name, as that's added later in the
series.

> Also, please explain in the commit message, why do we
> need to keep it configurable. Why the default is not enough?

We will need a separate callback to support HDMI 2.1, i.e. for link training
management.  I’ll update the commit message accordingly.

Thanks,
Cristian