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/vmwgfx: Never destroy STDU zero's Screen Target Date: Sat, 16 May 2026 10:41:44 +1000 Message-ID: In-Reply-To: <20260514153103.1465013-1-ian.forbes@broadcom.com> References: <20260514153103.1465013-1-ian.forbes@broadcom.com> <20260514153103.1465013-1-ian.forbes@broadcom.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 **Commit message:** Well written. Clearly explains the problem (client can'= t create a window, user can't wake VM), the root cause (Screen Target destr= oyed on disable), and the fix. The Fixes tag references the commit that int= roduced the "don't destroy on inactive" logic, which this patch extends. **Code review:** The change at `vmwgfx_stdu.c:455-456`: ```c if (stdu->base.unit =3D=3D 0) return; ``` This is placed after the bind-to-NULL + update sequence (lines 437-441) whi= ch blanks the display, and after the existing early return for the "enabled= but inactive" case (lines 446-449). So unit 0's Screen Target will still b= e unbound/blanked =E2=80=94 it just won't be destroyed. This is the right b= ehavior: the display goes dark but the ST metadata remains for clients to q= uery. **Minor concern =E2=80=94 skipped `content_fb_type` reset:** When we early-= return for unit 0, we also skip the `stdu->content_fb_type =3D SAME_AS_DISP= LAY` assignment at line 462. This is the same situation as the existing ear= ly return at line 449, which also skips it, so this is consistent with the = existing pattern. However, it's worth noting that `mode_set_nofb()` (line 3= 95-399) always destroys and resets `content_fb_type` before re-defining, so= the re-enable path should handle this correctly regardless. **Interaction with `mode_set_nofb`:** When the CRTC is re-enabled, `vmw_std= u_crtc_mode_set_nofb()` checks `stdu->defined` (line 388) and will destroy = + recreate the Screen Target with the new mode. Since we leave `stdu->defin= ed =3D true` for unit 0, this path works correctly =E2=80=94 it will tear d= own and rebuild as needed. **No leak on driver teardown:** `vmw_stdu_destroy_st()` checks `stdu->defin= ed` at the top (line 340) and is a no-op if false. Since unit 0 keeps `defi= ned =3D true`, any cleanup path that calls `destroy_st` will still properly= clean up. This looks safe. **Comment style:** The multi-line comment uses `/* ... */` style which is f= ine but the kernel generally prefers the opening `/*` on its own line for m= ulti-line comments (network-style). The existing comment above (lines 443-4= 45) uses the same style, so this is at least locally consistent. Not a bloc= ker. **Overall:** Clean, minimal fix. The approach of special-casing unit 0 is p= ragmatic =E2=80=94 it's the primary display and the one clients will look f= or. No correctness issues identified. **Reviewed-by worthy: Yes.** --- Generated by Claude Code Patch Reviewer