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/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory
Date: Tue, 10 Mar 2026 12:07:04 +1000
Message-ID: <review-patch1-20260309165635.1138413-1-wens@kernel.org>
In-Reply-To: <20260309165635.1138413-1-wens@kernel.org>
References: <20260309165635.1138413-1-wens@kernel.org> <20260309165635.1138413-1-wens@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**UI layer (`sun8i_ui_layer.c`) =E2=80=94 Clean conversion:**

The old code:
```c
gem =3D drm_fb_dma_get_gem_obj(fb, 0);
bpp =3D fb->format->cpp[0];
dma_addr =3D gem->dma_addr + fb->offsets[0];
dma_addr +=3D (state->src.x1 >> 16) * bpp;
dma_addr +=3D (state->src.y1 >> 16) * fb->pitches[0];
```

is replaced by:
```c
dma_addr =3D drm_fb_dma_get_gem_addr(fb, state, 0);
```

For plane 0 with standard (non-block) formats, the helper computes `obj->dm=
a_addr + offsets[0] + sample_x * char_per_block[0] + sample_y * pitches[0]`=
, which is equivalent. This conversion is correct.

**VI layer (`sun8i_vi_layer.c`) =E2=80=94 Subtle behavioral difference:**

The old code explicitly aligns source coordinates to chroma subsampling bou=
ndaries before computing offsets:
```c
src_x =3D (state->src.x1 >> 16) & ~(format->hsub - 1);
src_y =3D (state->src.y1 >> 16) & ~(format->vsub - 1);
```

The helper does not perform this alignment =E2=80=94 it just divides by hsu=
b/vsub directly:
```c
sample_x =3D (state->src_x >> 16) / h_div;
sample_y =3D (state->src_y >> 16) / v_div;
```

For **plane > 0** (chroma planes), these produce the same result: integer d=
ivision by hsub has the same effect as masking then dividing when the coord=
inates are valid (aligned to subsampling boundaries). The DRM atomic check =
framework should validate that source coordinates are properly aligned for =
subsampled formats.

For **plane 0** (luma), the old code applied the alignment mask before usin=
g the coordinates (`dx =3D src_x` without division), effectively rounding t=
he luma offset down to a chroma-aligned boundary. The helper does not do th=
is alignment for plane 0 (h_div=3Dv_div=3D1). In practice, for valid state,=
 source coordinates should already be aligned, so this is a no-op differenc=
e. If anything, the old behavior of rounding down luma coordinates was slig=
htly wrong =E2=80=94 it could silently shift the displayed region.

**Minor nits:**
- The `DRM_DEBUG_DRIVER("Using GEM @ %pad\n", ...)` debug prints are remove=
d. This is fine =E2=80=94 they add noise and the helper is well-established=
 across many drivers. Other sun4i files (`sun4i_backend.c`, `sun4i_frontend=
.c`) already use this helper without such debug prints.
- The unused variable `format` (alias for `fb->format`) is left in `sun8i_v=
i_layer_update_buffer` =E2=80=94 it's still used for `format->num_planes` i=
n the loop, so that's fine.

**Verdict:** The patch is correct. The alignment difference in the VI layer=
 is not a real-world concern since DRM enforces valid coordinates.

Reviewed-by: AI Review (informational, non-binding)

---
Generated by Claude Code Patch Reviewer