Message-ID: <3f584b2d-9d9e-e18d-e745-cdb8f3599c59@marek.ca>
Date:   Fri, 1 Mar 2019 18:01:06 -0500
From:   Jonathan Marek <jonathan@...ek.ca>
To:     Jordan Crouse <jcrouse@...eaurora.org>,
        freedreno@...ts.freedesktop.org
Cc:     jean-philippe.brucker@....com, linux-arm-msm@...r.kernel.org,
        dianders@...omimum.org, hoegsberg@...gle.com,
        baolu.lu@...ux.intel.com,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Kees Cook <keescook@...omium.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Sean Paul <sean@...rly.run>,
        Sharat Masetty <smasetty@...eaurora.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Rob Clark <robdclark@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Mamta Shukla <mamtashukla555@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [RFC PATCH v1 09/15] drm/msm/gpu: Move address space setup to the
 GPU targets

There is an error in the a2xx part of this patch: 0xfff in adreno_gpu.c 
became 0xff in a2xx_gpu.c

On 3/1/19 2:38 PM, Jordan Crouse wrote:
> Move the address space steup code out of the generic msm GPU code to
> to the individual GPU targets. This allows us to do target specific
> setup such as gpummu for a2xx or split pagetables and per-instance
> pagetables for newer a5xx and a6xx targets. All this is at the
> expense of duplicated code in some of the target files but I think
> it pays for itself in improved code flow and flexibility.
> 
> Signed-off-by: Jordan Crouse <jcrouse@...eaurora.org>
> ---
> 
>   drivers/gpu/drm/msm/adreno/a2xx_gpu.c   | 37 ++++++++++++++++------
>   drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 50 ++++++++++++++++++++++--------
>   drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 51 +++++++++++++++++++++++--------
>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 37 +++++++++++++++++++---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 37 +++++++++++++++++++---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  7 -----
>   drivers/gpu/drm/msm/msm_gem.h           |  1 +
>   drivers/gpu/drm/msm/msm_gpu.c           | 54 ++-------------------------------
>   drivers/gpu/drm/msm/msm_gpu.h           |  2 ++
>   9 files changed, 173 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index 1f83bc1..49241d0 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -401,6 +401,30 @@ static struct msm_gpu_state *a2xx_gpu_state_get(struct msm_gpu *gpu)
>   	return state;
>   }
>   
> +static struct msm_gem_address_space *
> +a2xx_create_address_space(struct msm_gpu *gpu)
> +{
> +	struct msm_gem_address_space *aspace;
> +	int ret;
> +
> +	aspace = msm_gem_address_space_create_a2xx(&gpu->pdev->dev, gpu,
> +		"gpu", SZ_16M, SZ_16M + 0xff * SZ_64K);
> +	if (IS_ERR(aspace)) {
> +		DRM_DEV_ERROR(gpu->dev->dev,
> +			"No memory protection without MMU\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> +	if (ret) {
> +		msm_gem_address_space_put(aspace);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return aspace;
> +}
> +
> +
>   /* Register offset defines for A2XX - copy of A3XX */
>   static const unsigned int a2xx_register_offsets[REG_ADRENO_REGISTER_MAX] = {
>   	REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_BASE, REG_AXXX_CP_RB_BASE),
> @@ -429,6 +453,7 @@ static const struct adreno_gpu_funcs funcs = {
>   #endif
>   		.gpu_state_get = a2xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
> +		.create_address_space = a2xx_create_address_space,
>   	},
>   };
>   
> @@ -473,16 +498,8 @@ struct msm_gpu *a2xx_gpu_init(struct drm_device *dev)
>   	adreno_gpu->reg_offsets = a2xx_register_offsets;
>   
>   	ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
> -	if (ret)
> -		goto fail;
> -
> -	if (!gpu->aspace) {
> -		dev_err(dev->dev, "No memory protection without MMU\n");
> -		ret = -ENXIO;
> -		goto fail;
> -	}
> -
> -	return gpu;
> +	if (!ret)
> +		return gpu;
>   
>   fail:
>   	if (a2xx_gpu)
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index c3b4bc6..33ab5e8 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -21,6 +21,7 @@
>   #  include <mach/ocmem.h>
>   #endif
>   
> +#include "msm_gem.h"
>   #include "a3xx_gpu.h"
>   
>   #define A3XX_INT0_MASK \
> @@ -433,6 +434,41 @@ static struct msm_gpu_state *a3xx_gpu_state_get(struct msm_gpu *gpu)
>   	return state;
>   }
>   
> +static struct msm_gem_address_space *
> +a3xx_create_address_space(struct msm_gpu *gpu)
> +{
> +	struct msm_gem_address_space *aspace;
> +	struct iommu_domain *iommu;
> +	int ret;
> +
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (!iommu) {
> +		DRM_DEV_ERROR(gpu->dev->dev,
> +			"No memory protection without IOMMU\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	iommu->geometry.aperture_start = SZ_16M;
> +	iommu->geometry.aperture_end = 0xffffffff;
> +
> +	aspace = msm_gem_address_space_create(&gpu->pdev->dev, iommu, "gpu");
> +	if (IS_ERR(aspace)) {
> +		iommu_domain_free(iommu);
> +		DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
> +			PTR_ERR(aspace));
> +		return aspace;
> +	}
> +
> +	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> +	if (ret) {
> +		msm_gem_address_space_put(aspace);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return aspace;
> +}
> +
> +
>   /* Register offset defines for A3XX */
>   static const unsigned int a3xx_register_offsets[REG_ADRENO_REGISTER_MAX] = {
>   	REG_ADRENO_DEFINE(REG_ADRENO_CP_RB_BASE, REG_AXXX_CP_RB_BASE),
> @@ -461,6 +497,7 @@ static const struct adreno_gpu_funcs funcs = {
>   #endif
>   		.gpu_state_get = a3xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
> +		.create_address_space = a3xx_create_address_space,
>   	},
>   };
>   
> @@ -520,19 +557,6 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
>   #endif
>   	}
>   
> -	if (!gpu->aspace) {
> -		/* TODO we think it is possible to configure the GPU to
> -		 * restrict access to VRAM carveout.  But the required
> -		 * registers are unknown.  For now just bail out and
> -		 * limp along with just modesetting.  If it turns out
> -		 * to not be possible to restrict access, then we must
> -		 * implement a cmdstream validator.
> -		 */
> -		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
> -		ret = -ENXIO;
> -		goto fail;
> -	}
> -
>   	return gpu;
>   
>   fail:
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index 18f9a8e..08a5729 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -15,6 +15,8 @@
>   #  include <soc/qcom/ocmem.h>
>   #endif
>   
> +#include "msm_gem.h"
> +
>   #define A4XX_INT0_MASK \
>   	(A4XX_INT0_RBBM_AHB_ERROR |        \
>   	 A4XX_INT0_RBBM_ATB_BUS_OVERFLOW | \
> @@ -530,6 +532,41 @@ static int a4xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
>   	return 0;
>   }
>   
> +static struct msm_gem_address_space *
> +a4xx_create_address_space(struct msm_gpu *gpu)
> +{
> +	struct msm_gem_address_space *aspace;
> +	struct iommu_domain *iommu;
> +	int ret;
> +
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (!iommu) {
> +		DRM_DEV_ERROR(gpu->dev->dev,
> +			"No memory protection without IOMMU\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	iommu->geometry.aperture_start = SZ_16M;
> +	iommu->geometry.aperture_end = 0xffffffff;
> +
> +	aspace = msm_gem_address_space_create(&gpu->pdev->dev, iommu, "gpu");
> +	if (IS_ERR(aspace)) {
> +		iommu_domain_free(iommu);
> +		DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
> +			PTR_ERR(aspace));
> +		return aspace;
> +	}
> +
> +	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> +	if (ret) {
> +		msm_gem_address_space_put(aspace);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return aspace;
> +}
> +
> +
>   static const struct adreno_gpu_funcs funcs = {
>   	.base = {
>   		.get_param = adreno_get_param,
> @@ -547,6 +584,7 @@ static const struct adreno_gpu_funcs funcs = {
>   #endif
>   		.gpu_state_get = a4xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
> +		.create_address_space = a4xx_create_address_space,
>   	},
>   	.get_timestamp = a4xx_get_timestamp,
>   };
> @@ -600,19 +638,6 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
>   #endif
>   	}
>   
> -	if (!gpu->aspace) {
> -		/* TODO we think it is possible to configure the GPU to
> -		 * restrict access to VRAM carveout.  But the required
> -		 * registers are unknown.  For now just bail out and
> -		 * limp along with just modesetting.  If it turns out
> -		 * to not be possible to restrict access, then we must
> -		 * implement a cmdstream validator.
> -		 */
> -		DRM_DEV_ERROR(dev->dev, "No memory protection without IOMMU\n");
> -		ret = -ENXIO;
> -		goto fail;
> -	}
> -
>   	return gpu;
>   
>   fail:
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 45662d3..3d6f414 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1456,6 +1456,38 @@ static unsigned long a5xx_gpu_busy(struct msm_gpu *gpu)
>   	return (unsigned long)busy_time;
>   }
>   
> +static struct msm_gem_address_space *
> +a5xx_create_address_space(struct msm_gpu *gpu)
> +{
> +	struct msm_gem_address_space *aspace;
> +	struct iommu_domain *iommu;
> +	int ret;
> +
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (!iommu)
> +		return NULL;
> +
> +	iommu->geometry.aperture_start = 0x100000000ULL;
> +	iommu->geometry.aperture_end = 0x1ffffffffULL;
> +
> +	aspace = msm_gem_address_space_create(&gpu->pdev->dev, iommu, "gpu");
> +	if (IS_ERR(aspace)) {
> +		iommu_domain_free(iommu);
> +		DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
> +			PTR_ERR(aspace));
> +		return aspace;
> +	}
> +
> +	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> +	if (ret) {
> +		msm_gem_address_space_put(aspace);
> +		return ERR_PTR(ret);
> +	}
> +
> +	msm_mmu_set_fault_handler(aspace->mmu, gpu, a5xx_fault_handler);
> +	return aspace;
> +}
> +
>   static const struct adreno_gpu_funcs funcs = {
>   	.base = {
>   		.get_param = adreno_get_param,
> @@ -1477,6 +1509,7 @@ static const struct adreno_gpu_funcs funcs = {
>   		.gpu_busy = a5xx_gpu_busy,
>   		.gpu_state_get = a5xx_gpu_state_get,
>   		.gpu_state_put = a5xx_gpu_state_put,
> +		.create_address_space = a5xx_create_address_space,
>   	},
>   	.get_timestamp = a5xx_get_timestamp,
>   };
> @@ -1523,7 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>   
>   	adreno_gpu->registers = a5xx_registers;
>   	adreno_gpu->reg_offsets = a5xx_register_offsets;
> -
>   	a5xx_gpu->lm_leakage = 0x4E001A;
>   
>   	check_speed_bin(&pdev->dev);
> @@ -1534,9 +1566,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>   		return ERR_PTR(ret);
>   	}
>   
> -	if (gpu->aspace)
> -		msm_mmu_set_fault_handler(gpu->aspace->mmu, gpu, a5xx_fault_handler);
> -
>   	/* Set up the preemption specific bits and pieces for each ringbuffer */
>   	a5xx_preempt_init(gpu);
>   
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 1c20d59..f2e0800 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -783,6 +783,38 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
>   	return (unsigned long)busy_time;
>   }
>   
> +static struct msm_gem_address_space *
> +a6xx_create_address_space(struct msm_gpu *gpu)
> +{
> +	struct msm_gem_address_space *aspace;
> +	struct iommu_domain *iommu;
> +	int ret;
> +
> +	iommu = iommu_domain_alloc(&platform_bus_type);
> +	if (!iommu)
> +		return NULL;
> +
> +	iommu->geometry.aperture_start = 0x100000000ULL;
> +	iommu->geometry.aperture_end = 0x1ffffffffULL;
> +
> +	aspace = msm_gem_address_space_create(&gpu->pdev->dev, iommu, "gpu");
> +	if (IS_ERR(aspace)) {
> +		iommu_domain_free(iommu);
> +		DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
> +			PTR_ERR(aspace));
> +		return aspace;
> +	}
> +
> +	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> +	if (ret) {
> +		msm_gem_address_space_put(aspace);
> +		return ERR_PTR(ret);
> +	}
> +
> +	msm_mmu_set_fault_handler(aspace->mmu, gpu, a6xx_fault_handler);
> +	return aspace;
> +}
> +
>   static const struct adreno_gpu_funcs funcs = {
>   	.base = {
>   		.get_param = adreno_get_param,
> @@ -803,6 +835,7 @@ static const struct adreno_gpu_funcs funcs = {
>   		.gpu_set_freq = a6xx_gmu_set_freq,
>   		.gpu_state_get = a6xx_gpu_state_get,
>   		.gpu_state_put = a6xx_gpu_state_put,
> +		.create_address_space = a6xx_create_address_space,
>   	},
>   	.get_timestamp = a6xx_get_timestamp,
>   };
> @@ -845,9 +878,5 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   		return ERR_PTR(ret);
>   	}
>   
> -	if (gpu->aspace)
> -		msm_mmu_set_fault_handler(gpu->aspace->mmu, gpu,
> -				a6xx_fault_handler);
> -
>   	return gpu;
>   }
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 2cfee1a..dc9ea82 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -765,13 +765,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   	adreno_gpu->rev = config->rev;
>   
>   	adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";
> -
> -	adreno_gpu_config.va_start = SZ_16M;
> -	adreno_gpu_config.va_end = 0xffffffff;
> -	/* maximum range of a2xx mmu */
> -	if (adreno_is_a2xx(adreno_gpu))
> -		adreno_gpu_config.va_end = SZ_16M + 0xfff * SZ_64K;
> -
>   	adreno_gpu_config.nr_rings = nr_rings;
>   
>   	adreno_get_pwrlevels(&pdev->dev, gpu);
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 5e21d01..777f5fb 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -21,6 +21,7 @@
>   #include <linux/kref.h>
>   #include <linux/reservation.h>
>   #include "msm_drv.h"
> +#include "msm_mmu.h"
>   
>   /* Additional internal-use only BO flags: */
>   #define MSM_BO_STOLEN        0x10000000    /* try to use stolen/splash memory */
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 79b71b1..ec48bb3 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -20,7 +20,6 @@
>   #include "msm_mmu.h"
>   #include "msm_fence.h"
>   #include "msm_gpu_trace.h"
> -#include "adreno/adreno_gpu.h"
>   
>   #include <generated/utsrelease.h>
>   #include <linux/string_helpers.h>
> @@ -821,51 +820,6 @@ static int get_clocks(struct platform_device *pdev, struct msm_gpu *gpu)
>   	return 0;
>   }
>   
> -static struct msm_gem_address_space *
> -msm_gpu_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev,
> -		uint64_t va_start, uint64_t va_end)
> -{
> -	struct msm_gem_address_space *aspace;
> -	int ret;
> -
> -	/*
> -	 * Setup IOMMU.. eventually we will (I think) do this once per context
> -	 * and have separate page tables per context.  For now, to keep things
> -	 * simple and to get something working, just use a single address space:
> -	 */
> -	if (!adreno_is_a2xx(to_adreno_gpu(gpu))) {
> -		struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> -		if (!iommu)
> -			return NULL;
> -
> -		iommu->geometry.aperture_start = va_start;
> -		iommu->geometry.aperture_end = va_end;
> -
> -		DRM_DEV_INFO(gpu->dev->dev, "%s: using IOMMU\n", gpu->name);
> -
> -		aspace = msm_gem_address_space_create(&pdev->dev, iommu, "gpu");
> -		if (IS_ERR(aspace))
> -			iommu_domain_free(iommu);
> -	} else {
> -		aspace = msm_gem_address_space_create_a2xx(&pdev->dev, gpu, "gpu",
> -			va_start, va_end);
> -	}
> -
> -	if (IS_ERR(aspace)) {
> -		DRM_DEV_ERROR(gpu->dev->dev, "failed to init mmu: %ld\n",
> -			PTR_ERR(aspace));
> -		return ERR_CAST(aspace);
> -	}
> -
> -	ret = aspace->mmu->funcs->attach(aspace->mmu, NULL, 0);
> -	if (ret) {
> -		msm_gem_address_space_put(aspace);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return aspace;
> -}
> -
>   int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   		struct msm_gpu *gpu, const struct msm_gpu_funcs *funcs,
>   		const char *name, struct msm_gpu_config *config)
> @@ -938,12 +892,8 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   
>   	msm_devfreq_init(gpu);
>   
> -	gpu->aspace = msm_gpu_create_address_space(gpu, pdev,
> -		config->va_start, config->va_end);
> -
> -	if (gpu->aspace == NULL)
> -		DRM_DEV_INFO(drm->dev, "%s: no IOMMU, fallback to VRAM carveout!\n", name);
> -	else if (IS_ERR(gpu->aspace)) {
> +	gpu->aspace = gpu->funcs->create_address_space(gpu);
> +	if (IS_ERR(gpu->aspace)) {
>   		ret = PTR_ERR(gpu->aspace);
>   		goto fail;
>   	}
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index ca17086..81b9861 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -74,6 +74,8 @@ struct msm_gpu_funcs {
>   	int (*gpu_state_put)(struct msm_gpu_state *state);
>   	unsigned long (*gpu_get_freq)(struct msm_gpu *gpu);
>   	void (*gpu_set_freq)(struct msm_gpu *gpu, unsigned long freq);
> +	struct msm_gem_address_space *(*create_address_space)
> +		(struct msm_gpu *gpu);
>   };
>   
>   struct msm_gpu {
> 

Powered by blists - more mailing lists