From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx Date: Tue, 10 Mar 2026 12:41:16 +1000 Message-ID: In-Reply-To: <20260309-dp_aux_timeout-v1-3-08c610a63a84@intel.com> References: <20260309-dp_aux_timeout-v1-0-08c610a63a84@intel.com> <20260309-dp_aux_timeout-v1-3-08c610a63a84@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 =E2=80=94 the function name says "get" but it now "sets". It also ti= ghtly couples what was a clean value-computation function to specific regis= ter 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 se= lect 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` w= hich 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 =E2=80=94 not actual configuration values read f= rom 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 regist= er: ```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 =E2=80=94 `hsw_psr_setup_aux` j= ust 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 cl= ear and set masks are the same, this will always set these status bits rega= rdless of their current state. These are W1C (write-1-to-clear) bits in har= dware, 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 corr= ectly 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_RECEI= VE_ERROR` but the value being set also includes it (from the original code)= . However, the original value computation included `DP_AUX_CH_CTL_RECEIVE_E= RROR` which is a W1C bit =E2=80=94 it was set in the original `send_ctl` to= ensure it gets cleared before the transaction. The RMW approach changes th= is 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 tr= ansitions 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/va= lue is just an OR. It should clear `DP_AUX_CH_CTL_TBT_IO` in the main rmw c= all 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 shoul= d naturally preserve it since it's not in the clear mask, but the cover let= ter frames this as intentional. This should be called out more clearly in t= he commit message as it's the key behavioral change, and needs careful vali= dation that no code path depends on the explicit set. --- Generated by Claude Code Patch Reviewer