[PATCH RFC 0/3] Some updates over DP AUX Transactions

[PATCH RFC 0/3] Some updates over DP AUX Transactions

[Arun R Murthy]

Based on the discussions/comments on the patch
https://lore.kernel.org/intel-xe/aaVWbdt1vOFxGAb1@ideak-desk.lan/
in order to address the real HW(DPTX) generated timeout and adress the
fix, this series is targetted.
Along with this it was noticed that the the AUX power request bit in the
port control register is being touched outside the power well framework.
The limitation due to the way aux port control register was programmed.
Replacing the aux control register write to read/mask/write thereby
writing only the required bits.

Note: This is RFC and full round of testing the pending.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
Arun R Murthy (3):
      drm/display/dp: Export function to wake the sink AUX_CH
      drm/i915/dp: On AUX_CH tx timeout, wake up the sink
      drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

 drivers/gpu/drm/display/drm_dp_helper.c            | 36 +++++++++
 drivers/gpu/drm/i915/display/intel_display_types.h |  6 +-
 drivers/gpu/drm/i915/display/intel_dp_aux.c        | 94 +++++++++++++++-------
 drivers/gpu/drm/i915/display/intel_psr.c           | 29 ++++---
 include/drm/display/drm_dp_helper.h                |  1 +
 5 files changed, 123 insertions(+), 43 deletions(-)
---
base-commit: 6884fe03ff2bc5a2f501ba4710f950dd4933ac84
change-id: 20260309-dp_aux_timeout-9d5b1b35a0d8

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

[PATCH RFC 1/3] drm/display/dp: Export function to wake the sink AUX_CH

[Arun R Murthy]

On a native AUX_CH transaction timeout, as per Spec 2.1 Section 2.3.4
says that if the AUX Reply Timeout timer timesout, DPTX can rety for 3
times because the No Reply may be due to the DPRX waking up from
power-saving state.
Export a function to wake the DP sink AUX_CH.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 36 +++++++++++++++++++++++++++++++++
 include/drm/display/drm_dp_helper.h     |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index a697cc227e28964cd8322803298178e7d788e820..1b8aa543ec3dc5819948af6f2724f3514b96b918 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -701,6 +701,42 @@ void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
 }
 EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
 
+/**
+ * drm_dp_wake_sink() - Try to wake up the sink device AUX_CH if in sleep
+ *
+ * @aux: DisplayPort AUX channel
+ */
+void drm_dp_wake_sink(struct drm_dp_aux *aux)
+{
+	const u8 wake_retry = 6;
+	u8 value = 0;
+	int ret = 0;
+
+	/*
+	 * Wake the sink device
+	 * Spec DP2.1 section 2.3.1.2 if AUX CH is powered down by writing 0x02
+	 * to DP_SET_POWER dpcd reg, 1ms time would be required to wake it up
+	 */
+	ret = poll_timeout_us(ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value),
+			      ret > 0,
+			      1000, wake_retry * 1000, true);
+
+	/*
+	 * If sink is in D3 then it may not respond to the AUX tx so
+	 * wake it up to D3_AUX_ON state
+	 * If the above poll_timeout_us fails, try waking the sink.
+	 */
+	if (value == DP_SET_POWER_D3 || ret < 0) {
+		/* After setting to D0 need a min of 1ms to wake(Spec DP2.1 sec 2.3.1.2) */
+		drm_dp_dpcd_writeb(aux, DP_SET_POWER,
+				   DP_SET_POWER_D0);
+		fsleep(1000);
+		drm_dp_dpcd_writeb(aux, DP_SET_POWER,
+				   DP_SET_POWER_D3_AUX_ON);
+	}
+}
+EXPORT_SYMBOL(drm_dp_wake_sink);
+
 /**
  * drm_dp_dpcd_set_probe() - Set whether a probing before DPCD access is done
  * @aux: DisplayPort AUX channel
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 1d0acd58f48676f60ff6a07cc6812f72cbb452e8..933428025c4d75ab7a0aa63039ed5f8e45ca625b 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -546,6 +546,7 @@ struct drm_dp_aux {
 	bool dpcd_probe_disabled;
 };
 
+void drm_dp_wake_sink(struct drm_dp_aux *aux);
 int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
 void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
 void drm_dp_dpcd_set_probe(struct drm_dp_aux *aux, bool enable);

-- 
2.25.1

Claude review: drm/display/dp: Export function to wake the sink AUX_CH

[Claude Code Review Bot]

Patch Review

**`poll_timeout_us` misuse:**

```c
ret = poll_timeout_us(ret = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value),
                      ret > 0,
                      1000, wake_retry * 1000, true);
```

The variable `ret` is used both as the operation result (inside the macro expansion) AND as the macro's return value. After the macro completes, `ret` holds the macro's return value (0 or `-ETIMEDOUT`), not the `drm_dp_dpcd_readb` return value. This means the `ret < 0` check below is only checking for `-ETIMEDOUT` from the poll, not a DPCD read failure. This is confusing and fragile — use a separate variable for the poll result.

**Condition logic concern:**

```c
if (value == DP_SET_POWER_D3 || ret < 0) {
```

If the poll times out (ret == -ETIMEDOUT), `value` might contain stale or partial data from a failed read. The function proceeds to write to `DP_SET_POWER` without checking whether DPCD writes actually succeed — no error checking on either `drm_dp_dpcd_writeb` call.

**Wake sequence concern:**

```c
drm_dp_dpcd_writeb(aux, DP_SET_POWER, DP_SET_POWER_D0);
fsleep(1000);
drm_dp_dpcd_writeb(aux, DP_SET_POWER, DP_SET_POWER_D3_AUX_ON);
```

Setting to D0 and then immediately to D3_AUX_ON seems odd. The cover letter and spec reference (Section 2.3.4) say the goal is to wake the sink for AUX communication, but D3_AUX_ON is still a low-power state. If the caller is about to retry an AUX transaction, why not leave it in D0? The intent should be clarified.

**Missing `#include`:** The function uses `poll_timeout_us` from `<linux/iopoll.h>` and `fsleep`. Ensure the drm_dp_helper.c file includes the necessary headers.

**Naming:** `drm_dp_wake_sink` is generic but it specifically deals with AUX channel power states. A name like `drm_dp_aux_wake_sink` might be more precise.

**Documentation:** The kerneldoc is incomplete — it should describe the behavior (what it does on success/failure, return value semantics — though it's void).

---

---
Generated by Claude Code Patch Reviewer

[PATCH RFC 2/3] drm/i915/dp: On AUX_CH tx timeout, wake up the sink

[Arun R Murthy]

On a native AUX_CH transaction when the timer on DPTX timesout this can
be due to DPTX sending invalid command or the sink is waking up from
sleept state and hence there is a No Reply from DPRX. In this case try
to wake up the sink device.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_aux.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index b20ec3e589fadc4972efb77286978f38a527bd1c..0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -372,8 +372,17 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 			 *   Timeout errors from the HW already meet this
 			 *   requirement so skip to next iteration
 			 */
