Re: [PATCH v3 6/9] drm/vc4: hdmi: Convert to common HDMI 2.0 scrambling infrastructure

[Cristian Ciocaltea]

Hi Maíra,

On 10/27/26 3:35 PM, Maíra Canal wrote:
> Hi Cristian,
> 
> On 13/10/26 22:03, Cristian Ciocaltea wrote:
>> Replace driver local scrambling logic with the newly introduced common
>> HDMI 2.0 scrambling support:
>>
>> - Advertise source-side scrambling capability by setting supported HDMI
>>    version based on the variant's max_pixel_clock.
>>
>> - Provide minimal .scrambler_{enable,disable} connector callbacks that
>>    only toggle the VC5 HDMI_SCRAMBLER_CTL register, while delegating
>>    sink-side SCDC programming and periodic status monitoring to
>>    drm_connector_hdmi_{enable,disable}_scrambling().
>>
>> - Drop vc4_hdmi_reset_link() and vc4_hdmi_handle_hotplug(), switching
>>    the .detect_ctx() path to drm_atomic_helper_connector_hdmi_hotplug()
>>    which internally calls drm_connector_hdmi_sync_scdc() to trigger a
>>    CRTC reset on reconnection.
>>
>> - Drop the local scrambling_work delayed workqueue and scdc_enabled
>>    flag, now tracked by the common drm_connector_hdmi layer, as well as
>>    the now unused output_{bpc,color_format} driver data members.
>>
>> No functional change intended.
>>
>> Acked-by: Maxime Ripard <mripard@xxxxxxxxxx>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>> ---
>>   drivers/gpu/drm/vc4/vc4_hdmi.c | 230 +++++------------------------------------
>>   drivers/gpu/drm/vc4/vc4_hdmi.h |  21 ----
>>   2 files changed, 28 insertions(+), 223 deletions(-)
>>
> 
> [...]
> 
>>   +    /*
>> +     * Since we don't know the state of the controller and its
>> +     * display (if any), let's assume it's always enabled.
>> +     * drm_connector_hdmi_disable_scrambling() will thus run at boot,
>> +     * make sure it's disabled, and avoid any inconsistency.
>> +     */
>> +    connector->hdmi.scrambler_enabled = drm_connector_hdmi_scrambler_supported(connector);
>> +
> 
> I'm okay merging this series to drm-misc-next, but I believe that
> Sashiko's report has a point about this chunk. If the display if off or
> disconnected, the boot-time disable would never run. Could you please
> take a look at it?

Thanks for checking this out!

I initially overlooked Sashiko's finding, as it only complains about a
theoretical display flicker - the conditions required to hit it are quite
narrow, all of which must hold at once:

- an HDMI 2.0-capable source, so RPi 0..3 are out
- nothing active at boot, so that vc4_crtc_disable_at_boot() bails out early and
  no boot-time disable runs
- a sink advertising SCDC + scrambling (i.e. 4k60-capable) which nevertheless
  gets a first modeset with a mode that does not need scrambling - pretty
  unusual, since the preferred mode is normally the highest supported one

Eventually the reset's disable leg clears the flag, so it's a single flicker and
the state self-corrects from then on.

Other than this one-time glitch, the driver conversion should preserve the
original behaviour.  The thing is that vc4 kind of abuses the common API, since
normally connector->hdmi.scrambler_enabled is not supposed to be flipped
manually.

Regardless, I think we can handle this by making drm_connector_hdmi_sync_scdc()
rely on the committed connector state rather than on the current source-side
scrambler status, i.e. conn_state->hdmi.scrambler_needed, which is computed
during atomic check and correctly describes the active mode.

> Also, while greping the code, one thing caught my attention:
> drm_connector_hdmi_disable_scrambling() is called at
> vc4_crtc_disable_at_boot() before drm_dev_register(). Does that mean
> that connector->status might be connector_status_unknown at that point?
> I'm concerned due to the last if-block in
> drm_connector_hdmi_disable_scrambling(). Am I mistaken?

You're not mistaken: connector->status is expected to indicate
connector_status_unknown, while connector->display_info should be empty as well,
since no EDID has been read yet.  Hence, the sink-side SCDC writes are skipped -
only the source-side clear and the flag reset happen.

That is indeed a behavioral difference, as the pre-conversion
vc4_hdmi_disable_scrambling() had no such guards and issued the SCDC writes
unconditionally:

- with no display connected, the old code poked a non-existent sink and the I2C
  transfer simply NAK'ed, so skipping it is arguably better

- with a display connected and SCDC scrambling pre-enabled, the old code did
  clear the sink's scrambling, while the new one doesn't - if the first kernel
  modeset then picks a mode below 340 MHz, the sink could in theory be left
  trying to descramble unscrambled data

The second case also seems unlikely, as HDMI 2.0 sinks are expected to reset the
SCDC configuration when the connection is lost, while scrambling should be
disabled by default until the source enables it via SCDC.

A more realistic risk is the sink's state being left with scrambling enabled by
the board firmware before the kernel boots.  Probability-wise it needs the same
unlikely ingredient: the first kernel modeset picking a non-scrambled mode while
the firmware had a scrambled one.  The usual outcome is the same preferred 4k60
mode being selected, so enable_scrambling() reprograms the sink as needed.

Rather than issuing the SCDC writes blindly at boot, which would mean DDC
traffic towards a possibly absent sink, we should reset the sink status from
drm_connector_hdmi_enable_scrambling() when the committed state requires no
scrambling.  At that point the sink has been probed and its capabilities parsed,
so the operation is safe:

-       if (!conn_state->hdmi.scrambler_needed)
+       if (!conn_state->hdmi.scrambler_needed) {
+           drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
+
+           if (config & SCDC_SCRAMBLING_ENABLE)
+                   drm_scdc_set_scrambling(connector, false);
+
+           if (config & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40)
+                  drm_scdc_set_high_tmds_clock_ratio(connector, false);
+
            return 0;
+       }

I'm going to test both fixes and send them out as a separate series, so we're
not blocking on it.

Regards,
Cristian