[PATCH v6 00/22] Add HDMI 2.0 support to DW HDMI QP TX

[PATCH v6 00/22] Add HDMI 2.0 support to DW HDMI QP TX

[Cristian Ciocaltea]

Enable HDMI 2.0 display modes (e.g. 4K@60Hz) on the Synopsys DW HDMI QP
TX controller, as found in Rockchip RK3576 & RK3588 SoCs, by adding SCDC
management for high TMDS clock ratio and scrambling.

Since SCDC state is lost on sink disconnects, the bridge driver needs to
trigger a CRTC reset during connector detection.  To support this, the
series introduces the connector and bridge scrambling infrastructure
(patches 1-7), wires it up through the bridge connector layer with an
atomic-aware detect_ctx hook (patches 8-10), then implements the SCDC
scrambling feature in the DW HDMI QP bridge driver (patches 11-14).

Patches 15-17 are minor cleanups in the Rockchip platform driver.
Patches 18-22 improve HPD handling by deferring IRQ registration until
the connector is fully initialized, adding .enable_hpd()/.disable_hpd()
PHY ops, and restricting HPD events to the affected connector.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Changes in v6:
- Collected R-b & A-b tags from Dmitry and Heiko
- Restructured the series from 10 into 22 patches, splitting the SCDC
  scrambling helpers, connector infrastructure, bridge operations,
  bridge_connector plumbing, dw-hdmi-qp implementation, and Rockchip
  platform changes into distinct commits
- Added drm_scdc_dbg() macro to simplify debug messages (new patch)
- Added drm_scdc_start/stop/sync_status() helpers for full scrambling
  lifecycle management (reworked from v5 patch 5)
- Added drm_atomic_helper_connector_hdmi_hotplug_ctx() to propagate
  modeset acquire context for SCDC sync on hotplug (new patch)
- Added connector scrambler callbacks and SCDC work/flag infrastructure
  as a separate patch (split from v5 patch 5)
- Added DRM_BRIDGE_OP_HDMI_SCRAMBLER bridge operation with
  hdmi_scrambler_enable/disable callbacks (new patch)
- Implemented bridge_connector scrambler interface wiring (new patch)
- Added .enable_hpd()/.disable_hpd() PHY ops for dw-hdmi-qp bridge and
  Rockchip platform drivers, replacing the obsolete .setup_hpd() op
- Added dw_hdmi_qp_hpd_notify() helper for targeted connector-only HPD
  notification (split from v5 patch 10)
- Dropped drm_fb_helper_hotplug_event() unused variable (new cleanup)
- Dropped unused drm_simple_kms_helper.h include (new cleanup)
- Masked RK3576 HPD IRQ in io_init() for consistency with RK3588
- Rebased onto latest drm-misc-next
- Link to v5: https://patch.msgid.link/20260426-dw-hdmi-qp-scramb-v5-0-d778e70c317b@collabora.com

Changes in v5:
- Added new patches: 1/10, 3/10, 6/10, 7/10, 8/10
- Removed redundant no-op error check in drm_bridge_helper_reset_crtc()
  (patch 1)
- Removed the EDEADLK retry loop from the bridge .detect_ctx() callback,
  as that's already handled in the drm_bridge_detect_ctx() helper or by
  the caller when ctx is provided (patch 2)
- Refactored drm_bridge_detect() to delegate to drm_bridge_detect_ctx()
  and added a WARN_ON for unexpected negative return values (patch 2)
- Split the bridge-connector .detect_ctx() switch into a preparatory
  patch to use cached connector status in .get_modes() (patch 3)
- Improved error handling in SCDC scrambling setup: roll back high TMDS
  clock ratio on scrambling failure, reset scramb_enabled flag on
  set_scramb failure, and add SCDC version read/write error checks
  (patch 5)
- Annotated scramb_enabled with READ_ONCE/WRITE_ONCE for cross-context
  access between modeset paths and the scrambling work item (patch 5)
- Renamed SCDC_MIN_SOURCE_VERSION to SCDC_MAX_SOURCE_VERSION (patch 5)
- Rate limited i2c error messages (patch 6)
- Added missing newlines in dev_err_probe() messages (patch 7)
- Replaced indirect device pointer accesses with local dev variable in
  bind() (patch 8)
- Split the HPD connector restriction (formerly patch 4/4): register HPD
  IRQ after connector setup first (patch 9), then restrict HPD event to
  the affected connector (patch 10); also collected R-b from Heiko
- Rebased onto latest drm-misc-next
- Link to v4: https://lore.kernel.org/r/20260303-dw-hdmi-qp-scramb-v4-0-317d3b8bd219@collabora.com

Changes in v4:
- Fixed conflicts while rebasing onto latest drm-misc-next
- Link to v3: https://lore.kernel.org/r/20260119-dw-hdmi-qp-scramb-v3-0-bd8611730fc1@collabora.com

Changes in v3:
- Used drm_bridge_helper_reset_crtc() helper to reset the display
  pipeline and got rid of some boilerplate code (Maxime)
- Rebased onto latest drm-misc-next
- Link to v2: https://lore.kernel.org/r/20260113-dw-hdmi-qp-scramb-v2-0-ae7b2c58d24d@collabora.com

Changes in v2:
- Collected Tested-by tags from Diederik and Maud
- Rebased onto latest drm-misc-next
- Ensured the recently introduced 'no-hpd' support for dealing with
  unconnected/repurposed/broken HPD pin is limited to HDMI 1.4 rates
- Link to v1: https://lore.kernel.org/r/20251203-dw-hdmi-qp-scramb-v1-0-836fe7401a69@collabora.com