-			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
+			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
+				/*
+				 * Timeout can occur when there is no reply
+				 * from the sink and this can be either due to
+				 * DPTX sending wrong command or sink is sleep
+				 * state/waking up from sleep state.
+				 * Spec DP2.1 Section 2.11.7.1.5.8
+				 */
+				drm_dp_wake_sink(&intel_dp->aux);
 				continue;
+			}
 
 			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
 				usleep_range(400, 500);

-- 
2.25.1

Claude review: drm/i915/dp: On AUX_CH tx timeout, wake up the sink

[Claude Code Review Bot]

Patch Review

**Recursive AUX concern:**

```c
if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
    drm_dp_wake_sink(&intel_dp->aux);
    continue;
}
```

`drm_dp_wake_sink()` internally calls `drm_dp_dpcd_readb()` and `drm_dp_dpcd_writeb()`, which will call back into `intel_dp_aux_xfer()`. This is called inside the retry loop (up to 5 tries × number of clock dividers). Each invocation of `drm_dp_wake_sink` can itself do up to 6 retries with 1ms sleeps plus additional DPCD writes. This creates **nested AUX transactions within AUX transactions**, which could lead to lock recursion issues (the AUX mutex), or at minimum add very long delays (potentially tens of milliseconds per retry iteration).

The comment references "Spec DP2.1 Section 2.11.7.1.5.8" but the cover letter references "Section 2.3.4" — these should be consistent and correct.

**Performance:** On a disconnected sink (the common timeout case per the existing comment at line 410), this will add significant delay to every AUX probe by attempting to wake a non-existent device on every timeout retry.

---

---
Generated by Claude Code Patch Reviewer

[PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

[Arun R Murthy]

Use re_rmw and update the required bits for PORT_AUX_CTL and drop the
bit configurations that are not required(AUX Power Request setting of
bit 19). Also break writing to PORT_AUX_CTL into 2 steps with first step
for doing the configuration/settings and then second write to trigger
the AUX transaction.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_types.h |  6 +-
 drivers/gpu/drm/i915/display/intel_dp_aux.c        | 83 ++++++++++++++--------
 drivers/gpu/drm/i915/display/intel_psr.c           | 29 +++++---
 3 files changed, 76 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index e189f8c39ccb440f99cd642de177b18f3b605753..341749452579acfc3e08715d2f0b211bf6489dd9 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1882,10 +1882,10 @@ struct intel_dp {
 
 	u32 (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-	 * This function returns the value we have to program the AUX_CTL
-	 * register with to kick off an AUX transaction.
+	 * This function programs the configuration/settings for the AUX_CTL
+	 * register but dont kick off an AUX transaction.
 	 */
-	u32 (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
+	void (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
 				u32 aux_clock_divider);
 
 	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index 0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6..4fef378e0a8fbf79211fd98913e507e90b2b48ea 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -175,12 +175,13 @@ static int g4x_dp_aux_precharge_len(void)
 		precharge_min - preamble) / 2;
 }
 
-static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
-				int send_bytes,
-				u32 aux_clock_divider)
+static void g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
+				 int send_bytes,
+				 u32 aux_clock_divider)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
-	u32 timeout;
+	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+	u32 timeout, value;
 
 	/* Max timeout value on G4x-BDW: 1.6ms */
 	if (display->platform.broadwell)
