Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/rockchip: vop2: missing of_node_put() on error in vop2_create_crtcs() - [Medium] drm/rockchip: vop2: axi_rst pulsed in vop2_bind() without enabling aclk or PM --
commit 7b3ce842063d5aafe025d15a4339ddc7bddb6893 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: vop2: Reset AXI and DCLK to improve robustness This commit adds optional AXI and VP DCLK reset controls to the Rockchip VOP2 driver, pulsing the AXI reset during bind and CRTC disable and the VP DCLK reset during CRTC enable to leave the hardware in a clean state. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index b85cd94d1e9d9..98430587a7abe 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -2585,6 +2610,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: Medium] Does returning directly here leak the port device_node references acquired in earlier iterations of this loop? In vop2_create_crtcs(), each iteration for a connected video port acquires a port reference at the end of the loop body: drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:vop2_create_crtcs() { for (i = 0; i < vop2_data->nr_vps; i++) { ... port = of_graph_get_port_by_id(dev->of_node, i); if (!port) return dev_err_probe(drm->dev, -ENOENT, "no port node found for video_port%d\n", i); vp->crtc.port = port; nvps++; } } If devm_reset_control_get_optional_exclusive() fails on a subsequent iteration (i > 0), vop2_create_crtcs() returns immediately to vop2_bind(), which also returns the error without calling of_node_put() on vop2->vps[0 .. i - 1].crtc.port (a pre-existing issue also shared with the other early returns in vop2_create_crtcs()). > @@ -2944,6 +2975,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"); > + 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] As reported by Sashiko on v5 [1], can pulsing vop2->axi_rst here in vop2_bind() take effect if the VOP2 power domain is powered down or vop2->aclk is gated? In vop2_bind(), vop2_reset_assert_deassert() is called before pm_runtime_enable(&pdev->dev) and without resuming runtime PM or enabling vop2->aclk. After vop2_probe() completes, genpd_dev_pm_sync() schedules power-off for the RK3588_PD_VOP domain (which uses GENPD_FLAG_PM_CLK | GENPD_FLAG_NO_STAY_ON), and the core clocks are only prepared and enabled later in vop2_enable(): drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:vop2_enable() { ret = pm_runtime_resume_and_get(vop2->dev); if (ret < 0) { drm_err(vop2->drm, "failed to get pm runtime: %d\n", ret); return ret; } ret = vop2_core_clks_prepare_enable(vop2); if (ret) goto err_put_pm; } By contrast, vop2_disable() pulses vop2->axi_rst while runtime PM and the core clocks are still active, and vop_initial() in rockchip_drm_vop.c resumes runtime PM and enables hclk and aclk before pulsing resets. Should this reset be performed while the power domain and aclk are enabled? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
