On 8/25/26 12:29 PM, Maxime Ripard wrote:
> 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 <[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;
>>>> }
>>>> }
>>>
>>> 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'll drop that 'else' branch and have scrambler_supported() return false for
anything below HDMI 2.0, hence ignoring scrambler_{enable,disable} callbacks
presence in the non-HDMI2.0 cases:
drm_connector_hdmi_scrambler_supported(const struct drm_connector *connector)
{
return connector->hdmi.funcs &&
connector->hdmi.funcs->supported_hdmi_ver >= HDMI_VERSION_2_0;
}
>>> 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
Sounds good!
Thanks,
Cristian