@@ -188,8 +189,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 	else
 		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
 
-	return DP_AUX_CH_CTL_SEND_BUSY |
-		DP_AUX_CH_CTL_DONE |
+	value = DP_AUX_CH_CTL_DONE |
 		DP_AUX_CH_CTL_INTERRUPT |
 		DP_AUX_CH_CTL_TIME_OUT_ERROR |
 		timeout |
@@ -197,23 +197,35 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
 		DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) |
 		DP_AUX_CH_CTL_PRECHARGE_2US(g4x_dp_aux_precharge_len()) |
 		DP_AUX_CH_CTL_BIT_CLOCK_2X(aux_clock_divider);
+
+	intel_de_rmw(display, ch_ctl,
+		     (DP_AUX_CH_CTL_DONE |
+		      DP_AUX_CH_CTL_INTERRUPT |
+		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
+		      DP_AUX_CH_CTL_TIME_OUT_MASK |
+		      DP_AUX_CH_CTL_RECEIVE_ERROR |
+		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
+		      DP_AUX_CH_CTL_PRECHARGE_2US_MASK |
+		      DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK),
+		     value);
+	return;
 }
 
-static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
-				int send_bytes,
-				u32 unused)
+static void skl_get_aux_send_ctl(struct intel_dp *intel_dp,
+				 int send_bytes,
+				 u32 unused)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	u32 ret;
+	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
+	u32 value;
 
 	/*
 	 * Max timeout values:
 	 * SKL-GLK: 1.6ms
 	 * ICL+: 4ms
 	 */
-	ret = DP_AUX_CH_CTL_SEND_BUSY |
-		DP_AUX_CH_CTL_DONE |
+	value = DP_AUX_CH_CTL_DONE |
 		DP_AUX_CH_CTL_INTERRUPT |
 		DP_AUX_CH_CTL_TIME_OUT_ERROR |
 		DP_AUX_CH_CTL_TIME_OUT_MAX |
@@ -222,17 +234,22 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
 		DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len(intel_dp)) |
 		DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
 
-	if (intel_tc_port_in_tbt_alt_mode(dig_port))
-		ret |= DP_AUX_CH_CTL_TBT_IO;
+	intel_de_rmw(display, ch_ctl,
+		     (DP_AUX_CH_CTL_DONE |
+		      DP_AUX_CH_CTL_INTERRUPT |
+		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
+		      DP_AUX_CH_CTL_TIME_OUT_MASK |
+		      DP_AUX_CH_CTL_RECEIVE_ERROR |
+		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
+		      DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK |
+		      DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK),
+		     value);
 
-	/*
-	 * Power request bit is already set during aux power well enable.
-	 * Preserve the bit across aux transactions.
-	 */
-	if (DISPLAY_VER(display) >= 14)
-		ret |= XELPDP_DP_AUX_CH_CTL_POWER_REQUEST;
+	if (intel_tc_port_in_tbt_alt_mode(dig_port))
+		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
+			     DP_AUX_CH_CTL_TBT_IO);
 
-	return ret;
+	return;
 }
 
 static int
@@ -341,11 +358,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 	}
 
 	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
-		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
-							  send_bytes,
-							  aux_clock_divider);
+		intel_dp->get_aux_send_ctl(intel_dp, send_bytes,
+					   aux_clock_divider);
 
-		send_ctl |= aux_send_ctl_flags;
+		/* Update the flags */
+		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_AUX_AKSV_SELECT,
+			     aux_send_ctl_flags);
 
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
@@ -356,15 +374,20 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
 								 send_bytes - i));
 
 			/* Send the command and wait for it to complete */
-			intel_de_write(display, ch_ctl, send_ctl);
+			intel_de_rmw(display, ch_ctl,
+				     DP_AUX_CH_CTL_SEND_BUSY,
+				     DP_AUX_CH_CTL_SEND_BUSY);
 
 			status = intel_dp_aux_wait_done(intel_dp);
 
 			/* Clear done status and any errors */
