[v10,07/69] drm/connector: Add HDMI 2.0 scrambler infrastructure

Add the connector-level infrastructure to support HDMI 2.0 scrambling:

- A drm_connector_hdmi_scrambler_supported() helper to report whether
  the source supports the scrambling capability, based on the presence
  of the newly introduced .scrambler_{enable|disable}() callbacks in
  drm_connector_hdmi_funcs are mandatory
- 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) to monitor sink-side scrambling status
  and retry the setup if the sink resets it
- A scdc_work_initialized flag to support lazy initialization of the
  work item on the first scrambling enable and guard the teardown paths

These are intended to be used by SCDC scrambling helpers to coordinate
scrambling setup and teardown between the source driver and the DRM
core.

Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_connector.c | 31 ++++++++++++---
 include/drm/drm_connector.h     | 83 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 5 deletions(-)
  

On Fri, Jul 31, 2026 at 07:19:14PM +0300, Cristian Ciocaltea wrote:
> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
> 
> - A drm_connector_hdmi_scrambler_supported() helper to report whether
>   the source supports the scrambling capability, based on the presence
>   of the newly introduced .scrambler_{enable|disable}() callbacks in
>   drm_connector_hdmi_funcs are mandatory
> - 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) to monitor sink-side scrambling status
>   and retry the setup if the sink resets it
> - A scdc_work_initialized flag to support lazy initialization of the
>   work item on the first scrambling enable and guard the teardown paths
> 
> These are intended to be used by SCDC scrambling helpers to coordinate
> scrambling setup and teardown between the source driver and the DRM
> core.
> 
> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 31 ++++++++++++---
>  include/drm/drm_connector.h     | 83 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 109 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4721cdeafc84..a18410faf040 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -622,12 +622,29 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  	 * default with the actual controller capability. A value of zero keeps
>  	 * the limit inferred from supported_hdmi_ver.
>  	 */
> -	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
> +	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
> +		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
> +			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
> +			return -EINVAL;
> +		}
> +
>  		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> -		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> -		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> +	} else {
> +		/*
> +		 * Scrambler callbacks are only valid for connectors advertising
> +		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
> +		 * relies on their presence to report scrambling support.
> +		 */
> +		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
> +			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
> +			return -EINVAL;
> +		}
> +
> +		if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> +			connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> +		else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> +			connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> +	}

I'd put it into a separate test (possibly earlier). Merging both the
tmds rate default and the scrambler callbacks check makes it messier
than it would be if we had two separate tests.

