[PATCH -next 0/2] drm: verisilicon: add hardware cursor support

[PATCH -next 0/2] drm: verisilicon: add hardware cursor support

[Icenowy Zheng]

Verisilicon display controllers have the hardware cursor feature.

The hardware cursor layer is a simplistic and restrictive layer as the
topmost layer of the display output. It can only support ARGB8888 images
and some certain square power-of-two sizes (32x32 is supported on all
DCs).

This patchset adds support for the hardware cursor as a cursor plane.

Icenowy Zheng (2):
  drm: verisilicon: add max cursor size to HWDB
  drm: verisilicon: add support for cursor planes

 drivers/gpu/drm/verisilicon/Makefile          |   3 +-
 drivers/gpu/drm/verisilicon/vs_crtc.c         |  11 +-
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c | 273 ++++++++++++++++++
 .../drm/verisilicon/vs_cursor_plane_regs.h    |  44 +++
 drivers/gpu/drm/verisilicon/vs_hwdb.c         |   4 +
 drivers/gpu/drm/verisilicon/vs_hwdb.h         |   1 +
 drivers/gpu/drm/verisilicon/vs_plane.h        |   1 +
 .../drm/verisilicon/vs_primary_plane_regs.h   |   5 +-
 8 files changed, 338 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/verisilicon/vs_cursor_plane.c
 create mode 100644 drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h

-- 
2.52.0

[PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB

[Icenowy Zheng]

Different display controller variants support different maximum cursor
size. All known DC8200 variants support both 32x32 and 64x64, but some
DC8000 variants support either only 32x32 or up to 256x256.

The minimum size is fixed at 32 and only PoT square sizes are supported.

Add the max cursor size field to HWDB and fill all entries with 64.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/vs_hwdb.c | 4 ++++
 drivers/gpu/drm/verisilicon/vs_hwdb.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
index 09336af0900ae..2a0f7c59afa3a 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
@@ -95,6 +95,7 @@ static struct vs_chip_identity vs_chip_identities[] = {
 		.customer_id = ~0U,
 
 		.display_count = 2,
+		.max_cursor_size = 64,
 		.formats = &vs_formats_no_yuv444,
 	},
 	{
@@ -103,6 +104,7 @@ static struct vs_chip_identity vs_chip_identities[] = {
 		.customer_id = 0x30B,
 
 		.display_count = 2,
+		.max_cursor_size = 64,
 		.formats = &vs_formats_no_yuv444,
 	},
 	{
@@ -111,6 +113,7 @@ static struct vs_chip_identity vs_chip_identities[] = {
 		.customer_id = 0x310,
 
 		.display_count = 2,
+		.max_cursor_size = 64,
 		.formats = &vs_formats_with_yuv444,
 	},
 	{
@@ -119,6 +122,7 @@ static struct vs_chip_identity vs_chip_identities[] = {
 		.customer_id = 0x311,
 
 		.display_count = 2,
+		.max_cursor_size = 64,
 		.formats = &vs_formats_no_yuv444,
 	},
 };
diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.h b/drivers/gpu/drm/verisilicon/vs_hwdb.h
index 92192e4fa0862..c54c9326c1681 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.h
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.h
@@ -20,6 +20,7 @@ struct vs_chip_identity {
 	u32 customer_id;
 
 	u32 display_count;
+	int32_t max_cursor_size;
 	const struct vs_formats *formats;
 };
 
-- 
2.52.0

[PATCH -next 2/2] drm: verisilicon: add support for cursor planes

[Icenowy Zheng]

Verisilicon display controllers support hardware cursors per output
port.

Add support for them as cursor planes.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/gpu/drm/verisilicon/Makefile          |   3 +-
 drivers/gpu/drm/verisilicon/vs_crtc.c         |  11 +-
 drivers/gpu/drm/verisilicon/vs_cursor_plane.c | 273 ++++++++++++++++++
 .../drm/verisilicon/vs_cursor_plane_regs.h    |  44 +++
 drivers/gpu/drm/verisilicon/vs_plane.h        |   1 +
 .../drm/verisilicon/vs_primary_plane_regs.h   |   5 +-
 6 files changed, 333 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/verisilicon/vs_cursor_plane.c
 create mode 100644 drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h

diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile
index fd8d805fbcde1..426f4bcaa834d 100644
--- a/drivers/gpu/drm/verisilicon/Makefile
+++ b/drivers/gpu/drm/verisilicon/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_drm.o vs_hwdb.o vs_plane.o vs_primary_plane.o
+verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_drm.o vs_hwdb.o \
+	vs_plane.o vs_primary_plane.o vs_cursor_plane.o
 
 obj-$(CONFIG_DRM_VERISILICON_DC) += verisilicon-dc.o
diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
index f494017130006..5c9714a3e69a7 100644
--- a/drivers/gpu/drm/verisilicon/vs_crtc.c
+++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
@@ -159,7 +159,7 @@ struct vs_crtc *vs_crtc_init(struct drm_device *drm_dev, struct vs_dc *dc,
 			     unsigned int output)
 {
 	struct vs_crtc *vcrtc;
-	struct drm_plane *primary;
+	struct drm_plane *primary, *cursor;
 	int ret;
 
 	vcrtc = drmm_kzalloc(drm_dev, sizeof(*vcrtc), GFP_KERNEL);
@@ -175,9 +175,16 @@ struct vs_crtc *vs_crtc_init(struct drm_device *drm_dev, struct vs_dc *dc,
 		return ERR_PTR(PTR_ERR(primary));
 	}
 
+	/* Create our cursor plane */
+	cursor = vs_cursor_plane_init(drm_dev, dc);
+	if (IS_ERR(cursor)) {
+		drm_err(drm_dev, "Couldn't create the cursor plane\n");
+		return ERR_CAST(cursor);
+	}
+
 	ret = drmm_crtc_init_with_planes(drm_dev, &vcrtc->base,
 					 primary,
-					 NULL,
+					 cursor,
 					 &vs_crtc_funcs,
 					 NULL);
 	if (ret) {
diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
new file mode 100644
index 0000000000000..4c86519458077
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Institute of Software, Chinese Academy of Sciences (ISCAS)
+ *
+ * Authors:
+ * Icenowy Zheng <zhengxingda@iscas.ac.cn>
+ */
+
+#include <linux/regmap.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_print.h>
+
+#include "vs_crtc.h"
+#include "vs_plane.h"
+#include "vs_dc.h"
+#include "vs_hwdb.h"
+#include "vs_cursor_plane_regs.h"
+
+#define VSDC_CURSOR_LOCATION_MAX_POSITIVE BIT_MASK(15)
+#define VSDC_CURSOR_LOCATION_MAX_NEGATIVE BIT_MASK(5)
+
+static bool vs_cursor_plane_check_coord(int32_t coord)
+{
+	if (coord >= 0)
+		return coord <= VSDC_CURSOR_LOCATION_MAX_POSITIVE;
+	else
+		return (-coord) <= VSDC_CURSOR_LOCATION_MAX_NEGATIVE;
+}
+
+static int vs_cursor_plane_atomic_check(struct drm_plane *plane,
+					struct drm_atomic_state *state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
+										 plane);
+	struct drm_crtc *crtc = new_plane_state->crtc;
+	struct drm_framebuffer *fb = new_plane_state->fb;
+	struct drm_crtc_state *crtc_state = NULL;
+	struct vs_crtc *vcrtc;
+	struct vs_dc *dc;
+	int ret;
+
+	if (crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	ret = drm_atomic_helper_check_plane_state(new_plane_state,
+						  crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  true, true);
+	if (ret)
+		return ret;
+
+	if (!crtc)
+		return 0; /* Skip validity check */
+
+	vcrtc = drm_crtc_to_vs_crtc(crtc);
+	dc = vcrtc->dc;
+
+	/* Only certain square sizes is supported */
+	switch (new_plane_state->crtc_w) {
+	case 32:
+	case 64:
+	case 128:
+	case 256:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (new_plane_state->crtc_w > dc->identity.max_cursor_size)
+		return -EINVAL;
+
+	if (new_plane_state->crtc_w != new_plane_state->crtc_h)
+		return -EINVAL;
+
+	/* Check if the cursor is inside the register fields' range */
+	if (!vs_cursor_plane_check_coord(new_plane_state->crtc_x) ||
+	    !vs_cursor_plane_check_coord(new_plane_state->crtc_y))
+		return -EINVAL;
+
+	if (fb) {
+		/* Only ARGB8888 is supported */
+		if (drm_WARN_ON_ONCE(plane->dev,
+				     fb->format->format != DRM_FORMAT_ARGB8888))
+			return -EINVAL;
+
+		/* Extra line padding isn't supported */
+		if (fb->pitches[0] !=
+		    drm_format_info_min_pitch(fb->format, 0,
+					      new_plane_state->crtc_w))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void vs_cursor_plane_commit(struct vs_dc *dc, unsigned int output)
+{
+	regmap_set_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+			VSDC_CURSOR_CONFIG_COMMIT |
+			VSDC_CURSOR_CONFIG_IMG_UPDATE);
+}
+
+static void vs_cursor_plane_atomic_enable(struct drm_plane *plane,
+					   struct drm_atomic_state *atomic_state)
+{
+	struct drm_plane_state *state = drm_atomic_get_new_plane_state(atomic_state,
+								       plane);
+	struct drm_crtc *crtc = state->crtc;
+	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
+	unsigned int output = vcrtc->id;
+	struct vs_dc *dc = vcrtc->dc;
+
+	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+			   VSDC_CURSOR_CONFIG_FMT_MASK,
+			   VSDC_CURSOR_CONFIG_FMT_ARGB8888);
+
+	vs_cursor_plane_commit(dc, output);
+}
+
+static void vs_cursor_plane_atomic_disable(struct drm_plane *plane,
+					    struct drm_atomic_state *atomic_state)
+{
+	struct drm_plane_state *state = drm_atomic_get_old_plane_state(atomic_state,
+								       plane);
+	struct drm_crtc *crtc = state->crtc;
+	struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
+	unsigned int output = vcrtc->id;
+	struct vs_dc *dc = vcrtc->dc;
+
+	regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+			   VSDC_CURSOR_CONFIG_FMT_MASK,
+			   VSDC_CURSOR_CONFIG_FMT_OFF);
+
+	vs_cursor_plane_commit(dc, output);
+}
+
+static void vs_cursor_plane_atomic_update(struct drm_plane *plane,
+					   struct drm_atomic_state *atomic_state)
+{
+	struct drm_plane_state *state = drm_atomic_get_new_plane_state(atomic_state,
+								       plane);
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->crtc;
+	struct vs_dc *dc;
+	struct vs_crtc *vcrtc;
+	unsigned int output;
+	dma_addr_t dma_addr;
+
+	if (!state->visible) {
+		vs_cursor_plane_atomic_disable(plane, atomic_state);
+		return;
+	}
+
+	vcrtc = drm_crtc_to_vs_crtc(crtc);
+	output = vcrtc->id;
+	dc = vcrtc->dc;
+
+	switch (state->crtc_w) {
+	case 32:
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_SIZE_MASK,
+				   VSDC_CURSOR_CONFIG_SIZE_32);
+		break;
+	case 64:
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_SIZE_MASK,
+				   VSDC_CURSOR_CONFIG_SIZE_64);
+		break;
+	case 128:
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_SIZE_MASK,
+				   VSDC_CURSOR_CONFIG_SIZE_128);
+		break;
+	case 256:
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_SIZE_MASK,
+				   VSDC_CURSOR_CONFIG_SIZE_256);
+		break;
+	}
+
+	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
+
+	regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output),
+		     lower_32_bits(dma_addr));
+
+	/*
+	 * The X_OFF and Y_OFF fields define which point does the LOCATION
+	 * register represent in the cursor image, and LOCATION register
+	 * values are unsigned. To for positive left-top  coordinates the
+	 * offset is set to 0 and the location is set to the coordinate, for
+	 * negative coordinates the location is set to 0 and the offset
+	 * is set to the opposite number of the coordinate to offset the
+	 * cursor image partly off-screen.
+	 */
+	if (state->crtc_x >= 0) {
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_X_OFF_MASK, 0);
+		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
+				   VSDC_CURSOR_LOCATION_X_MASK,
+				   VSDC_CURSOR_LOCATION_X(state->crtc_x));
+	} else {
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_X_OFF_MASK,
+				   -state->crtc_x);
+		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
+				   VSDC_CURSOR_LOCATION_X_MASK, 0);
+	}
+
+	if (state->crtc_y >= 0) {
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_Y_OFF_MASK, 0);
+		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
+				   VSDC_CURSOR_LOCATION_Y_MASK,
+				   VSDC_CURSOR_LOCATION_Y(state->crtc_y));
+	} else {
+		regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
+				   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
+				   -state->crtc_y);
+		regmap_update_bits(dc->regs, VSDC_CURSOR_LOCATION(output),
+				   VSDC_CURSOR_LOCATION_Y_MASK, 0);
+	}
+
+	vs_cursor_plane_commit(dc, output);
+}
+
+static const struct drm_plane_helper_funcs vs_cursor_plane_helper_funcs = {
+	.atomic_check	= vs_cursor_plane_atomic_check,
+	.atomic_update	= vs_cursor_plane_atomic_update,
+	.atomic_enable	= vs_cursor_plane_atomic_enable,
+	.atomic_disable	= vs_cursor_plane_atomic_disable,
+};
+
+static const struct drm_plane_funcs vs_cursor_plane_funcs = {
+	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.disable_plane		= drm_atomic_helper_disable_plane,
+	.reset			= drm_atomic_helper_plane_reset,
+	.update_plane		= drm_atomic_helper_update_plane,
+};
+
+static const u32 vs_cursor_plane_formats[] = {
+	DRM_FORMAT_ARGB8888,
+};
+
+struct drm_plane *vs_cursor_plane_init(struct drm_device *drm_dev,
+				       struct vs_dc *dc)
+{
+	struct drm_plane *plane;
+
+	plane = drmm_universal_plane_alloc(drm_dev, struct drm_plane, dev, 0,
+					   &vs_cursor_plane_funcs,
+					   vs_cursor_plane_formats,
+					   ARRAY_SIZE(vs_cursor_plane_formats),
+					   NULL,
+					   DRM_PLANE_TYPE_CURSOR,
+					   NULL);
+
+	if (IS_ERR(plane))
+		return plane;
+
+	drm_plane_helper_add(plane, &vs_cursor_plane_helper_funcs);
+
+	return plane;
+}
diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h b/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h
new file mode 100644
index 0000000000000..99693f2c95b94
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane_regs.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2025 Icenowy Zheng <uwu@icenowy.me>
+ *
+ * Based on vs_dc_hw.h, which is:
+ *   Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
+ */
+
+#ifndef _VS_CURSOR_PLANE_REGS_H_
+#define _VS_CURSOR_PLANE_REGS_H_
+
+#include <linux/bits.h>
+
+#define VSDC_CURSOR_CONFIG(n)		(0x1468 + 0x1080 * (n))
+#define VSDC_CURSOR_CONFIG_FMT_MASK	GENMASK(1, 0)
+#define VSDC_CURSOR_CONFIG_FMT_ARGB8888	(0x2 << 0)
+#define VSDC_CURSOR_CONFIG_FMT_OFF	(0x0 << 0)
+#define VSDC_CURSOR_CONFIG_IMG_UPDATE	BIT(2)
+#define VSDC_CURSOR_CONFIG_COMMIT	BIT(3)
+#define VSDC_CURSOR_CONFIG_SIZE_MASK	GENMASK(7, 5)
+#define VSDC_CURSOR_CONFIG_SIZE_32	(0x0 << 5)
+#define VSDC_CURSOR_CONFIG_SIZE_64	(0x1 << 5)
+#define VSDC_CURSOR_CONFIG_SIZE_128	(0x2 << 5)
+#define VSDC_CURSOR_CONFIG_SIZE_256	(0x3 << 5)
+#define VSDC_CURSOR_CONFIG_Y_OFF_MASK	GENMASK(12, 8)
+#define VSDC_CURSOR_CONFIG_Y_OFF(v)	((v) << 8)
+#define VSDC_CURSOR_CONFIG_X_OFF_MASK	GENMASK(20, 16)
+#define VSDC_CURSOR_CONFIG_X_OFF(v)	((v) << 16)
+
+#define VSDC_CURSOR_ADDRESS(n)		(0x146C + 0x1080 * (n))
+
+#define VSDC_CURSOR_LOCATION(n)		(0x1470 + 0x1080 * (n))
+#define VSDC_CURSOR_LOCATION_X_MASK	GENMASK(14, 0)
+#define VSDC_CURSOR_LOCATION_X(v)	((v) << 0)
+#define VSDC_CURSOR_LOCATION_Y_MASK	GENMASK(30, 16)
+#define VSDC_CURSOR_LOCATION_Y(v)	((v) << 16)
+
+#define VSDC_CURSOR_BACKGROUND(n)	(0x1474 + 0x1080 * (n))
+#define VSDC_CURSOR_BACKGRUOND_DEFAULT	0x00FFFFFF
+
+#define VSDC_CURSOR_FOREGROUND(n)	(0x1478 + 0x1080 * (n))
+#define VSDC_CURSOR_FOREGRUOND_DEFAULT	0x00AAAAAA
+
+#endif /* _VS_CURSOR_PLANE_REGS_H_ */
diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index 41875ea3d66a5..60b5b3a1bc22a 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.h
+++ b/drivers/gpu/drm/verisilicon/vs_plane.h
@@ -68,5 +68,6 @@ dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
 			      const struct drm_rect *src_rect);
 
 struct drm_plane *vs_primary_plane_init(struct drm_device *dev, struct vs_dc *dc);
