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/amd/display: Wrap DCN32 phantom-plane allocation in DC_RUN_WITH_PREEMPTION_ENABLED
Date: Tue, 05 May 2026 07:55:36 +1000
Message-ID: <review-patch1-20260504201905.90667-1-mikhail.v.gavrilov@gmail.com>
In-Reply-To: <20260504201905.90667-1-mikhail.v.gavrilov@gmail.com>
References: <20260504201905.90667-1-mikhail.v.gavrilov@gmail.com> <20260504201905.90667-1-mikhail.v.gavrilov@gmail.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

**Positive aspects:**

- Excellent commit message with full root-cause analysis, stack trace, repr=
oducer, and hardware verification.
- Correct identification of the problem: sleeping vmalloc called from softi=
rq-disabled (FP) context.
- The `DC_RUN_WITH_PREEMPTION_ENABLED()` macro is the right mechanism =E2=
=80=94 it's already used in `dc_create_stream_for_sink()` (dc_stream.c:180)=
 and DML2 wrappers for exactly this purpose.

**Issue 1: Redundant `#if !defined` guard (minor, should remove)**

```c
+#if !defined(DC_RUN_WITH_PREEMPTION_ENABLED)
+#define DC_RUN_WITH_PREEMPTION_ENABLED(code) code
+#endif
```

This guard is unnecessary. The patch also adds `#include "dc_fpu.h"` (line =
95), and `dc_fpu.h` unconditionally defines `DC_RUN_WITH_PREEMPTION_ENABLED=
` in all three branches of its preprocessor logic:
- With `CONFIG_DRM_AMD_DC_FP`: full implementation (dc_fpu.h:39-47)
- Without `CONFIG_DRM_AMD_DC_FP`: no-op `code` (dc_fpu.h:49)
- With `_LINUX_FPU_COMPILATION_UNIT`: no-op `code` (dc_fpu.h:54)

The `#if !defined` block can never trigger. It should be removed to avoid i=
mplying that the header might not define the macro.

**Issue 2: Fix should be in `dc_create_plane_state()`, not the call site (d=
esign concern)**

The analogous function `dc_create_stream_for_sink()` already wraps its own =
allocation internally:

```c
// dc_stream.c:180
DC_RUN_WITH_PREEMPTION_ENABLED(stream =3D kzalloc_obj(struct dc_stream_stat=
e, GFP_ATOMIC));
```

The fix should follow the same pattern by wrapping the allocation inside `d=
c_create_plane_state()` itself (dc_surface.c:89):

```c
struct dc_plane_state *dc_create_plane_state(const struct dc *dc)
{
    struct dc_plane_state *plane_state;
    DC_RUN_WITH_PREEMPTION_ENABLED(plane_state =3D kvzalloc_obj(*plane_stat=
e, GFP_ATOMIC));
    ...
}
```

This would protect **all** callers of `dc_create_plane_state()`, not just t=
he one in `dcn32_enable_phantom_plane()`. Currently `dc_state_create_phanto=
m_plane()` (dc_state.c:789) directly calls `dc_create_plane_state(dc)` =E2=
=80=94 it's the only intermediate caller, but fixing at the allocator level=
 is more robust and consistent with how `dc_create_stream_for_sink` handles=
 it.

**Issue 3: `dc_fpu.h` include may already be unnecessary (very minor)**

The `#include "dc_fpu.h"` addition is only needed because the `#if !defined=
` guard needs to reference the macro. If the guard is dropped and the fix m=
oves into `dc_create_plane_state()` (which is in dc_surface.c, where dc_fpu=
.h is presumably already included), then this include addition to dcn32_res=
ource.c becomes unnecessary.

**No correctness issue with the current code:** The macro expansion with th=
e `do { ... } while (0)` wrapper correctly assigns to the outer-scope `phan=
tom_plane` variable, and the `if (!phantom_plane) continue;` check on the n=
ext line handles allocation failure. The fix as-written does work =E2=80=94=
 the concerns above are about robustness and style.

---
Generated by Claude Code Patch Reviewer