[PATCH 0/3] drm/display: hdmi: Fix SCDC scrambling corner cases

[PATCH 0/3] drm/display: hdmi: Fix SCDC scrambling corner cases

[Cristian Ciocaltea]

This series fixes a few corner cases in the scrambling helpers
introduced by commit d653818324ed ("drm/display: hdmi: Add HDMI 2.0
scrambling management helpers").

Patch 1 fixes a potential NULL pointer dereference in
drm_connector_hdmi_sync_scdc() reported by Smatch.

Patch 2 makes the SCDC resync logic rely on the committed state
(scrambler_needed) rather than the source-side scrambler_enabled flag.
The flag can be stale after a boot-time disable, which may cause a
spurious CRTC reset and flicker on non-scrambled modes.

Patch 3 ensures the sink's scrambling status is reset on non-scrambled
modesets.  Without this, a sink left scrambling by the firmware may
never be cleared if the first modeset uses a mode below 340 MHz.  The
reset logic is factored into a common helper, which only writes to SCDC
when bits are actually set.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (3):
      drm/display: hdmi: Prevent NULL pointer dereference in sync_scdc()
      drm/display: hdmi: Use committed state to decide on SCDC resync
      drm/display: hdmi: Reset sink scrambling on non-scrambled modesets

 drivers/gpu/drm/display/drm_hdmi_helper.c | 65 ++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 15 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261104-hdmi-scdc-scramb-fixes-750545a23603

[PATCH 1/3] drm/display: hdmi: Prevent NULL pointer dereference in sync_scdc()

[Cristian Ciocaltea]

The connector pointer is used to initialize the local variables hdmi and
dev before the NULL check takes place, leading to the following Smatch
complaint:

  drivers/gpu/drm/display/drm_hdmi_helper.c:663 drm_connector_hdmi_sync_scdc()
  warn: variable dereferenced before check 'connector' (see line 657)

Move those initializations after the sanity check to avoid a potential
NULL pointer dereference.

Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/all/arJo7A7L5hud3tyc@stanley.mountain/
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index f85cce3d6254..872f7e32e8d2 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -653,9 +653,9 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
 int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
 				 struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_connector_hdmi *hdmi = &connector->hdmi;
-	struct drm_device *dev = connector->dev;
+	struct drm_connector_hdmi *hdmi;
 	struct drm_connector_state *conn_state;
+	struct drm_device *dev;
 	struct drm_crtc *crtc;
 	u8 config;
 	int ret;
@@ -663,6 +663,9 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
 	if (!connector || !plugged || !ctx)
 		return 0;
 
+	hdmi = &connector->hdmi;
+	dev = connector->dev;
+
 	/*
 	 * This is normally part of .detect_ctx() call path, which already holds
 	 * connection_mutex through @ctx.  However, re-acquiring it with the

-- 
2.55.0

[PATCH 2/3] drm/display: hdmi: Use committed state to decide on SCDC resync

[Cristian Ciocaltea]

drm_connector_hdmi_sync_scdc() uses the scrambler_enabled flag to decide
whether the sink-side SCDC status is worth checking on a hotplug/detect
event.  That flag tracks the source-side scrambler status, which is not
necessarily in sync with what the committed mode actually requires.

In particular, a source driver may deliberately set scrambler_enabled at
init to force a disable at boot and resync hardware left enabled by the
firmware, as vc4 does.  If the pipeline doesn't get disabled afterwards,
e.g. because no display was active at probe time, the flag remains
stale.  A subsequent detect (e.g. an HPD event) then finds scrambling
disabled on the sink while the flag claims otherwise, and triggers a
CRTC reset.  This results in a one-time spurious flicker on an otherwise
correctly configured non-scrambled mode.

Verify conn_state->hdmi.scrambler_needed instead, as it is computed
during atomic check and reliably describes the requirements of the
active mode.

Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-170670@kernel.org/
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index 872f7e32e8d2..974d5de6d380 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -653,7 +653,6 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
 int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
 				 struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_connector_hdmi *hdmi;
 	struct drm_connector_state *conn_state;
 	struct drm_device *dev;
 	struct drm_crtc *crtc;
@@ -663,7 +662,6 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
 	if (!connector || !plugged || !ctx)
 		return 0;
 
-	hdmi = &connector->hdmi;
 	dev = connector->dev;
 
 	/*
@@ -677,14 +675,21 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
 
 	/* TODO: Also handle HDMI 2.1 FRL link training */
 
-	if (!drm_connector_hdmi_scrambler_supported(connector) ||
-	    !READ_ONCE(hdmi->scrambler_enabled))
-		return 0;
-
 	conn_state = connector->state;
 	if (!conn_state)
 		return 0;
 
+	/*
+	 * Rely on the committed connector state rather than on the current
+	 * scrambler status: a source driver may force scrambler_enabled at
+	 * init to get a disable at boot (e.g. vc4), and that value would
+	 * remain stale until the first disable actually happens, leading to
+	 * a spurious CRTC reset whenever the active mode needs no scrambling.
+	 */
+	if (!drm_connector_hdmi_scrambler_supported(connector) ||
+	    !conn_state->hdmi.scrambler_needed)
+		return 0;
+
 	crtc = conn_state->crtc;
 	if (!crtc)
 		return 0;

-- 
2.55.0

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

[Cristian Ciocaltea]

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>
---
 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);
 }

