Re: [PATCH v4 3/3] drm/msm: handle for EPROBE_DEFER for of_icc_get

[Jordan Crouse <jcrouse@codeaurora.org>]

On Mon, Jul 13, 2020 at 06:53:42PM -0400, Jonathan Marek wrote:
> Check for errors instead of silently not using icc if the msm driver
> probes before the interconnect driver.
> 
> Allow ENODATA for ocmem path, as it is optional and this error
> is returned when "gfx-mem" path is provided but not "ocmem".
> 
> Because msm_gpu_cleanup assumes msm_gpu_init has been called, the icc path
> init needs to be after msm_gpu_init for the error path to work.

A possible future improvement would be to move the ocmem check to the target
specific code for 3xx and 4xx where you could be a bit more demanding that the
ocmem path actually exist.

Reviewed-by: Jordan Crouse <jcrouse@codeaurora.org>

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 65 +++++++++++++++----------
>  1 file changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index ad64d4b7e8d7..3f1ecc1de965 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -895,7 +895,7 @@ static int adreno_get_legacy_pwrlevels(struct device *dev)
>  	return 0;
>  }
>  
> -static int adreno_get_pwrlevels(struct device *dev,
> +static void adreno_get_pwrlevels(struct device *dev,
>  		struct msm_gpu *gpu)
>  {
>  	unsigned long freq = ULONG_MAX;
> @@ -930,24 +930,6 @@ static int adreno_get_pwrlevels(struct device *dev,
>  	}
>  
>  	DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
> -
> -	/* Check for an interconnect path for the bus */
> -	gpu->icc_path = of_icc_get(dev, "gfx-mem");
> -	if (!gpu->icc_path) {
> -		/*
> -		 * Keep compatbility with device trees that don't have an
> -		 * interconnect-names property.
> -		 */
> -		gpu->icc_path = of_icc_get(dev, NULL);
> -	}
> -	if (IS_ERR(gpu->icc_path))
> -		gpu->icc_path = NULL;
> -
> -	gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> -	if (IS_ERR(gpu->ocmem_icc_path))
> -		gpu->ocmem_icc_path = NULL;
> -
> -	return 0;
>  }
>  
>  int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> @@ -993,9 +975,11 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		struct adreno_gpu *adreno_gpu,
>  		const struct adreno_gpu_funcs *funcs, int nr_rings)
>  {
> -	struct adreno_platform_config *config = pdev->dev.platform_data;
> +	struct device *dev = &pdev->dev;
> +	struct adreno_platform_config *config = dev->platform_data;
>  	struct msm_gpu_config adreno_gpu_config  = { 0 };
>  	struct msm_gpu *gpu = &adreno_gpu->base;
> +	int ret;
>  
>  	adreno_gpu->funcs = funcs;
>  	adreno_gpu->info = adreno_info(config->rev);
> @@ -1007,15 +991,42 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  
>  	adreno_gpu_config.nr_rings = nr_rings;
>  
> -	adreno_get_pwrlevels(&pdev->dev, gpu);
> +	adreno_get_pwrlevels(dev, gpu);
>  
> -	pm_runtime_set_autosuspend_delay(&pdev->dev,
> +	pm_runtime_set_autosuspend_delay(dev,
>  		adreno_gpu->info->inactive_period);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);
>  
> -	return msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
> +	ret = msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
>  			adreno_gpu->info->name, &adreno_gpu_config);
> +	if (ret)
> +		return ret;
> +
> +	/* Check for an interconnect path for the bus */
> +	gpu->icc_path = of_icc_get(dev, "gfx-mem");
> +	if (!gpu->icc_path) {
> +		/*
> +		 * Keep compatbility with device trees that don't have an
> +		 * interconnect-names property.
> +		 */
> +		gpu->icc_path = of_icc_get(dev, NULL);
> +	}
> +	if (IS_ERR(gpu->icc_path)) {
> +		ret = PTR_ERR(gpu->icc_path);
> +		gpu->icc_path = NULL;
> +		return ret;
> +	}
> +
> +	gpu->ocmem_icc_path = of_icc_get(dev, "ocmem");
> +	if (IS_ERR(gpu->ocmem_icc_path)) {
> +		ret = PTR_ERR(gpu->ocmem_icc_path);
> +		gpu->ocmem_icc_path = NULL;
> +		/* allow -ENODATA, ocmem icc is optional */
> +		if (ret != -ENODATA)
> +			return ret;
> +	}
> +	return 0;
>  }
>  
>  void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
> @@ -1029,8 +1040,8 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
>  
>  	pm_runtime_disable(&priv->gpu_pdev->dev);
>  
> +	msm_gpu_cleanup(&adreno_gpu->base);
> +
>  	icc_put(gpu->icc_path);
>  	icc_put(gpu->ocmem_icc_path);
> -
> -	msm_gpu_cleanup(&adreno_gpu->base);
>  }
> -- 
> 2.26.1
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project