Re: [PATCH v5 5/7] drm/verisilicon: add DC8000 (DCUltraLite) display controller support

[sashiko-bot@kernel.org]

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