Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The disable sequence in `vs_dc8200_bridge_disable` reverses the order of clearing `VSDC_DISP_PANEL_START` and `VSDC_DISP_PANEL_CONFIG`. - [High] Incorrect teardown sequence in CRTC disable path where pixel clock is disabled before calling the CRTC hardware disable hook. Pre-existing issues: - [High] The plane disable function incorrectly sets the enable bit instead of clearing it. - [High] Calling `clk_prepare_enable()` and `clk_disable_unprepare()` in the CRTC atomic enable/disable callbacks violates atomic context rules. - [High] Potential NULL pointer dereference if IRQ fires before drm_dev is initialized. -- commit 7843af23baf865801f80d2910ca556ff5e144ece Author: Joey Lu <[email protected]> Subject: drm/verisilicon: introduce per-variant hardware ops table The DC8200 and DCUltraLite share a broadly similar register layout but differ in how the bridge, CRTC, primary plane and IRQ paths are driven. Introduce a vs_dc_funcs vtable so each variant can supply its own implementation without scattering conditionals across multiple files. > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c > b/drivers/gpu/drm/verisilicon/vs_crtc.c > index 0b8a35d09cd2e..679d6541ba1b9 100644 > --- a/drivers/gpu/drm/verisilicon/vs_crtc.c > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c [ ... ] > @@ -30,6 +53,9 @@ static void vs_crtc_atomic_disable(struct drm_crtc *crtc, > drm_crtc_vblank_off(crtc); > > clk_disable_unprepare(dc->pix_clk[output]); [Severity: High] This is a pre-existing issue, but does calling clk_disable_unprepare() here violate atomic context rules? The DRM subsystem requires that CRTC atomic disable callbacks run in atomic context where sleeping is forbidden, but clk_disable_unprepare() can sleep. > + > + if (dc->funcs->crtc_disable) > + dc->funcs->crtc_disable(dc, output); > } [Severity: High] Could disabling the pixel clock before invoking the hardware-specific crtc_disable callback cause bus hangs? If the crtc_disable hook requires the pixel clock to be active to perform register writes, turning it off first might result in dropped writes or stalls. Should the clock be disabled after the hook is called, reversing the setup sequence? > > static void vs_crtc_atomic_enable(struct drm_crtc *crtc, > @@ -42,6 +68,9 @@ static void vs_crtc_atomic_enable(struct drm_crtc *crtc, > drm_WARN_ON(&dc->drm_dev->base, > clk_prepare_enable(dc->pix_clk[output])); [Severity: High] This is a pre-existing issue, but does this clk_prepare_enable() call also violate atomic context rules? Since atomic_enable runs in atomic context, calling sleeping functions can cause kernel warnings or potential deadlocks during non-blocking commits. > > + if (dc->funcs->crtc_enable) > + dc->funcs->crtc_enable(dc, output); > + > drm_crtc_vblank_on(crtc); > } > > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c > b/drivers/gpu/drm/verisilicon/vs_dc.c > index dad9967bc10b8..c949570241890 100644 > --- a/drivers/gpu/drm/verisilicon/vs_dc.c > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c > @@ -33,7 +31,7 @@ static irqreturn_t vs_dc_irq_handler(int irq, void *private) > struct vs_dc *dc = private; > u32 irqs; > > - regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs); > + irqs = dc->funcs->irq_handler(dc); > > vs_drm_handle_irq(dc, irqs); [Severity: High] This isn't a bug introduced by this patch, but is it possible for the IRQ to fire before the DRM device is fully initialized? If the bootloader leaves the display hardware active and generating VSYNC interrupts, vs_dc_irq_handler() could execute and call vs_drm_handle_irq(), which accesses dc->drm_dev. Since devm_request_irq() is called in vs_dc_probe() before vs_drm_initialize() allocates dc->drm_dev, could this lead to a NULL pointer dereference during early boot? [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c > b/drivers/gpu/drm/verisilicon/vs_dc8200.c > new file mode 100644 > index 0000000000000..db9e1b3cd9032 > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c [ ... ] > +static void vs_dc8200_bridge_disable(struct vs_dc *dc, unsigned int output) > +{ > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output), > + VSDC_DISP_PANEL_CONFIG_RUNNING); > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START, > + VSDC_DISP_PANEL_START_MULTI_DISP_SYNC | > + VSDC_DISP_PANEL_START_RUNNING(output)); [Severity: High] Does this reversal of the clearing sequence cause bus stalls or visual tearing? In the original code, VSDC_DISP_PANEL_START was cleared before VSDC_DISP_PANEL_CONFIG. This refactored sequence disables the panel configuration while the pipeline is potentially still active. Should VSDC_DISP_PANEL_START be cleared first to safely stop the pipeline? [ ... ] > +static void vs_dc8200_plane_disable_ex(struct vs_dc *dc, unsigned int output) > +{ > + regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), > + VSDC_FB_CONFIG_EX_FB_EN); > + > + vs_dc8200_plane_commit(dc, output); > +} [Severity: High] This is a pre-existing issue carried over from the original code, but does this actually disable the plane? By using regmap_set_bits() with VSDC_FB_CONFIG_EX_FB_EN, the frame buffer enable bit is being set to 1 instead of being cleared. Should this use regmap_clear_bits() instead to properly disable the hardware plane? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