>  	if (hdmi_funcs->supported_tmds_char_rate) {
>  		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
> @@ -635,6 +652,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  				connector->hdmi.max_tmds_char_rate);
>  			return -EINVAL;
>  		}
> +
>  		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
>  	}
>  
> @@ -918,6 +936,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		    DRM_CONNECTOR_REGISTERED))
>  		drm_connector_unregister(connector);
>  
> +	if (connector->hdmi.scdc_work_initialized)
> +		cancel_delayed_work_sync(&connector->hdmi.scdc_work);
> +
>  	platform_device_unregister(connector->hdmi_audio.codec_pdev);
>  
>  	if (connector->privacy_screen) {
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index a6de3e63b462..89a140d6f064 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -28,6 +28,7 @@
>  #include <linux/ctype.h>
>  #include <linux/hdmi.h>
>  #include <linux/notifier.h>
> +#include <linux/workqueue.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_util.h>
>  #include <drm/drm_property.h>
> @@ -1131,6 +1132,17 @@ struct drm_connector_hdmi_state {
>  	 * @tmds_char_rate: TMDS Character Rate, in Hz.
>  	 */
>  	unsigned long long tmds_char_rate;
> +
> +	/**
> +	 * @scrambler_needed: Whether HDMI 2.0 SCDC scrambling is required
> +	 * for the negotiated mode/bpc/format.
> +	 *
> +	 * Computed by drm_atomic_helper_connector_hdmi_check() according to
> +	 * the HDMI 2.0 specification: scrambling is mandatory above a 340 MHz
> +	 * TMDS character rate. Optional scrambling at lower rates is
> +	 * deliberately not requested by the helper.
> +	 */
> +	bool scrambler_needed;
>  };
>  
>  /**
> @@ -1481,6 +1493,36 @@ struct drm_connector_hdmi_funcs {
>  	 */
>  	const struct drm_edid *(*read_edid)(struct drm_connector *connector);
>  
> +	/**
> +	 * @scrambler_enable:
> +	 *
> +	 * The callback is invoked via @drm_connector_hdmi_enable_scrambling
> +	 * during commit to setup SCDC scrambling and high TMDS clock ratio on
> +	 * the source side.
> +	 *
> +	 * The @scrambler_enable callback is mandatory if HDMI 2.0 is to be
> +	 * supported.
> +	 *
> +	 * Returns:
> +	 * 0 on success, a negative error code otherwise
> +	 */
> +	int (*scrambler_enable)(struct drm_connector *connector);
> +
> +	/**
> +	 * @scrambler_disable:
> +	 *
> +	 * The callback is invoked via @drm_connector_hdmi_disable_scrambling
> +	 * during commit to tear down SCDC scrambling and high TMDS clock ratio
> +	 * on the source side.
> +	 *
> +	 * The @scrambler_disable callback is mandatory if HDMI 2.0 is to be
> +	 * supported.
> +	 *
> +	 * Returns:
> +	 * 0 on success, a negative error code otherwise
> +	 */
> +	int (*scrambler_disable)(struct drm_connector *connector);
> +
>  	/**
>  	 * @avi:
>  	 *
> @@ -2111,6 +2153,25 @@ struct drm_connector_hdmi {
>  	 */
>  	unsigned long long max_tmds_char_rate;
>  
> +	/**
> +	 * @scrambler_enabled: Tracks whether HDMI 2.0 scrambler is currently enabled.
> +	 */
> +	bool scrambler_enabled;
> +
> +	/**
> +	 * @scdc_work: Work item currently used to monitor sink-side scrambling
> +	 * status and retry setup if the sink resets it.
> +	 */
> +	struct delayed_work scdc_work;
> +
> +	/**
> +	 * @scdc_work_initialized: Tracks whether @scdc_work has been set up via
> +	 * INIT_DELAYED_WORK(). The work item is initialized lazily on the first
> +	 * scrambling enable, so this guards the teardown paths against touching
> +	 * an uninitialized work item.
> +	 */
> +	bool scdc_work_initialized;
> +

Why should we track whether it's initialized or not? I'd always
initialize it, but only ever schedule something if we're using the
scrambler.

Maxime
  

On 8/19/26 1:12 PM, Maxime Ripard wrote:
> On Fri, Jul 31, 2026 at 07:19:14PM +0300, Cristian Ciocaltea wrote:
>> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
>>
>> - A drm_connector_hdmi_scrambler_supported() helper to report whether
>>   the source supports the scrambling capability, based on the presence
>>   of the newly introduced .scrambler_{enable|disable}() callbacks in
>>   drm_connector_hdmi_funcs are mandatory
>> - 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) to monitor sink-side scrambling status
>>   and retry the setup if the sink resets it
>> - A scdc_work_initialized flag to support lazy initialization of the
>>   work item on the first scrambling enable and guard the teardown paths
>>
>> These are intended to be used by SCDC scrambling helpers to coordinate
>> scrambling setup and teardown between the source driver and the DRM
>> core.
>>
>> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
>> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 31 ++++++++++++---
>>  include/drm/drm_connector.h     | 83 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 109 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 4721cdeafc84..a18410faf040 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -622,12 +622,29 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>>  	 * default with the actual controller capability. A value of zero keeps
>>  	 * the limit inferred from supported_hdmi_ver.
>>  	 */
>> -	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
>> +	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
>> +		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
>> +			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
>> +			return -EINVAL;
>> +		}
>> +
>>  		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
>> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
>> -		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
>> -		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>> +	} else {
>> +		/*
>> +		 * Scrambler callbacks are only valid for connectors advertising
>> +		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
>> +		 * relies on their presence to report scrambling support.
>> +		 */
>> +		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
>> +			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
>> +			connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>> +		else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
>> +			connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>> +	}
> 
> I'd put it into a separate test (possibly earlier). Merging both the
> tmds rate default and the scrambler callbacks check makes it messier
> than it would be if we had two separate tests.