-			intel_de_write(display, ch_ctl,
-				       status | DP_AUX_CH_CTL_DONE |
-				       DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				       DP_AUX_CH_CTL_RECEIVE_ERROR);
+			intel_de_rmw(display, ch_ctl,
+				     (DP_AUX_CH_CTL_DONE |
+				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
+				      DP_AUX_CH_CTL_RECEIVE_ERROR),
+				     (DP_AUX_CH_CTL_DONE |
+				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
+				      DP_AUX_CH_CTL_RECEIVE_ERROR));
 
 			/*
 			 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 9296ca3a4ff468a6e61babd81217e4ba19b15062..e06e04f20355d511e5c58fc28866aa763fd65a4b 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -722,7 +722,9 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
+	i915_reg_t ch_ctl = psr_aux_ctl_reg(display, cpu_transcoder);
 	u32 aux_clock_divider, aux_ctl;
+
 	/* write DP_SET_POWER=D0 */
 	static const u8 aux_msg[] = {
 		[0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER >> 16) & 0xf),
@@ -742,17 +744,26 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
 	/* Start with bits set for DDI_AUX_CTL register */
-	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
-					     aux_clock_divider);
+	intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
+				   aux_clock_divider);
 
 	/* Select only valid bits for SRD_AUX_CTL */
-	aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
-		EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
-		EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
-		EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
-
-	intel_de_write(display, psr_aux_ctl_reg(display, cpu_transcoder),
-		       aux_ctl);
+	aux_ctl = EDP_PSR_AUX_CTL_TIME_OUT_MASK |
+		  EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
+		  EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
+		  EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
+
+	intel_de_rmw(display, ch_ctl,
+		     (EDP_PSR_AUX_CTL_TIME_OUT_MASK |
+		      EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
+		      EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
+		      EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK),
+		     aux_ctl);
+
+	/* Send the command or intitate the AUX transaction */
+	intel_de_rmw(display, ch_ctl,
+		     DP_AUX_CH_CTL_SEND_BUSY,
+		     DP_AUX_CH_CTL_SEND_BUSY);
 }
 
 static bool psr2_su_region_et_valid(struct intel_connector *connector, bool panel_replay)

-- 
2.25.1