---
Cristian Ciocaltea (22):
      drm/fb-helper: Remove unused local variable in hotplug_event()
      drm/connector: Add HDMI 2.0 scrambler infrastructure
      drm/display: scdc_helper: Add macro to simplify debugging
      drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers
      drm/display: hdmi_state_helper: Add ctx-aware hotplug helper for SCDC sync
      drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()
      drm/bridge: Add HDMI 2.0 scrambler bridge operation and callbacks
      drm/display: bridge_connector: Use cached connector status in .get_modes()
      drm/display: bridge_connector: Switch to .detect_ctx() connector helper
      drm/display: bridge_connector: Wire up HDMI 2.0 scrambler callbacks
      drm/bridge: dw-hdmi-qp: Rate limit i2c read error messages
      drm/bridge: dw-hdmi-qp: Provide .{enable|disable}_hpd() PHY ops
      drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling support`
      drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper
      drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages
      drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind()
      drm/rockchip: dw_hdmi_qp: Drop unnecessary #include
      drm/rockchip: dw_hdmi_qp: Defer HPD IRQ enable until after connector setup
      drm/rockchip: dw_hdmi_qp: Mask HPD IRQ in rk3576_io_init()
      drm/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops
      drm/rockchip: dw_hdmi_qp: Switch to dw_hdmi_qp_hpd_notify()
      drm/bridge: dw-hdmi-qp: Remove obsolete .setup_hpd() phy op

 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c    | 132 +++++++++---
 drivers/gpu/drm/display/drm_bridge_connector.c  | 118 +++++++----
 drivers/gpu/drm/display/drm_hdmi_state_helper.c |  36 +++-
 drivers/gpu/drm/display/drm_scdc_helper.c       | 259 ++++++++++++++++++++++--
 drivers/gpu/drm/drm_bridge_helper.c             |   2 -
 drivers/gpu/drm/drm_connector.c                 |  14 ++
 drivers/gpu/drm/drm_fb_helper.c                 |  11 +-
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c  | 130 ++++++------
 include/drm/bridge/dw_hdmi_qp.h                 |   4 +-
 include/drm/display/drm_hdmi_state_helper.h     |   4 +
 include/drm/display/drm_scdc_helper.h           |   6 +-
 include/drm/drm_bridge.h                        |  26 +++
 include/drm/drm_connector.h                     |  52 +++++
 13 files changed, 623 insertions(+), 171 deletions(-)
---
base-commit: a48bbcc7ac739e93562d6148c6fa504c2e9f22f8
change-id: 20251203-dw-hdmi-qp-scramb-cdbd8b57ccf9

[PATCH v6 01/22] drm/fb-helper: Remove unused local variable in hotplug_event()

[Cristian Ciocaltea]

Remove the 'err' local variable in drm_fb_helper_hotplug_event() which
only stores a return value that is never used beyond the immediate
return statement.  This simplifies the code without behavior changes.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 38d25dce7f33..d8fb90160b90 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1744,21 +1744,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  */
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
-	int err = 0;
-
 	if (!drm_fbdev_emulation || !fb_helper)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	if (fb_helper->deferred_setup) {
-		err = __drm_fb_helper_initial_config_and_unlock(fb_helper);
-		return err;
-	}
+	if (fb_helper->deferred_setup)
+		return __drm_fb_helper_initial_config_and_unlock(fb_helper);
 
 	if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&fb_helper->lock);
-		return err;
+		return 0;
 	}
 
 	drm_master_internal_release(fb_helper->dev);
@@ -1802,4 +1798,3 @@ bool drm_fb_helper_gem_is_fb(const struct drm_fb_helper *fb_helper,
 	return gem == obj;
 }
 EXPORT_SYMBOL_GPL(drm_fb_helper_gem_is_fb);
-

-- 
2.53.0

Claude review: drm/fb-helper: Remove unused local variable in hotplug_event()

[Claude Code Review Bot]

Patch Review

Straightforward cleanup. The `err` variable was redundant since it was only used for an immediate return. No issues.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 02/22] drm/connector: Add HDMI 2.0 scrambler infrastructure

[Cristian Ciocaltea]

Add the connector-level infrastructure to support HDMI 2.0 scrambling:

- .scrambler_src_{enable|disable}() callbacks in
  drm_connector_hdmi_funcs for source-side scrambling control
- A delayed work item (scdc_work) with an associated callback (scdc_cb)
  for periodic monitoring of sink-side scrambling status
- A scrambler_enabled flag to track whether scrambling is currently
  active

These are intended to be used by SCDC scrambling helpers to coordinate
scrambling setup and teardown between the source driver and the DRM
core.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_connector.c | 14 +++++++++++
 include/drm/drm_connector.h     | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3fa4d2082cd7..91e58362fbc0 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -220,6 +220,19 @@ void drm_connector_free_work_fn(struct work_struct *work)
 	}
 }
 
+static void drm_connector_hdmi_scdc_work(struct work_struct *work)
+{
+	struct drm_connector *connector;
+	struct drm_connector_hdmi *hdmi;
+
+	hdmi = container_of(to_delayed_work(work), struct drm_connector_hdmi,
+			    scdc_work);
+	connector = container_of(hdmi, struct drm_connector, hdmi);
+
+	if (hdmi->scdc_cb)
+		hdmi->scdc_cb(connector);
+}
+
 static int drm_connector_init_only(struct drm_device *dev,
 				   struct drm_connector *connector,
 				   const struct drm_connector_funcs *funcs,
@@ -285,6 +298,7 @@ static int drm_connector_init_only(struct drm_device *dev,
 	mutex_init(&connector->edid_override_mutex);
 	mutex_init(&connector->hdmi.infoframes.lock);
 	mutex_init(&connector->hdmi_audio.lock);
+	INIT_DELAYED_WORK(&connector->hdmi.scdc_work, drm_connector_hdmi_scdc_work);
 	connector->edid_blob_ptr = NULL;
 	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 5ad62c207d00..49eaa30b1329 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -28,6 +28,7 @@
 #include <linux/ctype.h>
 #include <linux/hdmi.h>
 #include <linux/notifier.h>
+#include <linux/workqueue.h>
 #include <drm/drm_mode_object.h>
 #include <drm/drm_util.h>
 #include <drm/drm_property.h>
@@ -1358,6 +1359,36 @@ struct drm_connector_hdmi_funcs {
 	 */
 	const struct drm_edid *(*read_edid)(struct drm_connector *connector);
 
+	/**
+	 * @scrambler_src_enable:
+	 *
+	 * This callback is invoked through @drm_scdc_start_scrambling during
+	 * a commit to setup SCDC scrambling and high TMDS clock ratio on
+	 * source side.
+	 *
+	 * The @scrambler_src_enable callback is mandatory if HDMI 2.0 is
+	 * to be supported.
+	 *
+	 * Returns:
+	 * 0 on success, a negative error code otherwise
+	 */
+	int (*scrambler_src_enable)(struct drm_connector *connector);
+
+	/**
+	 * @scrambler_src_disable:
+	 *
+	 * This callback is invoked through @drm_scdc_stop_scrambling during
+	 * a commit to disable SCDC scrambling and high TMDS clock ratio on
+	 * source side.
+	 *
+	 * The @scrambler_src_disable callback is mandatory if HDMI 2.0 is
+	 * to be supported.
+	 *
+	 * Returns:
+	 * 0 on success, a negative error code otherwise
+	 */
+	int (*scrambler_src_disable)(struct drm_connector *connector);
+
 	/**
 	 * @avi:
 	 *
@@ -1944,6 +1975,27 @@ struct drm_connector_hdmi {
 	 */
 	unsigned long supported_formats;
 
+	/**
+	 * @scrambler_enabled: Tracks whether HDMI 2.0 scrambler is currently enabled.
+	 */
+	bool scrambler_enabled;
+
+	/**
+	 * @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_cb: Callback to be invoked as part of @scdc_work.
+	 *
+	 * Currently used to monitor sink-side scrambling status and retry
+	 * setup if the sink resets it.
+	 *
+	 * This is assigned by the framework when making use of
+	 * drm_scdc_start_scrambling() helper.
+	 */
+	void (*scdc_cb)(struct drm_connector *connector);
+
 	/**
 	 * @funcs: HDMI connector Control Functions
 	 */

-- 
2.53.0

Re: [PATCH v6 02/22] drm/connector: Add HDMI 2.0 scrambler infrastructure

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4981 bytes --]

Hi,

Thanks for working on this!

On Wed, May 20, 2026 at 09:38:13PM +0300, Cristian Ciocaltea wrote:
> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
> 
> - .scrambler_src_{enable|disable}() callbacks in
>   drm_connector_hdmi_funcs for source-side scrambling control
> - A delayed work item (scdc_work) with an associated callback (scdc_cb)
>   for periodic monitoring of sink-side scrambling status
> - A scrambler_enabled flag to track whether scrambling is currently
>   active
> 
> These are intended to be used by SCDC scrambling helpers to coordinate
> scrambling setup and teardown between the source driver and the DRM
> core.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 14 +++++++++++
>  include/drm/drm_connector.h     | 52 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)

So we would need kunit tests for this.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3fa4d2082cd7..91e58362fbc0 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -220,6 +220,19 @@ void drm_connector_free_work_fn(struct work_struct *work)
>  	}
>  }
>  
> +static void drm_connector_hdmi_scdc_work(struct work_struct *work)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_hdmi *hdmi;
> +
> +	hdmi = container_of(to_delayed_work(work), struct drm_connector_hdmi,
> +			    scdc_work);
> +	connector = container_of(hdmi, struct drm_connector, hdmi);
> +
> +	if (hdmi->scdc_cb)
> +		hdmi->scdc_cb(connector);
> +}
> +
>  static int drm_connector_init_only(struct drm_device *dev,
>  				   struct drm_connector *connector,
>  				   const struct drm_connector_funcs *funcs,
> @@ -285,6 +298,7 @@ static int drm_connector_init_only(struct drm_device *dev,
>  	mutex_init(&connector->edid_override_mutex);
>  	mutex_init(&connector->hdmi.infoframes.lock);
>  	mutex_init(&connector->hdmi_audio.lock);
> +	INIT_DELAYED_WORK(&connector->hdmi.scdc_work, drm_connector_hdmi_scdc_work);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5ad62c207d00..49eaa30b1329 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -28,6 +28,7 @@
>  #include <linux/ctype.h>
>  #include <linux/hdmi.h>
>  #include <linux/notifier.h>
> +#include <linux/workqueue.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_util.h>
>  #include <drm/drm_property.h>
> @@ -1358,6 +1359,36 @@ struct drm_connector_hdmi_funcs {
>  	 */
>  	const struct drm_edid *(*read_edid)(struct drm_connector *connector);
>  
> +	/**
> +	 * @scrambler_src_enable:
> +	 *
> +	 * This callback is invoked through @drm_scdc_start_scrambling during
> +	 * a commit to setup SCDC scrambling and high TMDS clock ratio on
> +	 * source side.
> +	 *
> +	 * The @scrambler_src_enable callback is mandatory if HDMI 2.0 is
> +	 * to be supported.
> +	 *
> +	 * Returns:
> +	 * 0 on success, a negative error code otherwise
> +	 */
> +	int (*scrambler_src_enable)(struct drm_connector *connector);
> +
> +	/**
> +	 * @scrambler_src_disable:
> +	 *
> +	 * This callback is invoked through @drm_scdc_stop_scrambling during
> +	 * a commit to disable SCDC scrambling and high TMDS clock ratio on
> +	 * source side.
> +	 *
> +	 * The @scrambler_src_disable callback is mandatory if HDMI 2.0 is
> +	 * to be supported.
> +	 *
> +	 * Returns:
> +	 * 0 on success, a negative error code otherwise
> +	 */
> +	int (*scrambler_src_disable)(struct drm_connector *connector);
> +
>  	/**
>  	 * @avi:
>  	 *
> @@ -1944,6 +1975,27 @@ struct drm_connector_hdmi {
>  	 */
>  	unsigned long supported_formats;
>  
> +	/**
> +	 * @scrambler_enabled: Tracks whether HDMI 2.0 scrambler is currently enabled.
> +	 */
> +	bool scrambler_enabled;
> +
> +	/**
> +	 * @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_cb: Callback to be invoked as part of @scdc_work.
> +	 *
> +	 * Currently used to monitor sink-side scrambling status and retry
> +	 * setup if the sink resets it.
> +	 *
> +	 * This is assigned by the framework when making use of
> +	 * drm_scdc_start_scrambling() helper.
> +	 */
> +	void (*scdc_cb)(struct drm_connector *connector);
> +

I'm really not sure what the monitor thing is about. If we have setup
the scrambler at enable time, and we set it again on hotplugging, why
would we need to monitor anything?

Also, scrambling is only relevant for HDMI 2.0. We need a way to expose
that somehow and make sure that HDMI 2.0 drivers actually have
scrambling setup.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v6 02/22] drm/connector: Add HDMI 2.0 scrambler infrastructure

[Cristian Ciocaltea]

Hi Maxime,

On 5/21/26 10:52 AM, Maxime Ripard wrote:
> Hi,
> 
> Thanks for working on this!

Thanks for your quick review! :-)

> 
> On Wed, May 20, 2026 at 09:38:13PM +0300, Cristian Ciocaltea wrote:
>> Add the connector-level infrastructure to support HDMI 2.0 scrambling:
>>
>> - .scrambler_src_{enable|disable}() callbacks in
>>   drm_connector_hdmi_funcs for source-side scrambling control
>> - A delayed work item (scdc_work) with an associated callback (scdc_cb)
>>   for periodic monitoring of sink-side scrambling status
>> - A scrambler_enabled flag to track whether scrambling is currently
>>   active
>>
>> These are intended to be used by SCDC scrambling helpers to coordinate
>> scrambling setup and teardown between the source driver and the DRM
>> core.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 14 +++++++++++
>>  include/drm/drm_connector.h     | 52 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+)
> 
> So we would need kunit tests for this.

Ack.

> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 3fa4d2082cd7..91e58362fbc0 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -220,6 +220,19 @@ void drm_connector_free_work_fn(struct work_struct *work)
>>  	}
>>  }

[...]

>> +	/**
>> +	 * @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_cb: Callback to be invoked as part of @scdc_work.
>> +	 *
>> +	 * Currently used to monitor sink-side scrambling status and retry
>> +	 * setup if the sink resets it.
>> +	 *
>> +	 * This is assigned by the framework when making use of
>> +	 * drm_scdc_start_scrambling() helper.
>> +	 */
>> +	void (*scdc_cb)(struct drm_connector *connector);
>> +
> 
> I'm really not sure what the monitor thing is about. If we have setup
> the scrambler at enable time, and we set it again on hotplugging, why
> would we need to monitor anything?

This is similar to 257d36d493e9 ("drm/vc4: hdmi: Add a workqueue to set
scrambling") in order to deal with some displays needing a retry on scrambling
setup.

> Also, scrambling is only relevant for HDMI 2.0. We need a way to expose
> that somehow and make sure that HDMI 2.0 drivers actually have
> scrambling setup.

I assumed drivers that do not reject the HDMI 2.0 rates in their
.tmds_char_rate_valid() are implicitly expected to support scrambling. 
Thinking again we could extend the framework to provide some additional checks,
but let's discuss the topic on the SCDC patch.

Regards,
Cristian

Claude review: drm/connector: Add HDMI 2.0 scrambler infrastructure

[Claude Code Review Bot]

Patch Review

The infrastructure additions look reasonable. A few observations:

1. **`INIT_DELAYED_WORK` in `drm_connector_init_only()`**: This initializes `scdc_work` for *all* connectors, not just HDMI ones. While harmless (the work is never scheduled for non-HDMI connectors), it's slightly wasteful. However, this is consistent with how `mutex_init(&connector->hdmi.infoframes.lock)` is already done unconditionally on the adjacent line, so this follows existing pattern.

2. **`scdc_cb` function pointer without locking documentation**: The `scdc_cb` callback pointer is assigned and cleared from `drm_scdc_start_scrambling()`/`drm_scdc_stop_scrambling()`, and read from the work item `drm_connector_hdmi_scdc_work()`. The `scrambler_enabled` flag is protected by `READ_ONCE`/`WRITE_ONCE`, but `scdc_cb` itself has no such annotation. There's a potential race between `drm_scdc_stop_scrambling()` setting `scdc_cb = NULL` after `cancel_delayed_work_sync()`, and the work item checking `scdc_cb`. Since `cancel_delayed_work_sync()` ensures the work item completes before returning, and subsequent scheduling is prevented by `scrambler_enabled` being false, this is actually safe in practice. But a comment explaining the ordering guarantee would be helpful.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 03/22] drm/display: scdc_helper: Add macro to simplify debugging

[Cristian Ciocaltea]

Introduce the drm_scdc_dbg() wrapper over drm_dbg_kms() to help getting
rid of the boilerplate around prefixing the debug messages with the
connector information.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index df878aad4a36..cb6632346aad 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -55,6 +55,10 @@
 
 #define SCDC_I2C_SLAVE_ADDRESS 0x54
 
+#define drm_scdc_dbg(connector, fmt, ...)					\
+	drm_dbg_kms((connector)->dev, "[CONNECTOR:%d:%s] " fmt,			\
+		    (connector)->base.id, (connector)->name, ##__VA_ARGS__)
+
 /**
  * drm_scdc_read - read a block of data from SCDC
  * @adapter: I2C controller
@@ -158,9 +162,7 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector)
 
 	ret = drm_scdc_readb(connector->ddc, SCDC_SCRAMBLER_STATUS, &status);
 	if (ret < 0) {
-		drm_dbg_kms(connector->dev,
-			    "[CONNECTOR:%d:%s] Failed to read scrambling status: %d\n",
-			    connector->base.id, connector->name, ret);
+		drm_scdc_dbg(connector, "Failed to read scrambling status: %d\n", ret);
 		return false;
 	}
 
@@ -188,9 +190,7 @@ bool drm_scdc_set_scrambling(struct drm_connector *connector,
 
 	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
 	if (ret < 0) {
-		drm_dbg_kms(connector->dev,
-			    "[CONNECTOR:%d:%s] Failed to read TMDS config: %d\n",
-			    connector->base.id, connector->name, ret);
+		drm_scdc_dbg(connector, "Failed to read TMDS config: %d\n", ret);
 		return false;
 	}
 
@@ -201,9 +201,7 @@ bool drm_scdc_set_scrambling(struct drm_connector *connector,
 
 	ret = drm_scdc_writeb(connector->ddc, SCDC_TMDS_CONFIG, config);
 	if (ret < 0) {
-		drm_dbg_kms(connector->dev,
-			    "[CONNECTOR:%d:%s] Failed to enable scrambling: %d\n",
-			    connector->base.id, connector->name, ret);
+		drm_scdc_dbg(connector, "Failed to enable scrambling: %d\n", ret);
 		return false;
 	}
 
@@ -248,9 +246,7 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
 
 	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
 	if (ret < 0) {
-		drm_dbg_kms(connector->dev,
-			    "[CONNECTOR:%d:%s] Failed to read TMDS config: %d\n",
-			    connector->base.id, connector->name, ret);
+		drm_scdc_dbg(connector, "Failed to read TMDS config: %d\n", ret);
 		return false;
 	}
 
@@ -261,9 +257,7 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
 
 	ret = drm_scdc_writeb(connector->ddc, SCDC_TMDS_CONFIG, config);
 	if (ret < 0) {
-		drm_dbg_kms(connector->dev,
-			    "[CONNECTOR:%d:%s] Failed to set TMDS clock ratio: %d\n",
-			    connector->base.id, connector->name, ret);
+		drm_scdc_dbg(connector, "Failed to set TMDS clock ratio: %d\n", ret);
 		return false;
 	}
 

-- 
2.53.0

Claude review: drm/display: scdc_helper: Add macro to simplify debugging

[Claude Code Review Bot]

Patch Review

Clean refactoring. The `drm_scdc_dbg()` macro is a nice de-duplication of the connector-prefixed debug pattern.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 04/22] drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers

[Cristian Ciocaltea]

Add drm_scdc_start_scrambling(), drm_scdc_stop_scrambling() and
drm_scdc_sync_status() helpers to manage the full lifecycle of HDMI 2.0
SCDC scrambling on both source and sink sides.

drm_scdc_start_scrambling() configures SCDC scrambling and high TMDS
clock ratio and starts a periodic work item that monitors the sink's
SCDC scrambling status, retrying setup when the sink loses state.

drm_scdc_stop_scrambling() tears down scrambling on both sides and
cancels the monitoring work.

drm_scdc_sync_status() triggers a CRTC reset on reconnection to restore
SCDC state lost during sink disconnects within an active display
pipeline.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_scdc_helper.c | 235 +++++++++++++++++++++++++++++-
 include/drm/display/drm_scdc_helper.h     |   6 +-
 2 files changed, 236 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
index cb6632346aad..5bacb886d373 100644
--- a/drivers/gpu/drm/display/drm_scdc_helper.c
+++ b/drivers/gpu/drm/display/drm_scdc_helper.c
@@ -21,16 +21,22 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/i2c.h>
+#include <linux/minmax.h>
 #include <linux/slab.h>
-#include <linux/delay.h>
 
-#include <drm/display/drm_scdc_helper.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge_helper.h>
 #include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
 #include <drm/drm_print.h>
 
+#include <drm/display/drm_scdc_helper.h>
+
 /**
  * DOC: scdc helpers
  *
@@ -50,10 +56,14 @@
  * has to track the connector status changes using interrupts and
  * restore the SCDC status. The typical solution for this is to trigger an
  * empty modeset in drm_connector_helper_funcs.detect_ctx(), like what vc4 does
- * in vc4_hdmi_reset_link().
+ * in vc4_hdmi_reset_link(). Alternatively, use the HDMI connector framework
+ * which ensures drm_scdc_sync_status() is called in the context of
+ * drm_atomic_helper_connector_hdmi_hotplug_ctx().
  */
 
-#define SCDC_I2C_SLAVE_ADDRESS 0x54
+#define SCDC_I2C_SLAVE_ADDRESS		0x54
+#define SCDC_MAX_SOURCE_VERSION		0x1
+#define SCDC_STATUS_POLL_DELAY_MS	3000
 
 #define drm_scdc_dbg(connector, fmt, ...)					\
 	drm_dbg_kms((connector)->dev, "[CONNECTOR:%d:%s] " fmt,			\
@@ -270,3 +280,220 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
 	return true;
 }
 EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
+
+static int drm_scdc_setup_scrambler(struct drm_connector *connector)
+{
+	bool done;
+
+	done = drm_scdc_set_high_tmds_clock_ratio(connector, true);
+	if (!done)
+		return -EIO;
+
+	done = drm_scdc_set_scrambling(connector, true);
+	if (!done)
+		return -EIO;
+
+	schedule_delayed_work(&connector->hdmi.scdc_work,
+			      msecs_to_jiffies(SCDC_STATUS_POLL_DELAY_MS));
+	return 0;
+}
+
+static void drm_scdc_monitor_scrambler(struct drm_connector *connector)
+{
+	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
+	    !drm_scdc_get_scrambling_status(connector))
+		drm_scdc_setup_scrambler(connector);
+}
+
+static int drm_scdc_reset_crtc(struct drm_connector *connector,
+			       struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_crtc *crtc;
+	u8 config;
+	int ret;
+
+	if (!ctx || !connector->state)
+		return 0;
+
+	crtc = connector->state->crtc;
+	if (!crtc || !crtc->state || !crtc->state->active)
+		return 0;
+
+	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
+	if (ret) {
+		drm_scdc_dbg(connector, "Failed to read TMDS config: %d\n", ret);
+		return ret;
+	}
+
+	if ((config & SCDC_SCRAMBLING_ENABLE) &&
+	    (config & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40))
+		return 0;
+
+	/*
+	 * Reset the CRTC to suspend TMDS transmission, conforming to HDMI 2.0
+	 * spec which requires scrambled data not to be sent before the sink is
+	 * configured, and TMDS clock to be suspended while changing the clock
+	 * ratio.  The disable/re-enable cycle triggered by the reset should
+	 * call drm_scdc_start_scrambling() during re-enable, properly
+	 * configuring the sink before data transmission resumes.
+	 */
+
+	drm_scdc_dbg(connector, "Resetting CRTC to restore SCDC status\n");
+
+	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
+	if (ret && ret != -EDEADLOCK)
+		drm_scdc_dbg(connector, "Failed to reset CRTC: %d\n", ret);
+
+	return ret;
+}
+
+/**
+ * drm_scdc_start_scrambling - activate scrambling and monitor SCDC status
+ * @connector: connector
+ *
+ * Enables scrambling and high TMDS clock ratio on both source and sink sides.
+ * Additionally, use a delayed work item to monitor the scrambling status on
+ * the sink side and retry the operation, as some displays refuse to set the
+ * scrambling bit right away.
+ *
+ * Returns:
+ * Zero if scrambling is set successfully, an error code otherwise.
+ */
+int drm_scdc_start_scrambling(struct drm_connector *connector)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_connector_hdmi *hdmi = &connector->hdmi;
+	int ret;
+	u8 ver;
+
+	if (!hdmi->funcs ||
+	    !hdmi->funcs->scrambler_src_enable ||
+	    !hdmi->funcs->scrambler_src_disable) {
+		drm_scdc_dbg(connector, "Function not implemented, bailing.\n");
+		return -EINVAL;
+	}
+
+	if (!info->is_hdmi ||
+	    !info->hdmi.scdc.supported ||
+	    !info->hdmi.scdc.scrambling.supported) {
+		drm_scdc_dbg(connector, "Sink doesn't support scrambling.\n");
+		return -EINVAL;
+	}
+
+	drm_scdc_dbg(connector, "Enabling scrambling\n");
+
+	ret = drm_scdc_readb(connector->ddc, SCDC_SINK_VERSION, &ver);
+	if (ret) {
+		drm_scdc_dbg(connector, "Failed to read SCDC_SINK_VERSION: %d\n", ret);
+		return ret;
+	}
+
+	ret = drm_scdc_writeb(connector->ddc, SCDC_SOURCE_VERSION,
+			      min_t(u8, ver, SCDC_MAX_SOURCE_VERSION));
+	if (ret) {
+		drm_scdc_dbg(connector, "Failed to write SCDC_SOURCE_VERSION: %d\n", ret);
+		return ret;
+	}
+
+	hdmi->scdc_cb = drm_scdc_monitor_scrambler;
+	WRITE_ONCE(hdmi->scrambler_enabled, true);
+
+	ret = drm_scdc_setup_scrambler(connector);
+	if (!ret)
+		ret = hdmi->funcs->scrambler_src_enable(connector);
+
+	if (ret) {
+		WRITE_ONCE(hdmi->scrambler_enabled, false);
+		cancel_delayed_work_sync(&hdmi->scdc_work);
+		hdmi->scdc_cb = NULL;
+
+		drm_scdc_set_scrambling(connector, false);
+		drm_scdc_set_high_tmds_clock_ratio(connector, false);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_scdc_start_scrambling);
+
+/**
+ * drm_scdc_stop_scrambling - deactivate scrambling and SCDC status monitor
+ * @connector: connector
+ *
+ * Disables scrambling and high TMDS clock ratio on both source and sink sides.
+ * Also cancels the SCDC status monitoring work item, if it is still pending.
+ *
+ * Returns:
+ * Zero if scrambling is reset successfully, an error code otherwise.
+ */
+int drm_scdc_stop_scrambling(struct drm_connector *connector)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_connector_hdmi *hdmi = &connector->hdmi;
+
+	if (!hdmi->funcs ||
+	    !hdmi->funcs->scrambler_src_disable) {
+		drm_scdc_dbg(connector, "Function not implemented, bailing.\n");
+		return -EINVAL;
+	}
+
+	if (!READ_ONCE(hdmi->scrambler_enabled))
+		return 0;
+
+	drm_scdc_dbg(connector, "Disabling scrambling\n");
+
+	WRITE_ONCE(hdmi->scrambler_enabled, false);
+	cancel_delayed_work_sync(&hdmi->scdc_work);
+	hdmi->scdc_cb = NULL;
+
+	if (connector->status == connector_status_connected &&
+	    info->is_hdmi && info->hdmi.scdc.supported &&
+	    info->hdmi.scdc.scrambling.supported) {
+		drm_scdc_set_scrambling(connector, false);
+		drm_scdc_set_high_tmds_clock_ratio(connector, false);
+	}
+
+	return hdmi->funcs->scrambler_src_disable(connector);
+}
+EXPORT_SYMBOL(drm_scdc_stop_scrambling);
+
+/**
+ * drm_scdc_sync_status - resync the sink-side SCDC upon reconnect
+ * @connector: connector
+ * @plugged: connector plugged status event
+ * @ctx: lock acquisition context
+ *
+ * When receiving hotplug disconnect/reconnect event, while the display is
+ * still active (CRTC enabled), the SCDC status on the sink side is reset
+ * and must be explicitly restored.
+ *
+ * The typical solution for this is to trigger an empty modeset in
+ * drm_connector_helper_funcs.detect_ctx(), which is what this helper does
+ * by triggering a CRTC reset on reconnection.
+ *
+ * When making use of the HDMI connector framework, this is automatically
+ * triggered via drm_atomic_helper_connector_hdmi_hotplug_ctx().
+ *
+ * Returns:
+ * Zero on success, an error code otherwise, including -EDEADLOCK.
+ */
+int drm_scdc_sync_status(struct drm_connector *connector, bool plugged,
+			 struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_connector_hdmi *hdmi = &connector->hdmi;
+
+	if (!hdmi->funcs)
+		return 0;
+
+	if (plugged && READ_ONCE(hdmi->scrambler_enabled)) {
+		if (!hdmi->funcs->scrambler_src_enable ||
+		    !hdmi->funcs->scrambler_src_disable)
+			return 0;
+
+		return drm_scdc_reset_crtc(connector, ctx);
+	}
+
+	// TODO: Also handle HDMI 2.1 FRL link training
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_scdc_sync_status);
diff --git a/include/drm/display/drm_scdc_helper.h b/include/drm/display/drm_scdc_helper.h
index 34600476a1b9..5d9a37bbb362 100644
--- a/include/drm/display/drm_scdc_helper.h
+++ b/include/drm/display/drm_scdc_helper.h
@@ -29,6 +29,7 @@
 #include <drm/display/drm_scdc.h>
 
 struct drm_connector;
+struct drm_modeset_acquire_ctx;
 struct i2c_adapter;
 
 ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
@@ -76,5 +77,8 @@ bool drm_scdc_get_scrambling_status(struct drm_connector *connector);
 
 bool drm_scdc_set_scrambling(struct drm_connector *connector, bool enable);
 bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector, bool set);
-
+int drm_scdc_start_scrambling(struct drm_connector *connector);
+int drm_scdc_stop_scrambling(struct drm_connector *connector);
+int drm_scdc_sync_status(struct drm_connector *connector, bool plugged,
+			 struct drm_modeset_acquire_ctx *ctx);
 #endif

-- 
2.53.0

Re: [PATCH v6 04/22] drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6904 bytes --]

On Wed, May 20, 2026 at 09:38:15PM +0300, Cristian Ciocaltea wrote:
> Add drm_scdc_start_scrambling(), drm_scdc_stop_scrambling() and
> drm_scdc_sync_status() helpers to manage the full lifecycle of HDMI 2.0
> SCDC scrambling on both source and sink sides.
> 
> drm_scdc_start_scrambling() configures SCDC scrambling and high TMDS
> clock ratio and starts a periodic work item that monitors the sink's
> SCDC scrambling status, retrying setup when the sink loses state.
> 
> drm_scdc_stop_scrambling() tears down scrambling on both sides and
> cancels the monitoring work.
> 
> drm_scdc_sync_status() triggers a CRTC reset on reconnection to restore
> SCDC state lost during sink disconnects within an active display
> pipeline.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_scdc_helper.c | 235 +++++++++++++++++++++++++++++-
>  include/drm/display/drm_scdc_helper.h     |   6 +-
>  2 files changed, 236 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> index cb6632346aad..5bacb886d373 100644
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
> @@ -21,16 +21,22 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/export.h>
>  #include <linux/i2c.h>
> +#include <linux/minmax.h>
>  #include <linux/slab.h>
> -#include <linux/delay.h>
>  
> -#include <drm/display/drm_scdc_helper.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge_helper.h>
>  #include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
>  #include <drm/drm_device.h>
>  #include <drm/drm_print.h>
>  
> +#include <drm/display/drm_scdc_helper.h>
> +
>  /**
>   * DOC: scdc helpers
>   *
> @@ -50,10 +56,14 @@
>   * has to track the connector status changes using interrupts and
>   * restore the SCDC status. The typical solution for this is to trigger an
>   * empty modeset in drm_connector_helper_funcs.detect_ctx(), like what vc4 does
> - * in vc4_hdmi_reset_link().
> + * in vc4_hdmi_reset_link(). Alternatively, use the HDMI connector framework
> + * which ensures drm_scdc_sync_status() is called in the context of
> + * drm_atomic_helper_connector_hdmi_hotplug_ctx().
>   */
>  
> -#define SCDC_I2C_SLAVE_ADDRESS 0x54
> +#define SCDC_I2C_SLAVE_ADDRESS		0x54
> +#define SCDC_MAX_SOURCE_VERSION		0x1
> +#define SCDC_STATUS_POLL_DELAY_MS	3000
>  
>  #define drm_scdc_dbg(connector, fmt, ...)					\
>  	drm_dbg_kms((connector)->dev, "[CONNECTOR:%d:%s] " fmt,			\
> @@ -270,3 +280,220 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
>  	return true;
>  }
>  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
> +
> +static int drm_scdc_setup_scrambler(struct drm_connector *connector)
> +{
> +	bool done;
> +
> +	done = drm_scdc_set_high_tmds_clock_ratio(connector, true);
> +	if (!done)
> +		return -EIO;
> +
> +	done = drm_scdc_set_scrambling(connector, true);
> +	if (!done)
> +		return -EIO;
> +
> +	schedule_delayed_work(&connector->hdmi.scdc_work,
> +			      msecs_to_jiffies(SCDC_STATUS_POLL_DELAY_MS));
> +	return 0;
> +}

There's a very tight deadline in the spec to enable the scrambler
relative to the video. Debouncing (I assume?) for three seconds break
it. Drivers might not be able to do better, but it's really not
something we should entertain at the framework level and we should call
the callback straight-away.

> +static void drm_scdc_monitor_scrambler(struct drm_connector *connector)
> +{
> +	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> +	    !drm_scdc_get_scrambling_status(connector))
> +		drm_scdc_setup_scrambler(connector);
> +}
> +
> +static int drm_scdc_reset_crtc(struct drm_connector *connector,
> +			       struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_crtc *crtc;
> +	u8 config;
> +	int ret;
> +
> +	if (!ctx || !connector->state)
> +		return 0;
> +
> +	crtc = connector->state->crtc;
> +	if (!crtc || !crtc->state || !crtc->state->active)
> +		return 0;
> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
> +	if (ret) {
> +		drm_scdc_dbg(connector, "Failed to read TMDS config: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if ((config & SCDC_SCRAMBLING_ENABLE) &&
> +	    (config & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40))
> +		return 0;
> +
> +	/*
> +	 * Reset the CRTC to suspend TMDS transmission, conforming to HDMI 2.0
> +	 * spec which requires scrambled data not to be sent before the sink is
> +	 * configured, and TMDS clock to be suspended while changing the clock
> +	 * ratio.  The disable/re-enable cycle triggered by the reset should
> +	 * call drm_scdc_start_scrambling() during re-enable, properly
> +	 * configuring the sink before data transmission resumes.
> +	 */
> +
> +	drm_scdc_dbg(connector, "Resetting CRTC to restore SCDC status\n");
> +
> +	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
> +	if (ret && ret != -EDEADLOCK)
> +		drm_scdc_dbg(connector, "Failed to reset CRTC: %d\n", ret);
> +
> +	return ret;
> +}

locking is a bit more involved than that, I'd suggest to take
vc4_hdmi_reset_link() and turn it into a helper.

> +/**
> + * drm_scdc_start_scrambling - activate scrambling and monitor SCDC status
> + * @connector: connector
> + *
> + * Enables scrambling and high TMDS clock ratio on both source and sink sides.
> + * Additionally, use a delayed work item to monitor the scrambling status on
> + * the sink side and retry the operation, as some displays refuse to set the
> + * scrambling bit right away.
> + *
> + * Returns:
> + * Zero if scrambling is set successfully, an error code otherwise.
> + */
> +int drm_scdc_start_scrambling(struct drm_connector *connector)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
> +	int ret;
> +	u8 ver;
> +
> +	if (!hdmi->funcs ||
> +	    !hdmi->funcs->scrambler_src_enable ||
> +	    !hdmi->funcs->scrambler_src_disable) {
> +		drm_scdc_dbg(connector, "Function not implemented, bailing.\n");
> +		return -EINVAL;
> +	}

This case shouldn't be a thing. The driver must not probe if it's not set.

> +	if (!info->is_hdmi ||
> +	    !info->hdmi.scdc.supported ||
> +	    !info->hdmi.scdc.scrambling.supported) {
> +		drm_scdc_dbg(connector, "Sink doesn't support scrambling.\n");
> +		return -EINVAL;
> +	}

You should also compute whether we actually need to enable the scrambler
here, based on the mode, format and bpc.

I'd also like to see what a !bridge user of this would look like. The
vc4 driver has all the infrastructure you need already, so converting it
to it would be great.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v6 04/22] drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers

[Cristian Ciocaltea]

On 5/21/26 11:10 AM, Maxime Ripard wrote:
> On Wed, May 20, 2026 at 09:38:15PM +0300, Cristian Ciocaltea wrote:
>> Add drm_scdc_start_scrambling(), drm_scdc_stop_scrambling() and
>> drm_scdc_sync_status() helpers to manage the full lifecycle of HDMI 2.0
>> SCDC scrambling on both source and sink sides.
>>
>> drm_scdc_start_scrambling() configures SCDC scrambling and high TMDS
>> clock ratio and starts a periodic work item that monitors the sink's
>> SCDC scrambling status, retrying setup when the sink loses state.
>>
>> drm_scdc_stop_scrambling() tears down scrambling on both sides and
>> cancels the monitoring work.
>>
>> drm_scdc_sync_status() triggers a CRTC reset on reconnection to restore
>> SCDC state lost during sink disconnects within an active display
>> pipeline.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_scdc_helper.c | 235 +++++++++++++++++++++++++++++-
>>  include/drm/display/drm_scdc_helper.h     |   6 +-
>>  2 files changed, 236 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
>> index cb6632346aad..5bacb886d373 100644
>> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
>> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
>> @@ -21,16 +21,22 @@
>>   * DEALINGS IN THE SOFTWARE.
>>   */
>>  
>> +#include <linux/delay.h>
>>  #include <linux/export.h>
>>  #include <linux/i2c.h>
>> +#include <linux/minmax.h>
>>  #include <linux/slab.h>
>> -#include <linux/delay.h>
>>  
>> -#include <drm/display/drm_scdc_helper.h>
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_bridge_helper.h>
>>  #include <drm/drm_connector.h>
>> +#include <drm/drm_crtc.h>
>>  #include <drm/drm_device.h>
>>  #include <drm/drm_print.h>
>>  
>> +#include <drm/display/drm_scdc_helper.h>
>> +
>>  /**
>>   * DOC: scdc helpers
>>   *
>> @@ -50,10 +56,14 @@
>>   * has to track the connector status changes using interrupts and
>>   * restore the SCDC status. The typical solution for this is to trigger an
>>   * empty modeset in drm_connector_helper_funcs.detect_ctx(), like what vc4 does
>> - * in vc4_hdmi_reset_link().
>> + * in vc4_hdmi_reset_link(). Alternatively, use the HDMI connector framework
>> + * which ensures drm_scdc_sync_status() is called in the context of
>> + * drm_atomic_helper_connector_hdmi_hotplug_ctx().
>>   */
>>  
>> -#define SCDC_I2C_SLAVE_ADDRESS 0x54
>> +#define SCDC_I2C_SLAVE_ADDRESS		0x54
>> +#define SCDC_MAX_SOURCE_VERSION		0x1
>> +#define SCDC_STATUS_POLL_DELAY_MS	3000
>>  
>>  #define drm_scdc_dbg(connector, fmt, ...)					\
>>  	drm_dbg_kms((connector)->dev, "[CONNECTOR:%d:%s] " fmt,			\
>> @@ -270,3 +280,220 @@ bool drm_scdc_set_high_tmds_clock_ratio(struct drm_connector *connector,
>>  	return true;
>>  }
>>  EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
>> +
>> +static int drm_scdc_setup_scrambler(struct drm_connector *connector)
>> +{
>> +	bool done;
>> +
>> +	done = drm_scdc_set_high_tmds_clock_ratio(connector, true);
>> +	if (!done)
>> +		return -EIO;
>> +
>> +	done = drm_scdc_set_scrambling(connector, true);
>> +	if (!done)
>> +		return -EIO;
>> +
>> +	schedule_delayed_work(&connector->hdmi.scdc_work,
>> +			      msecs_to_jiffies(SCDC_STATUS_POLL_DELAY_MS));
>> +	return 0;
>> +}
> 
> There's a very tight deadline in the spec to enable the scrambler
> relative to the video. Debouncing (I assume?) for three seconds break
> it. Drivers might not be able to do better, but it's really not
> something we should entertain at the framework level and we should call
> the callback straight-away.

In drm_scdc_start_scrambling() we call this function directly, the callback is
only meant to retry the call if the sink didn't set the scrambling for whatever
reason.

Or do you mean we shouldn't wait that long before retrying? vc4 uses 1s, I got
some mixed results in a few occasions and experimented with increased values,
but I'll revert or we can adjust to whatever you think it'd be more sensible.

>> +static void drm_scdc_monitor_scrambler(struct drm_connector *connector)
>> +{
>> +	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
>> +	    !drm_scdc_get_scrambling_status(connector))
>> +		drm_scdc_setup_scrambler(connector);
>> +}
>> +
>> +static int drm_scdc_reset_crtc(struct drm_connector *connector,
>> +			       struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	struct drm_crtc *crtc;
>> +	u8 config;
>> +	int ret;
>> +
>> +	if (!ctx || !connector->state)
>> +		return 0;
>> +
>> +	crtc = connector->state->crtc;
>> +	if (!crtc || !crtc->state || !crtc->state->active)
>> +		return 0;
>> +
>> +	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
>> +	if (ret) {
>> +		drm_scdc_dbg(connector, "Failed to read TMDS config: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if ((config & SCDC_SCRAMBLING_ENABLE) &&
>> +	    (config & SCDC_TMDS_BIT_CLOCK_RATIO_BY_40))
>> +		return 0;
>> +
>> +	/*
>> +	 * Reset the CRTC to suspend TMDS transmission, conforming to HDMI 2.0
>> +	 * spec which requires scrambled data not to be sent before the sink is
>> +	 * configured, and TMDS clock to be suspended while changing the clock
>> +	 * ratio.  The disable/re-enable cycle triggered by the reset should
>> +	 * call drm_scdc_start_scrambling() during re-enable, properly
>> +	 * configuring the sink before data transmission resumes.
>> +	 */
>> +
>> +	drm_scdc_dbg(connector, "Resetting CRTC to restore SCDC status\n");
>> +
>> +	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
>> +	if (ret && ret != -EDEADLOCK)
>> +		drm_scdc_dbg(connector, "Failed to reset CRTC: %d\n", ret);
>> +
>> +	return ret;
>> +}
> 
> locking is a bit more involved than that, I'd suggest to take
> vc4_hdmi_reset_link() and turn it into a helper.

The current non-NULL ctx call chain goes through drm_helper_probe_detect_ctx()
-> .detect_ctx -> ... -> drm_scdc_sync_status() -> drm_scdc_reset_crtc(), and
the connection mutex is already held by drm_helper_probe_detect() in that path. 

I'll make the helper safe under any caller by taking the connection_mutex via
the supplied ctx, since re-acquiring it with the same context when it is already
held should be a no-op.  Moreover, the racy crtc->state->active early out could
be dropped: when the CRTC is inactive, drm_atomic_helper_reset_crtc() should
result in a no-op commit anyway, so the check is at best an optimisation. 
This mirrors what drm_bridge_helper_reset_crtc() does.

>> +/**
>> + * drm_scdc_start_scrambling - activate scrambling and monitor SCDC status
>> + * @connector: connector
>> + *
>> + * Enables scrambling and high TMDS clock ratio on both source and sink sides.
>> + * Additionally, use a delayed work item to monitor the scrambling status on
>> + * the sink side and retry the operation, as some displays refuse to set the
>> + * scrambling bit right away.
>> + *
>> + * Returns:
>> + * Zero if scrambling is set successfully, an error code otherwise.
>> + */
>> +int drm_scdc_start_scrambling(struct drm_connector *connector)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
>> +	int ret;
>> +	u8 ver;
>> +
>> +	if (!hdmi->funcs ||
>> +	    !hdmi->funcs->scrambler_src_enable ||
>> +	    !hdmi->funcs->scrambler_src_disable) {
>> +		drm_scdc_dbg(connector, "Function not implemented, bailing.\n");
>> +		return -EINVAL;
>> +	}
> 
> This case shouldn't be a thing. The driver must not probe if it's not set.

When advertising DRM_BRIDGE_OP_HDMI_SCRAMBLER, drm_bridge_connector_init() will
fail if the scrambler hooks are not provided.  However, the check would be still
useful if one missed to set DRM_BRIDGE_OP_HDMI_SCRAMBLER before adding the
bridge.  I'll try to also handle non-bridge usecases - pls see the proposal
below.

>> +	if (!info->is_hdmi ||
>> +	    !info->hdmi.scdc.supported ||
>> +	    !info->hdmi.scdc.scrambling.supported) {
>> +		drm_scdc_dbg(connector, "Sink doesn't support scrambling.\n");
>> +		return -EINVAL;
>> +	}
> 
> You should also compute whether we actually need to enable the scrambler
> here, based on the mode, format and bpc.

I'm thinking to mirror the existing supported_formats / ycbcr_420_allowed
plumbing:

- Add a 'scrambling_supported' flag to struct drm_connector_hdmi and let
  drm_bridge_connector_init() to propagates it from the HDMI bridge before
  calling drmm_connector_hdmi_init().

- drmm_connector_hdmi_init() validates that the capability is consistent with
  the provided callbacks: when scrambling_supported is true, both
  scrambler_src_enable() and scrambler_src_disable() must be provided.

- Add a 'scrambling_needed' flag to struct drm_connector_hdmi_state to be set by
  hdmi_compute_clock() based on the TMDS character rate and the source/sink
  scrambling capabilities.

- Ensure hdmi_clock_valid() rejects modes having TMDS character rate exceeding
  340 MHz when either the source or the sink lacks the required SCDC scrambling
  capability.

> I'd also like to see what a !bridge user of this would look like. The
> vc4 driver has all the infrastructure you need already, so converting it
> to it would be great.

Sure, will do.  I've just ordered a RPI5 board, so I should be able to properly
validate this.

Thanks,
Cristian

Claude review: drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers

[Claude Code Review Bot]

Patch Review

This is the core patch. Several observations:

1. **`drm_scdc_monitor_scrambler()` re-schedules unconditionally**: In `drm_scdc_setup_scrambler()`, the delayed work is always scheduled when called. If the monitor callback finds that scrambling status was lost and calls `drm_scdc_setup_scrambler()` again, it re-schedules itself, which is correct periodic monitoring behavior.

2. **`drm_scdc_reset_crtc()` accesses `connector->state` without holding locks**: The function reads `connector->state->crtc` and `crtc->state->active` outside of any modeset lock. While this is in a detect context where the connection_mutex may be held, the connector state access is still technically racy. However, this mirrors what `drm_bridge_helper_reset_crtc()` does in the existing codebase, so it follows established (if imperfect) patterns.

3. **`// TODO: Also handle HDMI 2.1 FRL link training`**: C++ style comment. Kernel style prefers `/* */`. Minor, but will likely get flagged by checkpatch.