Ack. How about the following?

	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;

	if (hdmi_funcs->supported_tmds_char_rate) {
		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
			drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
				connector->hdmi.max_tmds_char_rate);
			return -EINVAL;
		}

		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
	}

	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
			return -EINVAL;
		}
	} else {
		/*
		 * Scrambler callbacks are only valid for connectors advertising
		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
		 * relies on their presence to report scrambling support.
		 */
		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
			return -EINVAL;
		}
	}

>>  	if (hdmi_funcs->supported_tmds_char_rate) {
>>  		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
>> @@ -635,6 +652,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>>  				connector->hdmi.max_tmds_char_rate);
>>  			return -EINVAL;
>>  		}
>> +
>>  		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
>>  	}

[...]

>> +	/**
>> +	 * @scdc_work: Work item currently used to monitor sink-side scrambling
>> +	 * status and retry setup if the sink resets it.
>> +	 */
>> +	struct delayed_work scdc_work;
>> +
>> +	/**
>> +	 * @scdc_work_initialized: Tracks whether @scdc_work has been set up via
>> +	 * INIT_DELAYED_WORK(). The work item is initialized lazily on the first
>> +	 * scrambling enable, so this guards the teardown paths against touching
>> +	 * an uninitialized work item.
>> +	 */
>> +	bool scdc_work_initialized;
>> +
> 
> Why should we track whether it's initialized or not? I'd always
> initialize it, but only ever schedule something if we're using the
> scrambler.

Having this initialized in the connector would lead to a module dependency
cycle.

Currently the work function lives in drm_hdmi_helper.c, which is built into
drm_display_helper module:

static void drm_connector_hdmi_scdc_work(struct work_struct *work)
{
	[...]
	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
	    !drm_scdc_get_scrambling_status(connector))
		drm_connector_hdmi_try_scrambling_setup(connector);
	[...]
}

int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
					 const struct drm_connector_state *conn_state)
{

	[...]
	if (!hdmi->scdc_work_initialized) {
		INIT_DELAYED_WORK(&hdmi->scdc_work,
				  drm_connector_hdmi_scdc_work);
		hdmi->scdc_work_initialized = true;
	}
	[...]
}

If we move INIT_DELAYED_WORK() into the connector (i.e. in drm.ko), the work
function has to be reachable from there.  The following attempts to accomplish
that would fail:

- Keep the work function in drm_hdmi_helper.c and export it from
  drm_display_helper.

- Move the work function into drm_connector.c and export 
  drm_connector_hdmi_try_scrambling_setup(), or a wrapper function, from 
  drm_display_helper.

Either way drm module ends up depending on drm_display_helper, which already 
depends on drm:

  depmod: ERROR: Cycle detected: drm_display_helper -> drm -> drm_display_helper

My previous approach provided the work function in the connector, and a callback 
set by the scrambling helper:

/* Part of drm module */

struct drm_connector_hdmi {
	[...]
	void (*scdc_cb)(struct drm_connector *connector);
	[...]
}

static void drm_connector_hdmi_scdc_work(struct work_struct *work)
{
	[...]
	if (hdmi->scdc_cb)
		hdmi->scdc_cb(connector);
	[...]
}

/* Part of drm_display_helper */

int drm_connector_hdmi_enable_scrambling()
{
	[...]
	hdmi->scdc_cb = drm_scdc_monitor_scrambler;
	[...]
}

Since it didn't get positive feedback, I ended up with lazy initialization
instead.  I think it's the better of the two, as it keeps all the implementation
logic inside the helper module rather than splitting it across drm and
drm_display_helper.

Thanks,
Cristian
  