Re: [PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

[Jani Nikula]

On Mon, 09 Mar 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Use re_rmw and update the required bits for PORT_AUX_CTL and drop the
> bit configurations that are not required(AUX Power Request setting of
> bit 19). Also break writing to PORT_AUX_CTL into 2 steps with first step
> for doing the configuration/settings and then second write to trigger
> the AUX transaction.

The primary question the commit message should answer is, "Why?"

There's a whole lot of "What?" here, indeed too much since the patch is
doing too many things in one go.

The point of an RFC patch is to solicit feedback on the idea. But the
idea remains vague here as there's no rationale why this is needed.


BR,
Jani.


>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_types.h |  6 +-
>  drivers/gpu/drm/i915/display/intel_dp_aux.c        | 83 ++++++++++++++--------
>  drivers/gpu/drm/i915/display/intel_psr.c           | 29 +++++---
>  3 files changed, 76 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e189f8c39ccb440f99cd642de177b18f3b605753..341749452579acfc3e08715d2f0b211bf6489dd9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1882,10 +1882,10 @@ struct intel_dp {
>  
>  	u32 (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  	/*
> -	 * This function returns the value we have to program the AUX_CTL
> -	 * register with to kick off an AUX transaction.
> +	 * This function programs the configuration/settings for the AUX_CTL
> +	 * register but dont kick off an AUX transaction.
>  	 */
> -	u32 (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
> +	void (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>  				u32 aux_clock_divider);
>  
>  	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index 0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6..4fef378e0a8fbf79211fd98913e507e90b2b48ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -175,12 +175,13 @@ static int g4x_dp_aux_precharge_len(void)
>  		precharge_min - preamble) / 2;
>  }
>  
> -static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				int send_bytes,
> -				u32 aux_clock_divider)
> +static void g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> +				 int send_bytes,
> +				 u32 aux_clock_divider)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> -	u32 timeout;
> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +	u32 timeout, value;
>  
>  	/* Max timeout value on G4x-BDW: 1.6ms */
>  	if (display->platform.broadwell)
> @@ -188,8 +189,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  	else
>  		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>  
> -	return DP_AUX_CH_CTL_SEND_BUSY |
> -		DP_AUX_CH_CTL_DONE |
> +	value = DP_AUX_CH_CTL_DONE |
>  		DP_AUX_CH_CTL_INTERRUPT |
>  		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  		timeout |
> @@ -197,23 +197,35 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>  		DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) |
>  		DP_AUX_CH_CTL_PRECHARGE_2US(g4x_dp_aux_precharge_len()) |
>  		DP_AUX_CH_CTL_BIT_CLOCK_2X(aux_clock_divider);
> +
> +	intel_de_rmw(display, ch_ctl,
> +		     (DP_AUX_CH_CTL_DONE |
> +		      DP_AUX_CH_CTL_INTERRUPT |
> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
> +		      DP_AUX_CH_CTL_PRECHARGE_2US_MASK |
> +		      DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK),
> +		     value);
> +	return;
>  }
>  
> -static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> -				int send_bytes,
> -				u32 unused)
> +static void skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> +				 int send_bytes,
> +				 u32 unused)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> -	u32 ret;
> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> +	u32 value;
>  
>  	/*
>  	 * Max timeout values:
>  	 * SKL-GLK: 1.6ms
>  	 * ICL+: 4ms
>  	 */
> -	ret = DP_AUX_CH_CTL_SEND_BUSY |
> -		DP_AUX_CH_CTL_DONE |
> +	value = DP_AUX_CH_CTL_DONE |
>  		DP_AUX_CH_CTL_INTERRUPT |
>  		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  		DP_AUX_CH_CTL_TIME_OUT_MAX |
> @@ -222,17 +234,22 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>  		DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len(intel_dp)) |
>  		DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
>  
> -	if (intel_tc_port_in_tbt_alt_mode(dig_port))
> -		ret |= DP_AUX_CH_CTL_TBT_IO;
> +	intel_de_rmw(display, ch_ctl,
> +		     (DP_AUX_CH_CTL_DONE |
> +		      DP_AUX_CH_CTL_INTERRUPT |
> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
> +		      DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK |
> +		      DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK),
> +		     value);
>  
> -	/*
> -	 * Power request bit is already set during aux power well enable.
> -	 * Preserve the bit across aux transactions.
> -	 */
> -	if (DISPLAY_VER(display) >= 14)
> -		ret |= XELPDP_DP_AUX_CH_CTL_POWER_REQUEST;
> +	if (intel_tc_port_in_tbt_alt_mode(dig_port))
> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
> +			     DP_AUX_CH_CTL_TBT_IO);
>  
> -	return ret;
> +	return;
>  }
>  
>  static int
> @@ -341,11 +358,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	}
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> -		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> -							  send_bytes,
> -							  aux_clock_divider);
> +		intel_dp->get_aux_send_ctl(intel_dp, send_bytes,
> +					   aux_clock_divider);
>  
> -		send_ctl |= aux_send_ctl_flags;
> +		/* Update the flags */
> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_AUX_AKSV_SELECT,
> +			     aux_send_ctl_flags);
>  
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
> @@ -356,15 +374,20 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  								 send_bytes - i));
>  
>  			/* Send the command and wait for it to complete */
> -			intel_de_write(display, ch_ctl, send_ctl);
> +			intel_de_rmw(display, ch_ctl,
> +				     DP_AUX_CH_CTL_SEND_BUSY,
> +				     DP_AUX_CH_CTL_SEND_BUSY);
>  
>  			status = intel_dp_aux_wait_done(intel_dp);
>  
>  			/* Clear done status and any errors */
> -			intel_de_write(display, ch_ctl,
> -				       status | DP_AUX_CH_CTL_DONE |
> -				       DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				       DP_AUX_CH_CTL_RECEIVE_ERROR);
> +			intel_de_rmw(display, ch_ctl,
> +				     (DP_AUX_CH_CTL_DONE |
> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +				      DP_AUX_CH_CTL_RECEIVE_ERROR),
> +				     (DP_AUX_CH_CTL_DONE |
> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +				      DP_AUX_CH_CTL_RECEIVE_ERROR));
>  
>  			/*
>  			 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 9296ca3a4ff468a6e61babd81217e4ba19b15062..e06e04f20355d511e5c58fc28866aa763fd65a4b 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -722,7 +722,9 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> +	i915_reg_t ch_ctl = psr_aux_ctl_reg(display, cpu_transcoder);
>  	u32 aux_clock_divider, aux_ctl;
> +
>  	/* write DP_SET_POWER=D0 */
>  	static const u8 aux_msg[] = {
>  		[0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER >> 16) & 0xf),
> @@ -742,17 +744,26 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>  
>  	/* Start with bits set for DDI_AUX_CTL register */
> -	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
> -					     aux_clock_divider);
> +	intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
> +				   aux_clock_divider);
>  
>  	/* Select only valid bits for SRD_AUX_CTL */
> -	aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
> -		EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
> -		EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
> -		EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
> -
> -	intel_de_write(display, psr_aux_ctl_reg(display, cpu_transcoder),
> -		       aux_ctl);
> +	aux_ctl = EDP_PSR_AUX_CTL_TIME_OUT_MASK |
> +		  EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
> +		  EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
> +		  EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
> +
> +	intel_de_rmw(display, ch_ctl,
> +		     (EDP_PSR_AUX_CTL_TIME_OUT_MASK |
> +		      EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
> +		      EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
> +		      EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK),
> +		     aux_ctl);
> +
> +	/* Send the command or intitate the AUX transaction */
> +	intel_de_rmw(display, ch_ctl,
> +		     DP_AUX_CH_CTL_SEND_BUSY,
> +		     DP_AUX_CH_CTL_SEND_BUSY);
>  }
>  
>  static bool psr2_su_region_et_valid(struct intel_connector *connector, bool panel_replay)