+struct drm_plane *vs_cursor_plane_init(struct drm_device *dev, struct vs_dc *dc);
 
 #endif /* _VS_PLANE_H_ */
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
index cbb125c46b390..61a48c2faa1b2 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
@@ -1,9 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Copyright (C) 2025 Icenowy Zheng <uwu@icenowy.me>
+ * Copyright (C) 2026 Institute of Software, Chinese Academy of Sciences (ISCAS)
  *
  * Based on vs_dc_hw.h, which is:
  *   Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
+ *
+ * Authors:
+ * Icenowy Zheng <zhengxingda@iscas.ac.cn>
  */
 
 #ifndef _VS_PRIMARY_PLANE_REGS_H_
-- 
2.52.0

Claude review: drm: verisilicon: add hardware cursor support

[Claude Code Review Bot]

Overall Series Review

Subject: drm: verisilicon: add hardware cursor support
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Patches: 3
Reviewed: 2026-03-10T12:36:01.366419

---

This 2-patch series adds hardware cursor plane support to the Verisilicon display controller driver. Patch 1 extends the HWDB with a max cursor size field, and Patch 2 implements the cursor plane itself. The structure follows the existing primary plane pattern in the driver well.

However, there are **two bugs** in the register programming of the cursor plane that will cause incorrect behavior for negative cursor coordinates, plus off-by-one errors in the coordinate range checks. There are also minor style and cosmetic issues.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: verisilicon: add max cursor size to HWDB

