Re: [PATCH v3 3/5] drm/verisilicon: introduce per-variant hardware ops table

From: Joey Lu

Date: Mon Jun 08 2026 - 05:47:06 EST

On 6/8/2026 2:24 PM, Icenowy Zheng wrote:

在 2026-06-08一的 10:32 +0800，Joey Lu写道：

The DC8200 and DCUltraLite share a broadly similar register layout
but
differ in how the bridge, CRTC, primary plane and IRQ paths are
driven.
Introduce a vs_dc_funcs vtable so each variant can supply its own
implementation without scattering conditionals across multiple files.

Add enum vs_dc_generation (VSDC_GEN_DC8000 / VSDC_GEN_DC8200) to
vs_hwdb.h and a generation field to struct vs_chip_identity.
Annotate
all four existing DC8200 HWDB entries with VSDC_GEN_DC8200.

Extract the DC8200-specific hardware ops into a new vs_dc8200.c:
bridge_enable / bridge_disable - PANEL_CONFIG/START + CONFIG_EX
commit
enable_vblank / disable_vblank - TOP_IRQ_EN VSYNC bit
plane_enable_ex / disable_ex / update_ex - FB_CONFIG_EX path
irq_handler - reads TOP_IRQ_ACK

Update vs_bridge.c, vs_crtc.c, vs_primary_plane.c and vs_dc.c to
dispatch through dc->funcs instead of directly touching registers.
vs_crtc.c gains atomic_begin and atomic_flush hooks to allow variants
to gate per-frame commit cycles.

No behaviour change for existing DC8200 platforms.

Signed-off-by: Joey Lu <a0987203069@xxxxxxxxx>
---
drivers/gpu/drm/verisilicon/Makefile          |   2 +-
drivers/gpu/drm/verisilicon/vs_bridge.c       | 20 +---
drivers/gpu/drm/verisilicon/vs_crtc.c         | 38 ++++++-
drivers/gpu/drm/verisilicon/vs_dc.c           |   6 +-
drivers/gpu/drm/verisilicon/vs_dc.h           | 33 ++++++
drivers/gpu/drm/verisilicon/vs_dc8200.c       | 107
++++++++++++++++++
drivers/gpu/drm/verisilicon/vs_hwdb.c         |   4 +
drivers/gpu/drm/verisilicon/vs_hwdb.h         |   6 +
.../gpu/drm/verisilicon/vs_primary_plane.c    | 32 +-----
9 files changed, 197 insertions(+), 51 deletions(-)
create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8200.c

============ 8< ==================

diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.c
b/drivers/gpu/drm/verisilicon/vs_bridge.c
index 7a93049368db..6a9af10c64e6 100644
--- a/drivers/gpu/drm/verisilicon/vs_bridge.c
+++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
@@ -162,15 +162,8 @@ static void vs_bridge_enable_common(struct
vs_crtc *crtc,
VSDC_DISP_PANEL_CONFIG_DE_EN |
VSDC_DISP_PANEL_CONFIG_DAT_EN |
VSDC_DISP_PANEL_CONFIG_CLK_EN);
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
- VSDC_DISP_PANEL_CONFIG_RUNNING);
- regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
- VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
- VSDC_DISP_PANEL_START_RUNNING(output));
-
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc-

id),

- VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+
+ dc->funcs->bridge_enable(dc, output);

The code here being called "bridge" is only internal to kernel. Naming
it in such a way is okay, but maybe naming it "panel" is better
(because they're configuring PANEL-named registers).

And, as the common code setting common fields of DcregPanelConfig0 is
still here, maybe the helper name should be named "panel_enable_ex" (or
"bridge_enable_ex") ?

Understood. I will rename `bridge_enable`/`bridge_disable` to `panel_enable_ex`/`panel_disable_ex` throughout: in `vs_dc_funcs`, `vs_dc8200.c`, `vs_dc8000.c`, and the call sites in `vs_bridge.c`.

}
static const struct drm_bridge_funcs vs_dpi_bridge_funcs = {
====== 8< ==============

diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c
b/drivers/gpu/drm/verisilicon/vs_dc.c
index dad9967bc10b..c94957024189 100644
--- a/drivers/gpu/drm/verisilicon/vs_dc.c
+++ b/drivers/gpu/drm/verisilicon/vs_dc.c
@@ -8,9 +8,7 @@
#include <linux/of.h>
#include <linux/of_graph.h>
-#include "vs_crtc.h"
#include "vs_dc.h"
-#include "vs_dc_top_regs.h"
#include "vs_drm.h"
#include "vs_hwdb.h"
@@ -33,7 +31,7 @@ static irqreturn_t vs_dc_irq_handler(int irq, void
*private)
struct vs_dc *dc = private;
u32 irqs;
- regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs);
+ irqs = dc->funcs->irq_handler(dc);