-- 
Jani Nikula, Intel

Re: [PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

[Murthy, Arun R]

On 09-03-2026 18:39, Jani Nikula wrote:
> On Mon, 09 Mar 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> Use re_rmw and update the required bits for PORT_AUX_CTL and drop the
>> bit configurations that are not required(AUX Power Request setting of
>> bit 19). Also break writing to PORT_AUX_CTL into 2 steps with first step
>> for doing the configuration/settings and then second write to trigger
>> the AUX transaction.
> The primary question the commit message should answer is, "Why?"
>
> There's a whole lot of "What?" here, indeed too much since the patch is
> doing too many things in one go.
>
> The point of an RFC patch is to solicit feedback on the idea. But the
> idea remains vague here as there's no rationale why this is needed.
Before initiating the AUX transaction, power well is enabled by calling 
intel_display_power_get().

Here the AUX Power Request bit19 of PORT_AUX_CTL register is being set 
and with a timeout of 600us the AUX Power State is checked to see if the 
power is enabled.

Then as part of the AUX transaction, PORT_AUX_CTL is written using 
intel_reg_write. Since we are writing to the entire register the same 
bit19 AUX Power Request is again being set to ensure the bit is set. 
This setting of bit is un-necessary as its already being set in 
intel_display_power_get.
Hence in order to overcome this un-necessary setting of bit, 
read/mask/write is being used.

Thanks and Regards,
Arun R Murthy
-------------------

>
> BR,
> Jani.
>
>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display_types.h |  6 +-
>>   drivers/gpu/drm/i915/display/intel_dp_aux.c        | 83 ++++++++++++++--------
>>   drivers/gpu/drm/i915/display/intel_psr.c           | 29 +++++---
>>   3 files changed, 76 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index e189f8c39ccb440f99cd642de177b18f3b605753..341749452579acfc3e08715d2f0b211bf6489dd9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1882,10 +1882,10 @@ struct intel_dp {
>>   
>>   	u32 (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>>   	/*
>> -	 * This function returns the value we have to program the AUX_CTL
>> -	 * register with to kick off an AUX transaction.
>> +	 * This function programs the configuration/settings for the AUX_CTL
>> +	 * register but dont kick off an AUX transaction.
>>   	 */
>> -	u32 (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>> +	void (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>>   				u32 aux_clock_divider);
>>   
>>   	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> index 0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6..4fef378e0a8fbf79211fd98913e507e90b2b48ea 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> @@ -175,12 +175,13 @@ static int g4x_dp_aux_precharge_len(void)
>>   		precharge_min - preamble) / 2;
>>   }
>>   
>> -static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> -				int send_bytes,
>> -				u32 aux_clock_divider)
>> +static void g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> +				 int send_bytes,
>> +				 u32 aux_clock_divider)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>> -	u32 timeout;
>> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> +	u32 timeout, value;
>>   
>>   	/* Max timeout value on G4x-BDW: 1.6ms */
>>   	if (display->platform.broadwell)
>> @@ -188,8 +189,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   	else
>>   		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>>   
>> -	return DP_AUX_CH_CTL_SEND_BUSY |
>> -		DP_AUX_CH_CTL_DONE |
>> +	value = DP_AUX_CH_CTL_DONE |
>>   		DP_AUX_CH_CTL_INTERRUPT |
>>   		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>   		timeout |
>> @@ -197,23 +197,35 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   		DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) |
>>   		DP_AUX_CH_CTL_PRECHARGE_2US(g4x_dp_aux_precharge_len()) |
>>   		DP_AUX_CH_CTL_BIT_CLOCK_2X(aux_clock_divider);
>> +
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (DP_AUX_CH_CTL_DONE |
>> +		      DP_AUX_CH_CTL_INTERRUPT |
>> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
>> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
>> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> +		      DP_AUX_CH_CTL_PRECHARGE_2US_MASK |
>> +		      DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK),
>> +		     value);
>> +	return;
>>   }
>>   
>> -static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> -				int send_bytes,
>> -				u32 unused)
>> +static void skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> +				 int send_bytes,
>> +				 u32 unused)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>>   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> -	u32 ret;
>> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> +	u32 value;
>>   
>>   	/*
>>   	 * Max timeout values:
>>   	 * SKL-GLK: 1.6ms
>>   	 * ICL+: 4ms
>>   	 */
>> -	ret = DP_AUX_CH_CTL_SEND_BUSY |
>> -		DP_AUX_CH_CTL_DONE |
>> +	value = DP_AUX_CH_CTL_DONE |
>>   		DP_AUX_CH_CTL_INTERRUPT |
>>   		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>   		DP_AUX_CH_CTL_TIME_OUT_MAX |
>> @@ -222,17 +234,22 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   		DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len(intel_dp)) |
>>   		DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
>>   
>> -	if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> -		ret |= DP_AUX_CH_CTL_TBT_IO;
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (DP_AUX_CH_CTL_DONE |
>> +		      DP_AUX_CH_CTL_INTERRUPT |
>> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
>> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
>> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> +		      DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK |
>> +		      DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK),
>> +		     value);
>>   
>> -	/*
>> -	 * Power request bit is already set during aux power well enable.
>> -	 * Preserve the bit across aux transactions.
>> -	 */
>> -	if (DISPLAY_VER(display) >= 14)
>> -		ret |= XELPDP_DP_AUX_CH_CTL_POWER_REQUEST;
>> +	if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
>> +			     DP_AUX_CH_CTL_TBT_IO);
>>   
>> -	return ret;
>> +	return;
>>   }
>>   
>>   static int
>> @@ -341,11 +358,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>   	}
>>   
>>   	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>> -		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
>> -							  send_bytes,
>> -							  aux_clock_divider);
>> +		intel_dp->get_aux_send_ctl(intel_dp, send_bytes,
>> +					   aux_clock_divider);
>>   
>> -		send_ctl |= aux_send_ctl_flags;
>> +		/* Update the flags */
>> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_AUX_AKSV_SELECT,
>> +			     aux_send_ctl_flags);
>>   
>>   		/* Must try at least 3 times according to DP spec */
>>   		for (try = 0; try < 5; try++) {
>> @@ -356,15 +374,20 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>   								 send_bytes - i));
>>   
>>   			/* Send the command and wait for it to complete */
>> -			intel_de_write(display, ch_ctl, send_ctl);
>> +			intel_de_rmw(display, ch_ctl,
>> +				     DP_AUX_CH_CTL_SEND_BUSY,
>> +				     DP_AUX_CH_CTL_SEND_BUSY);
>>   
>>   			status = intel_dp_aux_wait_done(intel_dp);
>>   
>>   			/* Clear done status and any errors */
>> -			intel_de_write(display, ch_ctl,
>> -				       status | DP_AUX_CH_CTL_DONE |
>> -				       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -				       DP_AUX_CH_CTL_RECEIVE_ERROR);
>> +			intel_de_rmw(display, ch_ctl,
>> +				     (DP_AUX_CH_CTL_DONE |
>> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +				      DP_AUX_CH_CTL_RECEIVE_ERROR),
>> +				     (DP_AUX_CH_CTL_DONE |
>> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +				      DP_AUX_CH_CTL_RECEIVE_ERROR));
>>   
>>   			/*
>>   			 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9296ca3a4ff468a6e61babd81217e4ba19b15062..e06e04f20355d511e5c58fc28866aa763fd65a4b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -722,7 +722,9 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>>   	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> +	i915_reg_t ch_ctl = psr_aux_ctl_reg(display, cpu_transcoder);
>>   	u32 aux_clock_divider, aux_ctl;
>> +
>>   	/* write DP_SET_POWER=D0 */
>>   	static const u8 aux_msg[] = {
>>   		[0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER >> 16) & 0xf),
>> @@ -742,17 +744,26 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>>   	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>>   
>>   	/* Start with bits set for DDI_AUX_CTL register */
>> -	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> -					     aux_clock_divider);
>> +	intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> +				   aux_clock_divider);
>>   
>>   	/* Select only valid bits for SRD_AUX_CTL */
>> -	aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> -		EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> -		EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> -		EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> -
>> -	intel_de_write(display, psr_aux_ctl_reg(display, cpu_transcoder),
>> -		       aux_ctl);
>> +	aux_ctl = EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> +		  EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> +		  EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> +		  EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> +
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> +		      EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> +		      EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> +		      EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK),
>> +		     aux_ctl);
>> +
>> +	/* Send the command or intitate the AUX transaction */
>> +	intel_de_rmw(display, ch_ctl,
>> +		     DP_AUX_CH_CTL_SEND_BUSY,
>> +		     DP_AUX_CH_CTL_SEND_BUSY);
>>   }
>>   
>>   static bool psr2_su_region_et_valid(struct intel_connector *connector, bool panel_replay)