[Claude Code Review Bot]

Patch Review

This patch is straightforward and mostly fine.

**Type style issue:** In `vs_hwdb.h`:
```c
int32_t max_cursor_size;
```
Kernel convention for struct members is to use the kernel-native types (`s32` or just `u32` since the value is always positive). All other fields in `struct vs_chip_identity` use `u32`. Since cursor size is always a positive value (32, 64, 128, 256), `u32` would be more consistent and appropriate.

**Missing `cursor_width`/`cursor_height` in mode_config:** The `vs_mode_config_init()` in `vs_drm.c` does not set `drm->mode_config.cursor_width` and `drm->mode_config.cursor_height`. These default to 64x64, which happens to match the current HWDB entries, but if a DC variant supports 128 or 256 this would be wrong. These should be set from `dc->identity.max_cursor_size` so userspace gets the correct max cursor size advertised via `DRM_CAP_CURSOR_WIDTH`/`DRM_CAP_CURSOR_HEIGHT`. This could be done in patch 2 but is worth noting here.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: verisilicon: add support for cursor planes

[Claude Code Review Bot]

Patch Review

**Bug (critical) — Negative coordinate offset values not shifted to bit field positions:**

In `vs_cursor_plane_atomic_update()`, when handling negative `crtc_x`:
```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_X_OFF_MASK,
                   -state->crtc_x);
```
`VSDC_CURSOR_CONFIG_X_OFF_MASK` is `GENMASK(20, 16)` (bits 20:16), but `-state->crtc_x` is not shifted to that position. Since `regmap_update_bits` performs `(old & ~mask) | (val & mask)`, the unshifted value will be ANDed with `0x1F0000`, yielding zero for any small coordinate value. The X offset will always be programmed as 0.

The fix is to use the existing shift macro:
```c
VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x)
```

The same bug exists for negative `crtc_y`:
```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
                   -state->crtc_y);
```
`VSDC_CURSOR_CONFIG_Y_OFF_MASK` is `GENMASK(12, 8)`, but the value is not shifted. Should use `VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y)`.

**Bug — Off-by-one errors in coordinate range checks:**

```c
#define VSDC_CURSOR_LOCATION_MAX_POSITIVE BIT_MASK(15)
#define VSDC_CURSOR_LOCATION_MAX_NEGATIVE BIT_MASK(5)
```
`BIT_MASK(n)` in the kernel is `(1UL << n)`, so `BIT_MASK(15)` = 32768 and `BIT_MASK(5)` = 32. But the LOCATION X/Y fields are 15 bits wide (`GENMASK(14, 0)` / `GENMASK(30, 16)`), which can hold 0–32767 max. And the offset fields are 5 bits wide, which can hold 0–31 max. The checks `coord <= BIT_MASK(15)` and `(-coord) <= BIT_MASK(5)` each allow one value too many. These should use `BIT_MASK(15) - 1` and `BIT_MASK(5) - 1`, or equivalently `GENMASK(14, 0)` and `GENMASK(4, 0)`.

**Comment typo:**
```c
 * To for positive left-top  coordinates the
```
Should be "For positive left-top coordinates the" (extra word "To" and double space).

