* [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support
@ 2026-06-25 9:44 Joey Lu
2026-06-25 9:44 ` [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants Joey Lu
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw)
To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, robh, krzk+dt, conor+dt
Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel,
linux-kernel, Joey Lu
This series adds support for the Verisilicon DCUltraLite display
controller as integrated in the Nuvoton MA35D1 SoC.
The Verisilicon DC driver and its DT binding were originally written by
Icenowy Zheng <zhengxingda@iscas.ac.cn> for the T-Head TH1520 SoC, which
carries a DC8200 IP block. The present series builds on that foundation
with gratitude to Icenowy for the original work.
The DCUltraLite is a different variant in the DC IP family. While the two
IPs share a broadly similar register layout, a number of differences
prevent the existing driver from working on the MA35D1 without
modification:
- No CONFIG_EX commit path: the DC8200 staging registers
(FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT, FB_BLEND_CONFIG,
PANEL_CONFIG_EX) are absent. The DCUltraLite uses enable (bit 0) and
reset (bit 4) bits in FB_CONFIG for direct framebuffer updates, and
requires a per-frame VALID bit toggle (FB_CONFIG bit 3) to latch
configuration changes.
- No PANEL_START register: panel output begins when
PANEL_CONFIG.RUNNING is set; the DC8200 multi-display sync start
register at 0x1CCC does not exist.
- Different IRQ registers: DISP_IRQ_STA at 0x147C / DISP_IRQ_EN at
0x1480, versus the DC8200's TOP_IRQ_ACK at 0x0010 / TOP_IRQ_EN at
0x0014.
- Simpler clock topology: two clocks ("core" bus gate and "pix0" pixel
divider); no axi or ahb clocks required.
- Single display output: no per-output indexing beyond index 0 is
needed.
- Hardware-discoverable identity: the DCUltraLite exposes chip identity
registers whose model field reads 0x0 (revision 0x5560,
customer_id 0x305), allowing the existing vs_fill_chip_identity()
path to identify the variant purely through register reads.
Patch 1 generalises the verisilicon,dc DT binding to accommodate the
Nuvoton MA35D1 SoC-specific compatible and the variant's two-clock,
one-reset, single-port topology.
Patch 2 adds the register-level macros needed by the DC8000 ops.
Patches 3-5 introduce the driver changes in three logical steps: the
vs_dc_funcs hardware ops vtable with DC8200 ops extracted into
vs_dc8200.c; making axi/ahb clocks optional as a separate atomic change;
and the DC8000 ops in vs_dc8000.c. Patch 6 adds the DCUltraLite HWDB
entry that gates hardware recognition once all support is in place.
Patch 7 adds the Kconfig dependency on ARCH_MA35, placed last because it
is only meaningful after the HWDB entry is added.
All patches have been tested on Nuvoton MA35D1 hardware.
Changes from v4:
- [dt-bindings] Kept clock and reset item descriptions in the global
clocks:/resets: properties; per-compatible sections only constrain
minItems/maxItems and override clock-names items for nuvoton,ma35d1-dcu.
- [dt-bindings] Dropped redundant global minItems/maxItems on clocks:
and clock-names:.
- [dt-bindings] Dropped the extra-space typo fix in port@0 description
to keep the patch atomic; left for a separate patch later.
- [ops] Renamed crtc_enable/crtc_disable hooks to crtc_enable_ex/
crtc_disable_ex.
- [ops] Added unified IRQ bit definitions; each irq_ack() implementation
now translates hardware-specific bits before returning.
- [clocks] Split the axi/ahb optional-clock change into its own patch
for atomicity.
- [hwdb] Simplified the commit message for patch 6.
- [kconfig] Simplified the commit message for patch 7.
Joey Lu (7):
dt-bindings: display: verisilicon,dc: generalize for single-output
variants
drm/verisilicon: add register-level macros for DC8000
drm/verisilicon: introduce per-variant hardware ops table
drm/verisilicon: make axi and ahb clocks optional
drm/verisilicon: add DC8000 (DCUltraLite) display controller support
drm/verisilicon: add DCUltraLite chip identity to HWDB
drm/verisilicon: extend Kconfig to support ARCH_MA35 platforms
.../bindings/display/verisilicon,dc.yaml | 57 +++++++++
drivers/gpu/drm/verisilicon/Kconfig | 2 +-
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_crtc_regs.h | 1 +
drivers/gpu/drm/verisilicon/vs_dc.c | 13 +-
drivers/gpu/drm/verisilicon/vs_dc.h | 33 +++++
drivers/gpu/drm/verisilicon/vs_dc8000.c | 86 +++++++++++++
drivers/gpu/drm/verisilicon/vs_dc8200.c | 115 ++++++++++++++++++
drivers/gpu/drm/verisilicon/vs_drm.c | 5 +-
drivers/gpu/drm/verisilicon/vs_drm.h | 8 ++
drivers/gpu/drm/verisilicon/vs_hwdb.c | 14 +++
drivers/gpu/drm/verisilicon/vs_hwdb.h | 6 +
.../gpu/drm/verisilicon/vs_primary_plane.c | 32 +----
.../drm/verisilicon/vs_primary_plane_regs.h | 3 +
16 files changed, 378 insertions(+), 57 deletions(-)
create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8000.c
create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8200.c
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 9:54 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 2/7] drm/verisilicon: add register-level macros for DC8000 Joey Lu ` (5 subsequent siblings) 6 siblings, 1 reply; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu The verisilicon,dc binding was originally written for the T-Head TH1520 SoC carrying a DC8200, and hard-codes five clocks, three resets and two output ports. Add the Nuvoton MA35D1 DCUltraLite (nuvoton,ma35d1-dcu) to the binding. The DCUltraLite uses only two clocks (core, pix0) and one reset (core), with a single output port. Use allOf/if blocks to express per-variant constraints rather than hard-coding the DC8200 topology at the top level. Each compatible's block constrains the clock and reset item counts; the nuvoton block additionally overrides clock-names to the two names it actually uses. Signed-off-by: Joey Lu <a0987203069@gmail.com> --- .../bindings/display/verisilicon,dc.yaml | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml index 9dc35ab973f2..1e751f3c7ce8 100644 --- a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml +++ b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml @@ -17,6 +17,7 @@ properties: items: - enum: - thead,th1520-dc8200 + - nuvoton,ma35d1-dcu - const: verisilicon,dc # DC IPs have discoverable ID/revision registers reg: @@ -77,6 +78,62 @@ required: - clock-names - ports +allOf: + - if: + properties: + compatible: + contains: + const: thead,th1520-dc8200 + then: + properties: + clocks: + minItems: 5 + maxItems: 5 + + clock-names: + minItems: 5 + maxItems: 5 + + resets: + minItems: 3 + maxItems: 3 + + reset-names: + minItems: 3 + maxItems: 3 + + required: + - resets + - reset-names + + - if: + properties: + compatible: + contains: + const: nuvoton,ma35d1-dcu + then: + properties: + clocks: + minItems: 2 + maxItems: 2 + + clock-names: + items: + - const: core + - const: pix0 + + resets: + minItems: 1 + maxItems: 1 + + reset-names: + items: + - const: core + + required: + - resets + - reset-names + additionalProperties: false examples: -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants 2026-06-25 9:44 ` [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants Joey Lu @ 2026-06-25 9:54 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-06-25 9:54 UTC (permalink / raw) To: Joey Lu; +Cc: devicetree, robh, dri-devel, conor+dt Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The Nuvoton MA35D1 conditional schema block logically conflicts with top-level constraints, causing validation of valid Nuvoton device trees to unconditionally fail. - [Medium] The Nuvoton MA35D1 conditional block fails to restrict the `ports` property to a single output port, contradicting the hardware description. -- commit 6779aa6e54db7988d8c51ba66d2a7972829a44e6 Author: Joey Lu <a0987203069@gmail.com> dt-bindings: display: verisilicon,dc: generalize for single-output variants This commit adds the Nuvoton MA35D1 DCUltraLite to the verisilicon,dc binding. It introduces conditional blocks to express per-variant constraints for clocks and resets rather than hard-coding the DC8200 topology at the top level. > diff --git a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml > index 9dc35ab973f20..1e751f3c7ce83 100644 > --- a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml > +++ b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml [ ... ] > +allOf: [ ... ] > + - if: > + properties: > + compatible: > + contains: > + const: nuvoton,ma35d1-dcu > + then: > + properties: > + clocks: > + minItems: 2 > + maxItems: 2 > + > + clock-names: > + items: > + - const: core > + - const: pix0 [Severity: High] Does this conditional schema conflict with the top-level constraints? Any device tree node containing the compatible string for the Nuvoton variant is evaluated against both the top-level schema and this conditional block concurrently. The top-level definitions strictly enforce a 5-item array and specific constant values for clock-names (such as the second item being axi). This block attempts to override those with a maximum of 2 items and restricts the second item to pix0. Since a length cannot be both exactly 5 and a maximum of 2, and an element cannot be both axi and pix0, this creates an unsatisfiable constraint. Will this cause validation of valid Nuvoton device trees to unconditionally fail? > + > + resets: > + minItems: 1 > + maxItems: 1 > + > + reset-names: > + items: > + - const: core > + > + required: > + - resets > + - reset-names [Severity: Medium] Does this block need to explicitly restrict the ports property to a single output port? The commit message states that the Nuvoton MA35D1 DCUltraLite has a single output port. However, because this conditional block does not explicitly disallow the second port, it inherits the top-level allowances. Will malformed device trees incorrectly specifying two output ports for the Nuvoton variant silently pass schema validation, bypassing the hardware topology checks? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260625094449.708386-1-a0987203069@gmail.com?part=1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 2/7] drm/verisilicon: add register-level macros for DC8000 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu 2026-06-25 9:44 ` [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 9:44 ` [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table Joey Lu ` (4 subsequent siblings) 6 siblings, 0 replies; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu Add register-level constants needed by the forthcoming DC8000 (DCUltraLite) hardware ops: VSDC_DISP_IRQ_VSYNC(n) in vs_crtc_regs.h: bit mask for per-output VSYNC interrupt bits in DISP_IRQ_STA (0x147C) / DISP_IRQ_EN (0x1480), which are the IRQ registers used by DCUltraLite in place of the DC8200 TOP_IRQ_ACK / TOP_IRQ_EN registers. VSDC_FB_CONFIG_ENABLE (bit 0), VSDC_FB_CONFIG_VALID (bit 3) and VSDC_FB_CONFIG_RESET (bit 4) in vs_primary_plane_regs.h: control bits in the FB_CONFIG register used by DCUltraLite for framebuffer enable and per-frame commit handshake. No behaviour change for existing DC8200 platforms. Signed-off-by: Joey Lu <a0987203069@gmail.com> Reviewed-by: Icenowy Zheng <zhengxingda@iscas.ac.cn> --- drivers/gpu/drm/verisilicon/vs_crtc_regs.h | 1 + drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h | 3 +++ 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/verisilicon/vs_crtc_regs.h b/drivers/gpu/drm/verisilicon/vs_crtc_regs.h index c7930e817635..d4da22b08cd5 100644 --- a/drivers/gpu/drm/verisilicon/vs_crtc_regs.h +++ b/drivers/gpu/drm/verisilicon/vs_crtc_regs.h @@ -54,6 +54,7 @@ #define VSDC_DISP_GAMMA_DATA(n) (0x1460 + 0x4 * (n)) #define VSDC_DISP_IRQ_STA 0x147C +#define VSDC_DISP_IRQ_VSYNC(n) BIT(n) #define VSDC_DISP_IRQ_EN 0x1480 diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h index cbb125c46b39..67d4b00f294e 100644 --- a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h @@ -16,6 +16,9 @@ #define VSDC_FB_STRIDE(n) (0x1408 + 0x4 * (n)) #define VSDC_FB_CONFIG(n) (0x1518 + 0x4 * (n)) +#define VSDC_FB_CONFIG_ENABLE BIT(0) +#define VSDC_FB_CONFIG_VALID BIT(3) +#define VSDC_FB_CONFIG_RESET BIT(4) #define VSDC_FB_CONFIG_CLEAR_EN BIT(8) #define VSDC_FB_CONFIG_ROT_MASK GENMASK(13, 11) #define VSDC_FB_CONFIG_ROT(v) ((v) << 11) -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu 2026-06-25 9:44 ` [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants Joey Lu 2026-06-25 9:44 ` [PATCH v5 2/7] drm/verisilicon: add register-level macros for DC8000 Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 10:00 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional Joey Lu ` (3 subsequent siblings) 6 siblings, 1 reply; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, 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 a generation field to struct vs_chip_identity to distinguish variants. Extract the DC8200-specific hardware ops into vs_dc8200.c and add unified IRQ bit definitions so implementations can translate hardware-specific bits to a common set. Update the shared code to dispatch through dc->funcs. No behaviour change for existing DC8200 platforms. Signed-off-by: Joey Lu <a0987203069@gmail.com> --- 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 | 32 +++++ drivers/gpu/drm/verisilicon/vs_dc8200.c | 115 ++++++++++++++++++ drivers/gpu/drm/verisilicon/vs_drm.c | 5 +- drivers/gpu/drm/verisilicon/vs_drm.h | 8 ++ drivers/gpu/drm/verisilicon/vs_hwdb.c | 4 + drivers/gpu/drm/verisilicon/vs_hwdb.h | 6 + .../gpu/drm/verisilicon/vs_primary_plane.c | 32 +---- 11 files changed, 214 insertions(+), 54 deletions(-) create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8200.c diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile index 426f4bcaa834..9d4cd16452fa 100644 --- a/drivers/gpu/drm/verisilicon/Makefile +++ b/drivers/gpu/drm/verisilicon/Makefile @@ -1,6 +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 \ +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.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_bridge.c b/drivers/gpu/drm/verisilicon/vs_bridge.c index dc7c85b07fe3..3fbc8d57f8a1 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->panel_enable_ex(dc, output); } static void vs_bridge_atomic_enable_dpi(struct drm_bridge *bridge, @@ -228,14 +221,7 @@ static void vs_bridge_atomic_disable(struct drm_bridge *bridge, struct vs_dc *dc = crtc->dc; unsigned int output = crtc->id; - regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START, - VSDC_DISP_PANEL_START_MULTI_DISP_SYNC | - VSDC_DISP_PANEL_START_RUNNING(output)); - regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output), - VSDC_DISP_PANEL_CONFIG_RUNNING); - - regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id), - VSDC_DISP_PANEL_CONFIG_EX_COMMIT); + dc->funcs->panel_disable_ex(dc, output); } static const struct drm_bridge_funcs vs_dpi_bridge_funcs = { diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c index 0b8a35d09cd2..1c4aac708669 100644 --- a/drivers/gpu/drm/verisilicon/vs_crtc.c +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c @@ -16,10 +16,33 @@ #include "vs_crtc_regs.h" #include "vs_crtc.h" #include "vs_dc.h" -#include "vs_dc_top_regs.h" #include "vs_drm.h" #include "vs_plane.h" +static void vs_crtc_atomic_begin(struct drm_crtc *crtc, + struct drm_atomic_commit *state) +{ + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); + struct vs_dc *dc = vcrtc->dc; + unsigned int output = vcrtc->id; + + if (dc->funcs->crtc_begin) + dc->funcs->crtc_begin(dc, output); +} + +static void vs_crtc_atomic_flush(struct drm_crtc *crtc, + struct drm_atomic_commit *state) +{ + struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); + struct vs_dc *dc = vcrtc->dc; + unsigned int output = vcrtc->id; + + if (dc->funcs->crtc_flush) + dc->funcs->crtc_flush(dc, output); + + drm_crtc_vblank_atomic_flush(crtc, state); +} + static void vs_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state) { @@ -30,6 +53,9 @@ static void vs_crtc_atomic_disable(struct drm_crtc *crtc, drm_crtc_vblank_off(crtc); clk_disable_unprepare(dc->pix_clk[output]); + + if (dc->funcs->crtc_disable_ex) + dc->funcs->crtc_disable_ex(dc, output); } static void vs_crtc_atomic_enable(struct drm_crtc *crtc, @@ -42,6 +68,9 @@ static void vs_crtc_atomic_enable(struct drm_crtc *crtc, drm_WARN_ON(&dc->drm_dev->base, clk_prepare_enable(dc->pix_clk[output])); + if (dc->funcs->crtc_enable_ex) + dc->funcs->crtc_enable_ex(dc, output); + drm_crtc_vblank_on(crtc); } @@ -119,7 +148,8 @@ static bool vs_crtc_mode_fixup(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vs_crtc_helper_funcs = { - .atomic_flush = drm_crtc_vblank_atomic_flush, + .atomic_begin = vs_crtc_atomic_begin, + .atomic_flush = vs_crtc_atomic_flush, .atomic_enable = vs_crtc_atomic_enable, .atomic_disable = vs_crtc_atomic_disable, .mode_set_nofb = vs_crtc_mode_set_nofb, @@ -132,7 +162,7 @@ static int vs_crtc_enable_vblank(struct drm_crtc *crtc) struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); struct vs_dc *dc = vcrtc->dc; - regmap_set_bits(dc->regs, VSDC_TOP_IRQ_EN, VSDC_TOP_IRQ_VSYNC(vcrtc->id)); + dc->funcs->enable_vblank(dc, vcrtc->id); return 0; } @@ -142,7 +172,7 @@ static void vs_crtc_disable_vblank(struct drm_crtc *crtc) struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc); struct vs_dc *dc = vcrtc->dc; - regmap_clear_bits(dc->regs, VSDC_TOP_IRQ_EN, VSDC_TOP_IRQ_VSYNC(vcrtc->id)); + dc->funcs->disable_vblank(dc, vcrtc->id); } static const struct drm_crtc_funcs vs_crtc_funcs = { diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c index dad9967bc10b..9729b693d360 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_ack(dc); 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..825f5dd6bf17 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 (*panel_enable_ex)(struct vs_dc *dc, unsigned int output); + void (*panel_disable_ex)(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_ex)(struct vs_dc *dc, unsigned int output); + void (*crtc_disable_ex)(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 (*primary_plane_enable_ex)(struct vs_dc *dc, unsigned int output); + void (*primary_plane_disable_ex)(struct vs_dc *dc, unsigned int output); + void (*primary_plane_update_ex)(struct vs_dc *dc, unsigned int output, + struct drm_plane_state *state); + + /* IRQ acknowledge */ + u32 (*irq_ack)(struct vs_dc *dc); +}; struct vs_dc { struct regmap *regs; @@ -33,6 +62,9 @@ struct vs_dc { struct vs_drm_dev *drm_dev; struct vs_chip_identity identity; + const struct vs_dc_funcs *funcs; }; +extern const struct vs_dc_funcs vs_dc8200_funcs; + #endif /* _VS_DC_H_ */ diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c b/drivers/gpu/drm/verisilicon/vs_dc8200.c new file mode 100644 index 000000000000..17378f4ef96d --- /dev/null +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2025 Icenowy Zheng <uwu@icenowy.me> + */ + +#include <linux/regmap.h> + +#include "vs_bridge_regs.h" +#include "vs_dc.h" +#include "vs_dc_top_regs.h" +#include "vs_drm.h" +#include "vs_plane.h" +#include "vs_primary_plane_regs.h" + +static void vs_dc8200_panel_enable_ex(struct vs_dc *dc, unsigned int output) +{ + 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(output), + VSDC_DISP_PANEL_CONFIG_EX_COMMIT); +} + +static void vs_dc8200_panel_disable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_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 | + VSDC_DISP_PANEL_START_RUNNING(output)); + + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(output), + VSDC_DISP_PANEL_CONFIG_EX_COMMIT); +} + +static void vs_dc8200_enable_vblank(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_TOP_IRQ_EN, + VSDC_TOP_IRQ_VSYNC(output)); +} + +static void vs_dc8200_disable_vblank(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_bits(dc->regs, VSDC_TOP_IRQ_EN, + VSDC_TOP_IRQ_VSYNC(output)); +} + +static void vs_dc8200_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_dc8200_primary_plane_enable_ex(struct vs_dc *dc, unsigned int output) +{ + 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_dc8200_plane_commit(dc, output); +} + +static void vs_dc8200_primary_plane_disable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), + VSDC_FB_CONFIG_EX_FB_EN); + + vs_dc8200_plane_commit(dc, output); +} + +static void vs_dc8200_primary_plane_update_ex(struct vs_dc *dc, unsigned int output, + struct drm_plane_state *state) +{ + 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_BLEND_CONFIG(output), + VSDC_FB_BLEND_CONFIG_BLEND_DISABLE); + + vs_dc8200_plane_commit(dc, output); +} + +static u32 vs_dc8200_irq_ack(struct vs_dc *dc) +{ + u32 hw_irqs, unified = 0; + unsigned int i; + + regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &hw_irqs); + + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { + if (hw_irqs & VSDC_TOP_IRQ_VSYNC(i)) + unified |= VSDC_IRQ_VSYNC(i); + } + + return unified; +} + +const struct vs_dc_funcs vs_dc8200_funcs = { + .panel_enable_ex = vs_dc8200_panel_enable_ex, + .panel_disable_ex = vs_dc8200_panel_disable_ex, + .enable_vblank = vs_dc8200_enable_vblank, + .disable_vblank = vs_dc8200_disable_vblank, + .primary_plane_enable_ex = vs_dc8200_primary_plane_enable_ex, + .primary_plane_disable_ex = vs_dc8200_primary_plane_disable_ex, + .primary_plane_update_ex = vs_dc8200_primary_plane_update_ex, + .irq_ack = vs_dc8200_irq_ack, +}; diff --git a/drivers/gpu/drm/verisilicon/vs_drm.c b/drivers/gpu/drm/verisilicon/vs_drm.c index fd259d53f49f..24e9d0b008f3 100644 --- a/drivers/gpu/drm/verisilicon/vs_drm.c +++ b/drivers/gpu/drm/verisilicon/vs_drm.c @@ -25,7 +25,6 @@ #include "vs_bridge.h" #include "vs_crtc.h" #include "vs_dc.h" -#include "vs_dc_top_regs.h" #include "vs_drm.h" #define DRIVER_NAME "verisilicon" @@ -168,8 +167,8 @@ void vs_drm_handle_irq(struct vs_dc *dc, u32 irqs) unsigned int i; for (i = 0; i < dc->identity.display_count; i++) { - if (irqs & VSDC_TOP_IRQ_VSYNC(i)) { - irqs &= ~VSDC_TOP_IRQ_VSYNC(i); + if (irqs & VSDC_IRQ_VSYNC(i)) { + irqs &= ~VSDC_IRQ_VSYNC(i); if (dc->drm_dev->crtcs[i]) drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base); } diff --git a/drivers/gpu/drm/verisilicon/vs_drm.h b/drivers/gpu/drm/verisilicon/vs_drm.h index 606338206a42..6a89c20879df 100644 --- a/drivers/gpu/drm/verisilicon/vs_drm.h +++ b/drivers/gpu/drm/verisilicon/vs_drm.h @@ -6,6 +6,7 @@ #ifndef _VS_DRM_H_ #define _VS_DRM_H_ +#include <linux/bits.h> #include <linux/platform_device.h> #include <linux/types.h> @@ -13,6 +14,13 @@ struct vs_dc; +/* + * DC variants use different interrupt registers with diverging bit + * assignments; each irq_ack() implementation must translate its + * hardware-specific bits into these definitions. + */ +#define VSDC_IRQ_VSYNC(n) BIT(n) + struct vs_drm_dev { struct drm_device base; 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..f992cb277f61 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->primary_plane_enable_ex) + dc->funcs->primary_plane_enable_ex(dc, output); } 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->primary_plane_disable_ex) + dc->funcs->primary_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->primary_plane_update_ex) + dc->funcs->primary_plane_update_ex(dc, output, state); } static const struct drm_plane_helper_funcs vs_primary_plane_helper_funcs = { -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table 2026-06-25 9:44 ` [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table Joey Lu @ 2026-06-25 10:00 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-06-25 10:00 UTC (permalink / raw) To: Joey Lu; +Cc: conor+dt, devicetree, dri-devel, robh Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: New issues: - [High] CRTC disable hardware operation is called after disabling the pixel clock. - [High] The register clearing sequence in `vs_dc8200_panel_disable_ex` is inverted compared to the original code, clearing the configuration bit before stopping the panel. - [Medium] Unknown hardware interrupts are silently filtered out by the irq_ack abstraction, rendering the unknown interrupt diagnostic warning unreachable. Pre-existing issues: - [High] Primary plane disable function incorrectly sets the enable bit rather than clearing it. - [High] The driver requests its IRQ line before it fully initializes its internal state, creating a race condition that can lead to a NULL pointer dereference. - [High] `vs_dc_irq_handler` unconditionally returns `IRQ_HANDLED` regardless of whether any interrupt bits were set. -- commit d0e2c6765378754f6b5239526ba6f62ca9de0dd3 Author: Joey Lu <a0987203069@gmail.com> drm/verisilicon: introduce per-variant hardware ops table The DC8200 and DCUltraLite share a broadly similar register layout but differ in how the bridge, CRTC, primary plane and IRQ paths are driven. This commit introduces a vs_dc_funcs vtable so each variant can supply its own implementation without scattering conditionals across multiple files. > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c > index 0b8a35d09cd2e..1c4aac7086694 100644 > --- a/drivers/gpu/drm/verisilicon/vs_crtc.c > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c [ ... ] > @@ -30,6 +53,9 @@ static void vs_crtc_atomic_disable(struct drm_crtc *crtc, > drm_crtc_vblank_off(crtc); > > clk_disable_unprepare(dc->pix_clk[output]); > + > + if (dc->funcs->crtc_disable_ex) > + dc->funcs->crtc_disable_ex(dc, output); > } [Severity: High] Is it safe to access hardware registers via crtc_disable_ex() after the pixel clock has been gated? The teardown order should typically reverse the initialization order, meaning the hook must be invoked before gating the clock. [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c > index dad9967bc10b8..9729b693d360e 100644 > --- a/drivers/gpu/drm/verisilicon/vs_dc.c > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c > @@ -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_ack(dc); > > vs_drm_handle_irq(dc, irqs); > > return IRQ_HANDLED; [Severity: High] This isn't a bug introduced by this patch, but does unconditionally returning IRQ_HANDLED prevent the kernel from detecting and disabling a stuck shared interrupt line? If the handler processes no interrupts, shouldn't it return IRQ_NONE? [Severity: High] This is another pre-existing issue, but in vs_dc_probe(), the IRQ is requested before vs_drm_initialize() is called. Can the IRQ handler fire before dc->drm_dev is allocated, resulting in a NULL pointer dereference when vs_drm_handle_irq() executes? [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c b/drivers/gpu/drm/verisilicon/vs_dc8200.c > new file mode 100644 > index 0000000000000..17378f4ef96d1 > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c [ ... ] > +static void vs_dc8200_panel_disable_ex(struct vs_dc *dc, unsigned int output) > +{ > + regmap_clear_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 | > + VSDC_DISP_PANEL_START_RUNNING(output)); [Severity: High] Does reversing the clear order of the configuration and start registers risk hardware underflows or state desynchronization? In the original vs_bridge_atomic_disable() implementation, the panel was shut down by first clearing VSDC_DISP_PANEL_START_RUNNING and then clearing VSDC_DISP_PANEL_CONFIG_RUNNING. [ ... ] > +static void vs_dc8200_primary_plane_disable_ex(struct vs_dc *dc, unsigned int output) > +{ > + regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > + VSDC_FB_CONFIG_EX_FB_EN); > + > + vs_dc8200_plane_commit(dc, output); > +} [Severity: High] This is a pre-existing issue carried over from the original code, but does the primary plane disable sequence leave the plane enabled by using regmap_set_bits() instead of clearing the VSDC_FB_CONFIG_EX_FB_EN bit? [ ... ] > +static u32 vs_dc8200_irq_ack(struct vs_dc *dc) > +{ > + u32 hw_irqs, unified = 0; > + unsigned int i; > + > + regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &hw_irqs); > + > + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { > + if (hw_irqs & VSDC_TOP_IRQ_VSYNC(i)) > + unified |= VSDC_IRQ_VSYNC(i); > + } > + > + return unified; > +} [Severity: Medium] Does this filter out unknown hardware interrupts, making the diagnostic warning in vs_drm_handle_irq() unreachable? By only passing through known VSDC_IRQ_VSYNC bits, any unmapped hardware interrupt bits are silently discarded. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260625094449.708386-1-a0987203069@gmail.com?part=3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu ` (2 preceding siblings ...) 2026-06-25 9:44 ` [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 10:01 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support Joey Lu ` (2 subsequent siblings) 6 siblings, 1 reply; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu The Nuvoton MA35D1 SoC integrates a DCUltraLite display controller whose AXI and AHB bus clocks share a single gate enable bit with the display core clock, so the clock driver does not expose them separately. This patch makes the axi and ahb clocks optional in the probe. Signed-off-by: Joey Lu <a0987203069@gmail.com> --- drivers/gpu/drm/verisilicon/vs_dc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c index 9729b693d360..fd1f5fe67a68 100644 --- a/drivers/gpu/drm/verisilicon/vs_dc.c +++ b/drivers/gpu/drm/verisilicon/vs_dc.c @@ -90,13 +90,13 @@ static int vs_dc_probe(struct platform_device *pdev) return PTR_ERR(dc->core_clk); } - dc->axi_clk = devm_clk_get_enabled(dev, "axi"); + dc->axi_clk = devm_clk_get_optional_enabled(dev, "axi"); if (IS_ERR(dc->axi_clk)) { dev_err(dev, "can't get axi clock\n"); return PTR_ERR(dc->axi_clk); } - dc->ahb_clk = devm_clk_get_enabled(dev, "ahb"); + dc->ahb_clk = devm_clk_get_optional_enabled(dev, "ahb"); if (IS_ERR(dc->ahb_clk)) { dev_err(dev, "can't get ahb clock\n"); return PTR_ERR(dc->ahb_clk); -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional 2026-06-25 9:44 ` [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional Joey Lu @ 2026-06-25 10:01 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-06-25 10:01 UTC (permalink / raw) To: Joey Lu; +Cc: devicetree, conor+dt, dri-devel, robh Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Possible NULL pointer dereference in the IRQ handler if an interrupt triggers before DRM initialization completes. -- commit 2cd6077ab2fb5ed065ee2cef55004b713c80f827 Author: Joey Lu <a0987203069@gmail.com> drm/verisilicon: make axi and ahb clocks optional The Nuvoton MA35D1 SoC integrates a DCUltraLite display controller whose AXI and AHB bus clocks share a single gate enable bit with the display core clock, so the clock driver does not expose them separately. This patch makes the axi and ahb clocks optional in the probe. > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c > index 9729b693d360e..fd1f5fe67a68b 100644 > --- a/drivers/gpu/drm/verisilicon/vs_dc.c > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c [ ... ] > @@ -90,13 +90,13 @@ static int vs_dc_probe(struct platform_device *pdev) > return PTR_ERR(dc->core_clk); > } > > - dc->axi_clk = devm_clk_get_enabled(dev, "axi"); > + dc->axi_clk = devm_clk_get_optional_enabled(dev, "axi"); > if (IS_ERR(dc->axi_clk)) { [Severity: High] This is a pre-existing issue, but is it possible for an interrupt to trigger a NULL pointer dereference during probe if the bootloader left the display active? Looking further down in vs_dc_probe(), the IRQ handler is registered before vs_drm_initialize() is called: ret = devm_request_irq(dev, irq, vs_dc_irq_handler, 0, dev_name(dev), dc); if (ret) { dev_err(dev, "can't request irq\n"); goto err_rst_assert; } dev_set_drvdata(dev, dc); ret = vs_drm_initialize(dc, pdev); If a pending interrupt (such as VSYNC) fires immediately after devm_request_irq() is called, vs_dc_irq_handler() will be triggered and will eventually call vs_drm_handle_irq(). Looking at vs_drm_handle_irq() in vs_drm.c, it unconditionally dereferences the drm_dev pointer: if (dc->drm_dev->crtcs[i]) drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base); Since vs_drm_initialize() hasn't allocated or assigned dc->drm_dev yet, could this cause a kernel panic during boot on platforms where the bootloader leaves interrupts enabled? Would it be safer to request the IRQ after the DRM device is fully initialized, or to mask interrupts in the hardware before registering the handler? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260625094449.708386-1-a0987203069@gmail.com?part=4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu ` (3 preceding siblings ...) 2026-06-25 9:44 ` [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 10:10 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB Joey Lu 2026-06-25 9:44 ` [PATCH v5 7/7] drm/verisilicon: extend Kconfig to support ARCH_MA35 platforms Joey Lu 6 siblings, 1 reply; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu The Nuvoton MA35D1 SoC integrates a Verisilicon DCUltraLite display controller (DC8000 generation) whose register layout differs from the DC8200 in several important ways: 1. No CONFIG_EX commit path: framebuffer updates use the enable (bit 0) and reset (bit 4) bits in FB_CONFIG instead of the DC8200 staging registers (FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT, FB_BLEND_CONFIG, PANEL_CONFIG_EX). 2. No PANEL_START register: panel output starts when PANEL_CONFIG.RUNNING is set; there is no multi-display sync start register. 3. Different IRQ registers: DCUltraLite uses DISP_IRQ_STA (0x147C) / DISP_IRQ_EN (0x1480) versus DC8200's TOP_IRQ_ACK (0x0010) / TOP_IRQ_EN (0x0014). 4. Simpler clock topology: only 'core' (bus gate) and 'pix0' (pixel divider) clocks; no axi or ahb clocks required. Signed-off-by: Joey Lu <a0987203069@gmail.com> --- drivers/gpu/drm/verisilicon/Makefile | 2 +- drivers/gpu/drm/verisilicon/vs_dc.c | 5 +- drivers/gpu/drm/verisilicon/vs_dc.h | 1 + drivers/gpu/drm/verisilicon/vs_dc8000.c | 86 +++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8000.c diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile index 9d4cd16452fa..d2fd8e4dff24 100644 --- a/drivers/gpu/drm/verisilicon/Makefile +++ b/drivers/gpu/drm/verisilicon/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only -verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_drm.o vs_hwdb.o \ +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_dc8000.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_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c index fd1f5fe67a68..9499fffbca58 100644 --- a/drivers/gpu/drm/verisilicon/vs_dc.c +++ b/drivers/gpu/drm/verisilicon/vs_dc.c @@ -134,7 +134,10 @@ 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 (dc->identity.generation == VSDC_GEN_DC8200) + dc->funcs = &vs_dc8200_funcs; + else + dc->funcs = &vs_dc8000_funcs; if (port_count > dc->identity.display_count) { dev_err(dev, "too many downstream ports than HW capability\n"); diff --git a/drivers/gpu/drm/verisilicon/vs_dc.h b/drivers/gpu/drm/verisilicon/vs_dc.h index 825f5dd6bf17..ac96ad701199 100644 --- a/drivers/gpu/drm/verisilicon/vs_dc.h +++ b/drivers/gpu/drm/verisilicon/vs_dc.h @@ -66,5 +66,6 @@ struct vs_dc { }; extern const struct vs_dc_funcs vs_dc8200_funcs; +extern const struct vs_dc_funcs vs_dc8000_funcs; #endif /* _VS_DC_H_ */ diff --git a/drivers/gpu/drm/verisilicon/vs_dc8000.c b/drivers/gpu/drm/verisilicon/vs_dc8000.c new file mode 100644 index 000000000000..fbe0fa516cac --- /dev/null +++ b/drivers/gpu/drm/verisilicon/vs_dc8000.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2026 Joey Lu <yclu4@nuvoton.com> + */ + +#include <linux/regmap.h> + +#include "vs_crtc_regs.h" +#include "vs_dc.h" +#include "vs_drm.h" +#include "vs_primary_plane_regs.h" + +static void vs_dc8000_panel_enable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_RESET); +} + +static void vs_dc8000_panel_disable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_RESET); +} + +static void vs_dc8000_crtc_begin(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_VALID); +} + +static void vs_dc8000_crtc_flush(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_VALID); +} + +static void vs_dc8000_crtc_enable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_ENABLE); +} + +static void vs_dc8000_crtc_disable_ex(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_ENABLE); +} + +static void vs_dc8000_enable_vblank(struct vs_dc *dc, unsigned int output) +{ + regmap_set_bits(dc->regs, VSDC_DISP_IRQ_EN, + VSDC_DISP_IRQ_VSYNC(output)); +} + +static void vs_dc8000_disable_vblank(struct vs_dc *dc, unsigned int output) +{ + regmap_clear_bits(dc->regs, VSDC_DISP_IRQ_EN, + VSDC_DISP_IRQ_VSYNC(output)); +} + +static u32 vs_dc8000_irq_ack(struct vs_dc *dc) +{ + u32 hw_irqs, unified = 0; + unsigned int i; + + regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &hw_irqs); + + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { + if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i)) + unified |= VSDC_IRQ_VSYNC(i); + } + + return unified; +} + +const struct vs_dc_funcs vs_dc8000_funcs = { + .panel_enable_ex = vs_dc8000_panel_enable_ex, + .panel_disable_ex = vs_dc8000_panel_disable_ex, + .crtc_begin = vs_dc8000_crtc_begin, + .crtc_flush = vs_dc8000_crtc_flush, + .crtc_enable_ex = vs_dc8000_crtc_enable_ex, + .crtc_disable_ex = vs_dc8000_crtc_disable_ex, + .enable_vblank = vs_dc8000_enable_vblank, + .disable_vblank = vs_dc8000_disable_vblank, + .irq_ack = vs_dc8000_irq_ack, +}; -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support 2026-06-25 9:44 ` [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support Joey Lu @ 2026-06-25 10:10 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-06-25 10:10 UTC (permalink / raw) To: Joey Lu; +Cc: conor+dt, devicetree, robh, dri-devel Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [High] The DC8000 panel enable, CRTC begin, and CRTC flush operations use incorrect registers and bits, directly contradicting the commit message and breaking hardware functionality. - [Medium] The hw_irqs variable is passed to regmap_read() uninitialized and used without checking for read failures. Pre-existing issues: - [High] The DC8200 primary plane disable function erroneously sets the enable bit instead of clearing it, keeping the plane active. - [High] The IRQ handler unconditionally dereferences dc->drm_dev, but the interrupt is enabled before dc->drm_dev is allocated and remains active after it is freed. -- commit a518460571afb18a0b4ff8e14af894a66907ebde Author: Joey Lu <a0987203069@gmail.com> drm/verisilicon: add DC8000 (DCUltraLite) display controller support The Nuvoton MA35D1 SoC integrates a Verisilicon DCUltraLite display controller (DC8000 generation) whose register layout differs from the DC8200 in several important ways: > 1. No CONFIG_EX commit path: framebuffer updates use the enable (bit 0) > and reset (bit 4) bits in FB_CONFIG instead of the DC8200 staging > registers (FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT, > FB_BLEND_CONFIG, PANEL_CONFIG_EX). > > 2. No PANEL_START register: panel output starts when > PANEL_CONFIG.RUNNING is set; there is no multi-display sync start > register. [Severity: High] Do the functions implemented in this patch use the correct registers and bits as described in the commit message? In vs_dc8000_panel_enable_ex(), the code sets a reset bit (VSDC_FB_CONFIG_RESET) in VSDC_FB_CONFIG instead of writing to PANEL_CONFIG. Additionally, vs_dc8000_crtc_begin() and vs_dc8000_crtc_flush() use an unmentioned VALID bit (VSDC_FB_CONFIG_VALID), and clear it after updates complete, which seems to invert normal latch semantics. Could this mismatch prevent the display panel from enabling properly? > 3. Different IRQ registers: DCUltraLite uses DISP_IRQ_STA (0x147C) / > DISP_IRQ_EN (0x1480) versus DC8200's TOP_IRQ_ACK (0x0010) / > TOP_IRQ_EN (0x0014). > > 4. Simpler clock topology: only 'core' (bus gate) and 'pix0' (pixel > divider) clocks; no axi or ahb clocks required. > > Signed-off-by: Joey Lu <a0987203069@gmail.com> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile > index 9d4cd16452fa1..d2fd8e4dff242 100644 > --- a/drivers/gpu/drm/verisilicon/Makefile > +++ b/drivers/gpu/drm/verisilicon/Makefile > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > > -verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_drm.o vs_hwdb.o \ > +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o vs_dc8000.o vs_drm.o vs_hwdb.o \ [Severity: High] This is a pre-existing issue, but while reviewing the driver files included here, I noticed a potential problem in vs_dc8200.c:vs_dc8200_primary_plane_disable_ex(). The function appears to call regmap_set_bits() on VSDC_FB_CONFIG_EX_FB_EN instead of regmap_clear_bits(): vs_dc8200.c:vs_dc8200_primary_plane_disable_ex() { ... regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), VSDC_FB_CONFIG_EX_FB_EN); ... } Does this erroneously keep the primary plane active when the DRM framework expects it to be turned off, potentially breaking power management? > 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_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c > index fd1f5fe67a68b..9499fffbca58e 100644 > --- a/drivers/gpu/drm/verisilicon/vs_dc.c > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c > @@ -134,7 +134,10 @@ 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 (dc->identity.generation == VSDC_GEN_DC8200) > + dc->funcs = &vs_dc8200_funcs; > + else > + dc->funcs = &vs_dc8000_funcs; [Severity: High] This is a pre-existing issue, but I noticed a potential lifecycle regression in vs_dc_probe() just below this block. The function calls devm_request_irq() to register vs_dc_irq_handler() before calling vs_drm_initialize(), which is responsible for allocating and assigning dc->drm_dev. If an interrupt fires during this window, for example if the bootloader left VSYNC interrupts enabled, the handler is invoked: vs_drm.c:vs_drm_handle_irq() { ... if (dc->drm_dev->crtcs[i]) drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base); ... } Will this unconditionally dereference dc->drm_dev and cause a NULL pointer dereference before vs_drm_initialize() has completed? > > if (port_count > dc->identity.display_count) { > dev_err(dev, "too many downstream ports than HW capability\n"); [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8000.c b/drivers/gpu/drm/verisilicon/vs_dc8000.c > new file mode 100644 > index 0000000000000..fbe0fa516cac9 > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8000.c > @@ -0,0 +1,86 @@ [ ... ] > +static u32 vs_dc8000_irq_ack(struct vs_dc *dc) > +{ > + u32 hw_irqs, unified = 0; > + unsigned int i; > + > + regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &hw_irqs); [Severity: Medium] Is it safe to use hw_irqs here if regmap_read() fails? If regmap_read() returns an error code, hw_irqs will remain uninitialized stack memory. The function then proceeds to bitwise-AND this uninitialized value to determine which VSYNC interrupts fired: for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i)) unified |= VSDC_IRQ_VSYNC(i); } Could this result in spurious VSYNC events being dispatched to the DRM core if a read failure occurs? > + > + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { > + if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i)) > + unified |= VSDC_IRQ_VSYNC(i); > + } > + > + return unified; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/20260625094449.708386-1-a0987203069@gmail.com?part=5 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu ` (4 preceding siblings ...) 2026-06-25 9:44 ` [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support Joey Lu @ 2026-06-25 9:44 ` Joey Lu 2026-06-25 10:22 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 7/7] drm/verisilicon: extend Kconfig to support ARCH_MA35 platforms Joey Lu 6 siblings, 1 reply; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu The Nuvoton MA35D1 chip contains a DCUltraLite display controller with model number 0x0 (sic, the model name contains no number either), revision 0x5560 and customer ID 0x305. It has a similar register map with DC8000, only one display output and only 32x32 cursor supported. Signed-off-by: Joey Lu <a0987203069@gmail.com> drivers/gpu/drm/verisilicon/vs_hwdb.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c index 91524d16f778..7d630a667a3f 100644 --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c @@ -129,6 +129,16 @@ static struct vs_chip_identity vs_chip_identities[] = { .max_cursor_size = 64, .formats = &vs_formats_no_yuv444, }, + { + .model = 0x0, /* DCUltraLite */ + .revision = 0x5560, + .customer_id = 0x305, + + .generation = VSDC_GEN_DC8000, + .display_count = 1, + .max_cursor_size = 32, + .formats = &vs_formats_no_yuv444, + }, }; int vs_fill_chip_identity(struct regmap *regs, -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB 2026-06-25 9:44 ` [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB Joey Lu @ 2026-06-25 10:22 ` sashiko-bot 0 siblings, 0 replies; 13+ messages in thread From: sashiko-bot @ 2026-06-25 10:22 UTC (permalink / raw) To: Joey Lu; +Cc: conor+dt, robh, devicetree, dri-devel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The newly added DCUltraLite chip restricts the max cursor size to 32x32, but the driver does not configure `drm->mode_config.cursor_width` or `cursor_height`. DRM core defaults to 64x64 when these fields are 0. This incorrectly exposes a `DRM_CAP_CURSOR_WIDTH` of 64 to userspace, causing atomic commits with 64x64 hardware cursors to be rejected with `-EINVAL`, breaking display compositors. -- commit a3c0d8a714c977c54ec25009535f44e9ded7c881 Author: Joey Lu <a0987203069@gmail.com> drm/verisilicon: add DCUltraLite chip identity to HWDB The Nuvoton MA35D1 chip contains a DCUltraLite display controller with model number 0x0 (sic, the model name contains no number either), revision 0x5560 and customer ID 0x305. It has a similar register map with DC8000, only one display output and only 32x32 cursor supported. > diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c > index 91524d16f7785..7d630a667a3f0 100644 > --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c > +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c > @@ -129,6 +129,16 @@ static struct vs_chip_identity vs_chip_identities[] = { > .max_cursor_size = 64, > .formats = &vs_formats_no_yuv444, > }, > + { > + .model = 0x0, /* DCUltraLite */ > + .revision = 0x5560, > + .customer_id = 0x305, > + > + .generation = VSDC_GEN_DC8000, > + .display_count = 1, > + .max_cursor_size = 32, [Severity: High] Does this code expose an incorrect DRM_CAP_CURSOR_WIDTH to userspace? Looking at vs_mode_config_init() in drivers/gpu/drm/verisilicon/vs_drm.c, the driver does not seem to configure drm->mode_config.cursor_width or cursor_height: drivers/gpu/drm/verisilicon/vs_drm.c:vs_mode_config_init() { ... drm->mode_config.helper_private = &vs_mode_config_helper_funcs; } When these fields are 0, the DRM core defaults to 64x64. This means userspace compositors might query the capability, receive 64x64, and attempt to commit a 64x64 hardware cursor. If this new chip restricts the max cursor size to 32x32, will the driver's atomic check reject the 64x64 state and break display compositors? > + .formats = &vs_formats_no_yuv444, > + }, > }; -- Sashiko AI review · https://sashiko.dev/#/patchset/20260625094449.708386-1-a0987203069@gmail.com?part=6 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 7/7] drm/verisilicon: extend Kconfig to support ARCH_MA35 platforms 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu ` (5 preceding siblings ...) 2026-06-25 9:44 ` [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB Joey Lu @ 2026-06-25 9:44 ` Joey Lu 6 siblings, 0 replies; 13+ messages in thread From: Joey Lu @ 2026-06-25 9:44 UTC (permalink / raw) To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied, simona, robh, krzk+dt, conor+dt Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel, linux-kernel, Joey Lu Add ARCH_MA35 to the platform dependencies to allow the driver to be built for Nuvoton MA35D1. Signed-off-by: Joey Lu <a0987203069@gmail.com> Reviewed-by: Icenowy Zheng <zhengxingda@iscas.ac.cn> --- drivers/gpu/drm/verisilicon/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/verisilicon/Kconfig b/drivers/gpu/drm/verisilicon/Kconfig index 7cce86ec8603..295d246eb4b4 100644 --- a/drivers/gpu/drm/verisilicon/Kconfig +++ b/drivers/gpu/drm/verisilicon/Kconfig @@ -2,7 +2,7 @@ config DRM_VERISILICON_DC tristate "DRM Support for Verisilicon DC-series display controllers" depends on DRM && COMMON_CLK - depends on RISCV || COMPILE_TEST + depends on RISCV || ARCH_MA35 || COMPILE_TEST select DRM_BRIDGE_CONNECTOR select DRM_CLIENT_SELECTION select DRM_DISPLAY_HELPER -- 2.43.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-06-25 10:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-06-25 9:44 [PATCH v5 0/7] drm/verisilicon: add Nuvoton MA35D1 DCU Lite support Joey Lu 2026-06-25 9:44 ` [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants Joey Lu 2026-06-25 9:54 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 2/7] drm/verisilicon: add register-level macros for DC8000 Joey Lu 2026-06-25 9:44 ` [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table Joey Lu 2026-06-25 10:00 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional Joey Lu 2026-06-25 10:01 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support Joey Lu 2026-06-25 10:10 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 6/7] drm/verisilicon: add DCUltraLite chip identity to HWDB Joey Lu 2026-06-25 10:22 ` sashiko-bot 2026-06-25 9:44 ` [PATCH v5 7/7] drm/verisilicon: extend Kconfig to support ARCH_MA35 platforms Joey Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox