* [PATCH -next 0/2] drm: verisilicon: add hardware cursor support
@ 2026-03-09 8:53 Icenowy Zheng
2026-03-09 8:53 ` [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB Icenowy Zheng
2026-03-09 8:53 ` [PATCH -next 2/2] drm: verisilicon: add support for cursor planes Icenowy Zheng
0 siblings, 2 replies; 9+ messages in thread
From: Icenowy Zheng @ 2026-03-09 8:53 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB
2026-03-09 8:53 [PATCH -next 0/2] drm: verisilicon: add hardware cursor support Icenowy Zheng
@ 2026-03-09 8:53 ` Icenowy Zheng
2026-03-10 9:10 ` Thomas Zimmermann
2026-03-09 8:53 ` [PATCH -next 2/2] drm: verisilicon: add support for cursor planes Icenowy Zheng
1 sibling, 1 reply; 9+ messages in thread
From: Icenowy Zheng @ 2026-03-09 8:53 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH -next 2/2] drm: verisilicon: add support for cursor planes
2026-03-09 8:53 [PATCH -next 0/2] drm: verisilicon: add hardware cursor support Icenowy Zheng
2026-03-09 8:53 ` [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB Icenowy Zheng
@ 2026-03-09 8:53 ` Icenowy Zheng
2026-03-10 9:54 ` Thomas Zimmermann
1 sibling, 1 reply; 9+ messages in thread
From: Icenowy Zheng @ 2026-03-09 8:53 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB
2026-03-09 8:53 ` [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB Icenowy Zheng
@ 2026-03-10 9:10 ` Thomas Zimmermann
2026-03-10 9:56 ` Thomas Zimmermann
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Zimmermann @ 2026-03-10 9:10 UTC (permalink / raw)
To: Icenowy Zheng, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao
Hi
Am 09.03.26 um 09:53 schrieb 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;
Just 'unsigned int'.
BTW the other fields should have used regular C types unless they are
required to have a certain size.
I would also store this as max_cursor_width and max_cursor_height. The
term size somehow implies "raw size in bytes".
Best regards
Thomas
> const struct vs_formats *formats;
> };
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 2/2] drm: verisilicon: add support for cursor planes
2026-03-09 8:53 ` [PATCH -next 2/2] drm: verisilicon: add support for cursor planes Icenowy Zheng
@ 2026-03-10 9:54 ` Thomas Zimmermann
2026-03-24 5:34 ` Icenowy Zheng
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Zimmermann @ 2026-03-10 9:54 UTC (permalink / raw)
To: Icenowy Zheng, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao
Hi
Am 09.03.26 um 09:53 schrieb 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)
Use "if (!new_plane_state->visible)" for this test. The plane might be
invisible if the crtc has been set. you should return in all such cases
unless you have a good reason not to.
> + 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:
Instead of using max_cursor_size, could this be a HW-specific bit field
that you test the cursor size against? Like
if (!is_power_of_2(crtc_w) || !(max_cursor_size & crtc_w)
return -EINVAL
But also see my comment on the switch in atomic_update.
> + 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;
We already do this check at [1], including supported modifiers.
[1]
https://elixir.bootlin.com/linux/v6.19.6/source/drivers/gpu/drm/drm_atomic.c#L739
> +
> + /* 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;
> + }
You already set atomic_disable in the plane_helper_funcs. Therefore you
will never take this conditional.
> +
> + 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;
> + }
Here is another case where you can move a potential error state into the
atomic_check. If you add a custom vc_cursor_plane_state, you can add a
size field that stores the size constants. Then it would make sense to
keep the switch statement in atomic check and set the field right
there. Here in atomic_update, you'd need no switch.
> +
> + 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,
> +};
Maybe also add
static const u64 vs_cursor_plane_format_modifiers[] = {
DRM_FORMAT_MOD_LINEAR,
DRM_FORMAT_MOD_INVALID,
};
and pass it to drmm_universal_plane_alloc(). Using NULL there is
supported, but I'd consider it bad style. Your choice.
Best regards
Thomas
> +
> +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_
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB
2026-03-10 9:10 ` Thomas Zimmermann
@ 2026-03-10 9:56 ` Thomas Zimmermann
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Zimmermann @ 2026-03-10 9:56 UTC (permalink / raw)
To: Icenowy Zheng, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter
Cc: linux-kernel, dri-devel, Icenowy Zheng, Han Gao
Hi
Am 10.03.26 um 10:10 schrieb Thomas Zimmermann:
> Hi
>
> Am 09.03.26 um 09:53 schrieb 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;
>
> Just 'unsigned int'.
>
> BTW the other fields should have used regular C types unless they are
> required to have a certain size.
>
> I would also store this as max_cursor_width and max_cursor_height. The
> term size somehow implies "raw size in bytes".
Reading the other patch, I realized that this is a hardware limit where
width and height have to be equal. Then maybe add a comment that size
refers to width and height in pixels and scanlines.
Best regards
Thomas
>
> Best regards
> Thomas
>
>> const struct vs_formats *formats;
>> };
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 2/2] drm: verisilicon: add support for cursor planes
2026-03-10 9:54 ` Thomas Zimmermann
@ 2026-03-24 5:34 ` Icenowy Zheng
2026-03-24 7:45 ` Thomas Zimmermann
0 siblings, 1 reply; 9+ messages in thread
From: Icenowy Zheng @ 2026-03-24 5:34 UTC (permalink / raw)
To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard,
David Airlie, Simona Vetter
Cc: linux-kernel, dri-devel, Han Gao
在 2026-03-10二的 10:54 +0100，Thomas Zimmermann写道：
> Hi
>
> Am 09.03.26 um 09:53 schrieb 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)
>
> Use "if (!new_plane_state->visible)" for this test. The plane might
> be
> invisible if the crtc has been set. you should return in all such
> cases
> unless you have a good reason not to.
>
> > + 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:
>
> Instead of using max_cursor_size, could this be a HW-specific bit
> field
> that you test the cursor size against? Like
>
> if (!is_power_of_2(crtc_w) || !(max_cursor_size & crtc_w)
> return -EINVAL
>
> But also see my comment on the switch in atomic_update.
>
>
> > + 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;
>
> We already do this check at [1], including supported modifiers.
>
> [1]
> https://elixir.bootlin.com/linux/v6.19.6/source/drivers/gpu/drm/drm_atomic.c#L739
>
> > +
> > + /* 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;
> > + }
>
> You already set atomic_disable in the plane_helper_funcs. Therefore
> you
> will never take this conditional.
Well it looks like the condition to call atomic_disable instead of
atomic_update is drm_atomic_plane_disabling(), which only checks
whether the plane has a CRTC bound before and no CRTC bound after.
Here the code needs to be here to handle the situation that the plane
is bound but not visible, like what's mentioned in your previous
comment of atomic_check .
Thanks,
Icenowy
>
> > +
> > + 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;
> > + }
>
> Here is another case where you can move a potential error state into
> the
> atomic_check. If you add a custom vc_cursor_plane_state, you can add
> a
> size field that stores the size constants. Then it would make sense
> to
> keep the switch statement in atomic check and set the field right
> there. Here in atomic_update, you'd need no switch.
>
> > +
> > + 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,
> > +};
>
> Maybe also add
>
> static const u64 vs_cursor_plane_format_modifiers[] = {
> DRM_FORMAT_MOD_LINEAR,
> DRM_FORMAT_MOD_INVALID,
> };
>
> and pass it to drmm_universal_plane_alloc(). Using NULL there is
> supported, but I'd consider it bad style. Your choice.
>
> Best regards
> Thomas
>
> > +
> > +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_
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 2/2] drm: verisilicon: add support for cursor planes
2026-03-24 5:34 ` Icenowy Zheng
@ 2026-03-24 7:45 ` Thomas Zimmermann
2026-03-24 7:49 ` Icenowy Zheng
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Zimmermann @ 2026-03-24 7:45 UTC (permalink / raw)
To: Icenowy Zheng, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter
Cc: linux-kernel, dri-devel, Han Gao
Hi
Am 24.03.26 um 06:34 schrieb Icenowy Zheng:
> 在 2026-03-10二的 10:54 +0100，Thomas Zimmermann写道：
>> Hi
>>
>> Am 09.03.26 um 09:53 schrieb 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)
>> Use "if (!new_plane_state->visible)" for this test. The plane might
>> be
>> invisible if the crtc has been set. you should return in all such
>> cases
>> unless you have a good reason not to.
>>
>>> + 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:
>> Instead of using max_cursor_size, could this be a HW-specific bit
>> field
>> that you test the cursor size against? Like
>>
>> if (!is_power_of_2(crtc_w) || !(max_cursor_size & crtc_w)
>> return -EINVAL
>>
>> But also see my comment on the switch in atomic_update.
>>
>>
>>> + 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;
>> We already do this check at [1], including supported modifiers.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.19.6/source/drivers/gpu/drm/drm_atomic.c#L739
>>
>>> +
>>> + /* 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;
>>> + }
>> You already set atomic_disable in the plane_helper_funcs. Therefore
>> you
>> will never take this conditional.
> Well it looks like the condition to call atomic_disable instead of
> atomic_update is drm_atomic_plane_disabling(), which only checks
> whether the plane has a CRTC bound before and no CRTC bound after.
>
> Here the code needs to be here to handle the situation that the plane
> is bound but not visible, like what's mentioned in your previous
> comment of atomic_check .
I'd have to test, but my impression is that you'd never get here if
!visible. But I cannot find any such logic either, so I might be wrong
about that.
Two more questions come to my mind:
What happens if you keep the plane enabled? Is the hardware not able to
handle that?
What happens is you return -EINVAL in atomic_check if !visible?
Best regards
Thomas
>
> Thanks,
> Icenowy
>
>>> +
>>> + 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;
>>> + }
>> Here is another case where you can move a potential error state into
>> the
>> atomic_check. If you add a custom vc_cursor_plane_state, you can add
>> a
>> size field that stores the size constants. Then it would make sense
>> to
>> keep the switch statement in atomic check and set the field right
>> there. Here in atomic_update, you'd need no switch.
>>
>>> +
>>> + 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,
>>> +};
>> Maybe also add
>>
>> static const u64 vs_cursor_plane_format_modifiers[] = {
>> DRM_FORMAT_MOD_LINEAR,
>> DRM_FORMAT_MOD_INVALID,
>> };
>>
>> and pass it to drmm_universal_plane_alloc(). Using NULL there is
>> supported, but I'd consider it bad style. Your choice.
>>
>> Best regards
>> Thomas
>>
>>> +
>>> +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_
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 2/2] drm: verisilicon: add support for cursor planes
2026-03-24 7:45 ` Thomas Zimmermann
@ 2026-03-24 7:49 ` Icenowy Zheng
0 siblings, 0 replies; 9+ messages in thread
From: Icenowy Zheng @ 2026-03-24 7:49 UTC (permalink / raw)
To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard,
David Airlie, Simona Vetter
Cc: linux-kernel, dri-devel, Han Gao
在 2026-03-24二的 08:45 +0100，Thomas Zimmermann写道：
> Hi
>
> Am 24.03.26 um 06:34 schrieb Icenowy Zheng:
> > 在 2026-03-10二的 10:54 +0100，Thomas Zimmermann写道：
> > > Hi
> > >
> > > Am 09.03.26 um 09:53 schrieb 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)
> > > Use "if (!new_plane_state->visible)" for this test. The plane
> > > might
> > > be
> > > invisible if the crtc has been set. you should return in all
> > > such
> > > cases
> > > unless you have a good reason not to.
> > >
> > > > + 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:
> > > Instead of using max_cursor_size, could this be a HW-specific bit
> > > field
> > > that you test the cursor size against? Like
> > >
> > > if (!is_power_of_2(crtc_w) || !(max_cursor_size & crtc_w)
> > > return -EINVAL
> > >
> > > But also see my comment on the switch in atomic_update.
> > >
> > >
> > > > + 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;
> > > We already do this check at [1], including supported modifiers.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v6.19.6/source/drivers/gpu/drm/drm_atomic.c#L739
> > >
> > > > +
> > > > + /* 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;
> > > > + }
> > > You already set atomic_disable in the plane_helper_funcs.
> > > Therefore
> > > you
> > > will never take this conditional.
> > Well it looks like the condition to call atomic_disable instead of
> > atomic_update is drm_atomic_plane_disabling(), which only checks
> > whether the plane has a CRTC bound before and no CRTC bound after.
> >
> > Here the code needs to be here to handle the situation that the
> > plane
> > is bound but not visible, like what's mentioned in your previous
> > comment of atomic_check .
>
> I'd have to test, but my impression is that you'd never get here if
> !visible. But I cannot find any such logic either, so I might be
> wrong
> about that.
>
> Two more questions come to my mind:
>
> What happens if you keep the plane enabled? Is the hardware not able
> to
> handle that?
I think the hardware does not have a visibility bit -- its enable state
is even encoded into the format part (a special format value is encoded
as disabled).
So to make the cursor disappear it needs to be disabled (with the
format set to DISABLED).
>
> What happens is you return -EINVAL in atomic_check if !visible?
This sounds strange, maybe I should test it...
Thanks,
Icenowy
>
> Best regards
> Thomas
>
> >
> > Thanks,
> > Icenowy
> >
> > > > +
> > > > + 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;
> > > > + }
> > > Here is another case where you can move a potential error state
> > > into
> > > the
> > > atomic_check. If you add a custom vc_cursor_plane_state, you can
> > > add
> > > a
> > > size field that stores the size constants. Then it would make
> > > sense
> > > to
> > > keep the switch statement in atomic check and set the field right
> > > there. Here in atomic_update, you'd need no switch.
> > >
> > > > +
> > > > + 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,
> > > > +};
> > > Maybe also add
> > >
> > > static const u64 vs_cursor_plane_format_modifiers[] = {
> > > DRM_FORMAT_MOD_LINEAR,
> > > DRM_FORMAT_MOD_INVALID,
> > > };
> > >
> > > and pass it to drmm_universal_plane_alloc(). Using NULL there is
> > > supported, but I'd consider it bad style. Your choice.
> > >
> > > Best regards
> > > Thomas
> > >
> > > > +
> > > > +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_
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-03-24 7:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-03-09 8:53 [PATCH -next 0/2] drm: verisilicon: add hardware cursor support Icenowy Zheng
2026-03-09 8:53 ` [PATCH -next 1/2] drm: verisilicon: add max cursor size to HWDB Icenowy Zheng
2026-03-10 9:10 ` Thomas Zimmermann
2026-03-10 9:56 ` Thomas Zimmermann
2026-03-09 8:53 ` [PATCH -next 2/2] drm: verisilicon: add support for cursor planes Icenowy Zheng
2026-03-10 9:54 ` Thomas Zimmermann
2026-03-24 5:34 ` Icenowy Zheng
2026-03-24 7:45 ` Thomas Zimmermann
2026-03-24 7:49 ` Icenowy Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®