**Typos in register defines** (`vs_cursor_plane_regs.h`):
```c
#define VSDC_CURSOR_BACKGRUOND_DEFAULT	0x00FFFFFF
#define VSDC_CURSOR_FOREGRUOND_DEFAULT	0x00AAAAAA
```
`BACKGRUOND` → `BACKGROUND`, `FOREGRUOND` → `FOREGROUND`. These are unused currently but will cause confusion if referenced later.

**Unrelated copyright change** in `vs_primary_plane_regs.h`: This patch changes the copyright header of `vs_primary_plane_regs.h` from Icenowy Zheng's personal email to the ISCAS institutional copyright and updates the year. This is unrelated to cursor plane support and should be a separate patch if desired.

**Missing `.prepare_fb` helper:** The cursor plane helper funcs don't include `.prepare_fb = drm_gem_plane_helper_prepare_fb` (or the shadow-plane equivalent). The primary plane appears to also omit this, so it may be handled elsewhere, but it's worth verifying that GEM buffer pinning/mapping is handled correctly for cursor buffers.

**`drmm_universal_plane_alloc` usage:**
```c
plane = drmm_universal_plane_alloc(drm_dev, struct drm_plane, dev, 0, ...);
```
The `member` argument `dev` refers to the `struct drm_device *dev` pointer within `struct drm_plane`. This is unusual — normally the macro is used with a wrapping struct where `member` names the embedded `struct drm_plane`. However, this matches the primary plane's existing usage and appears to be an existing pattern in this driver, so not an issue introduced by this series.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: verisilicon: add support for cursor planes

[Claude Code Review Bot]

Patch Review

**Bug: Missing `FIELD_PREP`/shift for X_OFF and Y_OFF values**

When writing negative offsets to the CONFIG register:

```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_X_OFF_MASK,
                   -state->crtc_x);
```

The X_OFF field is at bits [20:16] (`GENMASK(20, 16)`), but the raw value `-state->crtc_x` is not shifted. It should use the defined `VSDC_CURSOR_CONFIG_X_OFF(v)` macro:

```c
VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x)
```

Same issue for Y_OFF at bits [12:8]:

```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
                   -state->crtc_y);
```

Should be `VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y)`. Without these shifts, the offset value lands in the wrong bit positions (bits [4:0] instead of [20:16] for X, and [4:0] instead of [12:8] for Y), corrupting other fields in the CONFIG register.

**Issue: `mode_config.cursor_width/height` not set**

The DRM core exposes cursor size limits to userspace via `drm_mode_config.cursor_width` and `cursor_height`. These are not set anywhere in the driver (checked `vs_drm.c`). Without this, userspace won't know the maximum cursor size. This should be set in the mode_config init, e.g.:

```c
drm_dev->mode_config.cursor_width = dc->identity.max_cursor_size;
drm_dev->mode_config.cursor_height = dc->identity.max_cursor_size;
```

**Nit: Typos in register header**

```c
#define VSDC_CURSOR_BACKGRUOND_DEFAULT  0x00FFFFFF
#define VSDC_CURSOR_FOREGRUOND_DEFAULT  0x00AAAAAA
```

`BACKGRUOND` and `FOREGRUOND` should be `BACKGROUND` and `FOREGROUND`. These macros are currently unused but should be fixed before they get used.

**Nit: Comment typo**

```c
 * To for positive left-top  coordinates the
```

Should read "For positive left-top coordinates the" (remove "To", fix double space).

**Nit: `atomic_update` calls `atomic_disable` directly**

```c
if (!state->visible) {
    vs_cursor_plane_atomic_disable(plane, atomic_state);
    return;
}
```

This works but is unusual — normally `atomic_disable` is only called by the DRM core. The DRM atomic helper should call `atomic_disable` when the plane becomes invisible, so this explicit call may be redundant. Worth verifying that the DRM helpers handle this transition correctly for cursor planes, or if this is an intentional workaround.

**Style: `ERR_PTR(PTR_ERR(primary))` vs `ERR_CAST`**

In the existing code at `vs_crtc.c:175`:
```c
return ERR_PTR(PTR_ERR(primary));
```

The new cursor error path correctly uses `ERR_CAST(cursor)`. The pre-existing primary path should also use `ERR_CAST` for consistency, though that's outside the scope of this series.

---
Generated by Claude Code Patch Reviewer