[PATCH] drm: verisilicon: fix cursor offset field update

[PATCH] drm: verisilicon: fix cursor offset field update

[Icenowy Zheng]

The 4th argument of regmap_update_bits() expects a pre-shifted register
value instead of the field value when it's used to set a single field,
however I forgot this when writing the code setting the cursor X/Y
offset fields, thus the write operation doesn't happen at all (because
of the non-shifted values being masked by regmap_update_bits()).

Fix this problem so the cursor is correctly clipped.

The framebuffer address is also clipped so the cursor offset problem was
previously not discovered, and the framebuffer address deserves another
fix for proper cursor behavior when it's clipped.

Fixes: 8c4ae2189125 ("drm: verisilicon: add support for cursor planes")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
index fa4f601dd0c87..4ce311ae48823 100644
--- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
@@ -199,7 +199,7 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
 	} else {
 		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
 				   VSDC_CURSOR_CONFIG_X_OFF_MASK,
-				   -state->crtc_x);
+				   VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x));
 		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
 				   VSDC_CURSOR_LOCATION_X_MASK, 0);
 	}
@@ -213,7 +213,7 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
 	} else {
 		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
 				   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
-				   -state->crtc_y);
+				   VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y));
 		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
 				   VSDC_CURSOR_LOCATION_Y_MASK, 0);
 	}
-- 
2.52.0

Re: [PATCH] drm: verisilicon: fix cursor offset field update

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces a visual regression (double offset and shearing) by enabling hardware offsets for partially off-screen cursors without removing the existing offset applied to the DMA address.
--

--- Patch [1]: [PATCH] drm: verisilicon: fix cursor offset field update ---
commit 390079034c32d7765a68a40d73ac95602ccd1720
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

drm: verisilicon: fix cursor offset field update

This patch corrects the regmap_update_bits arguments for setting the cursor
X/Y offset fields by wrapping them in the appropriate macros, ensuring the
hardware offsets are correctly programmed when the cursor is clipped.

> diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> index fa4f601dd0c87..4ce311ae48823 100644
> --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
> @@ -199,7 +199,7 @@ static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
>  	} else {
>  		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
>  				   VSDC_CURSOR_CONFIG_X_OFF_MASK,
> -				   -state->crtc_x);
> +				   VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x));

[Severity: Medium]
By enabling the hardware offsets here, does this apply the offset twice since
the DMA address is already being shifted for clipped coordinates?

Earlier in vs_cursor_plane_atomic_update(), the DMA address is calculated
using the clipped state->src coordinates:

	dma_addr = vs_fb_get_dma_addr(fb, &state->src);

Looking at vs_fb_get_dma_addr(), it adjusts the base DMA address linearly:

	/* Fixup framebuffer address for src coordinates */
	dma_addr += drm_format_info_min_pitch(fb->format, 0,
					      src_rect->x1 >> 16);

Because the hardware offset is now functional with this patch, keeping the
existing DMA shift causes the cursor image to be offset by twice the intended
amount. Furthermore, linearly shifting a 2D hardware cursor DMA address typically
causes row shearing.

Does applying this patch independently from the framebuffer address fix
mentioned in the commit message introduce an active visual regression for
partially off-screen cursors?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812154829.671777-1-zhengxingda@iscas.ac.cn?part=1