4. **Error handling in `drm_scdc_start_scrambling()`**: The rollback on failure is thorough — cancels the work, clears the callback, and resets both scrambling and clock ratio on the sink. Good.

5. **`drm_scdc_sync_status()` early returns**: The function returns 0 when `hdmi->funcs` is NULL, and also returns 0 when scrambler callbacks are not set. This means non-HDMI-2.0 bridges silently succeed, which is the right behavior.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 05/22] drm/display: hdmi_state_helper: Add ctx-aware hotplug helper for SCDC sync

[Cristian Ciocaltea]

Introduce drm_atomic_helper_connector_hdmi_hotplug_ctx(), a variant of
drm_atomic_helper_connector_hdmi_hotplug() that accepts a
drm_modeset_acquire_ctx.

This enables SCDC status synchronization on hotplug events, which
requires lock acquisition context for performing the CRTC reset
triggered by drm_scdc_sync_status().

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 36 ++++++++++++++++++++-----
 include/drm/display/drm_hdmi_state_helper.h     |  4 +++
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 4867edbf2622..26c491047a6a 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -12,6 +12,7 @@
 #include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
+#include <drm/display/drm_scdc_helper.h>
 
 /**
  * DOC: hdmi helpers
@@ -1149,18 +1150,20 @@ drm_atomic_helper_connector_hdmi_clear_audio_infoframe(struct drm_connector *con
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_clear_audio_infoframe);
 
-static void
+static int
 drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
-					enum drm_connector_status status)
+					enum drm_connector_status status,
+					struct drm_modeset_acquire_ctx *ctx)
 {
 	const struct drm_edid *drm_edid;
+	int ret = 0;
 
 	if (status == connector_status_disconnected) {
-		// TODO: also handle scramber, HDMI sink disconnected.
+		drm_scdc_sync_status(connector, false, ctx);
 		drm_connector_hdmi_audio_plugged_notify(connector, false);
 		drm_edid_connector_update(connector, NULL);
 		drm_connector_cec_phys_addr_invalidate(connector);
-		return;
+		return 0;
 	}
 
 	if (connector->hdmi.funcs->read_edid)
@@ -1173,10 +1176,12 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
 	drm_edid_free(drm_edid);
 
 	if (status == connector_status_connected) {
-		// TODO: also handle scramber, HDMI sink is now connected.
+		ret = drm_scdc_sync_status(connector, true, ctx);
 		drm_connector_hdmi_audio_plugged_notify(connector, true);
 		drm_connector_cec_phys_addr_set(connector);
 	}
+
+	return ret;
 }
 
 /**
@@ -1190,10 +1195,27 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
 void drm_atomic_helper_connector_hdmi_hotplug(struct drm_connector *connector,
 					      enum drm_connector_status status)
 {
-	drm_atomic_helper_connector_hdmi_update(connector, status);
+	drm_atomic_helper_connector_hdmi_update(connector, status, NULL);
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_hotplug);
 
+/**
+ * drm_atomic_helper_connector_hdmi_hotplug_ctx - Handle the hotplug event for the HDMI connector
+ * @connector: A pointer to the HDMI connector
+ * @status: Connection status
+ * @ctx: Lock acquisition context to be used for resetting CRTC
+ *
+ * This function should be called as a part of the .detect() / .detect_ctx()
+ * callbacks for all status changes.
+ */
+int drm_atomic_helper_connector_hdmi_hotplug_ctx(struct drm_connector *connector,
+						 enum drm_connector_status status,
+						 struct drm_modeset_acquire_ctx *ctx)
+{
+	return drm_atomic_helper_connector_hdmi_update(connector, status, ctx);
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_hotplug_ctx);
+
 /**
  * drm_atomic_helper_connector_hdmi_force - HDMI Connector implementation of the force callback
  * @connector: A pointer to the HDMI connector
@@ -1205,6 +1227,6 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_hotplug);
  */
 void drm_atomic_helper_connector_hdmi_force(struct drm_connector *connector)
 {
-	drm_atomic_helper_connector_hdmi_update(connector, connector->status);
+	drm_atomic_helper_connector_hdmi_update(connector, connector->status, NULL);
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_force);
diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h
index 0adc30c55ec9..a572fe2bf9aa 100644
--- a/include/drm/display/drm_hdmi_state_helper.h
+++ b/include/drm/display/drm_hdmi_state_helper.h
@@ -7,6 +7,7 @@ struct drm_atomic_commit;
 struct drm_connector;
 struct drm_connector_state;
 struct drm_display_mode;
