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/imx: Add support for i.MX94 DCIF Date: Thu, 05 Mar 2026 13:34:49 +1000 Message-ID: In-Reply-To: <20260304-dcif-upstreaming-v8-5-bec5c047edd4@oss.nxp.com> References: <20260304-dcif-upstreaming-v8-0-bec5c047edd4@oss.nxp.com> <20260304-dcif-upstreaming-v8-5-bec5c047edd4@oss.nxp.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 This is the main driver patch. Several observations: **Potential issue =E2=80=94 `params[i] < 0` is always false (dcif-crc.c:203= 3)**: ```c ret =3D kstrtouint(opt, 10, ¶ms[i]); if (ret < 0) return ret; if (params[i] < 0) return -EINVAL; ``` `params` is declared as `int params[4]` but `kstrtouint` parses unsigned in= tegers. So this negative check is dead code =E2=80=94 `kstrtouint` will nev= er produce a negative value in `params[i]` (it would just fail to parse). E= ither use `kstrtoint` if you want to catch negatives, or remove the check. **regmap locking disabled (dcif-drv.c:2962)**: ```c static const struct regmap_config dcif_regmap_config =3D { ... .disable_locking =3D true, }; ``` Locking is disabled on the regmap, but CRC values are read from the IRQ han= dler under `spinlock_irqsave`, and regmap writes happen from both atomic co= mmit paths and IRQ context. If all access is single-threaded or properly se= rialized by the DRM framework, this is fine, but it's worth flagging. The `= dcif_crc_is_enabled` flag is accessed without explicit synchronization betw= een the atomic commit path and the IRQ handler (it's written in `dcif_crtc_= enable_crc_source`/`dcif_crtc_disable_crc_source` and read in `dcif_irq_han= dler` under `spinlock_irqsave`). The enable/disable functions are called fr= om atomic flush which doesn't hold the event_lock spinlock, so there's a sm= all race window. **`dcif_crtc_mode_valid` compares `crtc_clock` vs constant (dcif-crtc.c:233= 9)**: ```c #define DCIF_MAX_PIXEL_CLOCK 148500000 if (mode->crtc_clock > DCIF_MAX_PIXEL_CLOCK) ``` `mode->crtc_clock` is in kHz while `DCIF_MAX_PIXEL_CLOCK` is 148500000 (148= .5 MHz in Hz, or 148500 kHz). This comparison is therefore wrong: `crtc_clo= ck` in kHz would never exceed 148500000 kHz. The constant should be `148500= ` (kHz) instead of `148500000`. This is a **bug** =E2=80=94 the mode_valid = check will never reject any mode based on clock. **Duplicate format switch in crtc and plane (dcif-crtc.c:2377-2423, dcif-pl= ane.c:3465-3507)**: The pixel format switch statement is duplicated between= `dcif_set_formats()` and `dcif_plane_atomic_update()`. Both set the same C= TRLDESC0 register bits. When `dcif_crtc_atomic_enable` is called, it calls = `dcif_crtc_mode_set_nofb` =E2=86=92 `dcif_set_formats` which writes format = to layer 0's CTRLDESC0, then later the plane's `atomic_update` also writes = to CTRLDESC0 for the same layer. The plane update's write (line 3520) does = a full `regmap_write` that would overwrite the format bits already set by `= dcif_set_formats`. This should work correctly since both compute the same f= ormat value, but it's redundant. **Overlay plane declares YUV rejection too late (dcif-plane.c:3509-3512)**: ```c if (plane->type =3D=3D DRM_PLANE_TYPE_OVERLAY && yuv_fmt =3D=3D CTRLDESCL0_= YUV_FORMAT_Y2UY1V) { dev_err(dcif->drm.dev, "Overlay plane could not support YUV format\n"); return; } ``` This check is in `atomic_update`, not `atomic_check`. Invalid formats shoul= d be rejected during `atomic_check`, not silently skipped during update. Al= so, the overlay format list `dcif_overlay_plane_formats` doesn't include an= y YUV formats, so this check would only trigger if the code has a bug elsew= here. If the intent is to protect against YUV on the overlay, the format li= sts already handle this correctly, making this check dead code (but in the = wrong place regardless). **`drm_bridge_put` usage on non-refcounted bridge (dcif-crtc.c:2658)**: ```c struct drm_bridge *bridge __free(drm_bridge_put) =3D NULL; ``` `dcif_crtc_get_bridge()` calls `drm_bridge_chain_get_first_bridge()` which = returns a bridge pointer without incrementing a refcount. Using `__free(drm= _bridge_put)` on it would call `drm_bridge_put()` on a bridge that wasn't `= drm_bridge_get()`'d, which could lead to a use-after-free. This was noted a= s reviewed by Luca Ceresoli for "bridge refcounting" but it looks like `drm= _bridge_chain_get_first_bridge` doesn't take a reference. Worth verifying. **`dcif_driver` should be `const` (dcif-drv.c:2925)**: ```c static struct drm_driver dcif_driver =3D { ``` Minor: `dcif_driver` should be `static const struct drm_driver`. --- Generated by Claude Code Patch Reviewer