Re: [PATCH v2 6/8] drm/msm: Refactor address space initialization

From: Rob Clark
Date: Mon Dec 09 2019 - 15:14:23 EST

Next message: Nathan Chancellor: "[PATCH] xen/blkfront: Adjust indentation in xlvbd_alloc_gendisk"
Previous message: Sean Christopherson: "[PATCH] KVM: x86: Add a WARN on TIF_NEED_FPU_LOAD in kvm_load_guest_fpu()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Nov 22, 2019 at 3:32 PM Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> wrote:
>
> Refactor how address space initialization works. Instead of having the
> address space function create the MMU object (and thus require separate but
> equal functions for gpummu and iommu) use a single function and pass the
> MMU struct in. Make the generic code cleaner by using target specific
> functions to create the address space so a2xx can do its own thing in its
> own space. For all the other targets use a generic helper to initialize
> IOMMU but leave the door open for newer targets to use customization
> if they need it.
>
> Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>

Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>

> ---
>
> drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 16 ++++++++++
> drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 +
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 23 ++++++++++----
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 8 +++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +++---
> drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 14 +++++----
> drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 4 ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 +++++--
> drivers/gpu/drm/msm/msm_drv.h | 8 ++---
> drivers/gpu/drm/msm/msm_gem_vma.c | 52 +++++---------------------------
> drivers/gpu/drm/msm/msm_gpu.c | 40 ++----------------------
> drivers/gpu/drm/msm/msm_gpu.h | 4 +--
> drivers/gpu/drm/msm/msm_iommu.c | 3 ++
> 16 files changed, 83 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index 1f83bc1..60f6472 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -401,6 +401,21 @@ 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 platform_device *pdev)
> +{
> + struct msm_mmu *mmu = msm_gpummu_new(&pdev->dev, gpu);
> + struct msm_gem_address_space *aspace;
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
> + SZ_16M + 0xfff * SZ_64K);
> +
> + if (IS_ERR(aspace) && !IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> +
> + 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 +444,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,
> },
> };
>
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index 7ad1493..41e51e0 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -441,6 +441,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 = adreno_iommu_create_address_space,
> },
> };
>
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index b01388a..3655440 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -532,6 +532,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 = adreno_iommu_create_address_space,
> },
> .get_timestamp = a4xx_get_timestamp,
> };
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index b02e204..0f5db72 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1432,6 +1432,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 = adreno_iommu_create_address_space,
> },
> .get_timestamp = a5xx_get_timestamp,
> };
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index dc8ec2c..5dc0b2c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -832,6 +832,7 @@ static const struct adreno_gpu_funcs funcs = {
> #if defined(CONFIG_DRM_MSM_GPU_STATE)
> .gpu_state_get = a6xx_gpu_state_get,
> .gpu_state_put = a6xx_gpu_state_put,
> + .create_address_space = adreno_iommu_create_address_space,
> #endif
> },
> .get_timestamp = a6xx_get_timestamp,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 0783e4b..09c57891 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -157,6 +157,23 @@ int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
> return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
> }
>
> +struct msm_gem_address_space *
> +adreno_iommu_create_address_space(struct msm_gpu *gpu,
> + struct platform_device *pdev)
> +{
> + struct iommu_domain *iommu = iommu_domain_alloc(&platform_bus_type);
> + struct msm_mmu *mmu = msm_iommu_new(&pdev->dev, iommu);
> + struct msm_gem_address_space *aspace;
> +
> + aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
> + 0xfffffff);
> +
> + if (IS_ERR(aspace) && !IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> +
> + return aspace;
> +}
> +
> int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -949,12 +966,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>
> 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/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index e71a757..5c1aa12 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -263,6 +263,14 @@ int adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state);
> int adreno_gpu_state_put(struct msm_gpu_state *state);
>
> /*
> + * Common helper function to initialize the default address space for arm-smmu
> + * attached targets
> + */
> +struct msm_gem_address_space *
> +adreno_iommu_create_address_space(struct msm_gpu *gpu,
> + struct platform_device *pdev);
> +
> +/*
> * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
> * out of secure mode
> */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index b082b23..4e6ebbd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -704,18 +704,18 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
> {
> struct iommu_domain *domain;
> struct msm_gem_address_space *aspace;
> + struct msm_mmu *mmu;
>
> domain = iommu_domain_alloc(&platform_bus_type);
> if (!domain)
> return 0;
>
> - domain->geometry.aperture_start = 0x1000;
> - domain->geometry.aperture_end = 0xffffffff;
> + mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
> + aspace = msm_gem_address_space_create(mmu, "dpu1",
> + 0x1000, 0xfffffff);
>
> - aspace = msm_gem_address_space_create(dpu_kms->dev->dev,
> - domain, "dpu1");
> if (IS_ERR(aspace)) {
> - iommu_domain_free(domain);
> + mmu->funcs->destroy(mmu);
> return PTR_ERR(aspace);
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 9dba37c..0889718 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -510,9 +510,15 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->iommu) {
> - aspace = msm_gem_address_space_create(&pdev->dev,
> - config->iommu, "mdp4");
> + struct msm_mmu *mmu = msm_iommu_new(&pdev->dev,
> + config->iommu);
> +
> + aspace = msm_gem_address_space_create(mmu,
> + "mdp4", 0x1000, 0xffffffff);
> +
> if (IS_ERR(aspace)) {
> + if (!IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> ret = PTR_ERR(aspace);
> goto fail;
> }
> @@ -565,10 +571,6 @@ static struct mdp4_platform_config *mdp4_get_config(struct platform_device *dev)
> /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
> config.max_clk = 266667000;
> config.iommu = iommu_domain_alloc(&platform_bus_type);
> - if (config.iommu) {
> - config.iommu->geometry.aperture_start = 0x1000;
> - config.iommu->geometry.aperture_end = 0xffffffff;
> - }
>
> return &config;
> }
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> index 1f48f64..ebd651a 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
> @@ -941,10 +941,6 @@ static struct mdp5_cfg_platform *mdp5_get_config(struct platform_device *dev)
> static struct mdp5_cfg_platform config = {};
>
> config.iommu = iommu_domain_alloc(&platform_bus_type);
> - if (config.iommu) {
> - config.iommu->geometry.aperture_start = 0x1000;
> - config.iommu->geometry.aperture_end = 0xffffffff;
> - }
>
> return &config;
> }
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 653dab2..20bdff9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -724,13 +724,20 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
> mdelay(16);
>
> if (config->platform.iommu) {
> + struct msm_mmu *mmu;
> +
> iommu_dev = &pdev->dev;
> if (!iommu_dev->iommu_fwspec)
> iommu_dev = iommu_dev->parent;
>
> - aspace = msm_gem_address_space_create(iommu_dev,
> - config->platform.iommu, "mdp5");
> + mmu = msm_iommu_new(iommu_dev, config->platform.iommu);
> +
> + aspace = msm_gem_address_space_create(mmu, "mdp5",
> + 0x1000, 0xffffffff);
> +
> if (IS_ERR(aspace)) {
> + if (!IS_ERR(mmu))
> + mmu->funcs->destroy(mmu);
> ret = PTR_ERR(aspace);
> goto fail;
> }
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 71547e7..2203729 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -247,12 +247,8 @@ void msm_gem_close_vma(struct msm_gem_address_space *aspace,
> void msm_gem_address_space_put(struct msm_gem_address_space *aspace);
>
> struct msm_gem_address_space *
> -msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
> - const char *name);
> -
> -struct msm_gem_address_space *
> -msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
> - const char *name, uint64_t va_start, uint64_t va_end);
> +msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
> + u64 va_start, u64 va_end);
>
> int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
> void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 91d993a..075ce52 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -125,63 +125,25 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
> return 0;
> }
>
> -
> struct msm_gem_address_space *
> -msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain,
> - const char *name)
> -{
> - struct msm_gem_address_space *aspace;
> - u64 start = domain->geometry.aperture_start;
> - u64 size = domain->geometry.aperture_end - start;
> -
> - aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
> - if (!aspace)
> - return ERR_PTR(-ENOMEM);
> -
> - spin_lock_init(&aspace->lock);
> - aspace->name = name;
> - aspace->mmu = msm_iommu_new(dev, domain);
> - if (IS_ERR(aspace->mmu)) {
> - int ret = PTR_ERR(aspace->mmu);
> -
> - kfree(aspace);
> - return ERR_PTR(ret);
> - }
> -
> - /*
> - * Attaching the IOMMU device changes the aperture values so use the
> - * cached values instead
> - */
> - drm_mm_init(&aspace->mm, start >> PAGE_SHIFT, size >> PAGE_SHIFT);
> -
> - kref_init(&aspace->kref);
> -
> - return aspace;
> -}
> -
> -struct msm_gem_address_space *
> -msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu,
> - const char *name, uint64_t va_start, uint64_t va_end)
> +msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
> + u64 va_start, u64 va_end)
> {
> struct msm_gem_address_space *aspace;
> u64 size = va_end - va_start;
>
> + if (IS_ERR(mmu))
> + return ERR_CAST(mmu);
> +
> aspace = kzalloc(sizeof(*aspace), GFP_KERNEL);
> if (!aspace)
> return ERR_PTR(-ENOMEM);
>
> spin_lock_init(&aspace->lock);
> aspace->name = name;
> - aspace->mmu = msm_gpummu_new(dev, gpu);
> - if (IS_ERR(aspace->mmu)) {
> - int ret = PTR_ERR(aspace->mmu);
> -
> - kfree(aspace);
> - return ERR_PTR(ret);
> - }
> + aspace->mmu = mmu;
>
> - drm_mm_init(&aspace->mm, (va_start >> PAGE_SHIFT),
> - size >> PAGE_SHIFT);
> + drm_mm_init(&aspace->mm, va_start >> PAGE_SHIFT, size >> PAGE_SHIFT);
>
> kref_init(&aspace->kref);
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index f7bf80e..f11df53 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -803,42 +803,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;
> -
> - /*
> - * 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 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)
> @@ -911,8 +875,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);
> +
> + gpu->aspace = gpu->funcs->create_address_space(gpu, pdev);
>
> if (gpu->aspace == NULL)
> DRM_DEV_INFO(drm->dev, "%s: no IOMMU, fallback to VRAM carveout!\n", name);
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index ab8f0f9c..41d86c2 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -21,8 +21,6 @@ struct msm_gpu_state;
>
> struct msm_gpu_config {
> const char *ioname;
> - uint64_t va_start;
> - uint64_t va_end;
> unsigned int nr_rings;
> };
>
> @@ -64,6 +62,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 platform_device *pdev);
> };
>
> struct msm_gpu {
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 544c519..e773ef8 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -70,6 +70,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain)
> struct msm_iommu *iommu;
> int ret;
>
> + if (!domain)
> + return ERR_PTR(-ENODEV);
> +
> iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> if (!iommu)
> return ERR_PTR(-ENOMEM);
> --
> 2.7.4
>

Next message: Nathan Chancellor: "[PATCH] xen/blkfront: Adjust indentation in xlvbd_alloc_gendisk"
Previous message: Sean Christopherson: "[PATCH] KVM: x86: Add a WARN on TIF_NEED_FPU_LOAD in kvm_load_guest_fpu()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]