Re: [PATCH v6 3/6] drm/verisilicon: introduce per-variant hardware ops table

[sashiko-bot@kernel.org]

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 <a0987203069@gmail.com>

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/20260908092840.225220-1-a0987203069@gmail.com?part=3