+struct drm_modeset_acquire_ctx;
 struct hdmi_audio_infoframe;
 
 enum drm_connector_status;
@@ -24,6 +25,9 @@ int drm_atomic_helper_connector_hdmi_update_infoframes(struct drm_connector *con
 						       struct drm_atomic_commit *state);
 void drm_atomic_helper_connector_hdmi_hotplug(struct drm_connector *connector,
 					      enum drm_connector_status status);
+int drm_atomic_helper_connector_hdmi_hotplug_ctx(struct drm_connector *connector,
+						 enum drm_connector_status status,
+						 struct drm_modeset_acquire_ctx *ctx);
 void drm_atomic_helper_connector_hdmi_force(struct drm_connector *connector);
 
 enum drm_mode_status

-- 
2.53.0

Claude review: drm/display: hdmi_state_helper: Add ctx-aware hotplug helper for SCDC sync

[Claude Code Review Bot]

Patch Review

The approach of adding a `_ctx` variant alongside the existing context-free `drm_atomic_helper_connector_hdmi_hotplug()` is clean. The function signature change of `drm_atomic_helper_connector_hdmi_update()` from `void` to `int` is well-motivated.

One observation: on the disconnect path, `drm_scdc_sync_status(connector, false, ctx)` is called, but its return value is ignored. This is deliberate — on disconnect, there's nothing to recover, and the function currently returns 0 for the unplug case anyway. Fine.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 06/22] drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

