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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/verisilicon: missing IRQ acknowledgment causes interrupt storm
- [High] drm/verisilicon: wrong register configured in vs_dc8000_panel_enable_ex
- [High] drm/verisilicon: reversed VALID bit logic causes incomplete state latching
--

commit 091826cd38f41f7f76fdaab38a3712b19c6f3f16
Author: Joey Lu <a0987203069@gmail.com>

drm/verisilicon: add DC8000 (DCUltraLite) display controller support

This patch adds support for the DC8000 display controller to the
verisilicon DRM driver. It introduces a new vs_dc_funcs implementation
for the DC8000 generation.

> diff --git a/drivers/gpu/drm/verisilicon/vs_dc8000.c b/drivers/gpu/drm/verisilicon/vs_dc8000.c
> new file mode 100644
> index 0000000000000..df7bd5bdd7f2a
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_dc8000.c
> @@ -0,0 +1,92 @@

[ ... ]

> +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);
> +}

[Severity: High]
The commit message mentions that panel output starts as soon as
PANEL_CONFIG.RUNNING is set. However, does this code configure the wrong
register?

It sets and clears the VSDC_FB_CONFIG_RESET bit in the framebuffer
configuration register instead of the panel configuration register, which might
leave the panel hardware unstarted and the framebuffer held in 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);
> +}

[Severity: High]
Is the VALID bit logic reversed here? 

Setting VSDC_FB_CONFIG_VALID in vs_dc8000_crtc_begin() asserts to hardware
that the configuration is ready before new plane updates are written, and
clearing it in vs_dc8000_crtc_flush() marks the completed configuration as
invalid. 

Can this reversed signaling cause the hardware to latch an incomplete state
during atomic updates?

[ ... ]

> +static u32 vs_dc8000_irq_ack(struct vs_dc *dc)
> +{
> +	u32 hw_irqs, unified = 0, known = 0;
> +	unsigned int i;
> +
> +	regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &hw_irqs);
> +
> +	for (i = 0; i < VSDC_MAX_OUTPUTS; i++) {
> +		known |= VSDC_DISP_IRQ_VSYNC(i);
> +		if (hw_irqs & VSDC_DISP_IRQ_VSYNC(i))
> +			unified |= VSDC_IRQ_VSYNC(i);
> +	}
> +
> +	drm_WARN_ONCE(&dc->drm_dev->base, hw_irqs & ~known,
> +		      "Unknown hardware IRQ bits: %#x\n", hw_irqs & ~known);
> +
> +	return unified;
> +}

[Severity: High]
Does this function fail to clear the pending hardware interrupts? 

It reads VSDC_DISP_IRQ_STA to check pending interrupts but does not write back
to clear them. For hardware without a dedicated ACK register, the status
register usually acts as a write-1-to-clear mechanism. 

Can failing to write back leave the interrupt asserted in the hardware and
cause an infinite interrupt storm when the VBLANK interrupt fires?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918030125.315978-1-a0987203069@gmail.com?part=4