Re: [PATCH 2/3] drm/v3d: Allocate all resources before enabling the clock
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: 2025-07-28 14:32:02
Also in:
dri-devel, linux-clk
Hi Maíra, On Mo, 2025-07-28 at 09:35 -0300, Maíra Canal wrote:
Move all resource allocation operations before actually enabling the clock,
This patch moves code even before requesting the clock. But I don't think this is necessary, see below.
quoted hunk ↗ jump to hunk
as those operation don't require the GPU to be powered on. Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/v3d/v3d_drv.c | 92 ++++++++++++++++++++++--------------------- drivers/gpu/drm/v3d/v3d_drv.h | 3 +- drivers/gpu/drm/v3d/v3d_gem.c | 14 +++++-- drivers/gpu/drm/v3d/v3d_irq.c | 15 +++---- 4 files changed, 66 insertions(+), 58 deletions(-)diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 2def155ce496ec5f159f6bda9929aeaae141d1a6..6e6b830bee6587e4170fd64d354916766e59d2e5 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c@@ -347,14 +347,55 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) return ret; } + if (v3d->ver < V3D_GEN_41) { + ret = map_regs(v3d, &v3d->gca_regs, "gca"); + if (ret) + return ret; + } + + v3d->reset = devm_reset_control_get_exclusive(dev, NULL); + if (IS_ERR(v3d->reset)) { + ret = PTR_ERR(v3d->reset); + + if (ret == -EPROBE_DEFER) + return ret; + + v3d->reset = NULL;
Drive-by comment, not an issue with this (code-moving) patch: It looks like this open-codes devm_reset_control_get_optional_exclusive().
+ ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
+ if (ret) {
+ dev_err(dev,
+ "Failed to get reset control or bridge regs\n");
+ return ret;
+ }
+ }
+
+ v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
+ GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
+ if (!v3d->mmu_scratch) {
+ dev_err(dev, "Failed to allocate MMU scratch page\n");
+ return -ENOMEM;
+ }
+
+ ret = v3d_gem_allocate(drm);
+ if (ret)
+ goto dma_free;
+
+ ret = v3d_irq_init(v3d);
+ if (ret)
+ goto gem_destroy;These functions needing manual cleanup are called before another devm_ function below. With this, resources are not freed in inverse order of allocation. I don't see whether mixing devm and non-devm initialization is an actual problem in this case, but it would look cleaner if the devm_clk_get_optional() below was just moved back up before dma_alloc_wc(). If there are also devm_ functions called from inside the v3d_ functions, it might be better to move all cleanup into devm actions.
quoted hunk ↗ jump to hunk
+ v3d_perfmon_init(v3d); + v3d->clk = devm_clk_get_optional(dev, NULL); - if (IS_ERR(v3d->clk)) - return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n"); + if (IS_ERR(v3d->clk)) { + ret = dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n"); + goto gem_destroy; + } ret = clk_prepare_enable(v3d->clk); if (ret) { dev_err(&pdev->dev, "Couldn't enable the V3D clock\n"); - return ret; + goto gem_destroy; } v3d_idle_sms(v3d);@@ -381,45 +422,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) ident3 = V3D_READ(V3D_HUB_IDENT3); v3d->rev = V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPREV); - v3d_perfmon_init(v3d); - - v3d->reset = devm_reset_control_get_exclusive(dev, NULL); - if (IS_ERR(v3d->reset)) { - ret = PTR_ERR(v3d->reset); - - if (ret == -EPROBE_DEFER) - goto clk_disable; - - v3d->reset = NULL; - ret = map_regs(v3d, &v3d->bridge_regs, "bridge"); - if (ret) { - dev_err(dev, - "Failed to get reset control or bridge regs\n"); - goto clk_disable; - } - } - - if (v3d->ver < V3D_GEN_41) { - ret = map_regs(v3d, &v3d->gca_regs, "gca"); - if (ret) - goto clk_disable; - } - - v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr, - GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO); - if (!v3d->mmu_scratch) { - dev_err(dev, "Failed to allocate MMU scratch page\n"); - ret = -ENOMEM; - goto clk_disable; - } - - ret = v3d_gem_init(drm); - if (ret) - goto dma_free; - - ret = v3d_irq_init(v3d); - if (ret) - goto gem_destroy; + v3d_gem_init(drm); + v3d_irq_enable(v3d); ret = drm_dev_register(drm, 0); if (ret)@@ -435,12 +439,12 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) drm_dev_unregister(drm); irq_disable: v3d_irq_disable(v3d); +clk_disable: + clk_disable_unprepare(v3d->clk);
clk_disable_unprepare() is moved up in the error path, but not in v3d_platform_drm_remove(). Should this be kept consistent?
gem_destroy: v3d_gem_destroy(drm); dma_free: dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr); -clk_disable: - clk_disable_unprepare(v3d->clk); return ret; }
regards Philipp