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 <[email protected]>
>> Tested-by: Diederik de Haas <[email protected]> # NanoPC-T6 LTS, Rock
>> 5B
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> 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
