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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Using devm_* APIs in component bind callbacks causes memory leaks, duplicate resource registrations on unbind/rebind, and fatal bind failures. The new patch exacerbates this by requesting exclusive reset controls.
--

--- Patch [7]: [PATCH v3 07/14] drm/rockchip: vop2: Reset AXI and DCLK to improve robustness ---
commit 9b374dfb00e64eeab45f2b86b059f7b28a55a177
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

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

This patch adds assertions for the AXI reset in the CRTC disable path and the
VP DCLK reset in the enable path. These optional resets are intended to leave
the hardware in a clean state and prevent random display output glitches on
platforms like RK3588.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -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);
> +		if (IS_ERR(vp->dclk_rst))
> +			return dev_err_probe(drm->dev, PTR_ERR(vp->dclk_rst),
> +					     "failed to get %s reset\n", dclk_name);
> +

[Severity: High]
This is a pre-existing issue, but will this exclusive reset control request
fail with -EBUSY on component rebind?

Because vop2_create_crtcs() is called from the component bind path in
vop2_bind() in drivers/gpu/drm/rockchip/rockchip_drm_vop2.c, any resources
allocated with devm_ functions tied to the platform device (vop2->dev) are
not released when the component is unbound. 

When the component is rebound, the previous exclusive reset control is still
held. It appears this might cause devm_reset_control_get_optional_exclusive()
to deterministically fail with -EBUSY since the prior exclusive grab was
never released.

Should this use reset_control_get_optional_exclusive() instead and be
manually released in the unbind path, or use a component-specific managed
API?

>  		np = of_graph_get_remote_node(dev->of_node, i, -1);
>  		if (!np) {
>  			drm_dbg(vop2->drm, "%s: No remote for vp%d\n", __func__, i);

[ ... ]

> @@ -2955,6 +2986,11 @@ 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");
> +	if (IS_ERR(vop2->axi_rst))
> +		return dev_err_probe(drm->dev, PTR_ERR(vop2->axi_rst),
> +				     "failed to get axi reset\n");
> +

[Severity: High]
This is also a pre-existing issue, but will requesting an exclusive reset
control tied to the platform device here also cause a fatal -EBUSY error
during a component rebind?

Similar to the dclk_rst allocation, vop2->axi_rst is allocated in vop2_bind()
with the devm_ API. When the DRM component master is unbound and then rebound,
vop2_bind() executes again and attempts to grab the exclusive reset control.
Since the previous devm_ allocation is tied to the persistent platform device
and wasn't released during component unbind, will this fail immediately with
-EBUSY?

>  	vop2->irq = platform_get_irq(pdev, 0);
>  	if (vop2->irq < 0)
>  		return dev_err_probe(drm->dev, vop2->irq, "cannot find irq for vop2\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-dw-hdmi-qp-yuv-v3-0-a4a982a9f2e7@collabora.com?part=7