The IRQ isn't handled in this helper.

So maybe call it "irq_ack"?

Understood. I will rename `irq_handler` to `irq_ack` in `vs_dc_funcs`, `vs_dc8200.c`, `vs_dc8000.c`, and the call site in `vs_dc.c`.

vs_drm_handle_irq(dc, irqs);
@@ -136,6 +134,8 @@ static int vs_dc_probe(struct platform_device
*pdev)
dev_info(dev, "Found DC%x rev %x customer %x\n", dc-

identity.model,

dc->identity.revision, dc->identity.customer_id);
+ dc->funcs = &vs_dc8200_funcs;
+
if (port_count > dc->identity.display_count) {
dev_err(dev, "too many downstream ports than HW
capability\n");
ret = -EINVAL;
diff --git a/drivers/gpu/drm/verisilicon/vs_dc.h
b/drivers/gpu/drm/verisilicon/vs_dc.h
index ed1016f18758..d77d4a1babdf 100644
--- a/drivers/gpu/drm/verisilicon/vs_dc.h
+++ b/drivers/gpu/drm/verisilicon/vs_dc.h
@@ -14,6 +14,7 @@
#include <linux/reset.h>
#include <drm/drm_device.h>
+#include <drm/drm_plane.h>
#include "vs_hwdb.h"
@@ -22,6 +23,34 @@
struct vs_drm_dev;
struct vs_crtc;
+struct vs_dc;
+
+struct vs_dc_funcs {
+ /* Bridge: atomic_enable, atomic_disable */
+ void (*bridge_enable)(struct vs_dc *dc, unsigned int
output);
+ void (*bridge_disable)(struct vs_dc *dc, unsigned int
output);
+
+ /* CRTC: atomic_begin, atomic_flush */
+ void (*crtc_begin)(struct vs_dc *dc, unsigned int output);
+ void (*crtc_flush)(struct vs_dc *dc, unsigned int output);
+
+ /* CRTC: atomic_enable, atomic_disable */
+ void (*crtc_enable)(struct vs_dc *dc, unsigned int output);
+ void (*crtc_disable)(struct vs_dc *dc, unsigned int output);
+
+ /* CRTC: enable_vblank, disable_vblank */
+ void (*enable_vblank)(struct vs_dc *dc, unsigned int
output);
+ void (*disable_vblank)(struct vs_dc *dc, unsigned int
output);
+
+ /* Primary plane: atomic_enable, atomic_disable,
atomic_update */
+ void (*plane_enable_ex)(struct vs_dc *dc, unsigned int
output);
+ void (*plane_disable_ex)(struct vs_dc *dc, unsigned int
output);
+ void (*plane_update_ex)(struct vs_dc *dc, unsigned int
output,
+ struct drm_plane_state *state);
+
+ /* IRQ handler */
+ u32 (*irq_handler)(struct vs_dc *dc);

See my comments elsewhere for the helper naming.

Understood. I will rename all vtable members per the comments above: `panel_enable_ex`, `panel_disable_ex`, `primary_plane_enable`, `primary_plane_disable`, `primary_plane_update`, `irq_ack`.

+};
struct vs_dc {
struct regmap *regs;

============= 8< =================

diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c
b/drivers/gpu/drm/verisilicon/vs_hwdb.c
index 2a0f7c59afa3..91524d16f778 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
@@ -94,6 +94,7 @@ static struct vs_chip_identity vs_chip_identities[]
= {
.revision = 0x5720,
.customer_id = ~0U,
+ .generation = VSDC_GEN_DC8200,
.display_count = 2,
.max_cursor_size = 64,
.formats = &vs_formats_no_yuv444,
@@ -103,6 +104,7 @@ static struct vs_chip_identity
vs_chip_identities[] = {
.revision = 0x5721,
.customer_id = 0x30B,
+ .generation = VSDC_GEN_DC8200,
.display_count = 2,
.max_cursor_size = 64,
.formats = &vs_formats_no_yuv444,
@@ -112,6 +114,7 @@ static struct vs_chip_identity
vs_chip_identities[] = {
.revision = 0x5720,
.customer_id = 0x310,
+ .generation = VSDC_GEN_DC8200,
.display_count = 2,
.max_cursor_size = 64,
.formats = &vs_formats_with_yuv444,
@@ -121,6 +124,7 @@ static struct vs_chip_identity
vs_chip_identities[] = {
.revision = 0x5720,
.customer_id = 0x311,
+ .generation = VSDC_GEN_DC8200,
.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 2065ecb73043..a15c8b565604 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.h
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.h
@@ -9,6 +9,11 @@
#include <linux/regmap.h>
#include <linux/types.h>
+enum vs_dc_generation {
+ VSDC_GEN_DC8000,
+ VSDC_GEN_DC8200,
+};
+
struct vs_formats {
const u32 *array;
unsigned int num;
@@ -19,6 +24,7 @@ struct vs_chip_identity {
u32 revision;
u32 customer_id;
+ enum vs_dc_generation generation;
u32 display_count;
/*
* The hardware only supports square cursor planes, so this
field
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index 1f2be41ae496..75bc36a078f7 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -53,12 +53,6 @@ static int vs_primary_plane_atomic_check(struct
drm_plane *plane,
return 0;
}
-static void vs_primary_plane_commit(struct vs_dc *dc, unsigned int
output)
-{
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_COMMIT);
-}
-
static void vs_primary_plane_atomic_enable(struct drm_plane *plane,
   struct drm_atomic_commit
*atomic_state)
{
@@ -69,13 +63,8 @@ static void vs_primary_plane_atomic_enable(struct
drm_plane *plane,
unsigned int output = vcrtc->id;
struct vs_dc *dc = vcrtc->dc;
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_FB_EN);
- regmap_update_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
-    VSDC_FB_CONFIG_EX_DISPLAY_ID_MASK,
-    VSDC_FB_CONFIG_EX_DISPLAY_ID(output));
-
- vs_primary_plane_commit(dc, output);
+ if (dc->funcs->plane_enable_ex)
+ dc->funcs->plane_enable_ex(dc, output);

Please note that all theae codes are for primary planes, maybe the
helper should be named mentioning primary. Overlay planes will need a
different codepath because they change different registers.

Thanks,
Icenowy

Understood. To avoid confusion, I will rename `plane_enable_ex`, `plane_disable_ex`, and `plane_update_ex` to `primary_plane_enable`, `primary_plane_disable`, and `primary_plane_update` in `vs_dc_funcs`, `vs_dc8200.c`, and `vs_primary_plane.c`.

}
static void vs_primary_plane_atomic_disable(struct drm_plane *plane,
@@ -88,10 +77,8 @@ static void vs_primary_plane_atomic_disable(struct
drm_plane *plane,
unsigned int output = vcrtc->id;
struct vs_dc *dc = vcrtc->dc;
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_FB_EN);
-
- vs_primary_plane_commit(dc, output);
+ if (dc->funcs->plane_disable_ex)
+ dc->funcs->plane_disable_ex(dc, output);
}
static void vs_primary_plane_atomic_update(struct drm_plane *plane,
@@ -133,18 +120,11 @@ static void
vs_primary_plane_atomic_update(struct drm_plane *plane,
regmap_write(dc->regs, VSDC_FB_STRIDE(output),
     fb->pitches[0]);
- regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
-      VSDC_MAKE_PLANE_POS(state->crtc_x, state-

crtc_y));

- regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
-      VSDC_MAKE_PLANE_POS(state->crtc_x + state-

crtc_w,

- state->crtc_y + state-

crtc_h));

regmap_write(dc->regs, VSDC_FB_SIZE(output),
     VSDC_MAKE_PLANE_SIZE(state->crtc_w, state-

crtc_h));

- regmap_write(dc->regs, VSDC_FB_BLEND_CONFIG(output),
-      VSDC_FB_BLEND_CONFIG_BLEND_DISABLE);
-
- vs_primary_plane_commit(dc, output);
+ if (dc->funcs->plane_update_ex)
+ dc->funcs->plane_update_ex(dc, output, state);
}
static const struct drm_plane_helper_funcs
vs_primary_plane_helper_funcs = {