Re: [PATCH v5 07/15] drm/rockchip: vop2: Reset AXI and DCLK to improve robustness

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] devm_reset_control_get_optional_exclusive in component bind causes -EBUSY on rebind
- [Medium] drm/rockchip: vop2: AXI reset is ineffective if clocks are gated during bind
- [Medium] drm/rockchip: asymmetrical teardown disables PM runtime before core clocks
--

commit e900afe14d3a61065ef92e05c15c885d78bc0d20
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Reset AXI and DCLK to improve robustness

Assert the AXI reset in the bind and CRTC disable paths, and the VP DCLK
reset in the enable path. These resets are intended to leave the hardware
in a clean state for the next use.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 14c073f61c646..afa4bc6cbcc4d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -953,6 +974,8 @@ static int vop2_enable(struct vop2 *vop2)
>  
>  static void vop2_disable(struct vop2 *vop2)
>  {
> +	vop2_reset_assert_deassert(vop2, vop2->axi_rst);
> +
>  	rockchip_drm_dma_detach_device(vop2->drm, vop2->dev);
>  
>  	pm_runtime_put_sync(vop2->dev);

[Severity: Medium]
This problem wasn't introduced by this patch, but does the hardware
support having the PM runtime domain disabled before the core clocks?

In vop2_disable(), pm_runtime_put_sync() drops the device's PM reference,
which can power off the domain. However, the core clocks (pclk, aclk, hclk)
are disabled later in vop2_core_clks_disable_unprepare(). 

Dropping the power domain while functional clocks are still running might
violate hardware teardown sequencing and cause lockups. Since this patch
modifies the disable sequence by adding the reset, could the asymmetry be
corrected here as well?

[ ... ]

> @@ -2596,6 +2621,12 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>  			return dev_err_probe(drm->dev, PTR_ERR(vp->dclk),
>  					     "failed to get %s\n", dclk_name);
>  
> +		vp->dclk_rst = devm_reset_control_get_optional_exclusive(vop2->dev,
> +									 dclk_name);

[Severity: High]
Will this leak the exclusive reset handle if the DRM master is unbound
and later rebound?

Since devm_reset_control_get_optional_exclusive() is tied to the component
device (vop2->dev) rather than the aggregate DRM master device, the devres
allocation won't be released when the component unbind callback executes
because the component device itself isn't removed.

Upon rebind, a second call to this function will find the reset line
already exclusively acquired. This triggers a WARN_ON() in the reset core
and fails the bind with -EBUSY, breaking the driver after an unbind cycle.

[ ... ]

> @@ -2955,6 +2986,13 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>  		return dev_err_probe(drm->dev, PTR_ERR(vop2->pll_hdmiphy1),
>  				     "failed to get pll_hdmiphy1\n");
>  
> +	vop2->axi_rst = devm_reset_control_get_optional_exclusive(vop2->dev, "axi");

[Severity: High]
Similar to the dclk_rst allocation in vop2_create_crtcs(), will allocating
this device-managed resource here in the component bind path cause an
-EBUSY failure on rebind?

> +	if (IS_ERR(vop2->axi_rst))
> +		return dev_err_probe(drm->dev, PTR_ERR(vop2->axi_rst),
> +				     "failed to get axi reset\n");
> +
> +	vop2_reset_assert_deassert(vop2, vop2->axi_rst);

[Severity: Medium]
Will this synchronous reset be effective if the AXI and AHB clocks are
gated at this point?

The reset pulse is sent immediately after acquiring the handle in
vop2_bind(), but this happens before pm_runtime_enable() is called and
before the core clocks are explicitly prepared and enabled.

If the driver is loaded as a module or probed late, the kernel's unused
clock framework may have already gated the clocks. Pulsing a synchronous
reset while the IP block's clocks are gated usually acts as a silent
no-op. When vop2_enable() later powers on the domain, the hardware might
wake up with its previous dirty state, defeating the purpose of the reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-dw-hdmi-qp-yuv-v5-0-6fab0fe43a9b@collabora.com?part=7