diff mbox series

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

Message ID 20260908050229.754977-1-wenst@chromium.org (mailing list archive)
State New
Headers
Series [v2] drm/sun4i: Align VI buffer addresses for subsampled formats |

Commit Message

Chen-Yu Tsai Sept. 8, 2026, 5:02 a.m. UTC 
Commit 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to
get display memory") dropped the code to calculate the framebuffer's DMA
address in favor of drm_fb_dma_get_gem_addr().

This turned out to be wrong in a couple ways. The hardware can handle
pixel offsets within a sub-sampling block, but the buffer address must
start at the beginning of the subsampled block for all planes. The
hardware then does the interpolation internally. However the helper
gives the address exact pixel for luma, which messes up the result.

Instead of reverting the commit, calculate the intra-block offset and
adjust the buffer address so that it points to the start of the block.

Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
Cc: <stable@vger.kernel.org> # 7.1+
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
This is an alternative to Jernej's original revert:
https://lore.kernel.org/all/3980ea1aeb3f7fe8b4700e36560deeba3d050664.1785772659.git.jernej.skrabec@gmail.com/
---
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

sashiko-bot@kernel.org Sept. 8, 2026, 5:12 a.m. UTC | #1 
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.

> +		}
Chen-Yu Tsai Sept. 8, 2026, 5:17 a.m. UTC | #2 
On Tue, Sep 8, 2026 at 1:02 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> Commit 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to
> get display memory") dropped the code to calculate the framebuffer's DMA
> address in favor of drm_fb_dma_get_gem_addr().
>
> This turned out to be wrong in a couple ways. The hardware can handle
> pixel offsets within a sub-sampling block, but the buffer address must
> start at the beginning of the subsampled block for all planes. The
> hardware then does the interpolation internally. However the helper
> gives the address exact pixel for luma, which messes up the result.
>
> Instead of reverting the commit, calculate the intra-block offset and
> adjust the buffer address so that it points to the start of the block.
>
> Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> Cc: <stable@vger.kernel.org> # 7.1+
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Please ignore this one. As Sashiko pointed out, this could underflow the
buffer address.

Instead this should be sent along with another fix I have that switches
this back to the clipped buffer address.


Sorry for the noise.

> ---
> This is an alternative to Jernej's original revert:
> https://lore.kernel.org/all/3980ea1aeb3f7fe8b4700e36560deeba3d050664.1785772659.git.jernej.skrabec@gmail.com/
> ---
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index 2e9cda45c04e..185e9e5211c1 100644
> --- 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);
>
> +               /*
> +                * 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];
> +               }
> +
>                 /* Set the line width */
>                 DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
>                                  i + 1, fb->pitches[i]);
> --
> 2.55.0.979.g7e5102b832-goog
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 2e9cda45c04e..185e9e5211c1 100644
--- 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);
 
+		/*
+		 * 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];
+		}
+
 		/* Set the line width */
 		DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
 				 i + 1, fb->pitches[i]);