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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The `vs_dc8000_panel_enable_ex` and `vs_dc8000_panel_disable_ex` functions incorrectly modify the `VSDC_FB_CONFIG` register using the `VSDC_FB_CONFIG_RESET` bit, instead of setting the `PANEL_CONFIG.RUNNING` bit.
- [High] The `vs_dc8000_crtc_begin` and `vs_dc8000_crtc_flush` functions have inverted logic for the `VSDC_FB_CONFIG_VALID` bit.
- [Medium] The `vs_dc8000_irq_ack` function reads `VSDC_DISP_IRQ_STA` but fails to perform any hardware write to clear the pending hardware interrupts.

Pre-existing issues:
- [High] Early interrupt registration in `vs_dc_probe` causes a NULL pointer dereference if an interrupt fires before DRM initialization is complete.
--

commit 71018d22eb0156b0806e61d4495897f9c4f2d35b
Author: Joey Lu [off-list ref]

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

This commit adds support for the DC8000 (DCUltraLite) display controller
generation to the Verisilicon DRM driver. It introduces a new vs_dc_funcs
implementation that directly latches framebuffer updates through FB_CONFIG,
handles panel output, and acknowledges VSYNC interrupts.

[ ... ]

+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 states that "panel output starts as soon as
PANEL_CONFIG.RUNNING is set", but vs_dc8000_panel_enable_ex() and
vs_dc8000_panel_disable_ex() modify VSDC_FB_CONFIG with the
VSDC_FB_CONFIG_RESET bit instead of setting the PANEL_CONFIG.RUNNING bit.
Will this cause the panel output to never start and inadvertently hold the
framebuffer 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]
Does this invert the logic for the VSDC_FB_CONFIG_VALID bit? By asserting
validity in vs_dc8000_crtc_begin() before plane register updates occur, and
de-asserting it in vs_dc8000_crtc_flush() after the plane state is written,
could this prevent the hardware from latching the correct configuration and
lead to visual corruption?

[ ... ]

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

[Severity: Medium]
The commit message implies the VSYNC interrupt is acknowledged through
DISP_IRQ_STA/DISP_IRQ_EN, but vs_dc8000_irq_ack() only reads from
VSDC_DISP_IRQ_STA without performing any hardware write to clear the
pending interrupts. If the register does not auto-clear on read, could this
missing clear mechanism cause an interrupt storm and hang the system?

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

[Severity: High]
This is a pre-existing issue, but in vs_dc_probe() the interrupt line is
registered and unmasked via devm_request_irq() before vs_drm_initialize()
is called. If an interrupt fires early (e.g. left over from the bootloader),
vs_dc8000_irq_ack() will be called while dc->drm_dev is still NULL, causing
a NULL pointer dereference when accessing &dc->drm_dev->base here. Can the
interrupt registration be deferred until DRM initialization is complete?

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