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/imagination: Improve firmware power off for layout_mars config Date: Sat, 14 Mar 2026 07:22:40 +1000 Message-ID: In-Reply-To: <20260313-b4-staging-layout_mars_base-v2-1-9e3c251d278e@imgtec.com> References: <20260313-b4-staging-layout_mars_base-v2-0-9e3c251d278e@imgtec.com> <20260313-b4-staging-layout_mars_base-v2-1-9e3c251d278e@imgtec.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 **Logic and correctness:** The core logic change is correct =E2=80=94 when `layout_mars` is true, Side= kick and SLC have been powered down by the firmware, so the kernel should n= ot poll their idle registers. Instead, at the end, the MARS_IDLE register i= s polled for CPU and system arbiter (excluding SOCIF). **Variable declaration ordering (minor style):** ```c u64 layout_mars_value =3D 0; bool layout_mars =3D false; bool meta_fw =3D pvr_dev->fw_dev.processor_type =3D=3D PVR_FW_PROCESSOR_TY= PE_META; u32 reg_value; ``` The kernel coding style prefers reverse-Christmas-tree ordering for variabl= e declarations (longest line first). `meta_fw` is the longest declaration b= ut appears after the shorter ones. This is a minor nit. **Early SLC idle check addition:** The original code had the SLC idle check only after the "Extra Idle checks"= block (BIF_STATUS_MMU etc.). This patch adds an *additional* SLC idle chec= k before the MTS DM association unset: ```c if (!layout_mars) { /* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. = */ err =3D pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_= mask, sidekick_idle_mask, POLL_TIMEOUT_USEC); if (err) return err; /* Wait for SLC to signal IDLE. */ err =3D pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_M= ASKFULL, ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC); if (err) return err; } ``` The original code only checked Sidekick idle before MTS teardown, and check= ed SLC idle *after* the extra idle checks. Now both Sidekick and SLC are ch= ecked in *both* locations for non-MARS configurations. The commit message d= oesn't explain why an additional early SLC idle check is needed. This shoul= d be clarified =E2=80=94 is it a pre-existing bug fix or a new requirement?= It's a semantic change that goes beyond the stated goal of skipping checks= for LAYOUT_MARS. **Alignment issue:** ```c err =3D pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_M= ASKFULL, ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC); ``` There's a stray extra space before `ROGUE_CR_SLC_IDLE_MASKFULL` on the cont= inuation line (one space too many for alignment). Compare with the second o= ccurrence which aligns correctly with tabs only. **Comment style (minor):** ```c /* * As FW core has been moved from SIDEKICK to the new MARS domain, checki= ng * idle bits for CPU & System Arbiter excluding SOCIF which will never be * idle if Host polling on this register */ ``` Missing period at end of comment. Kernel style expects properly punctuated = multi-line comments. **Garten idle condition:** ```c if (meta_fw || !layout_mars) { if (!skip_garten_idle) { ``` This is correct: for META firmware, always check the Garten wrapper idle vi= a SIDEKICK_IDLE. For non-META, non-MARS, also check it. For non-META MARS, = use the MARS_IDLE register instead. The logic is sound, but the `else` bran= ch (MARS idle) is only reachable when `!meta_fw && layout_mars`, which is t= he RISC-V MARS case. A brief comment clarifying this would help readability. **Blank line before error check:** ```c POLL_TIMEOUT_USEC); if (err) return err; ``` There's a blank line between the function call and the `if (err)` check in = the MARS_IDLE polling block. This is inconsistent with every other poll+che= ck pattern in the function which has no blank line. --- Generated by Claude Code Patch Reviewer