From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/tegra: Enable cmu for Tegra186 and Tegra194
Date: Sun, 12 Apr 2026 14:04:28 +1000
Message-ID: <review-patch1-20260407-tegra-drm-cmu-v4-1-2fe95f305afd@gmail.com>
In-Reply-To: <20260407-tegra-drm-cmu-v4-1-2fe95f305afd@gmail.com>
References: <20260407-tegra-drm-cmu-v4-1-2fe95f305afd@gmail.com> <20260407-tegra-drm-cmu-v4-1-2fe95f305afd@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

**dc.c -- LUT table and probe changes**

The `default_srgb_lut[]` table contains 1025 u16 entries (~2 KiB of static const data), which correctly matches the `OUTPUT_LUT_SIZE_SIZE_1025` register setting. The values form a monotonically increasing sRGB gamma curve from 0x6000 to 0x9FFF. This is reasonable.

The probe-time allocation uses `dmam_alloc_coherent`, which is the right approach -- managed DMA memory automatically freed on device removal, no manual cleanup needed:

```c
dc->cmu_output_lut =
    dmam_alloc_coherent(dc->dev, ARRAY_SIZE(default_srgb_lut) * sizeof(u64),
                        &dc->cmu_output_lut_phys, GFP_KERNEL);
```

The LUT packing `(r << 32) | (r << 16) | r` applies the same gamma curve to all three color channels, which is correct for sRGB correction.

The `has_nvdisplay` guard is appropriate -- only Tegra186 and Tegra194 have nvdisplay (confirmed at dc.c:3001 and dc.c:3052).

**dc.h -- Register definitions**

The new register definitions reuse offsets 0x431-0x433 which are already defined for legacy display controllers:

```c
#define DC_DISP_CORE_HEAD_SET_CONTROL_OUTPUT_LUT    0x431  /* conflicts with DC_DISP_SHIFT_CLOCK_OPTIONS */
#define DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE    0x432  /* conflicts with DC_DISP_DATA_ENABLE_OPTIONS */
#define DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE_HI 0x433  /* conflicts with DC_DISP_SERIAL_INTERFACE_OPTIONS */
```

This is **not a bug** -- the nvdisplay register map is different from the legacy one, and this pattern already exists in the driver (e.g., 0x442 is both `DC_DISP_INIT_SEQ_CONTROL` and `DC_DISP_PCALC_HEAD_SET_CROPPED_POINT_IN_CURSOR`). The legacy `DC_DISP_SHIFT_CLOCK_OPTIONS` at 0x431 is only ever written when `dc->rgb` is set (dc.c:2280), and RGB output is not used on nvdisplay SoCs (the DT has no "rgb" child node), so there's no runtime conflict.

**Minor style nit**: A comment grouping the nvdisplay-specific register aliases (similar to the existing comment-free grouping at dc.h:735-736) might help readability, but this is not blocking.

`CMU_ENABLE_ENABLE` follows the `{FIELD}_{VALUE}` naming convention used in NVIDIA TRM documentation. The placement next to the other `DC_DISP_DISP_COLOR_CONTROL` field defines is correct (it's bit 20 of register 0x430).

**sor.c -- HDMI enable path**

The CMU setup is cleanly integrated into the existing `DC_DISP_DISP_COLOR_CONTROL` read-modify-write flow:

```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) {
    tegra_dc_writel(dc, lower_32_bits(dc->cmu_output_lut_phys),
                    DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE);
    tegra_dc_writel(dc, upper_32_bits(dc->cmu_output_lut_phys),
                    DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE_HI);
    tegra_dc_writel(dc, OUTPUT_LUT_MODE_INTERPOLATE | OUTPUT_LUT_SIZE_SIZE_1025,
                    DC_DISP_CORE_HEAD_SET_CONTROL_OUTPUT_LUT);
    value |= CMU_ENABLE_ENABLE;
}
```

The LUT base address and control registers are written first, then `CMU_ENABLE_ENABLE` is OR'd into the value that will be written back to `DC_DISP_DISP_COLOR_CONTROL` after the color size switch statement. The ordering is correct -- the LUT is fully configured before it is enabled.

**sor.c -- DP enable path**

The DP path is structurally similar but standalone rather than integrated into an existing register flow:

```c
if (dc->soc->has_nvdisplay) {
    value = tegra_dc_readl(dc, DC_DISP_DISP_COLOR_CONTROL);
    tegra_dc_writel(dc, lower_32_bits(dc->cmu_output_lut_phys),
                    DC_DISP_COREPVT_HEAD_SET_OUTPUT_LUT_BASE);
    ...
    value |= CMU_ENABLE_ENABLE;
    tegra_dc_writel(dc, value, DC_DISP_DISP_COLOR_CONTROL);
}
```

This works correctly. Unlike the HDMI path, the DP enable path does not have an existing `DC_DISP_DISP_COLOR_CONTROL` read-modify-write, so a separate one is needed.

**Observation**: The DP path doesn't clear `DITHER_CONTROL_MASK` or `BASE_COLOR_SIZE_MASK` and doesn't set a `BASE_COLOR_SIZE` value. This means whatever was previously in those fields is preserved. This is presumably intentional since the DP path may configure color control elsewhere, or it may not need to. It's worth confirming this is intentional, but it's consistent with the pre-patch DP code which also didn't touch `DC_DISP_DISP_COLOR_CONTROL`.

**Minor observation -- no disable path cleanup**: The CMU enable bit is set in the enable paths but not cleared in `tegra_sor_hdmi_disable` or `tegra_sor_dp_disable`. Since the DMA buffer persists for the device lifetime and the hardware presumably resets state on display disable, this should be fine. If display disable doesn't reset CMU state, it's still harmless since the LUT and enable bit will be reprogrammed on the next enable.

**Minor observation -- DRM color management integration**: This hardcodes a fixed sRGB LUT rather than integrating with `drm_crtc_state.gamma_lut`. A follow-up patch could expose this through the DRM color management properties, but the pragmatic approach here (matching vendor/downstream behavior) is reasonable for an initial enablement.

**Overall**: Looks good. The patch is well-structured, uses managed DMA allocation, guards all changes behind `has_nvdisplay`, follows existing driver patterns for register offset reuse, and has been tested by the authors and an independent tester. No blocking issues found.

---
Generated by Claude Code Patch Reviewer