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 <[email protected]> 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/[email protected]?part=4