On Wed, Aug 19, 2026 at 10:33:04PM +0300, Cristian Ciocaltea wrote:
> On 8/19/26 1:12 PM, Maxime Ripard wrote:
> > On Fri, Jul 31, 2026 at 07:19:14PM +0300, Cristian Ciocaltea wrote:
> >> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
> >>
> >> - A drm_connector_hdmi_scrambler_supported() helper to report whether
> >>   the source supports the scrambling capability, based on the presence
> >>   of the newly introduced .scrambler_{enable|disable}() callbacks in
> >>   drm_connector_hdmi_funcs are mandatory
> >> - 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) to monitor sink-side scrambling status
> >>   and retry the setup if the sink resets it
> >> - A scdc_work_initialized flag to support lazy initialization of the
> >>   work item on the first scrambling enable and guard the teardown paths
> >>
> >> These are intended to be used by SCDC scrambling helpers to coordinate
> >> scrambling setup and teardown between the source driver and the DRM
> >> core.
> >>
> >> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
> >> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  drivers/gpu/drm/drm_connector.c | 31 ++++++++++++---
> >>  include/drm/drm_connector.h     | 83 +++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 109 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index 4721cdeafc84..a18410faf040 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -622,12 +622,29 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >>  	 * default with the actual controller capability. A value of zero keeps
> >>  	 * the limit inferred from supported_hdmi_ver.
> >>  	 */
> >> -	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
> >> +	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
> >> +		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
> >> +			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >>  		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> >> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> >> -		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> >> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> >> -		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> >> +	} else {
> >> +		/*
> >> +		 * Scrambler callbacks are only valid for connectors advertising
> >> +		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
> >> +		 * relies on their presence to report scrambling support.
> >> +		 */
> >> +		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
> >> +			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> >> +			connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> >> +		else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> >> +			connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> >> +	}
> > 
> > I'd put it into a separate test (possibly earlier). Merging both the
> > tmds rate default and the scrambler callbacks check makes it messier
> > than it would be if we had two separate tests.
> 
> Ack. How about the following?
> 
> 	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
> 		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> 	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> 		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> 	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> 		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
> 
> 	if (hdmi_funcs->supported_tmds_char_rate) {
> 		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
> 			drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
> 				connector->hdmi.max_tmds_char_rate);
> 			return -EINVAL;
> 		}
> 
> 		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
> 	}
> 
> 	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
> 		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
> 			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
> 			return -EINVAL;
> 		}
> 	} else {
> 		/*
> 		 * Scrambler callbacks are only valid for connectors advertising
> 		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
> 		 * relies on their presence to report scrambling support.
> 		 */
> 		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
> 			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
> 			return -EINVAL;
> 		}
> 	}

I don't think we need the else clause at all. It's not valid, but it's
also not creating any issue.

I'd move that second check earlier together with the infoframe callbacks
checks and so on too.

> 
> >>  	if (hdmi_funcs->supported_tmds_char_rate) {
> >>  		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
> >> @@ -635,6 +652,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >>  				connector->hdmi.max_tmds_char_rate);
> >>  			return -EINVAL;
> >>  		}
> >> +
> >>  		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
> >>  	}
> 
> [...]
> 
> >> +	/**
> >> +	 * @scdc_work: Work item currently used to monitor sink-side scrambling
> >> +	 * status and retry setup if the sink resets it.
> >> +	 */
> >> +	struct delayed_work scdc_work;
> >> +
> >> +	/**
> >> +	 * @scdc_work_initialized: Tracks whether @scdc_work has been set up via
> >> +	 * INIT_DELAYED_WORK(). The work item is initialized lazily on the first
> >> +	 * scrambling enable, so this guards the teardown paths against touching
> >> +	 * an uninitialized work item.
> >> +	 */
> >> +	bool scdc_work_initialized;
> >> +
> > 
> > Why should we track whether it's initialized or not? I'd always
> > initialize it, but only ever schedule something if we're using the
> > scrambler.
> 
> Having this initialized in the connector would lead to a module dependency
> cycle.
> 
> Currently the work function lives in drm_hdmi_helper.c, which is built into
> drm_display_helper module:
> 
> static void drm_connector_hdmi_scdc_work(struct work_struct *work)
> {
> 	[...]
> 	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> 	    !drm_scdc_get_scrambling_status(connector))
> 		drm_connector_hdmi_try_scrambling_setup(connector);
> 	[...]
> }
> 
> int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
> 					 const struct drm_connector_state *conn_state)
> {
> 
> 	[...]
> 	if (!hdmi->scdc_work_initialized) {
> 		INIT_DELAYED_WORK(&hdmi->scdc_work,
> 				  drm_connector_hdmi_scdc_work);
> 		hdmi->scdc_work_initialized = true;
> 	}
> 	[...]
> }
> 
> If we move INIT_DELAYED_WORK() into the connector (i.e. in drm.ko), the work
> function has to be reachable from there.  The following attempts to accomplish
> that would fail:
>
> - Keep the work function in drm_hdmi_helper.c and export it from
>   drm_display_helper.
> 
> - Move the work function into drm_connector.c and export 
>   drm_connector_hdmi_try_scrambling_setup(), or a wrapper function, from 
>   drm_display_helper.

