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'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
signature.asc
Description: PGP signature