Claude review: drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

[Claude Code Review Bot]

Patch Review

This patch has the most issues:

**Broken abstraction:**

The `get_aux_send_ctl` callback is changed from returning a `u32` value to `void` with side effects (writing to hardware). This is a poor abstraction change — the function name says "get" but it now "sets". It also tightly couples what was a clean value-computation function to specific register operations.

**PSR code is broken:**

```c
/* Start with bits set for DDI_AUX_CTL register */
intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
                           aux_clock_divider);
```

The old code called `get_aux_send_ctl` to get a value, then masked it to select only PSR-valid bits, and wrote it to the **PSR AUX CTL register** (a *different* register than `ch_ctl`). The new code calls `get_aux_send_ctl` which now writes directly to the **DDI AUX CTL register** as a side effect. Then it does:

```c
intel_de_rmw(display, ch_ctl,
             (EDP_PSR_AUX_CTL_TIME_OUT_MASK | ...),
             aux_ctl);
```

Where `ch_ctl` is `psr_aux_ctl_reg(...)`, but `aux_ctl` is now set to just the mask values themselves — not actual configuration values read from the DDI register. The mask is being used as both the clear-mask and the value, which will just set all those bits to 1. This is clearly wrong.

Additionally, the function now triggers SEND_BUSY on the PSR AUX CTL register:

