Re: [PATCH 3/3] drm/display: hdmi: Reset sink scrambling on non-scrambled modesets

["Maíra Canal" <mcanal@igalia.com>]

On 04/11/26 10:26, Cristian Ciocaltea wrote:
> drm_connector_hdmi_disable_scrambling() only clears the sink SCDC status
> when the connector is known to be connected.  That is never true when a
> source driver disables scrambling at boot to clean up after the
> firmware, as vc4 does from vc4_crtc_disable_at_boot(), where
> connector->status is still unknown and no EDID has been read.
> 
> A sink left scrambling by the firmware thus keeps both Scrambling_Enable
> and the 1/40 TMDS bit clock ratio set.  If the first modeset picks a
> mode below 340 MHz, no disable happens in between, since the CRTC was
> never active in atomic state, and drm_connector_hdmi_enable_scrambling()
> returns early, leaving the sink trying to descramble an unscrambled
> stream.
> 
> Instead of blind SCDC writes at boot towards a possibly absent sink,
> reset the sink status from drm_connector_hdmi_enable_scrambling() when
> no scrambling is required.  By then the sink has been probed, and HDMI
> 2.0 expects a scrambling capable sink to have Scrambling_Enable cleared
> below 340 MHz anyway.  The reset runs regardless of source scrambler
> support, since the firmware may have left the sink scrambling.
> 
> Factor the reset out into a helper also used by the disable and error
> paths, gated only on drm_scdc_sink_supports_scrambling(), which suffices
> because drm_display_info is zeroed until the first EDID read and reset
> on disconnect, making the connector->status check redundant.  As the
> helper now runs on every non-scrambled modeset, read SCDC_TMDS_CONFIG
> first and only clear the bits actually set, so the common case costs a
> single DDC read.
> 
> Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best regards,
- Maíra

> ---
>   drivers/gpu/drm/display/drm_hdmi_helper.c | 45 ++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 974d5de6d380..278cfda38e18 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -470,6 +470,28 @@ static bool drm_scdc_sink_supports_scrambling(struct drm_connector *connector)
>   	       info->hdmi.scdc.scrambling.supported;
>   }
>   
> +static void drm_connector_hdmi_reset_sink_scrambling(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	u8 config;
> +	int ret;
> +
> +	if (!connector->ddc || !drm_scdc_sink_supports_scrambling(connector))
> +		return;
> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
> +	if (ret) {
> +		drm_dbg_kms(dev, "Failed to read TMDS config: %d\n", ret);
> +		return;
> +	}
> +
> +	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);
> +}
> +
>   static int drm_connector_hdmi_try_scrambling_setup(struct drm_connector *connector)
>   {
>   	bool done;
> @@ -522,6 +544,11 @@ static void drm_connector_hdmi_scdc_work(struct work_struct *work)
>    * the sink side and retry the operation, as some displays refuse to set the
>    * scrambling bit right away.
>    *
> + * When @conn_state doesn't require scrambling, the sink-side SCDC status is
> + * reset instead, so that a sink left scrambling by a state the driver did not
> + * control, e.g. set up by the firmware before boot, doesn't end up out of sync
> + * with the source.
> + *
>    * The work item is cancelled by drm_connector_hdmi_disable_scrambling(), hence
>    * source drivers must ensure the display pipeline is shut down on unbind, e.g.
>    * via drm_atomic_helper_shutdown(). Otherwise the work may outlive the driver's
> @@ -541,8 +568,14 @@ int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
>   	if (!conn_state)
>   		return -EINVAL;
>   
> -	if (!conn_state->hdmi.scrambler_needed)
> +	if (!conn_state->hdmi.scrambler_needed) {
> +		/*
> +		 * Resync the sink here, where it is known to be connected
> +		 * and its capabilities have been parsed.
> +		 */
> +		drm_connector_hdmi_reset_sink_scrambling(connector);
>   		return 0;
> +	}
>   
>   	if (!drm_connector_hdmi_scrambler_supported(connector)) {
>   		drm_dbg_kms(dev, "Source doesn't support scrambling.\n");
> @@ -581,8 +614,7 @@ int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
>   		WRITE_ONCE(hdmi->scrambler_enabled, false);
>   		cancel_delayed_work_sync(&hdmi->scdc_work);
>   
> -		drm_scdc_set_scrambling(connector, false);
> -		drm_scdc_set_high_tmds_clock_ratio(connector, false);
> +		drm_connector_hdmi_reset_sink_scrambling(connector);
>   	}
>   
>   	return ret;
> @@ -619,12 +651,7 @@ int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector)
>   	if (hdmi->scdc_work_initialized)
>   		cancel_delayed_work_sync(&hdmi->scdc_work);
>   
> -	if (connector->ddc &&
> -	    connector->status == connector_status_connected &&
> -	    drm_scdc_sink_supports_scrambling(connector)) {
> -		drm_scdc_set_scrambling(connector, false);
> -		drm_scdc_set_high_tmds_clock_ratio(connector, false);
> -	}
> +	drm_connector_hdmi_reset_sink_scrambling(connector);
>   
>   	return hdmi->funcs->scrambler_disable(connector);
>   }
>