-- 
2.55.0

Re: [PATCH 1/3] drm/display: hdmi: Prevent NULL pointer dereference in sync_scdc()

[Maíra Canal]

Hi Cristian,

On 04/11/26 10:26, Cristian Ciocaltea wrote:
> The connector pointer is used to initialize the local variables hdmi and
> dev before the NULL check takes place, leading to the following Smatch
> complaint:
> 
>    drivers/gpu/drm/display/drm_hdmi_helper.c:663 drm_connector_hdmi_sync_scdc()
>    warn: variable dereferenced before check 'connector' (see line 657)
> 
> Move those initializations after the sanity check to avoid a potential
> NULL pointer dereference.
> 
> Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/all/arJo7A7L5hud3tyc@stanley.mountain/
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index f85cce3d6254..872f7e32e8d2 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -653,9 +653,9 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
>   int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>   				 struct drm_modeset_acquire_ctx *ctx)
>   {
> -	struct drm_connector_hdmi *hdmi = &connector->hdmi;
> -	struct drm_device *dev = connector->dev;
> +	struct drm_connector_hdmi *hdmi;
>   	struct drm_connector_state *conn_state;
> +	struct drm_device *dev;
>   	struct drm_crtc *crtc;
>   	u8 config;
>   	int ret;
> @@ -663,6 +663,9 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>   	if (!connector || !plugged || !ctx)
>   		return 0;
>   
> +	hdmi = &connector->hdmi;

hdmi is only used once (and you are going to delete it in the next
commit), so personally, I would prefer to delete this variable in this
commit and use connector->hdmi directly when needed.

> +	dev = connector->dev;
> +
>   	/*
>   	 * This is normally part of .detect_ctx() call path, which already holds
>   	 * connection_mutex through @ctx.  However, re-acquiring it with the
> 

drm_modeset_lock() here could use the variable dev instead of
&connector->dev.

Anyway, these are just a few nits.

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

Best regards,
- Maíra

Re: [PATCH 2/3] drm/display: hdmi: Use committed state to decide on SCDC resync

[Maíra Canal]

Hi Cristian,

On 04/11/26 10:26, Cristian Ciocaltea wrote:
> drm_connector_hdmi_sync_scdc() uses the scrambler_enabled flag to decide
> whether the sink-side SCDC status is worth checking on a hotplug/detect
> event.  That flag tracks the source-side scrambler status, which is not
> necessarily in sync with what the committed mode actually requires.
> 
> In particular, a source driver may deliberately set scrambler_enabled at
> init to force a disable at boot and resync hardware left enabled by the
> firmware, as vc4 does.  If the pipeline doesn't get disabled afterwards,
> e.g. because no display was active at probe time, the flag remains
> stale.  A subsequent detect (e.g. an HPD event) then finds scrambling
> disabled on the sink while the flag claims otherwise, and triggers a
> CRTC reset.  This results in a one-time spurious flicker on an otherwise
> correctly configured non-scrambled mode.
> 
> Verify conn_state->hdmi.scrambler_needed instead, as it is computed
> during atomic check and reliably describes the requirements of the
> active mode.
> 
> Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/sashiko-outbox-170670@kernel.org/
> 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 | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 872f7e32e8d2..974d5de6d380 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -653,7 +653,6 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
>   int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>   				 struct drm_modeset_acquire_ctx *ctx)
>   {
> -	struct drm_connector_hdmi *hdmi;
>   	struct drm_connector_state *conn_state;
>   	struct drm_device *dev;
>   	struct drm_crtc *crtc;
> @@ -663,7 +662,6 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>   	if (!connector || !plugged || !ctx)
>   		return 0;
>   
> -	hdmi = &connector->hdmi;
>   	dev = connector->dev;
>   
>   	/*
> @@ -677,14 +675,21 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>   
>   	/* TODO: Also handle HDMI 2.1 FRL link training */
>   
> -	if (!drm_connector_hdmi_scrambler_supported(connector) ||
> -	    !READ_ONCE(hdmi->scrambler_enabled))
> -		return 0;
> -
>   	conn_state = connector->state;
>   	if (!conn_state)
>   		return 0;
>   
> +	/*
> +	 * Rely on the committed connector state rather than on the current
> +	 * scrambler status: a source driver may force scrambler_enabled at
> +	 * init to get a disable at boot (e.g. vc4), and that value would
> +	 * remain stale until the first disable actually happens, leading to
> +	 * a spurious CRTC reset whenever the active mode needs no scrambling.
> +	 */
> +	if (!drm_connector_hdmi_scrambler_supported(connector) ||
> +	    !conn_state->hdmi.scrambler_needed)
> +		return 0;
> +
>   	crtc = conn_state->crtc;
>   	if (!crtc)
>   		return 0;
> 

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

[Maíra Canal]

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);
>   }
> 

