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/tegra: Enable cmu for Tegra186 and Tegra194 Date: Sun, 12 Apr 2026 14:39:02 +1000 Message-ID: In-Reply-To: <20260406-tegra-drm-cmu-v3-1-dfcb1dda4ad6@gmail.com> References: <20260406-tegra-drm-cmu-v3-1-dfcb1dda4ad6@gmail.com> <20260406-tegra-drm-cmu-v3-1-dfcb1dda4ad6@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Commit message tags (must fix):** The `Co-developed-by` tag requires a corresponding `Signed-off-by` immediately after it per kernel documentation (Documentation/process/submitting-patches.rst). Currently: ``` Co-developed-by: Kurt Kiefer Signed-off-by: Aaron Kling ``` Should be: ``` Co-developed-by: Kurt Kiefer Signed-off-by: Kurt Kiefer Signed-off-by: Aaron Kling ``` **dc.c -- LUT table and probe:** The `default_srgb_lut` table has exactly 1025 entries, matching the `OUTPUT_LUT_SIZE_SIZE_1025` hardware setting. Good. The LUT entry packing looks correct: ```c r = default_srgb_lut[i]; dc->cmu_output_lut[i] = (r << 32) | (r << 16) | r; ``` This applies the same sRGB curve to R, G, and B channels within a 64-bit entry (three 16-bit fields in the lower 48 bits). This matches what's expected for a uniform gamma correction. Use of `dmam_alloc_coherent` is appropriate -- the LUT must persist for the device's lifetime and this avoids manual cleanup. No issues here. **dc.h -- Register definitions:** The new `CMU_ENABLE_ENABLE` is correctly placed as a bit flag for `DC_DISP_DISP_COLOR_CONTROL` (0x430), alongside the existing `BASE_COLOR_SIZE_*` and `DITHER_CONTROL_*` definitions. Bit 20 doesn't conflict with any existing masks (`BASE_COLOR_SIZE_MASK` is bits 0-3, `DITHER_CONTROL_MASK` is bits 8-9). The new nvdisplay-specific registers: ```c #define DC_DISP_CORE_HEAD_SET_CONTROL_OUTPUT_LUT 0x431 #define DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE 0x432 #define DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE_HI 0x433 ``` These overlap with legacy registers (`DC_DISP_SHIFT_CLOCK_OPTIONS` at 0x431, `DC_DISP_DATA_ENABLE_OPTIONS` at 0x432, `DC_DISP_SERIAL_INTERFACE_OPTIONS` at 0x433), which is expected since nvdisplay (Tegra186+) uses a different register layout at the same offsets. The placement after the existing `/* Tegra186 and later */` section at the `DC_DISP_CORE_SOR_SET_CONTROL` block is correct. A brief comment like `/* Tegra186+ CMU output LUT registers */` would aid readability but is not strictly necessary. **sor.c -- HDMI enable path:** The insertion point is well-chosen. The CMU setup is inserted between the mask-clearing and the `switch(state->bpc)`: ```c value = tegra_dc_readl(dc, DC_DISP_DISP_COLOR_CONTROL); value &= ~DITHER_CONTROL_MASK; value &= ~BASE_COLOR_SIZE_MASK; + if (dc->soc->has_nvdisplay && dc->cmu_output_lut) { + /* ... program LUT base & control ... */ + value |= CMU_ENABLE_ENABLE; + } switch (state->bpc) { ``` This is clean -- `CMU_ENABLE_ENABLE` (bit 20) is accumulated into `value` alongside the color size bits, and all are written together at the existing `tegra_dc_writel(dc, value, DC_DISP_DISP_COLOR_CONTROL)` at the end of the switch. No bit conflicts. **sor.c -- DP enable path:** ```c + if (dc->soc->has_nvdisplay && dc->cmu_output_lut) { + value = tegra_dc_readl(dc, DC_DISP_DISP_COLOR_CONTROL); + tegra_dc_writel(dc, lower_32_bits(dc->cmu_output_phys), + DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE); + ... + value |= CMU_ENABLE_ENABLE; + tegra_dc_writel(dc, value, DC_DISP_DISP_COLOR_CONTROL); + } ``` This is a standalone read-modify-write, which is correct for the DP path since there's no existing color control manipulation there (confirmed: `DC_DISP_DISP_COLOR_CONTROL` is only touched in the HDMI enable path at lines 2557/2584 of the current tree). **Code duplication (minor):** The LUT base/control/CMU-enable programming sequence is nearly identical between the HDMI and DP paths (~8 lines duplicated). Consider extracting a helper like `tegra_dc_setup_cmu(dc)` in dc.c to avoid this. This is a style preference, not a correctness issue. **No disable path (observation):** CMU is enabled but never explicitly disabled on encoder disable. This is acceptable because: (a) CMU is always desired on nvdisplay hardware (matching vendor behavior), (b) the LUT buffer persists via dmam_alloc_coherent, and (c) CMU is re-programmed on every enable. However, if future work adds runtime CMU control or drm color management properties, a disable path would be needed. **Summary of required changes:** 1. Add missing `Signed-off-by: Kurt Kiefer ` after the `Co-developed-by` tag. **Suggestions (optional):** 1. Extract the duplicated LUT programming into a small helper function. 2. Add a brief comment above the new nvdisplay register definitions noting they overlap with legacy register addresses. --- Generated by Claude Code Patch Reviewer