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

[Maxime Ripard]

On Fri, Aug 21, 2026 at 10:04:14PM +0300, Cristian Ciocaltea wrote:
> 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@xxxxxxxxxxx>
> >>>> Tested-by: Diederik de Haas <diederik@xxxxxxxxxxxxxx>  # NanoPC-T6 LTS, Rock 5B
> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
> >>>> ---
> >>>>  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().

This is the problem then. scrambler is mandatory for HDMI2.0, and
HDMI1.4 will never reach HDMI2.0 TMDS rates.

scrambler supported is HDMI 2.0 and scrambler_enable and
scrambler_disable are set. if HDMI 1.4 is used, then the scrambler must
not be supported, ever.

> > 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?

we can postpone it if you prefer, or even to a separate series

Maxime

Attachment: signature.asc
Description: PGP signature