An alternative could be to move drm_connector_hdmi_init to
drm_hdmi_helper.c, no?

But yeah, if we can't let's keep it like that

Maxime
  

On 8/20/26 7:56 PM, Maxime Ripard wrote:
> On Wed, Aug 19, 2026 at 10:33:04PM +0300, Cristian Ciocaltea wrote:
>> On 8/19/26 1:12 PM, Maxime Ripard wrote:
>>> On Fri, Jul 31, 2026 at 07:19:14PM +0300, Cristian Ciocaltea wrote:
>>>> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
>>>>
>>>> - A drm_connector_hdmi_scrambler_supported() helper to report whether
>>>>   the source supports the scrambling capability, based on the presence
>>>>   of the newly introduced .scrambler_{enable|disable}() callbacks in
>>>>   drm_connector_hdmi_funcs are mandatory
>>>> - 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) to monitor sink-side scrambling status
>>>>   and retry the setup if the sink resets it
>>>> - A scdc_work_initialized flag to support lazy initialization of the
>>>>   work item on the first scrambling enable and guard the teardown paths
>>>>
>>>> These are intended to be used by SCDC scrambling helpers to coordinate
>>>> scrambling setup and teardown between the source driver and the DRM
>>>> core.
>>>>
>>>> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
>>>> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_connector.c | 31 ++++++++++++---
>>>>  include/drm/drm_connector.h     | 83 +++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 109 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>> index 4721cdeafc84..a18410faf040 100644
>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>> @@ -622,12 +622,29 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>>>>  	 * default with the actual controller capability. A value of zero keeps
>>>>  	 * the limit inferred from supported_hdmi_ver.
>>>>  	 */
>>>> -	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
>>>> +	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
>>>> +		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
>>>> +			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>>  		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
>>>> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
>>>> -		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>>>> -	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
>>>> -		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>>>> +	} else {
>>>> +		/*
>>>> +		 * Scrambler callbacks are only valid for connectors advertising
>>>> +		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
>>>> +		 * relies on their presence to report scrambling support.
>>>> +		 */
>>>> +		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
>>>> +			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
>>>> +			connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>>>> +		else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
>>>> +			connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>>>> +	}
>>>
>>> I'd put it into a separate test (possibly earlier). Merging both the
>>> tmds rate default and the scrambler callbacks check makes it messier
>>> than it would be if we had two separate tests.
>>
>> Ack. How about the following?
>>
>> 	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
>> 		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
>> 	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
>> 		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
>> 	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
>> 		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;
>>
>> 	if (hdmi_funcs->supported_tmds_char_rate) {
>> 		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
>> 			drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
>> 				connector->hdmi.max_tmds_char_rate);
>> 			return -EINVAL;
>> 		}
>>
>> 		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
>> 	}
>>
>> 	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0) {
>> 		if (!hdmi_funcs->scrambler_enable || !hdmi_funcs->scrambler_disable) {
>> 			drm_err(dev, "Scrambler callbacks missing for HDMI 2.x\n");
>> 			return -EINVAL;
>> 		}
>> 	} else {
>> 		/*
>> 		 * Scrambler callbacks are only valid for connectors advertising
>> 		 * HDMI 2.0 capability. drm_connector_hdmi_scrambler_supported()
>> 		 * relies on their presence to report scrambling support.
>> 		 */
>> 		if (hdmi_funcs->scrambler_enable || hdmi_funcs->scrambler_disable) {
>> 			drm_err(dev, "Scrambler callbacks unexpected for HDMI 1.x\n");
>> 			return -EINVAL;
>> 		}
>> 	}
> 
> I don't think we need the else clause at all. It's not valid, but it's
> also not creating any issue.