Re: [PATCH 1/3] drm/display: hdmi: Prevent NULL pointer dereference in sync_scdc()

[Cristian Ciocaltea]

Hi Maíra,

On 11/9/26 2:00 PM, Maíra Canal wrote:
> Hi Cristian,
> 
> On 04/11/26 10:26, Cristian Ciocaltea wrote:
>> The connector pointer is used to initialize the local variables hdmi and
>> dev before the NULL check takes place, leading to the following Smatch
>> complaint:
>>
>>    drivers/gpu/drm/display/drm_hdmi_helper.c:663 drm_connector_hdmi_sync_scdc()
>>    warn: variable dereferenced before check 'connector' (see line 657)
>>
>> Move those initializations after the sanity check to avoid a potential
>> NULL pointer dereference.
>>
>> Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
>> Reported-by: Dan Carpenter <error27@gmail.com>
>> Closes: https://lore.kernel.org/all/arJo7A7L5hud3tyc@stanley.mountain/
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> index f85cce3d6254..872f7e32e8d2 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
>> @@ -653,9 +653,9 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
>>   int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>>                    struct drm_modeset_acquire_ctx *ctx)
>>   {
>> -    struct drm_connector_hdmi *hdmi = &connector->hdmi;
>> -    struct drm_device *dev = connector->dev;
>> +    struct drm_connector_hdmi *hdmi;
>>       struct drm_connector_state *conn_state;
>> +    struct drm_device *dev;
>>       struct drm_crtc *crtc;
>>       u8 config;
>>       int ret;
>> @@ -663,6 +663,9 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>>       if (!connector || !plugged || !ctx)
>>           return 0;
>>   +    hdmi = &connector->hdmi;
> 
> hdmi is only used once (and you are going to delete it in the next
> commit), so personally, I would prefer to delete this variable in this
> commit and use connector->hdmi directly when needed.

Ack.

> 
>> +    dev = connector->dev;
>> +
>>       /*
>>        * This is normally part of .detect_ctx() call path, which already holds
>>        * connection_mutex through @ctx.  However, re-acquiring it with the
>>
> 
> drm_modeset_lock() here could use the variable dev instead of
> &connector->dev.

Well spotted, I missed it.

> 
> Anyway, these are just a few nits.
> 
> Reviewed-by: Maíra Canal <mcanal@igalia.com>

Thank you for reviewing the series.  Please let me know if you'd still like me to
send a v2 to address your findings in this patch.

Kind regards,
Cristian

Re: [PATCH 1/3] drm/display: hdmi: Prevent NULL pointer dereference in sync_scdc()

[Maíra Canal]

On 09/11/26 09:39, Cristian Ciocaltea wrote:
> Hi Maíra,
> 
> On 11/9/26 2:00 PM, Maíra Canal wrote:
>> Hi Cristian,
>>
>> On 04/11/26 10:26, Cristian Ciocaltea wrote:
>>> The connector pointer is used to initialize the local variables hdmi and
>>> dev before the NULL check takes place, leading to the following Smatch
>>> complaint:
>>>
>>>     drivers/gpu/drm/display/drm_hdmi_helper.c:663 drm_connector_hdmi_sync_scdc()
>>>     warn: variable dereferenced before check 'connector' (see line 657)
>>>
>>> Move those initializations after the sanity check to avoid a potential
>>> NULL pointer dereference.
>>>
>>> Fixes: d653818324ed ("drm/display: hdmi: Add HDMI 2.0 scrambling management helpers")
>>> Reported-by: Dan Carpenter <error27@gmail.com>
>>> Closes: https://lore.kernel.org/all/arJo7A7L5hud3tyc@stanley.mountain/
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>    drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> index f85cce3d6254..872f7e32e8d2 100644
>>> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
>>> @@ -653,9 +653,9 @@ EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
>>>    int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>>>                     struct drm_modeset_acquire_ctx *ctx)
>>>    {
>>> -    struct drm_connector_hdmi *hdmi = &connector->hdmi;
>>> -    struct drm_device *dev = connector->dev;
>>> +    struct drm_connector_hdmi *hdmi;
>>>        struct drm_connector_state *conn_state;
>>> +    struct drm_device *dev;
>>>        struct drm_crtc *crtc;
>>>        u8 config;
>>>        int ret;
>>> @@ -663,6 +663,9 @@ int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
>>>        if (!connector || !plugged || !ctx)
>>>            return 0;
>>>    +    hdmi = &connector->hdmi;
>>
>> hdmi is only used once (and you are going to delete it in the next
>> commit), so personally, I would prefer to delete this variable in this
>> commit and use connector->hdmi directly when needed.
> 
> Ack.
> 
>>
>>> +    dev = connector->dev;
>>> +
>>>        /*
>>>         * This is normally part of .detect_ctx() call path, which already holds
>>>         * connection_mutex through @ctx.  However, re-acquiring it with the
>>>
>>
>> drm_modeset_lock() here could use the variable dev instead of
>> &connector->dev.
> 
> Well spotted, I missed it.
> 
>>
>> Anyway, these are just a few nits.
>>
>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
> 
> Thank you for reviewing the series.  Please let me know if you'd still like me to
> send a v2 to address your findings in this patch.
> 

These are very small nits, not worth a v2. Feel free to push the patches
to drm-misc-fixes.

Best regards,
- Maíra

> Kind regards,
> Cristian