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.
> as those operation don't require the GPU to be powered on.
>
> Signed-off-by: Maíra Canal <mca...@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.
> + 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