```c
intel_de_rmw(display, ch_ctl,
             DP_AUX_CH_CTL_SEND_BUSY,
             DP_AUX_CH_CTL_SEND_BUSY);
```

The original code never triggered SEND_BUSY — `hsw_psr_setup_aux` just configures the register for the PSR hardware to use later autonomously. Triggering it here would start an unwanted AUX transaction.

**RMW for status clearing is wrong:**

```c
/* Clear done status and any errors */
intel_de_rmw(display, ch_ctl,
             (DP_AUX_CH_CTL_DONE |
              DP_AUX_CH_CTL_TIME_OUT_ERROR |
              DP_AUX_CH_CTL_RECEIVE_ERROR),
             (DP_AUX_CH_CTL_DONE |
              DP_AUX_CH_CTL_TIME_OUT_ERROR |
              DP_AUX_CH_CTL_RECEIVE_ERROR));
```

`intel_de_rmw` does read-modify-write: `(reg & ~clear) | set`. Since the clear and set masks are the same, this will always set these status bits regardless of their current state. These are W1C (write-1-to-clear) bits in hardware, so you want to write 1 to clear them, but you should only clear the bits that are actually set (which the original `status |` approach did correctly by using the read-back `status` value). By always writing all three, you might clear status bits prematurely or cause unintended behavior.

**`return;` at end of void functions:**

```c
	     value);
	return;
}
```

Unnecessary `return;` at end of void functions (both `g4x_get_aux_send_ctl` and `skl_get_aux_send_ctl`).

**Missing `DP_AUX_CH_CTL_RECEIVE_ERROR` in g4x clear mask:**

In `g4x_get_aux_send_ctl`, the RMW clear mask includes `DP_AUX_CH_CTL_RECEIVE_ERROR` but the value being set also includes it (from the original code). However, the original value computation included `DP_AUX_CH_CTL_RECEIVE_ERROR` which is a W1C bit — it was set in the original `send_ctl` to ensure it gets cleared before the transaction. The RMW approach changes this semantic.

**TBT_IO handling:**

```c
if (intel_tc_port_in_tbt_alt_mode(dig_port))
    intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
                 DP_AUX_CH_CTL_TBT_IO);
```

This only ever sets `DP_AUX_CH_CTL_TBT_IO`, never clears it. If the port transitions out of TBT alt mode between transactions, the bit will remain set. The clear mask and set value are the same, so `rmw` with the same mask/value is just an OR. It should clear `DP_AUX_CH_CTL_TBT_IO` in the main rmw call above and only conditionally set it.

**Dropped `XELPDP_DP_AUX_CH_CTL_POWER_REQUEST` handling:**

The original code explicitly preserved this bit. The new RMW approach should naturally preserve it since it's not in the clear mask, but the cover letter frames this as intentional. This should be called out more clearly in the commit message as it's the key behavioral change, and needs careful validation that no code path depends on the explicit set.

---
Generated by Claude Code Patch Reviewer

Claude review: Some updates over DP AUX Transactions

[Claude Code Review Bot]

Overall Series Review

Subject: Some updates over DP AUX Transactions
Author: Arun R Murthy <arun.r.murthy@intel.com>
Patches: 6
Reviewed: 2026-03-10T12:41:15.829415

---

This RFC series attempts to address DP AUX transaction timeouts by (1) adding a helper to wake a sleeping sink, (2) calling it on timeout, and (3) refactoring `get_aux_send_ctl` from returning a value to directly programming the register via RMW. The series has several significant issues:

- **Patch 1** has a misuse of `poll_timeout_us` (using `ret` for two purposes simultaneously) and questionable wake logic.
- **Patch 2** is reasonable in intent but the wake call inside the retry loop may cause excessive delays.
- **Patch 3** is the most problematic: it fundamentally changes the `get_aux_send_ctl` abstraction from a pure value-returning function to one with side effects (writing to hardware registers), and the PSR changes are broken.

The stated motivation of avoiding touching the AUX power request bit (bit 19) is valid, but the approach is overly invasive and introduces multiple bugs. A simpler approach would be to keep the existing abstraction and just mask off / preserve the power request bit in the caller.

---

---
Generated by Claude Code Patch Reviewer