Re: [Freedreno] [PATCH v2 5/8] drm/msm: Attach the IOMMU device during initialization
On Fri, Nov 22, 2019 at 3:32 PM Jordan Crouse wrote: > > Everywhere an IOMMU object is created by msm_gpu_create_address_space > the IOMMU device is attached immediately after. Instead of carrying around > the infrastructure to do the attach from the device specific code do it > directly in the msm_iommu_init() function. This gets it out of the way for > more aggressive cleanups that follow. > > Signed-off-by: Jordan Crouse Hmm, yeah, now that we dropped the extra mmu->attach() args (which was some ancient downstream compat thing), we don't need the separate attach step. I suppose we probably should do a similar cleanup for mmu->detach(), but I guess that can be it's own patch Reviewed-by: Rob Clark > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 4 > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 7 --- > drivers/gpu/drm/msm/msm_gem_vma.c| 23 +++ > drivers/gpu/drm/msm/msm_gpu.c| 11 +-- > drivers/gpu/drm/msm/msm_gpummu.c | 6 -- > drivers/gpu/drm/msm/msm_iommu.c | 15 +++ > drivers/gpu/drm/msm/msm_mmu.h| 1 - > 8 files changed, 27 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 6c92f0f..b082b23 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -704,7 +704,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms) > { > struct iommu_domain *domain; > struct msm_gem_address_space *aspace; > - int ret; > > domain = iommu_domain_alloc(&platform_bus_type); > if (!domain) > @@ -720,13 +719,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms) > return PTR_ERR(aspace); > } > > - ret = aspace->mmu->funcs->attach(aspace->mmu); > - if (ret) { > - DPU_ERROR("failed to attach iommu %d\n", ret); > - msm_gem_address_space_put(aspace); > - return ret; > - } > - > dpu_kms->base.aspace = aspace; > return 0; > } > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > index dda0543..9dba37c 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > @@ -518,10 +518,6 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev) > } > > kms->aspace = aspace; > - > - ret = aspace->mmu->funcs->attach(aspace->mmu); > - if (ret) > - goto fail; > } else { > DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys " > "contig buffers for scanout\n"); > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index e43ecd4..653dab2 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -736,13 +736,6 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev) > } > > kms->aspace = aspace; > - > - ret = aspace->mmu->funcs->attach(aspace->mmu); > - if (ret) { > - DRM_DEV_ERROR(&pdev->dev, "failed to attach iommu: > %d\n", > - ret); > - goto fail; > - } > } else { > DRM_DEV_INFO(&pdev->dev, > "no iommu, fallback to phys contig buffers for > scanout\n"); > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c > b/drivers/gpu/drm/msm/msm_gem_vma.c > index 1af5354..91d993a 100644 > --- a/drivers/gpu/drm/msm/msm_gem_vma.c > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c > @@ -131,8 +131,8 @@ msm_gem_address_space_create(struct device *dev, struct > iommu_domain *domain, > const char *name) > { > struct msm_gem_address_space *aspace; > - u64 size = domain->geometry.aperture_end - > - domain->geometry.aperture_start; > + u64 start = domain->geometry.aperture_start; > + u64 size = domain->geometry.aperture_end - start; > > aspace = kzalloc(sizeof(*aspace), GFP_KERNEL); > if (!aspace) > @@ -141,9 +141,18 @@ msm_gem_address_space_create(struct device *dev, struct > iommu_domain *domain, > 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); > > - drm_mm_init(&aspace->mm, (domain->geometry.aperture_start >> > PAGE_SHIFT), > - size >> PAGE_SHIFT); > + kfree(aspace); > + return ERR_PTR(ret); > + } > + > + /* > +* Attaching the IOMMU device changes the aperture values so use the > +* cach
[Freedreno] [PATCH v2 5/8] drm/msm: Attach the IOMMU device during initialization
Everywhere an IOMMU object is created by msm_gpu_create_address_space the IOMMU device is attached immediately after. Instead of carrying around the infrastructure to do the attach from the device specific code do it directly in the msm_iommu_init() function. This gets it out of the way for more aggressive cleanups that follow. Signed-off-by: Jordan Crouse --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 4 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 7 --- drivers/gpu/drm/msm/msm_gem_vma.c| 23 +++ drivers/gpu/drm/msm/msm_gpu.c| 11 +-- drivers/gpu/drm/msm/msm_gpummu.c | 6 -- drivers/gpu/drm/msm/msm_iommu.c | 15 +++ drivers/gpu/drm/msm/msm_mmu.h| 1 - 8 files changed, 27 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 6c92f0f..b082b23 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -704,7 +704,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms) { struct iommu_domain *domain; struct msm_gem_address_space *aspace; - int ret; domain = iommu_domain_alloc(&platform_bus_type); if (!domain) @@ -720,13 +719,6 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms) return PTR_ERR(aspace); } - ret = aspace->mmu->funcs->attach(aspace->mmu); - if (ret) { - DPU_ERROR("failed to attach iommu %d\n", ret); - msm_gem_address_space_put(aspace); - return ret; - } - dpu_kms->base.aspace = aspace; return 0; } diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c index dda0543..9dba37c 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c @@ -518,10 +518,6 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev) } kms->aspace = aspace; - - ret = aspace->mmu->funcs->attach(aspace->mmu); - if (ret) - goto fail; } else { DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys " "contig buffers for scanout\n"); diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index e43ecd4..653dab2 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -736,13 +736,6 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev) } kms->aspace = aspace; - - ret = aspace->mmu->funcs->attach(aspace->mmu); - if (ret) { - DRM_DEV_ERROR(&pdev->dev, "failed to attach iommu: %d\n", - ret); - goto fail; - } } else { DRM_DEV_INFO(&pdev->dev, "no iommu, fallback to phys contig buffers for scanout\n"); diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index 1af5354..91d993a 100644 --- a/drivers/gpu/drm/msm/msm_gem_vma.c +++ b/drivers/gpu/drm/msm/msm_gem_vma.c @@ -131,8 +131,8 @@ msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain, const char *name) { struct msm_gem_address_space *aspace; - u64 size = domain->geometry.aperture_end - - domain->geometry.aperture_start; + u64 start = domain->geometry.aperture_start; + u64 size = domain->geometry.aperture_end - start; aspace = kzalloc(sizeof(*aspace), GFP_KERNEL); if (!aspace) @@ -141,9 +141,18 @@ msm_gem_address_space_create(struct device *dev, struct iommu_domain *domain, 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); - drm_mm_init(&aspace->mm, (domain->geometry.aperture_start >> PAGE_SHIFT), - size >> PAGE_SHIFT); + 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); @@ -164,6 +173,12 @@ msm_gem_address_space_create_a2xx(struct device *dev, struct msm_gpu *gpu, 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); + } drm_mm_init(&aspace->mm, (va_start