As discussed a while ago, we used to have a scrambler_supported flag, inferred
from supported_hdmi_ver, which allowed helpers to verify the capability when
needed.  That flag has now been removed and replaced by
drm_connector_hdmi_scrambler_supported(), which relies exclusively on the
presence of the scrambler callbacks to report whether the capability is
supported. 

If we don't ensure that these callbacks are *not* set for HDMI 1.x cases, one
could set supported_hdmi_ver to HDMI_VERSION_1_4, for example, while still
providing the scrambler_{enable,disable} funcs.  This would lead to an
inconsistency between the maximum TMDS character rate inferred from
supported_hdmi_ver and the capability reported by
drm_connector_hdmi_scrambler_supported().

> I'd move that second check earlier together with the infoframe callbacks
> checks and so on too.

Ack.

>>>>  	if (hdmi_funcs->supported_tmds_char_rate) {
>>>>  		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
>>>> @@ -635,6 +652,7 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>>>>  				connector->hdmi.max_tmds_char_rate);
>>>>  			return -EINVAL;
>>>>  		}
>>>> +
>>>>  		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
>>>>  	}
>>
>> [...]
>>
>>>> +	/**
>>>> +	 * @scdc_work: Work item currently used to monitor sink-side scrambling
>>>> +	 * status and retry setup if the sink resets it.
>>>> +	 */
>>>> +	struct delayed_work scdc_work;
>>>> +
>>>> +	/**
>>>> +	 * @scdc_work_initialized: Tracks whether @scdc_work has been set up via
>>>> +	 * INIT_DELAYED_WORK(). The work item is initialized lazily on the first
>>>> +	 * scrambling enable, so this guards the teardown paths against touching
>>>> +	 * an uninitialized work item.
>>>> +	 */
>>>> +	bool scdc_work_initialized;
>>>> +
>>>
>>> Why should we track whether it's initialized or not? I'd always
>>> initialize it, but only ever schedule something if we're using the
>>> scrambler.
>>
>> Having this initialized in the connector would lead to a module dependency
>> cycle.
>>
>> Currently the work function lives in drm_hdmi_helper.c, which is built into
>> drm_display_helper module:
>>
>> static void drm_connector_hdmi_scdc_work(struct work_struct *work)
>> {
>> 	[...]
>> 	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
>> 	    !drm_scdc_get_scrambling_status(connector))
>> 		drm_connector_hdmi_try_scrambling_setup(connector);
>> 	[...]
>> }
>>
>> int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
>> 					 const struct drm_connector_state *conn_state)
>> {
>>
>> 	[...]
>> 	if (!hdmi->scdc_work_initialized) {
>> 		INIT_DELAYED_WORK(&hdmi->scdc_work,
>> 				  drm_connector_hdmi_scdc_work);
>> 		hdmi->scdc_work_initialized = true;
>> 	}
>> 	[...]
>> }
>>
>> If we move INIT_DELAYED_WORK() into the connector (i.e. in drm.ko), the work
>> function has to be reachable from there.  The following attempts to accomplish
>> that would fail:
>>
>> - Keep the work function in drm_hdmi_helper.c and export it from
>>   drm_display_helper.
>>
>> - Move the work function into drm_connector.c and export 
>>   drm_connector_hdmi_try_scrambling_setup(), or a wrapper function, from 
>>   drm_display_helper.
> 
> An alternative could be to move drm_connector_hdmi_init to
> drm_hdmi_helper.c, no?

I haven't considered this option so far, as I believe it would also require some
refactoring to get right - for example, moving HDMI-related initialization from
the generic drm_connector_init_only() to drm_connector_hdmi_init(), and
splitting drm_connector_cleanup() into a dedicated drm_connector_hdmi_cleanup()
utility.

> But yeah, if we can't let's keep it like that

Should I proceed with this refactoring, or would it be better to postpone it
until I send out the HDMI 2.1 patches, to avoid expanding this series even
further?

Thanks,
Cristian