[Cristian Ciocaltea]

Remove the no-op error check after drm_atomic_helper_reset_crtc() since
the goto target is the immediately following label and the return value
is already propagated correctly without it.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/drm_bridge_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_helper.c b/drivers/gpu/drm/drm_bridge_helper.c
index 420f29cf3e54..0a3c8fee66b3 100644
--- a/drivers/gpu/drm/drm_bridge_helper.c
+++ b/drivers/gpu/drm/drm_bridge_helper.c
@@ -50,8 +50,6 @@ int drm_bridge_helper_reset_crtc(struct drm_bridge *bridge,
 
 	crtc = connector->state->crtc;
 	ret = drm_atomic_helper_reset_crtc(crtc, ctx);
-	if (ret)
-		goto out;
 
 out:
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);

-- 
2.53.0

Claude review: drm/bridge: Remove redundant error check in drm_bridge_helper_reset_crtc()

[Claude Code Review Bot]

Patch Review

Trivial correct cleanup. The `if (ret) goto out;` was a no-op since `out:` immediately follows.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 07/22] drm/bridge: Add HDMI 2.0 scrambler bridge operation and callbacks

[Cristian Ciocaltea]

Add DRM_BRIDGE_OP_HDMI_SCRAMBLER bridge operation flag and the
corresponding .hdmi_scrambler_{enable|disable}() bridge funcs callbacks.

Bridge drivers set DRM_BRIDGE_OP_HDMI_SCRAMBLER to advertise that they
implement source-side scrambling control, which the bridge connector
layer uses to wire up the connector's scrambler callbacks.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 include/drm/drm_bridge.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4ba3a5deef9a..a0fcbbc7086e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -667,6 +667,26 @@ struct drm_bridge_funcs {
 				     const struct drm_display_mode *mode,
 				     unsigned long long tmds_rate);
 
+	/**
+	 * @hdmi_scrambler_enable:
+	 *
+	 * Enable HDMI 2.0 SCDC scrambling and high TMDS clock ratio.
+	 *
+	 * This callback is optional but it must be implemented by bridges that
+	 * set the DRM_BRIDGE_OP_HDMI_SCRAMBLER flag in their &drm_bridge->ops.
+	 */
+	int (*hdmi_scrambler_enable)(struct drm_bridge *bridge);
+
+	/**
+	 * @hdmi_scrambler_disable:
+	 *
+	 * Disable HDMI 2.0 SCDC scrambling and high TMDS clock ratio.
+	 *
+	 * This callback is optional but it must be implemented by bridges that
+	 * set the DRM_BRIDGE_OP_HDMI_SCRAMBLER flag in their &drm_bridge->ops.
+	 */
+	int (*hdmi_scrambler_disable)(struct drm_bridge *bridge);
+
 	/**
 	 * @hdmi_clear_avi_infoframe:
 	 *
@@ -1092,6 +1112,12 @@ enum drm_bridge_ops {
 	 * &drm_bridge_funcs->hdmi_clear_spd_infoframe callbacks.
 	 */
 	DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME = BIT(10),
+	/**
+	 * @DRM_BRIDGE_OP_HDMI_SCRAMBLER: The bridge supports
+	 * &drm_bridge_funcs->hdmi_scrambler_enable and
+	 * &drm_bridge_funcs->hdmi_scrambler_disable callbacks.
+	 */
+	DRM_BRIDGE_OP_HDMI_SCRAMBLER = BIT(11),
 };
 
 /**

-- 
2.53.0

Claude review: drm/bridge: Add HDMI 2.0 scrambler bridge operation and callbacks

[Claude Code Review Bot]

Patch Review

Clean addition of `DRM_BRIDGE_OP_HDMI_SCRAMBLER = BIT(11)` and the corresponding `hdmi_scrambler_enable`/`hdmi_scrambler_disable` callbacks. The documentation clearly states these are mandatory when the flag is set. No issues.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 08/22] drm/display: bridge_connector: Use cached connector status in .get_modes()

[Cristian Ciocaltea]

Replace the active drm_bridge_connector_detect() call in get_modes()
with a read of the already-cached connector->status.

The .get_modes() callback is only invoked from
drm_helper_probe_single_connector_modes(), which has already retrieved
the connector status.  Calling detect again is redundant and triggers a
duplicate hotplug event.  This is also a prerequisite for switching to
the .detect_ctx() hook, which requires a drm_modeset_acquire_ctx not
available in the .get_modes() path.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 649969fca141..b69785eb49e2 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -294,12 +294,10 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
 					       struct drm_bridge *bridge)
 {
-	enum drm_connector_status status;
 	const struct drm_edid *drm_edid;
 	int n;
 
-	status = drm_bridge_connector_detect(connector, false);
-	if (status != connector_status_connected)
+	if (connector->status != connector_status_connected)
 		goto no_edid;
 
 	drm_edid = drm_bridge_edid_read(bridge, connector);

-- 
2.53.0

Claude review: drm/display: bridge_connector: Use cached connector status in .get_modes()

[Claude Code Review Bot]

Patch Review

Good correctness improvement. The old code called `drm_bridge_connector_detect()` from `get_modes()`, which was redundant since `drm_helper_probe_single_connector_modes()` has already populated `connector->status`. Using the cached value avoids duplicate detect calls and the duplicate hotplug event that came with them. This is also a necessary prerequisite for the `detect_ctx` switch.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 09/22] drm/display: bridge_connector: Switch to .detect_ctx() connector helper

[Cristian Ciocaltea]

Replace the .detect() connector_funcs callback with the atomic-aware
.detect_ctx() connector_helper_funcs hook.

This propagates the modeset acquire context through
drm_atomic_helper_connector_hdmi_hotplug_ctx() to the HDMI connector
framework, enabling SCDC state recovery via CRTC reset when the sink
reconnects during an active display pipeline.

Tested-by: Diederik de Haas <diederik@cknow-tech.com>
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 74 ++++++++++++++------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index b69785eb49e2..47bb0dcf509f 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -208,39 +208,6 @@ static void drm_bridge_connector_disable_hpd(struct drm_connector *connector)
  * Bridge Connector Functions
  */
 
-static enum drm_connector_status
-drm_bridge_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct drm_bridge_connector *bridge_connector =
-		to_drm_bridge_connector(connector);
-	struct drm_bridge *detect = bridge_connector->bridge_detect;
-	struct drm_bridge *hdmi = bridge_connector->bridge_hdmi;
-	enum drm_connector_status status;
-
-	if (detect) {
-		status = detect->funcs->detect(detect, connector);
-
-		if (hdmi)
-			drm_atomic_helper_connector_hdmi_hotplug(connector, status);
-
-		drm_bridge_connector_hpd_notify(connector, status);
-	} else {
-		switch (connector->connector_type) {
-		case DRM_MODE_CONNECTOR_DPI:
-		case DRM_MODE_CONNECTOR_LVDS:
-		case DRM_MODE_CONNECTOR_DSI:
-		case DRM_MODE_CONNECTOR_eDP:
-			status = connector_status_connected;
-			break;
-		default:
-			status = connector_status_unknown;
-			break;
-		}
-	}
-
-	return status;
-}
-
 static void drm_bridge_connector_force(struct drm_connector *connector)
 {
 	struct drm_bridge_connector *bridge_connector =
@@ -278,7 +245,6 @@ static void drm_bridge_connector_reset(struct drm_connector *connector)
 
 static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 	.reset = drm_bridge_connector_reset,
-	.detect = drm_bridge_connector_detect,
 	.force = drm_bridge_connector_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -291,6 +257,45 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
  * Bridge Connector Helper Functions
  */
 
+static int drm_bridge_connector_detect_ctx(struct drm_connector *connector,
+					   struct drm_modeset_acquire_ctx *ctx,
+					   bool force)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *detect = bridge_connector->bridge_detect;
+	struct drm_bridge *hdmi = bridge_connector->bridge_hdmi;
+	enum drm_connector_status status;
+	int ret;
+
+	if (detect) {
+		status = detect->funcs->detect(detect, connector);
+
+		if (hdmi) {
+			ret = drm_atomic_helper_connector_hdmi_hotplug_ctx(connector,
+									   status, ctx);
+			if (ret == -EDEADLOCK)
+				return ret;
+		}
+
+		drm_bridge_connector_hpd_notify(connector, status);
+	} else {
+		switch (connector->connector_type) {
+		case DRM_MODE_CONNECTOR_DPI:
+		case DRM_MODE_CONNECTOR_LVDS:
+		case DRM_MODE_CONNECTOR_DSI:
+		case DRM_MODE_CONNECTOR_eDP:
+			status = connector_status_connected;
+			break;
+		default:
+			status = connector_status_unknown;
+			break;
+		}
+	}
+
+	return status;
+}
+
 static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
 					       struct drm_bridge *bridge)
 {
@@ -382,6 +387,7 @@ static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
 
 static const struct drm_connector_helper_funcs drm_bridge_connector_helper_funcs = {
 	.get_modes = drm_bridge_connector_get_modes,
+	.detect_ctx = drm_bridge_connector_detect_ctx,
 	.mode_valid = drm_bridge_connector_mode_valid,
 	.enable_hpd = drm_bridge_connector_enable_hpd,
 	.disable_hpd = drm_bridge_connector_disable_hpd,

-- 
2.53.0

Claude review: drm/display: bridge_connector: Switch to .detect_ctx() connector helper

[Claude Code Review Bot]

Patch Review

This is an important structural change. The `.detect()` callback on `drm_connector_funcs` is replaced with `.detect_ctx()` on `drm_connector_helper_funcs`.

**Key concern — non-EDEADLOCK error handling**: When `drm_atomic_helper_connector_hdmi_hotplug_ctx()` returns a non-EDEADLOCK error, the code falls through and still returns `status` (cast to int). Since connector_status enum values are non-negative (0=unknown, 1=connected, 2=disconnected), and errors are negative, the caller can distinguish them. But the *error from the SCDC sync is silently ignored* — the connector status is reported normally even if the CRTC reset failed. This is probably intentional (you still want to report the connection, even if SCDC recovery failed), but worth noting.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 10/22] drm/display: bridge_connector: Wire up HDMI 2.0 scrambler callbacks

[Cristian Ciocaltea]

Connect the bridge connector's .scrambler_src_{enable|disable}()
callbacks to the underlying bridge's .hdmi_scrambler_{enable|disable}()
funcs when DRM_BRIDGE_OP_HDMI_SCRAMBLER is advertised.

This completes the bridge connector plumbing so that the SCDC
scrambling helpers can control source-side scrambling through the
bridge chain.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 40 +++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 47bb0dcf509f..6f65733b7dd9 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -548,6 +548,32 @@ static int drm_bridge_connector_write_spd_infoframe(struct drm_connector *connec
 	return bridge->funcs->hdmi_write_spd_infoframe(bridge, buffer, len);
 }
 
+static int drm_bridge_connector_scrambler_src_enable(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+	if (!bridge)
+		return -EINVAL;
+
+	return bridge->funcs->hdmi_scrambler_enable(bridge);
+}
+
+static int drm_bridge_connector_scrambler_src_disable(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+	if (!bridge)
+		return -EINVAL;
+
+	return bridge->funcs->hdmi_scrambler_disable(bridge);
+}
+
 static const struct drm_edid *
 drm_bridge_connector_read_edid(struct drm_connector *connector)
 {
@@ -573,7 +599,7 @@ static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
 		.clear_infoframe = drm_bridge_connector_clear_hdmi_infoframe,
 		.write_infoframe = drm_bridge_connector_write_hdmi_infoframe,
 	},
-	/* audio, hdr_drm and spd are set dynamically during init */
+	/* scrambler, audio, hdr_drm and spd are set dynamically during init */
 };
 
 static const struct drm_connector_infoframe_funcs drm_bridge_connector_hdmi_audio_infoframe = {
@@ -879,6 +905,11 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			     !bridge->funcs->hdmi_clear_spd_infoframe))
 				return ERR_PTR(-EINVAL);
 
+			if (bridge->ops & DRM_BRIDGE_OP_HDMI_SCRAMBLER &&
+			    (!bridge->funcs->hdmi_scrambler_enable ||
+			     !bridge->funcs->hdmi_scrambler_disable))
+				return ERR_PTR(-EINVAL);
+
 			bridge_connector->bridge_hdmi = drm_bridge_get(bridge);
 
 			if (bridge->supported_formats)
@@ -983,6 +1014,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			bridge_connector->hdmi_funcs.spd =
 				drm_bridge_connector_hdmi_spd_infoframe;
 
+		if (bridge_connector->bridge_hdmi->ops & DRM_BRIDGE_OP_HDMI_SCRAMBLER) {
+			bridge_connector->hdmi_funcs.scrambler_src_enable =
+				drm_bridge_connector_scrambler_src_enable;
+			bridge_connector->hdmi_funcs.scrambler_src_disable =
+				drm_bridge_connector_scrambler_src_disable;
+		}
+
 		ret = drmm_connector_hdmi_init(drm, connector,
 					       bridge_connector->bridge_hdmi->vendor,
 					       bridge_connector->bridge_hdmi->product,

-- 
2.53.0

Claude review: drm/display: bridge_connector: Wire up HDMI 2.0 scrambler callbacks

[Claude Code Review Bot]

Patch Review

Clean wiring of the bridge scrambler callbacks to the connector-level callbacks. The validation in `drm_bridge_connector_init()` that both enable/disable funcs exist when the flag is set mirrors the SPD infoframe pattern above it.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 11/22] drm/bridge: dw-hdmi-qp: Rate limit i2c read error messages

[Cristian Ciocaltea]

During EDID reads, repeated i2c errors can flood the kernel log:

[   25.361716] dwhdmiqp-rockchip fde80000.hdmi: i2c read error
[   25.363376] dwhdmiqp-rockchip fde80000.hdmi: i2c read error
...
[   25.368671] dwhdmiqp-rockchip fde80000.hdmi: i2c read error
[   25.369440] dwhdmiqp-rockchip fde80000.hdmi: failed to get edid

Switch to dev_err_ratelimited() in dw_hdmi_qp_i2c_read() to reduce log
spam while still reporting the condition.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 0dbb12743609..59da91eca929 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -563,7 +563,7 @@ static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
 				dev_dbg_ratelimited(hdmi->dev,
 						    "i2c read timed out\n");
 			else
-				dev_err(hdmi->dev, "i2c read timed out\n");
+				dev_err_ratelimited(hdmi->dev, "i2c read timed out\n");
 			dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0);
 			return -EAGAIN;
 		}
@@ -574,7 +574,7 @@ static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
 				dev_dbg_ratelimited(hdmi->dev,
 						    "i2c read error\n");
 			else
-				dev_err(hdmi->dev, "i2c read error\n");
+				dev_err_ratelimited(hdmi->dev, "i2c read error\n");
 			dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0);
 			return -EIO;
 		}

-- 
2.53.0

Claude review: drm/bridge: dw-hdmi-qp: Rate limit i2c read error messages

[Claude Code Review Bot]

Patch Review

Good practical fix — repeated i2c errors during EDID read can flood the log. Using `dev_err_ratelimited()` is the right approach.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 12/22] drm/bridge: dw-hdmi-qp: Provide .{enable|disable}_hpd() PHY ops

[Cristian Ciocaltea]

Implement the .hpd_enable() and .hpd_disable() bridge callbacks and
extend dw_hdmi_qp_phy_ops with corresponding hooks.

This enables the DRM core to control when HPD interrupts are armed,
which is needed to prevent premature interrupt delivery before the
connector is fully initialized, and to properly quiesce HPD during
suspend.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 18 ++++++++++++++++++
 include/drm/bridge/dw_hdmi_qp.h              |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 59da91eca929..efa798aa23ac 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -1191,6 +1191,22 @@ static int dw_hdmi_qp_cec_transmit(struct drm_bridge *bridge, u8 attempts,
 #define dw_hdmi_qp_cec_transmit NULL
 #endif /* CONFIG_DRM_DW_HDMI_QP_CEC */
 
+static void dw_hdmi_qp_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = bridge->driver_private;
+
+	if (hdmi->phy.ops->enable_hpd)
+		hdmi->phy.ops->enable_hpd(hdmi, hdmi->phy.data);
+}
+
+static void dw_hdmi_qp_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = bridge->driver_private;
+
+	if (hdmi->phy.ops->disable_hpd)
+		hdmi->phy.ops->disable_hpd(hdmi, hdmi->phy.data);
+}
+
 static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -1198,6 +1214,8 @@ static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.atomic_enable = dw_hdmi_qp_bridge_atomic_enable,
 	.atomic_disable = dw_hdmi_qp_bridge_atomic_disable,
 	.detect = dw_hdmi_qp_bridge_detect,
