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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The newly added `crtc_disable` hook is invoked after the pixel clock is disabled, which will cause bus errors.
- [High] The register clearing sequence in `vs_dc8200_bridge_disable` is inverted compared to the original code, causing a functional regression.
- [Medium] Unconditional assignment of `dc->funcs` without verifying hardware identity, contradicting the commit message.
--

commit 9ad7489590b539e39997172552aadb2d24ac579b
Author: Joey Lu <a0987203069@gmail.com>

drm/verisilicon: introduce per-variant hardware ops table

This commit refactors hardware-specific operations into a vs_dc_funcs
vtable to support multiple IP variants without scattering if/else branches,
extracting DC8200-specific operations into vs_dc8200.c.

> diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
> index 9080344398ca2..a87caa6f73bac 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]);
> +
> +	if (dc->funcs->crtc_disable)
> +		dc->funcs->crtc_disable(dc, output);
>  }

Will calling the crtc_disable hook here cause bus errors? It looks like
clk_disable_unprepare() disables the pixel clock before we call into the
hardware-specific teardown function, which likely accesses MMIO registers
while the clock is off. Symmetrically, vs_crtc_atomic_enable() invokes
crtc_enable after the clock is prepared and enabled.

> 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

[ ... ]

> @@ -136,6 +134,8 @@ static int vs_dc_probe(struct platform_device *pdev)
>  	dev_info(dev, "Found DC%x rev %x customer %x\n", dc->identity.model,
>  		 dc->identity.revision, dc->identity.customer_id);
>  
> +	dc->funcs = &vs_dc8200_funcs;
> +
>  	if (port_count > dc->identity.display_count) {
>  		dev_err(dev, "too many downstream ports than HW capability\n");
>  		ret = -EINVAL;

The commit message states "After vs_fill_chip_identity() confirms a DC8200
(or compatible) identity, vs_dc_probe() assigns dc->funcs = &vs_dc8200_funcs".

However, this assignment happens unconditionally without checking
dc->identity.model. Could this apply the wrong operations if probed with a
different hardware variant?

> 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));

Is this register clearing sequence inverted compared to the original code? 

The commit message says "No functional change for DC8200 platforms", but
the original code in vs_bridge_atomic_disable() cleared VSDC_DISP_PANEL_START
before clearing VSDC_DISP_PANEL_CONFIG:

    regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
              VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
              VSDC_DISP_PANEL_START_RUNNING(output));
    regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
              VSDC_DISP_PANEL_CONFIG_RUNNING);

Could reversing this order violate display hardware teardown constraints and
result in display glitches or pipeline lockups?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519055114.1886525-1-a0987203069@gmail.com?part=3