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/audio: skip ELD query for force-off connectors Date: Wed, 27 May 2026 14:32:39 +1000 Message-ID: In-Reply-To: <20260526161925.17041-1-pmenzel@molgen.mpg.de> References: <20260526161925.17041-1-pmenzel@molgen.mpg.de> <20260526161925.17041-1-pmenzel@molgen.mpg.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Locking issue =E2=80=94 `intel_audio_connector_is_forced_off()` called wi= thout the audio mutex, but that's fine for this data; however, `intel_attac= hed_encoder()` access needs consideration:** The patch moves the `mutex_unlock(&display->audio.mutex)` *before* calling = `intel_audio_connector_is_forced_off()`. The function iterates the connecto= r list (safe via `drm_connector_list_iter`) and reads `connector->encoder` = (via `intel_attached_encoder`) and `connector->base.force`. The `force` fie= ld is set at boot from the video=3D parameter and is effectively immutable = after that, so reading it locklessly is fine. The `connector->encoder` poin= ter is similarly stable for forced-off connectors (they won't get hotplugge= d). So this is probably safe in practice, though a comment noting why no lo= ck is needed would help a future reader. **MST edge case:** With MST (DisplayPort Multi-Stream), a single physical port can have multip= le connectors. The new function returns `true` if *any* connector on the po= rt is forced off. If a user forces off one MST connector but another MST co= nnector on the same port is active, this would incorrectly suppress the ELD= query for the entire port. This is unlikely in practice (users don't typic= ally force off individual MST connectors), but worth considering whether th= e check should require *all* connectors on the port to be forced off, not j= ust one. **Unnecessary `#include` addition:** The diff adds `#include "intel_display.h"` at line 39, but `intel_display.h= ` is already included at line 39 in the patched file (the original file alr= eady has it). Looking at the diff context more carefully =E2=80=94 the diff= shows adding `intel_display.h` between `intel_de.h` and `intel_display_typ= es.h`. Checking the actual file confirms `intel_display.h` was **already pr= esent** before the patch. This include addition is redundant and should be = dropped. **Semantic question =E2=80=94 should the return be 0 or -EINVAL?** The patch changes the error path to return 0 with `*enabled =3D false` for = forced-off connectors. This is a reasonable choice that tells the audio dri= ver "nothing is connected, stop asking." But the existing callers of `get_e= ld` in the HDA driver might distinguish between "not connected" (0 + disabl= ed) and "error" (-EINVAL) in meaningful ways. The commit message explains t= his is desired behavior, and it does match what happens for physically disc= onnected ports with no active encoder, so the semantics are correct. **Minor: the debug message is now printed without the audio mutex held:** ```c mutex_unlock(&display->audio.mutex); if (intel_audio_connector_is_forced_off(display, port)) { *enabled =3D false; return 0; } drm_dbg_kms(display->drm, "Not valid for port %c\n", port_name(port)); return -EINVAL; ``` The `drm_dbg_kms` call was previously inside the mutex region (before the o= riginal `mutex_unlock`). Moving it outside the mutex is harmless =E2=80=94 = the debug print doesn't access any protected state =E2=80=94 but it's worth= noting as a side-effect of this refactoring. **Summary:** - The redundant `#include "intel_display.h"` should be dropped. - The MST edge case (one forced-off connector vs. all forced-off on a port)= deserves a mention in the commit message or a comment, even if it's not a = practical concern. - The locking is safe but subtle =E2=80=94 worth a brief inline comment. - Otherwise the patch is correct and addresses a real issue cleanly. --- Generated by Claude Code Patch Reviewer