Re: [PATCH v2] drm/sun4i: Align VI buffer addresses for subsampled formats

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] DMA buffer underflow caused by mixing clipped and unclipped plane coordinates

Pre-existing issues:
- [High] Hardware clipping broken because `drm_fb_dma_get_gem_addr()` ignores clipped source coordinates
--

commit ecc3d1ed4b5c80402d5086efdcdc7ff6a120827c
Author: Chen-Yu Tsai <wenst@chromium.org>

drm/sun4i: Align VI buffer addresses for subsampled formats

This commit adjusts the DMA address returned by drm_fb_dma_get_gem_addr
for subsampled formats by calculating an intra-block offset and subtracting
it. This ensures the address points to the start of a subsampled block for
the display hardware.

> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -207,6 +207,20 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
>  		/* Get the start of the displayed memory */
>  		dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);

[Severity: High]
This is a pre-existing issue, but does using drm_fb_dma_get_gem_addr
break hardware clipping since it ignores clipped source coordinates?

The drm_fb_dma_get_gem_addr function calculates the base address using the
unclipped coordinates (state->src_x and state->src_y). 

If the hardware relies on the DMA base address to handle source X/Y offsets,
providing the unclipped address would cause it to fetch pixels starting from
the unclipped origin. This could shift the image and reveal regions that
should be hidden when a plane is placed partially off-screen on the left or
top edge.

>  
> +		/*
> +		 * The mixer can handle odd offsets into sub-sampled YUV
> +		 * planes, but needs the address of the first pixel in each
> +		 * sub-sampled block. Adjust the luma buffer address backwards.
> +		 */
> +		if (i == 0) {
> +			u32 x_diff, y_diff;
> +
> +			x_diff = (state->src.x1 >> 16) & (format->hsub - 1);
> +			y_diff = (state->src.y1 >> 16) & (format->vsub - 1);
> +			dma_addr -= y_diff * fb->pitches[i];
> +			dma_addr -= x_diff * format->cpp[i];

[Severity: High]
Can this subtraction lead to a DMA buffer underflow if the plane is clipped?

If userspace positions a sub-sampled YUV plane partially off-screen to the
left or top, the DRM core clips the source rectangle. This makes state->src.x1
or state->src.y1 positive, while the original state->src_x and state->src_y
remain 0.

Because drm_fb_dma_get_gem_addr calculates the base dma_addr using the
unclipped coordinates, it will start at offset 0. The patch then calculates
x_diff and y_diff using the clipped state->src.x1 and state->src.y1.

If the plane is clipped by 1 pixel, y_diff might be 1. Subtracting
y_diff * pitch from the unclipped base dma_addr (which is at offset 0)
would shift the pointer into unallocated memory before the start of the
buffer, which could trigger IOMMU faults or memory corruption.

> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908050229.754977-1-wenst@chromium.org?part=1