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