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/color: Program Pre-CSC registers for SDR Date: Sun, 12 Apr 2026 13:07:24 +1000 Message-ID: In-Reply-To: <20260408051514.608781-8-chaitanya.kumar.borah@intel.com> References: <20260408051514.608781-1-chaitanya.kumar.borah@intel.com> <20260408051514.608781-8-chaitanya.kumar.borah@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Issue (medium)**: Hardcoded `plane = plane - 3` offset: ```c /* * First 3 planes are HDR, so reduce by 3 to get to the right * SDR plane offset */ plane = plane - 3; ``` This assumes planes 0-2 are always HDR and SDR planes always start at index 3. While true for current hardware (`icl_hdr_plane_mask()` returns `BIT(PLANE_1) | BIT(PLANE_2) | BIT(PLANE_3)` = planes 0,1,2), this is a magic number that will silently break on future platforms with different layouts. Consider using a named constant or deriving the offset from `icl_hdr_plane_mask()` (e.g., `hweight8(icl_hdr_plane_mask())`). There is also no defensive check that `plane >= 3` before the subtraction. While `icl_is_hdr_plane()` guards the callers, an explicit `drm_WARN_ON` would catch future misuse. **Observation**: SDR LUT uses 16-bit precision (`1 << 16`) vs HDR's 24-bit (`1 << 24`), and writes to non-ENH register variants (`PLANE_PRE_CSC_GAMC_INDEX` vs `PLANE_PRE_CSC_GAMC_INDEX_ENH`). This matches the hardware. --- Generated by Claude Code Patch Reviewer