[PATCH 2/3] drm/v3d: Allocate all resources before enabling the clock

[Philipp Zabel]

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 <mcanal at 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