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: lcdif: Wait for vblank before disabling DMA Date: Tue, 31 Mar 2026 16:48:35 +1000 Message-ID: In-Reply-To: <20260330224619.2620782-4-paulk@sys-base.io> References: <20260330224619.2620782-1-paulk@sys-base.io> <20260330224619.2620782-4-paulk@sys-base.io> 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 is the actual bug fix. Two issues: **1. Malformed `Co-developed-by` tag (must fix):** The trailer has: ``` Signed-off-by: Paul Kocialkowski Co-developed-by: Lucas Stach ``` Per `Documentation/process/submitting-patches.rst`, every `Co-developed-by`= **must** be immediately followed by a `Signed-off-by` from that same perso= n. The correct form would be: ``` Co-developed-by: Lucas Stach Signed-off-by: Lucas Stach Signed-off-by: Paul Kocialkowski ``` (The patch author's `Signed-off-by` should generally come last.) **2. Duplicate error message (should fix):** ```c if (ret) drm_err(lcdif->drm, "Failed to disable controller!\n"); ``` This exact same string appears for both the existing `CTRLDESCL0_5_EN` poll= timeout and the new `CTRLDESCL0_5_SHADOW_LOAD_EN` poll timeout. If either = fails, the log gives no indication of *which* step timed out. The new messa= ge should be distinct, e.g., `"Failed to wait for shadow load completion!\n= "` or `"Timed out waiting for DMA frame completion!\n"`. **3. Approach observation (minor):** The technique of setting `SHADOW_LOAD_EN` and polling for hardware to clear= it as a proxy for "frame DMA is complete" is clever. The comment in the co= de explains the rationale well. The 36000 =C2=B5s timeout (~2 frames at 60 = Hz) matches the existing timeout above and is reasonable. One subtle question: after the first poll confirms `CTRLDESCL0_5_EN` is cle= ared, is the hardware guaranteed to still process a `SHADOW_LOAD_EN` reques= t? If `EN` being cleared means the layer is fully off, the shadow load migh= t never be consumed. This deserves verification against the hardware refere= nce or NXP BSP behavior =E2=80=94 though the fact that the NXP BSP uses a s= imilar (if cruder) approach with a 25 ms sleep suggests the hardware does h= onor it. --- Generated by Claude Code Patch Reviewer