+	.hpd_enable = dw_hdmi_qp_bridge_hpd_enable,
+	.hpd_disable = dw_hdmi_qp_bridge_hpd_disable,
 	.edid_read = dw_hdmi_qp_bridge_edid_read,
 	.hdmi_tmds_char_rate_valid = dw_hdmi_qp_bridge_tmds_char_rate_valid,
 	.hdmi_clear_avi_infoframe = dw_hdmi_qp_bridge_clear_avi_infoframe,
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index 6ea9c561cfef..b80fceffc315 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -17,6 +17,8 @@ struct dw_hdmi_qp_phy_ops {
 	void (*disable)(struct dw_hdmi_qp *hdmi, void *data);
 	enum drm_connector_status (*read_hpd)(struct dw_hdmi_qp *hdmi, void *data);
 	void (*setup_hpd)(struct dw_hdmi_qp *hdmi, void *data);
+	void (*enable_hpd)(struct dw_hdmi_qp *hdmi, void *data);
+	void (*disable_hpd)(struct dw_hdmi_qp *hdmi, void *data);
 };
 
 struct dw_hdmi_qp_plat_data {

-- 
2.53.0

Claude review: drm/bridge: dw-hdmi-qp: Provide .{enable|disable}_hpd() PHY ops

[Claude Code Review Bot]

Patch Review

Clean addition. The optional nature of the callbacks (guarded by `if (hdmi->phy.ops->enable_hpd)`) ensures backward compatibility with platform drivers that haven't implemented them yet — though in this series, all platform drivers are updated.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 13/22] drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling support`

[Cristian Ciocaltea]

Enable HDMI 2.0 display modes (e.g. 4K@60Hz) by implementing SCDC
scrambling and high TMDS clock ratio management for TMDS character
rates exceeding the 340 MHz HDMI 1.4b limit.

Reject modes requiring TMDS rates above 600 MHz since those require
HDMI 2.1 FRL which is not yet supported.  In no_hpd configurations,
further restrict to 340 MHz because SCDC requires a connected sink.

Tested-by: Diederik de Haas <diederik@cknow-tech.com>
Tested-by: Maud Spierings <maud_spierings@hotmail.com>
Acked-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 76 ++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index efa798aa23ac..001916a98da8 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (c) 2021-2022 Rockchip Electronics Co., Ltd.
  * Copyright (c) 2024 Collabora Ltd.
+ * Copyright (c) 2025 Amazon.com, Inc. or its affiliates.
  *
  * Author: Algea Cao <algea.cao@rock-chips.com>
  * Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
@@ -15,12 +16,12 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
-#include <linux/workqueue.h>
 
 #include <drm/bridge/dw_hdmi_qp.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
+#include <drm/display/drm_scdc_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -39,8 +40,7 @@
 #define DDC_SEGMENT_ADDR	0x30
 
 #define HDMI14_MAX_TMDSCLK	340000000
-
-#define SCRAMB_POLL_DELAY_MS	3000
+#define HDMI20_MAX_TMDSRATE	600000000
 
 /*
  * Unless otherwise noted, entries in this table are 100% optimization.
@@ -164,6 +164,7 @@ struct dw_hdmi_qp {
 	} phy;
 
 	unsigned long ref_clk_rate;
+	struct drm_connector *curr_conn;
 	struct regmap *regm;
 	int main_irq;
 
@@ -754,26 +755,35 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
 	struct drm_connector_state *conn_state;
-	struct drm_connector *connector;
 	unsigned int op_mode;
+	int ret;
 
-	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
-	if (WARN_ON(!connector))
+	hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
+								   bridge->encoder);
+	if (WARN_ON(!hdmi->curr_conn))
 		return;
 
-	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	conn_state = drm_atomic_get_new_connector_state(state, hdmi->curr_conn);
 	if (WARN_ON(!conn_state))
 		return;
 
-	if (connector->display_info.is_hdmi) {
-		dev_dbg(hdmi->dev, "%s mode=HDMI %s rate=%llu bpc=%u\n", __func__,
-			drm_hdmi_connector_get_output_format_name(conn_state->hdmi.output_format),
-			conn_state->hdmi.tmds_char_rate, conn_state->hdmi.output_bpc);
+	if (hdmi->curr_conn->display_info.is_hdmi) {
 		op_mode = 0;
 		hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
+
+		if (hdmi->tmds_char_rate > HDMI14_MAX_TMDSCLK) {
+			ret = drm_scdc_start_scrambling(hdmi->curr_conn);
+			if (ret)
+				dev_warn(hdmi->dev, "Failed to setup SCDC: %d\n", ret);
+		}
+
+		dev_dbg(hdmi->dev, "%s mode=HDMI %s rate=%llu bpc=%u scramb=%d\n", __func__,
+			drm_hdmi_connector_get_output_format_name(conn_state->hdmi.output_format),
+			conn_state->hdmi.tmds_char_rate, conn_state->hdmi.output_bpc,
+			hdmi->curr_conn->hdmi.scrambler_enabled);
 	} else {
-		dev_dbg(hdmi->dev, "%s mode=DVI\n", __func__);
 		op_mode = OPMODE_DVI;
+		dev_dbg(hdmi->dev, "%s mode=DVI\n", __func__);
 	}
 
 	hdmi->phy.ops->init(hdmi, hdmi->phy.data);
@@ -781,7 +791,7 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
 	dw_hdmi_qp_mod(hdmi, HDCP2_BYPASS, HDCP2_BYPASS, HDCP2LOGIC_CONFIG0);
 	dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
 
-	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
+	drm_atomic_helper_connector_hdmi_update_infoframes(hdmi->curr_conn, state);
 }
 
 static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -791,6 +801,9 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
 
 	hdmi->tmds_char_rate = 0;
 
+	drm_scdc_stop_scrambling(hdmi->curr_conn);
+
+	hdmi->curr_conn = NULL;
 	hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
 }
 
@@ -832,12 +845,12 @@ dw_hdmi_qp_bridge_tmds_char_rate_valid(const struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
 
-	/*
-	 * TODO: when hdmi->no_hpd is 1 we must not support modes that
-	 * require scrambling, including every mode with a clock above
-	 * HDMI14_MAX_TMDSCLK.
-	 */
-	if (rate > HDMI14_MAX_TMDSCLK) {
+	if (hdmi->no_hpd && rate > HDMI14_MAX_TMDSCLK) {
+		dev_dbg(hdmi->dev, "Unsupported TMDS char rate in no_hpd mode: %lld\n", rate);
+		return MODE_CLOCK_HIGH;
+	}
+
+	if (rate > HDMI20_MAX_TMDSRATE) {
 		dev_dbg(hdmi->dev, "Unsupported TMDS char rate: %lld\n", rate);
 		return MODE_CLOCK_HIGH;
 	}
@@ -845,6 +858,26 @@ dw_hdmi_qp_bridge_tmds_char_rate_valid(const struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static int dw_hdmi_qp_bridge_scrambler_enable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = bridge->driver_private;
+
+	dw_hdmi_qp_write(hdmi, 1, SCRAMB_CONFIG0);
+	dev_dbg(hdmi->dev, "scrambler enabled\n");
+
+	return 0;
+}
+
+static int dw_hdmi_qp_bridge_scrambler_disable(struct drm_bridge *bridge)
+{
+	struct dw_hdmi_qp *hdmi = bridge->driver_private;
+
+	dw_hdmi_qp_write(hdmi, 0, SCRAMB_CONFIG0);
+	dev_dbg(hdmi->dev, "scrambler disabled\n");
+
+	return 0;
+}
+
 static int dw_hdmi_qp_bridge_clear_avi_infoframe(struct drm_bridge *bridge)
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
@@ -1218,6 +1251,8 @@ static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
 	.hpd_disable = dw_hdmi_qp_bridge_hpd_disable,
 	.edid_read = dw_hdmi_qp_bridge_edid_read,
 	.hdmi_tmds_char_rate_valid = dw_hdmi_qp_bridge_tmds_char_rate_valid,
+	.hdmi_scrambler_enable = dw_hdmi_qp_bridge_scrambler_enable,
+	.hdmi_scrambler_disable = dw_hdmi_qp_bridge_scrambler_disable,
 	.hdmi_clear_avi_infoframe = dw_hdmi_qp_bridge_clear_avi_infoframe,
 	.hdmi_write_avi_infoframe = dw_hdmi_qp_bridge_write_avi_infoframe,
 	.hdmi_clear_hdmi_infoframe = dw_hdmi_qp_bridge_clear_hdmi_infoframe,
@@ -1344,7 +1379,8 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 			   DRM_BRIDGE_OP_HDMI |
 			   DRM_BRIDGE_OP_HDMI_AUDIO |
 			   DRM_BRIDGE_OP_HDMI_HDR_DRM_INFOFRAME |
-			   DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME;
+			   DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME |
+			   DRM_BRIDGE_OP_HDMI_SCRAMBLER;
 	if (!hdmi->no_hpd)
 		hdmi->bridge.ops |= DRM_BRIDGE_OP_HPD;
 	hdmi->bridge.of_node = pdev->dev.of_node;

-- 
2.53.0

Claude review: drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling support`

[Claude Code Review Bot]

Patch Review

This is the main driver-level implementation patch.

1. **`curr_conn` lifetime**: The `curr_conn` pointer is set in `atomic_enable` and cleared in `atomic_disable`. It is used in `dw_hdmi_qp_hpd_notify()` (patch 14) to decide which HPD notification path to take. There's a potential race if an HPD interrupt fires during `atomic_disable` after `curr_conn` is set to NULL but before the PHY is disabled. However, the HPD interrupt handler only schedules deferred work, so by the time the work runs, the state should be consistent.

2. **Scrambling before PHY init**: In `atomic_enable`, `drm_scdc_start_scrambling()` is called *before* `hdmi->phy.ops->init()`. This sets up the sink-side SCDC configuration and source-side scrambling before the PHY is initialized. The HDMI 2.0 spec requires TMDS clock to be suspended while changing clock ratio, so starting scrambling before PHY init (while clock is still off) is correct.

3. **Error path in atomic_enable**: If `drm_scdc_start_scrambling()` fails, a warning is logged but the enable continues. This is reasonable — you'd rather attempt to display something than fail silently.

4. **HDMI20_MAX_TMDSRATE = 600MHz**: This correctly caps the TMDS rate at the HDMI 2.0 maximum (600 MHz), above which HDMI 2.1 FRL would be needed.

5. **Copyright addition**: Amazon copyright is added alongside Collabora and Rockchip. This appears legitimate given the contribution.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 14/22] drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper

[Cristian Ciocaltea]

Export dw_hdmi_qp_hpd_notify() for platform drivers to report hot-plug
detection events.  Unlike drm_helper_hpd_irq_event() which polls all
connectors, this helper targets only the affected connector and ensures
.detect_ctx() is invoked on reconnection events to trigger SCDC state
recovery.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 31 ++++++++++++++++++++++++++++
 include/drm/bridge/dw_hdmi_qp.h              |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 001916a98da8..ed0c68d6c6fd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -29,6 +29,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
 
 #include <media/cec.h>
 
@@ -1431,6 +1432,36 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_qp_bind);
 
+/**
+ * dw_hdmi_qp_hpd_notify() - Notify a hot-plug detection event
+ * @hdmi: pointer to the DW HDMI QP controller
+ *
+ * Platform drivers should call this from their HPD interrupt handler
+ * or work function to notify the bridge of a connection status change.
+ * The bridge's .read_hpd() phy_ops callback is used to read the current
+ * connection status.
+ */
+void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi)
+{
+	enum drm_connector_status status;
+
+	status = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
+	dev_dbg(hdmi->dev, "%s status=%d\n", __func__, status);
+
+	/*
+	 * When the display pipeline has been already active, switch to
+	 * drm_connector_helper_hpd_irq_event() to ensure .detect_ctx()
+	 * gets invoked, i.e. via drm_helper_probe_detect(), because
+	 * drm_bridge_hpd_notify() defers to a delayed hotplug path in
+	 * this case.
+	 */
+	if (hdmi->curr_conn && status == connector_status_connected)
+		drm_connector_helper_hpd_irq_event(hdmi->curr_conn);
+	else
+		drm_bridge_hpd_notify(&hdmi->bridge, status);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_qp_hpd_notify);
+
 void dw_hdmi_qp_suspend(struct device *dev, struct dw_hdmi_qp *hdmi)
 {
 	disable_irq(hdmi->main_irq);
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index b80fceffc315..b4fb1c578a5b 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -36,6 +36,7 @@ struct dw_hdmi_qp_plat_data {
 struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 				   struct drm_encoder *encoder,
 				   const struct dw_hdmi_qp_plat_data *plat_data);
+void dw_hdmi_qp_hpd_notify(struct dw_hdmi_qp *hdmi);
 void dw_hdmi_qp_suspend(struct device *dev, struct dw_hdmi_qp *hdmi);
 void dw_hdmi_qp_resume(struct device *dev, struct dw_hdmi_qp *hdmi);
 #endif /* __DW_HDMI_QP__ */

-- 
2.53.0

Claude review: drm/bridge: dw-hdmi-qp: Provide dw_hdmi_qp_hpd_notify() helper

[Claude Code Review Bot]

Patch Review

The dual-path HPD notification logic is interesting:

```c
if (hdmi->curr_conn && status == connector_status_connected)
    drm_connector_helper_hpd_irq_event(hdmi->curr_conn);
else
    drm_bridge_hpd_notify(&hdmi->bridge, status);
```

The comment explains the reasoning: when the pipeline is already active and a reconnection occurs, `drm_bridge_hpd_notify()` defers to a delayed hotplug path that doesn't invoke `detect_ctx()`, which is needed for SCDC recovery. Using `drm_connector_helper_hpd_irq_event()` ensures the detect path runs immediately. This is a pragmatic solution.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 15/22] drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages

[Cristian Ciocaltea]

Add the missing trailing newlines to a couple of dev_err_probe() calls
in dw_hdmi_qp_rockchip_bind().

Fixes: b6736a4ea3fa ("drm/rockchip: dw_hdmi_qp: Improve error handling with dev_err_probe()")
Fixes: e1f7b7cbd74c ("drm/rockchip: dw_hdmi_qp: Switch to drmm_encoder_init()")
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index f35484715c2d..296f9a3ba66a 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -589,14 +589,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
 	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
-		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder");
+		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder\n");
 
 	platform_set_drvdata(pdev, hdmi);
 
 	hdmi->hdmi = dw_hdmi_qp_bind(pdev, encoder, &plat_data);
 	if (IS_ERR(hdmi->hdmi))
 		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->hdmi),
-				     "Failed to bind dw-hdmi-qp");
+				     "Failed to bind dw-hdmi-qp\n");
 
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector))

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages

[Claude Code Review Bot]

Patch Review

Trivial bug fix, correctly has `Fixes:` tags.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 16/22] drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind()

[Cristian Ciocaltea]

Replace indirect struct device accesses via hdmi->dev and pdev->dev with
the local dev parameter already available in dw_hdmi_qp_rockchip_bind(),
for consistency and readability.

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 33 +++++++++++++-------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 296f9a3ba66a..46df5abe31a4 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -475,7 +475,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	struct clk *ref_clk;
 	int ret, irq, i;
 
-	if (!pdev->dev.of_node)
+	if (!dev->of_node)
 		return -ENODEV;
 
 	hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);
@@ -495,7 +495,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(dev, -ENODEV, "Missing platform ctrl ops\n");
 
 	hdmi->ctrl_ops = cfg->ctrl_ops;
-	hdmi->dev = &pdev->dev;
+	hdmi->dev = dev;
 	hdmi->port_id = -ENODEV;
 
 	/* Identify port ID by matching base IO address */
@@ -506,7 +506,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		}
 	}
 	if (hdmi->port_id < 0)
-		return dev_err_probe(hdmi->dev, hdmi->port_id,
+		return dev_err_probe(dev, hdmi->port_id,
 				     "Failed to match HDMI port ID\n");
 
 	plat_data.phy_ops = cfg->phy_ops;
@@ -530,37 +530,36 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "rockchip,grf");
 	if (IS_ERR(hdmi->regmap))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->regmap),
+		return dev_err_probe(dev, PTR_ERR(hdmi->regmap),
 				     "Unable to get rockchip,grf\n");
 
 	hdmi->vo_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,vo-grf");
 	if (IS_ERR(hdmi->vo_regmap))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->vo_regmap),
+		return dev_err_probe(dev, PTR_ERR(hdmi->vo_regmap),
 				     "Unable to get rockchip,vo-grf\n");
 
-	ret = devm_clk_bulk_get_all_enabled(hdmi->dev, &clks);
+	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
 	if (ret < 0)
-		return dev_err_probe(hdmi->dev, ret, "Failed to get clocks\n");
+		return dev_err_probe(dev, ret, "Failed to get clocks\n");
 
-	ref_clk = clk_get(hdmi->dev, "ref");
+	ref_clk = clk_get(dev, "ref");
 	if (IS_ERR(ref_clk))
-		return dev_err_probe(hdmi->dev, PTR_ERR(ref_clk),
+		return dev_err_probe(dev, PTR_ERR(ref_clk),
 				     "Failed to get ref clock\n");
 
 	plat_data.ref_clk_rate = clk_get_rate(ref_clk);
 	clk_put(ref_clk);
 
-	hdmi->frl_enable_gpio = devm_gpiod_get_optional(hdmi->dev, "frl-enable",
+	hdmi->frl_enable_gpio = devm_gpiod_get_optional(dev, "frl-enable",
 							GPIOD_OUT_LOW);
 	if (IS_ERR(hdmi->frl_enable_gpio))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->frl_enable_gpio),
+		return dev_err_probe(dev, PTR_ERR(hdmi->frl_enable_gpio),
 				     "Failed to request FRL enable GPIO\n");
 
 	hdmi->phy = devm_of_phy_get_by_index(dev, dev->of_node, 0);
 	if (IS_ERR(hdmi->phy))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->phy),
-				     "Failed to get phy\n");
+		return dev_err_probe(dev, PTR_ERR(hdmi->phy), "Failed to get phy\n");
 
 	cfg->ctrl_ops->io_init(hdmi);
 
@@ -578,7 +577,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	if (irq < 0)
 		return irq;
 
-	ret = devm_request_threaded_irq(hdmi->dev, irq,
+	ret = devm_request_threaded_irq(dev, irq,
 					cfg->ctrl_ops->hardirq_callback,
 					cfg->ctrl_ops->irq_callback,
 					IRQF_SHARED, "dw-hdmi-qp-hpd",
@@ -589,18 +588,18 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
 	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
-		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder\n");
+		return dev_err_probe(dev, ret, "Failed to init encoder\n");
 
 	platform_set_drvdata(pdev, hdmi);
 
 	hdmi->hdmi = dw_hdmi_qp_bind(pdev, encoder, &plat_data);
 	if (IS_ERR(hdmi->hdmi))
-		return dev_err_probe(hdmi->dev, PTR_ERR(hdmi->hdmi),
+		return dev_err_probe(dev, PTR_ERR(hdmi->hdmi),
 				     "Failed to bind dw-hdmi-qp\n");
 
 	connector = drm_bridge_connector_init(drm, encoder);
 	if (IS_ERR(connector))
-		return dev_err_probe(hdmi->dev, PTR_ERR(connector),
+		return dev_err_probe(dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
 	return 0;

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Use local dev variable consistently in bind()

[Claude Code Review Bot]

Patch Review

Straightforward cleanup replacing `hdmi->dev` and `pdev->dev` with the already-available `dev` parameter. No functional change.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 17/22] drm/rockchip: dw_hdmi_qp: Drop unnecessary #include

[Cristian Ciocaltea]

drm/drm_simple_kms_helper.h is no longer used, remove the include
directive.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 46df5abe31a4..8d64b76a1aa5 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -24,7 +24,6 @@
 #include <drm/drm_managed.h>
 #include <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
 

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Drop unnecessary #include

[Claude Code Review Bot]

Patch Review

Trivial cleanup removing unused `drm_simple_kms_helper.h`.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 18/22] drm/rockchip: dw_hdmi_qp: Defer HPD IRQ enable until after connector setup

[Cristian Ciocaltea]

Request the HPD IRQ early in bind() with IRQF_NO_AUTOEN, keeping it
disabled until all DRM resources are fully initialized, at which point
enable_irq() arms it.  This prevents premature interrupt delivery
without forcing devm_request_threaded_irq() to the very end of the
initialization sequence, which would complicate error unwinding.

Note that IRQF_NO_AUTOEN is incompatible with IRQF_SHARED; the latter is
dropped as this IRQ has no other users.

Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 8d64b76a1aa5..27342094958c 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -579,7 +579,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 	ret = devm_request_threaded_irq(dev, irq,
 					cfg->ctrl_ops->hardirq_callback,
 					cfg->ctrl_ops->irq_callback,
-					IRQF_SHARED, "dw-hdmi-qp-hpd",
+					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
 					hdmi);
 	if (ret)
 		return ret;
@@ -601,6 +601,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
 		return dev_err_probe(dev, PTR_ERR(connector),
 				     "Failed to init bridge connector\n");
 
+	enable_irq(irq);
+
 	return 0;
 }
 

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Defer HPD IRQ enable until after connector setup

[Claude Code Review Bot]

Patch Review

Good fix for a real race condition. Using `IRQF_NO_AUTOEN` and explicitly calling `enable_irq()` after full initialization prevents premature interrupt delivery. The commit message correctly notes `IRQF_NO_AUTOEN` is incompatible with `IRQF_SHARED` and documents why dropping `IRQF_SHARED` is safe.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 19/22] drm/rockchip: dw_hdmi_qp: Mask HPD IRQ in rk3576_io_init()

[Cristian Ciocaltea]

Mask the RK3576 HPD interrupt in io_init() so it starts disabled,
matching the RK3588 behavior.  This prevents spurious interrupts before
the bridge framework enables HPD via the .hpd_enable() callback.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 27342094958c..62ea5c7cbbe6 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -346,7 +346,7 @@ static void dw_hdmi_qp_rk3576_io_init(struct rockchip_hdmi_qp *hdmi)
 
 	regmap_write(hdmi->vo_regmap, RK3576_VO0_GRF_SOC_CON14, val);
 
-	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0);
+	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1);
 	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
 }
 

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Mask HPD IRQ in rk3576_io_init()

[Claude Code Review Bot]

Patch Review

Simple one-line fix changing `RK3576_HDMI_HPD_INT_MSK` from 0 (unmasked) to 1 (masked), matching the RK3588 behavior. Correct.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 20/22] drm/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops

[Cristian Ciocaltea]

Implement .enable_hpd() and .disable_hpd() phy ops for RK3576 and RK3588
SoCs, used by the corresponding bridge callbacks for HPD activation
control.

Consolidate the interrupt clear-and-unmask sequence into enable_hpd()
and the mask-only operation into disable_hpd(), replacing the open-coded
register writes in the interrupt handlers and io_init().

The .setup_hpd() phy op, which was previously called from
dw_hdmi_qp_init_hw(), is no longer needed and its users are removed.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 78 +++++++++++++-------------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 62ea5c7cbbe6..b9e0ea56efd8 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -193,7 +193,7 @@ dw_hdmi_qp_rk3588_read_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	return val ? connector_status_connected : connector_status_disconnected;
 }
 
-static void dw_hdmi_qp_rk3588_setup_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 	u32 val;
@@ -208,11 +208,25 @@ static void dw_hdmi_qp_rk3588_setup_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
 }
 
+static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+{
+	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
+	u32 val;
+
+	if (hdmi->port_id)
+		val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 1);
+	else
+		val = FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_MSK, 1);
+
+	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
+}
+
 static const struct dw_hdmi_qp_phy_ops rk3588_hdmi_phy_ops = {
 	.init		= dw_hdmi_qp_rk3588_phy_init,
 	.disable	= dw_hdmi_qp_rk3588_phy_disable,
 	.read_hpd	= dw_hdmi_qp_rk3588_read_hpd,
-	.setup_hpd	= dw_hdmi_qp_rk3588_setup_hpd,
+	.enable_hpd	= dw_hdmi_qp_rk3588_enable_hpd,
+	.disable_hpd	= dw_hdmi_qp_rk3588_disable_hpd,
 };
 
 static enum drm_connector_status
@@ -227,7 +241,7 @@ dw_hdmi_qp_rk3576_read_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 		connector_status_connected : connector_status_disconnected;
 }
 
-static void dw_hdmi_qp_rk3576_setup_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+static void dw_hdmi_qp_rk3576_enable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
 	u32 val;
@@ -236,14 +250,22 @@ static void dw_hdmi_qp_rk3576_setup_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
 	       FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0));
 
 	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
-	regmap_write(hdmi->regmap, 0xa404, 0xffff0102);
+}
+
+static void dw_hdmi_qp_rk3576_disable_hpd(struct dw_hdmi_qp *dw_hdmi, void *data)
+{
+	struct rockchip_hdmi_qp *hdmi = (struct rockchip_hdmi_qp *)data;
+
+	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0,
+		     FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1));
 }
 
 static const struct dw_hdmi_qp_phy_ops rk3576_hdmi_phy_ops = {
 	.init		= dw_hdmi_qp_rk3588_phy_init,
 	.disable	= dw_hdmi_qp_rk3588_phy_disable,
 	.read_hpd	= dw_hdmi_qp_rk3576_read_hpd,
-	.setup_hpd	= dw_hdmi_qp_rk3576_setup_hpd,
+	.enable_hpd	= dw_hdmi_qp_rk3576_enable_hpd,
+	.disable_hpd	= dw_hdmi_qp_rk3576_disable_hpd,
 };
 
 static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
@@ -264,13 +286,12 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 static irqreturn_t dw_hdmi_qp_rk3576_hardirq(int irq, void *dev_id)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_id;
-	u32 intr_stat, val;
+	u32 intr_stat;
 
 	regmap_read(hdmi->regmap, RK3576_IOC_HDMI_HPD_STATUS, &intr_stat);
-	if (intr_stat) {
-		val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1);
 
-		regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
+	if (intr_stat) {
+		dw_hdmi_qp_rk3576_disable_hpd(NULL, hdmi);
 		return IRQ_WAKE_THREAD;
 	}
 
@@ -280,15 +301,11 @@ static irqreturn_t dw_hdmi_qp_rk3576_hardirq(int irq, void *dev_id)
 static irqreturn_t dw_hdmi_qp_rk3576_irq(int irq, void *dev_id)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_id;
-	u32 val;
 
-	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_CLR, 1);
-	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
 	mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
 			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
 
-	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0);
-	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
+	dw_hdmi_qp_rk3576_enable_hpd(NULL, hdmi);
 
 	return IRQ_HANDLED;
 }
@@ -296,16 +313,12 @@ static irqreturn_t dw_hdmi_qp_rk3576_irq(int irq, void *dev_id)
 static irqreturn_t dw_hdmi_qp_rk3588_hardirq(int irq, void *dev_id)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_id;
-	u32 intr_stat, val;
+	u32 intr_stat;
 
 	regmap_read(hdmi->regmap, RK3588_GRF_SOC_STATUS1, &intr_stat);
 
 	if (intr_stat) {
-		if (hdmi->port_id)
-			val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 1);
-		else
-			val = FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_MSK, 1);
-		regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
+		dw_hdmi_qp_rk3588_disable_hpd(NULL, hdmi);
 		return IRQ_WAKE_THREAD;
 	}
 
@@ -315,22 +328,11 @@ static irqreturn_t dw_hdmi_qp_rk3588_hardirq(int irq, void *dev_id)
 static irqreturn_t dw_hdmi_qp_rk3588_irq(int irq, void *dev_id)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_id;
-	u32 val;
-
-	if (hdmi->port_id)
-		val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_CLR, 1);
-	else
-		val = FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_CLR, 1);
-	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
 
 	mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
 			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
 
-	if (hdmi->port_id)
-		val |= FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 0);
-	else
-		val |= FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_MSK, 0);
-	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
+	dw_hdmi_qp_rk3588_enable_hpd(NULL, hdmi);
 
 	return IRQ_HANDLED;
 }
@@ -343,11 +345,11 @@ static void dw_hdmi_qp_rk3576_io_init(struct rockchip_hdmi_qp *hdmi)
 	      FIELD_PREP_WM16(RK3576_SDAIN_MASK, 1) |
 	      FIELD_PREP_WM16(RK3576_HDMI_GRANT_SEL, 1) |
 	      FIELD_PREP_WM16(RK3576_I2S_SEL_MASK, 1);
-
 	regmap_write(hdmi->vo_regmap, RK3576_VO0_GRF_SOC_CON14, val);
 
-	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 1);
-	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
+	regmap_write(hdmi->regmap, 0xa404, 0xffff0102);
+
+	dw_hdmi_qp_rk3576_disable_hpd(NULL, hdmi);
 }
 
 static void dw_hdmi_qp_rk3588_io_init(struct rockchip_hdmi_qp *hdmi)
@@ -372,11 +374,7 @@ static void dw_hdmi_qp_rk3588_io_init(struct rockchip_hdmi_qp *hdmi)
 		val = FIELD_PREP_WM16(RK3588_HDMI0_GRANT_SEL, 1);
 	regmap_write(hdmi->vo_regmap, RK3588_GRF_VO1_CON9, val);
 
-	if (hdmi->port_id)
-		val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 1);
-	else
-		val = FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_MSK, 1);
-	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
+	dw_hdmi_qp_rk3588_disable_hpd(NULL, hdmi);
 }
 
 static void dw_hdmi_qp_rk3576_enc_init(struct rockchip_hdmi_qp *hdmi,

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops

[Claude Code Review Bot]

Patch Review

This is a significant refactoring of the interrupt handling code.

**Key observation on RK3588 enable_hpd vs old setup_hpd**: The original `setup_hpd` wrote both CLR and MSK=0 in a single register write. The new `enable_hpd` does exactly the same — clear and unmask. Good.

**RK3576 `enable_hpd` does NOT write `0xa404`**: The old `setup_hpd` for RK3576 wrote to `0xa404` (a magic register). The new code moves this write to `io_init()`, which is correct since it's a one-time hardware initialization.

**Interrupt clear in threaded handler**: The old RK3588 threaded handler explicitly cleared the interrupt before re-enabling it. The new code calls `enable_hpd()`, which clears+unmasks atomically. The old RK3576 threaded handler similarly cleared then unmasked. Now both call their respective `enable_hpd()` which does both. This is functionally equivalent and cleaner.

**One subtle issue**: In `dw_hdmi_qp_rk3588_irq` (the threaded handler), the old code did:
```c
val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_CLR, 1);  // CLR only
regmap_write(..., val);
// ...
val |= FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 0); // CLR+MSK
regmap_write(..., val);
```
It cleared first, then later wrote CLR+MSK=0 in a second write. The new `enable_hpd` does both in one write. Since these are write-mask-16 registers (upper 16 bits are write-enable, lower 16 bits are the value), combining CLR and MSK into a single write is fine and may even be slightly better to avoid a transient state.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 21/22] drm/rockchip: dw_hdmi_qp: Switch to dw_hdmi_qp_hpd_notify()

[Cristian Ciocaltea]

Switch from drm_helper_hpd_irq_event(), which polls all connectors, to
the recently introduced dw_hdmi_qp_hpd_notify() helper, which runs the
detect cycle only on the affected connector.

This avoids unnecessary work and redundant detect calls on unrelated
connectors.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index b9e0ea56efd8..666631fc9162 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -23,7 +23,6 @@
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_of.h>
-#include <drm/drm_probe_helper.h>
 
 #include "rockchip_drm_drv.h"
 
@@ -273,14 +272,8 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
 	struct rockchip_hdmi_qp *hdmi = container_of(work,
 						     struct rockchip_hdmi_qp,
 						     hpd_work.work);
-	struct drm_device *drm = hdmi->encoder.encoder.dev;
-	bool changed;
 
-	if (drm) {
-		changed = drm_helper_hpd_irq_event(drm);
-		if (changed)
-			dev_dbg(hdmi->dev, "connector status changed\n");
-	}
+	dw_hdmi_qp_hpd_notify(hdmi->hdmi);
 }
 
 static irqreturn_t dw_hdmi_qp_rk3576_hardirq(int irq, void *dev_id)
@@ -645,8 +638,7 @@ static int __maybe_unused dw_hdmi_qp_rockchip_resume(struct device *dev)
 
 	dw_hdmi_qp_resume(dev, hdmi->hdmi);
 
-	if (hdmi->encoder.encoder.dev)
-		drm_helper_hpd_irq_event(hdmi->encoder.encoder.dev);
+	dw_hdmi_qp_hpd_notify(hdmi->hdmi);
 
 	return 0;
 }

-- 
2.53.0

Claude review: drm/rockchip: dw_hdmi_qp: Switch to dw_hdmi_qp_hpd_notify()

[Claude Code Review Bot]

Patch Review

Clean migration from `drm_helper_hpd_irq_event()` (which polls all connectors) to the targeted `dw_hdmi_qp_hpd_notify()`. Also removes the `drm_probe_helper.h` include that's no longer needed.

**Observation**: The old code had a `if (drm)` guard before calling `drm_helper_hpd_irq_event()`. The new code calls `dw_hdmi_qp_hpd_notify(hdmi->hdmi)` unconditionally. Inside `dw_hdmi_qp_hpd_notify()`, it accesses `hdmi->phy.ops->read_hpd()` without checking if the bridge is fully initialized. In the HPD work function context, this should be safe because the work is only scheduled after IRQ registration, which is after bind. In the resume path, `dw_hdmi_qp_resume()` calls `dw_hdmi_qp_init_hw()` first, so the hardware is ready. This looks safe.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 22/22] drm/bridge: dw-hdmi-qp: Remove obsolete .setup_hpd() phy op

[Cristian Ciocaltea]

With the completed transition to .{enable|disable}_hpd() phy ops,
.setup_hpd() became obsolete and unused.  Drop it.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 3 ---
 include/drm/bridge/dw_hdmi_qp.h              | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index ed0c68d6c6fd..f626cf4bdda9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -1316,9 +1316,6 @@ static void dw_hdmi_qp_init_hw(struct dw_hdmi_qp *hdmi)
 	/* Clear DONE and ERROR interrupts */
 	dw_hdmi_qp_write(hdmi, I2CM_OP_DONE_CLEAR | I2CM_NACK_RCVD_CLEAR,
 			 MAINUNIT_1_INT_CLEAR);
-
-	if (hdmi->phy.ops->setup_hpd)
-		hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data);
 }
 
 struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index b4fb1c578a5b..ad33f9cafdeb 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -16,7 +16,6 @@ struct dw_hdmi_qp_phy_ops {
 	int (*init)(struct dw_hdmi_qp *hdmi, void *data);
 	void (*disable)(struct dw_hdmi_qp *hdmi, void *data);
 	enum drm_connector_status (*read_hpd)(struct dw_hdmi_qp *hdmi, void *data);
-	void (*setup_hpd)(struct dw_hdmi_qp *hdmi, void *data);
 	void (*enable_hpd)(struct dw_hdmi_qp *hdmi, void *data);
 	void (*disable_hpd)(struct dw_hdmi_qp *hdmi, void *data);
 };

-- 
2.53.0

Claude review: drm/bridge: dw-hdmi-qp: Remove obsolete .setup_hpd() phy op

[Claude Code Review Bot]

Patch Review

Clean removal of the now-unused `setup_hpd` callback from both the bridge driver and the header. All callers have been migrated to `enable_hpd`/`disable_hpd` in previous patches.

---

**Summary**: This is a mature, well-decomposed v6 series. The architecture is sound, error handling is thorough, and the series has good test coverage (multiple Tested-by tags). The main substantive items are: (1) the C++ style `// TODO` comment in patch 4 should use `/* */`, and (2) the `scdc_cb` pointer access ordering relies on `cancel_delayed_work_sync()` guarantees that could benefit from a brief comment. Otherwise this looks ready for merge, assuming the maintainer (Maxime Ripard for the DRM HDMI framework parts) is happy with the connector/bridge interface additions.

---
Generated by Claude Code Patch Reviewer

Claude review: Add HDMI 2.0 support to DW HDMI QP TX

[Claude Code Review Bot]

Overall Series Review

Subject: Add HDMI 2.0 support to DW HDMI QP TX
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Patches: 27
Reviewed: 2026-05-25T21:15:02.230257

---

This is a well-structured 22-patch series adding HDMI 2.0 scrambling support (enabling 4K@60Hz) to the Synopsys DW HDMI QP TX controller, primarily for Rockchip RK3576/RK3588 SoCs. The series is at v6, has collected several Reviewed-by/Tested-by/Acked-by tags, and shows substantial refinement over previous iterations.

**Architecture:** The approach is sound — it builds layered infrastructure (SCDC helpers -> connector callbacks -> bridge ops -> bridge_connector wiring -> driver implementation) in a logical progression. The split from 10 to 22 patches is a good decomposition. The SCDC status monitoring via delayed work, and the CRTC-reset-on-reconnect pattern for restoring SCDC state after hotplug, are well-established patterns in the DRM subsystem (cf. vc4_hdmi).

**Concerns:** I have a few observations ranging from minor to moderate, detailed below.

---

---
Generated by Claude Code Patch Reviewer