Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Variant-specific CRTC disable hook (`crtc_disable_ex`) is called after disabling the pixel clock, leading to hardware faults. - [High] The panel disable sequence in `vs_dc8200_panel_disable_ex` reverses the order of clearing CONFIG and START registers, violating the original sequence and hardware state machine. Pre-existing issues: - [High] Primary plane disable function incorrectly sets the plane enable bit instead of clearing it. - [High] IRQ handler is registered before DRM device initialization is complete, causing a potential NULL pointer dereference on early interrupts. - [High] Sleeping clock functions (`clk_prepare_enable`, `clk_disable_unprepare`) are called from CRTC atomic callbacks, violating DRM atomic context requirements. -- commit c35af4d919e849e303afa29d897c4d1581720138 Author: Joey Lu <[email protected]> drm/verisilicon: introduce per-variant hardware ops table This commit introduces a hardware ops table for the Verisilicon DC driver to handle variant-specific implementations. It extracts DC8200-specific ops into a new file and updates the shared code to dispatch hardware sequences through the new function pointers. The patch aims to avoid scattering variant checks across the core driver. > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c > b/drivers/gpu/drm/verisilicon/vs_crtc.c > --- 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 calling clk_disable_unprepare() here sleeps in an atomic context, which is forbidden during non-blocking commits and can lead to system instability. > + > + if (dc->funcs->crtc_disable_ex) > + dc->funcs->crtc_disable_ex(dc, output); [Severity: High] Will this sequence cause hardware faults? The pixel clock is disabled via clk_disable_unprepare() right before calling the variant-specific crtc_disable_ex() callback. If follow-up implementations of crtc_disable_ex() attempt to access hardware registers within the now-disabled pixel clock domain, it could trigger an asynchronous external abort or bus error when userspace disables the display. Should the callback be invoked before disabling the clock? > } > > 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 calling clk_prepare_enable() from vs_crtc_atomic_enable() violate the DRM atomic context requirements? The clock framework functions clk_prepare_enable() and clk_disable_unprepare() can sleep. Since CRTC atomic enable, disable, and update callbacks run in an atomic context during non-blocking commits, this can cause scheduling while atomic warnings and system instability during userspace modesets. > + if (dc->funcs->crtc_enable_ex) > + dc->funcs->crtc_enable_ex(dc, output); > + > drm_crtc_vblank_on(crtc); > } [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c > b/drivers/gpu/drm/verisilicon/vs_dc8200.c > --- /dev/null > +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c > @@ -0,0 +1,121 @@ [ ... ] > +static void vs_dc8200_panel_disable_ex(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 reverse the original tear-down sequence? In the original vs_bridge_atomic_disable() code, VSDC_DISP_PANEL_START_RUNNING was cleared before clearing VSDC_DISP_PANEL_CONFIG_RUNNING. Here in vs_dc8200_panel_disable_ex(), VSDC_DISP_PANEL_CONFIG_RUNNING is cleared first. Modifying or clearing a pipeline's configuration while it is actively sending pixels could put the hardware into an invalid state and cause synchronization errors. > + > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(output), > + VSDC_DISP_PANEL_CONFIG_EX_COMMIT); > +} [ ... ] > +static void vs_dc8200_primary_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); [Severity: High] This is a pre-existing issue, but does this function actually enable the plane instead of disabling it? The code uses regmap_set_bits() to write a 1 to VSDC_FB_CONFIG_EX_FB_EN, which sets the enable bit. This could leave the primary plane active on the screen when userspace expects it to be hidden via vs_dc8200_primary_plane_disable_ex(). > + > + vs_dc8200_plane_commit(dc, output); > +} [ ... ] > +static u32 vs_dc8200_irq_ack(struct vs_dc *dc) > +{ > + u32 hw_irqs, unified = 0, known = 0; > + unsigned int i; > + > + regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &hw_irqs); > + > + for (i = 0; i < VSDC_MAX_OUTPUTS; i++) { > + known |= VSDC_TOP_IRQ_VSYNC(i); > + if (hw_irqs & VSDC_TOP_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 could this drm_WARN_ONCE() trigger a NULL pointer dereference during boot? In vs_dc_probe(), the IRQ handler is requested via devm_request_irq() before vs_drm_initialize() is called to allocate the DRM device. If a residual boot interrupt fires during this window, vs_dc_irq_handler() will run and call vs_dc8200_irq_ack() while dc->drm_dev is still NULL, causing the warning macro to dereference NULL. > + > + return unified; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
