On Wed, Oct 01, 2014 at 11:54:11AM -0400, Sean Paul wrote: > On Tue, Sep 30, 2014 at 2:48 PM, Sean Paul wrote: > > On Thu, Jun 26, 2014 at 4:49 PM, Thierry Reding > > wrote: > >> From: Thierry Reding > >> > >> When an IOMMU device is available on the platform bus, allocate an IOMMU > >> domain and attach the display controllers to it. The display controllers > >> can then scan out non-contiguous buffers by mapping them through the > >> IOMMU. > >> > > > > Hi Thierry, > > A few comments from Stéphane and myself that came up while we were > > reviewing this for our tree. > > > >> Signed-off-by: Thierry Reding > >> --- > >> drivers/gpu/drm/tegra/dc.c | 21 ++++ > >> drivers/gpu/drm/tegra/drm.c | 17 ++++ > >> drivers/gpu/drm/tegra/drm.h | 3 + > >> drivers/gpu/drm/tegra/fb.c | 16 ++- > >> drivers/gpu/drm/tegra/gem.c | 236 +++++++++++++++++++++++++++++++++++++++----- > >> drivers/gpu/drm/tegra/gem.h | 4 + > >> 6 files changed, 273 insertions(+), 24 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > >> index afcca04f5367..0f7452d04811 100644 > >> --- a/drivers/gpu/drm/tegra/dc.c > >> +++ b/drivers/gpu/drm/tegra/dc.c > >> @@ -9,6 +9,7 @@ > >> > >> #include > >> #include > >> +#include > >> #include > >> > >> #include "dc.h" > >> @@ -1283,8 +1284,18 @@ static int tegra_dc_init(struct host1x_client *client) > >> { > >> struct drm_device *drm = dev_get_drvdata(client->parent); > >> struct tegra_dc *dc = host1x_client_to_dc(client); > >> + struct tegra_drm *tegra = drm->dev_private; > >> int err; > >> > >> + if (tegra->domain) { > >> + err = iommu_attach_device(tegra->domain, dc->dev); > >> + if (err < 0) { > >> + dev_err(dc->dev, "failed to attach to IOMMU: %d\n", > >> + err); > >> + return err; > >> + } > > > > [from Stéphane] > > > > shouldn't we call detach in the error paths below? > > > > > >> + } > >> + > >> drm_crtc_init(drm, &dc->base, &tegra_crtc_funcs); > >> drm_mode_crtc_set_gamma_size(&dc->base, 256); > >> drm_crtc_helper_add(&dc->base, &tegra_crtc_helper_funcs); > >> @@ -1318,7 +1329,9 @@ static int tegra_dc_init(struct host1x_client *client) > >> > >> static int tegra_dc_exit(struct host1x_client *client) > >> { > >> + struct drm_device *drm = dev_get_drvdata(client->parent); > >> struct tegra_dc *dc = host1x_client_to_dc(client); > >> + struct tegra_drm *tegra = drm->dev_private; > >> int err; > >> > >> devm_free_irq(dc->dev, dc->irq, dc); > >> @@ -1335,6 +1348,8 @@ static int tegra_dc_exit(struct host1x_client *client) > >> return err; > >> } > >> > >> + iommu_detach_device(tegra->domain, dc->dev); > >> + > >> return 0; > >> } > >> > >> @@ -1462,6 +1477,12 @@ static int tegra_dc_probe(struct platform_device *pdev) > >> return -ENXIO; > >> } > >> > >> + err = iommu_attach(&pdev->dev); > >> + if (err < 0) { > >> + dev_err(&pdev->dev, "failed to attach to IOMMU: %d\n", err); > >> + return err; > >> + } > >> + > >> INIT_LIST_HEAD(&dc->client.list); > >> dc->client.ops = &dc_client_ops; > >> dc->client.dev = &pdev->dev; > >> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > >> index 59736bb810cd..1d2bbafad982 100644 > >> --- a/drivers/gpu/drm/tegra/drm.c > >> +++ b/drivers/gpu/drm/tegra/drm.c > >> @@ -8,6 +8,7 @@ > >> */ > >> > >> #include > >> +#include > >> > >> #include "drm.h" > >> #include "gem.h" > >> @@ -33,6 +34,16 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags) > >> if (!tegra) > >> return -ENOMEM; > >> > >> + if (iommu_present(&platform_bus_type)) { > >> + tegra->domain = iommu_domain_alloc(&platform_bus_type); > >> + if (IS_ERR(tegra->domain)) { > >> + kfree(tegra); > >> + return PTR_ERR(tegra->domain); > >> + } > >> + > >> + drm_mm_init(&tegra->mm, 0, SZ_2G); > > > > > > [from Stéphane]: > > > > none of these are freed in the error path below (iommu_domain_free and > > drm_mm_takedown) > > > > also |tegra| isn't freed either? > > > > > > > >> + } > >> + > >> mutex_init(&tegra->clients_lock); > >> INIT_LIST_HEAD(&tegra->clients); > >> drm->dev_private = tegra; > >> @@ -71,6 +82,7 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags) > >> static int tegra_drm_unload(struct drm_device *drm) > >> { > >> struct host1x_device *device = to_host1x_device(drm->dev); > >> + struct tegra_drm *tegra = drm->dev_private; > >> int err; > >> > >> drm_kms_helper_poll_fini(drm); > >> @@ -82,6 +94,11 @@ static int tegra_drm_unload(struct drm_device *drm) > >> if (err < 0) > >> return err; > >> > >> + if (tegra->domain) { > >> + iommu_domain_free(tegra->domain); > >> + drm_mm_takedown(&tegra->mm); > >> + } > >> + > >> return 0; > >> } > >> > >> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h > >> index 96d754e7b3eb..a07c796b7edc 100644 > >> --- a/drivers/gpu/drm/tegra/drm.h > >> +++ b/drivers/gpu/drm/tegra/drm.h > >> @@ -39,6 +39,9 @@ struct tegra_fbdev { > >> struct tegra_drm { > >> struct drm_device *drm; > >> > >> + struct iommu_domain *domain; > >> + struct drm_mm mm; > >> + > >> struct mutex clients_lock; > >> struct list_head clients; > >> > >> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c > >> index 7790d43ad082..21c65dd817c3 100644 > >> --- a/drivers/gpu/drm/tegra/fb.c > >> +++ b/drivers/gpu/drm/tegra/fb.c > >> @@ -65,8 +65,12 @@ static void tegra_fb_destroy(struct drm_framebuffer *framebuffer) > >> for (i = 0; i < fb->num_planes; i++) { > >> struct tegra_bo *bo = fb->planes[i]; > >> > >> - if (bo) > >> + if (bo) { > >> + if (bo->pages && bo->virt) > >> + vunmap(bo->virt); > >> + > >> drm_gem_object_unreference_unlocked(&bo->gem); > >> + } > >> } > >> > >> drm_framebuffer_cleanup(framebuffer); > >> @@ -252,6 +256,16 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, > >> offset = info->var.xoffset * bytes_per_pixel + > >> info->var.yoffset * fb->pitches[0]; > >> > >> + if (bo->pages) { > >> + bo->vaddr = vmap(bo->pages, bo->num_pages, VM_MAP, > >> + pgprot_writecombine(PAGE_KERNEL)); > >> + if (!bo->vaddr) { > >> + dev_err(drm->dev, "failed to vmap() framebuffer\n"); > >> + err = -ENOMEM; > >> + goto destroy; > >> + } > >> + } > >> + > >> drm->mode_config.fb_base = (resource_size_t)bo->paddr; > >> info->screen_base = (void __iomem *)bo->vaddr + offset; > >> info->screen_size = size; > >> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c > >> index c1e4e8b6e5ca..2912e61a2599 100644 > >> --- a/drivers/gpu/drm/tegra/gem.c > >> +++ b/drivers/gpu/drm/tegra/gem.c > >> @@ -14,8 +14,10 @@ > >> */ > >> > >> #include > >> +#include > >> #include > >> > >> +#include "drm.h" > >> #include "gem.h" > >> > >> static inline struct tegra_bo *host1x_to_tegra_bo(struct host1x_bo *bo) > >> @@ -90,14 +92,144 @@ static const struct host1x_bo_ops tegra_bo_ops = { > >> .kunmap = tegra_bo_kunmap, > >> }; > >> > >> +static int iommu_map_sg(struct iommu_domain *domain, struct sg_table *sgt, > >> + dma_addr_t iova, int prot) > >> +{ > >> + unsigned long offset = 0; > >> + struct scatterlist *sg; > >> + unsigned int i, j; > >> + int err; > >> + > >> + for_each_sg(sgt->sgl, sg, sgt->nents, i) { > >> + dma_addr_t phys = sg_phys(sg); > >> + size_t length = sg->offset; > >> + > >> + phys = sg_phys(sg) - sg->offset; > >> + length = sg->length + sg->offset; > >> + > >> + err = iommu_map(domain, iova + offset, phys, length, prot); > >> + if (err < 0) > >> + goto unmap; > >> + > >> + offset += length; > >> + } > >> + > >> + return 0; > >> + > >> +unmap: > >> + offset = 0; > >> + > >> + for_each_sg(sgt->sgl, sg, i, j) { > >> + size_t length = sg->length + sg->offset; > >> + iommu_unmap(domain, iova + offset, length); > >> + offset += length; > >> + } > >> + > >> + return err; > >> +} > >> + > >> +static int iommu_unmap_sg(struct iommu_domain *domain, struct sg_table *sgt, > >> + dma_addr_t iova) > >> +{ > >> + unsigned long offset = 0; > >> + struct scatterlist *sg; > >> + unsigned int i; > >> + > >> + for_each_sg(sgt->sgl, sg, sgt->nents, i) { > >> + dma_addr_t phys = sg_phys(sg); > >> + size_t length = sg->offset; > >> + > >> + phys = sg_phys(sg) - sg->offset; > >> + length = sg->length + sg->offset; > >> + > >> + iommu_unmap(domain, iova + offset, length); > >> + offset += length; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int tegra_bo_iommu_map(struct tegra_drm *tegra, struct tegra_bo *bo) > >> +{ > >> + int prot = IOMMU_READ | IOMMU_WRITE; > >> + int err; > >> + > >> + if (bo->mm) > >> + return -EBUSY; > >> + > >> + bo->mm = kzalloc(sizeof(*bo->mm), GFP_KERNEL); > >> + if (!bo->mm) > >> + return -ENOMEM; > >> + > >> + err = drm_mm_insert_node_generic(&tegra->mm, bo->mm, bo->gem.size, > >> + PAGE_SIZE, 0, 0, 0); > >> + if (err < 0) { > >> + dev_err(tegra->drm->dev, "out of virtual memory: %d\n", err); > >> + return err; > >> + } > >> + > >> + bo->paddr = bo->mm->start; > >> + > >> + err = iommu_map_sg(tegra->domain, bo->sgt, bo->paddr, prot); > >> + if (err < 0) { > >> + dev_err(tegra->drm->dev, "failed to map buffer: %d\n", err); > >> + return err; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int tegra_bo_iommu_unmap(struct tegra_drm *tegra, struct tegra_bo *bo) > >> +{ > >> + if (!bo->mm) > >> + return 0; > >> + > >> + iommu_unmap_sg(tegra->domain, bo->sgt, bo->paddr); > >> + drm_mm_remove_node(bo->mm); > >> + > >> + kfree(bo->mm); > >> + return 0; > >> +} > >> + > >> static void tegra_bo_destroy(struct drm_device *drm, struct tegra_bo *bo) > >> { > >> - dma_free_writecombine(drm->dev, bo->gem.size, bo->vaddr, bo->paddr); > >> + if (!bo->pages) > >> + dma_free_writecombine(drm->dev, bo->gem.size, bo->vaddr, > >> + bo->paddr); > > One more thing. If tegra_bo_alloc fails, we'll have bo->vaddr == NULL > and bo->paddr == ~0 here, which causes a crash. > > I posted https://lkml.org/lkml/2014/9/30/659 to check for the error > condition in the mm code, but it seems like reviewer consensus is to > check for this before calling free. > > As such, we'll need to make sure bo->vaddr != NULL before calling > dma_free_writecombine to avoid this situation. > > Would you prefer I send a patch up to fix this separately, or would > you like to roll this into your next version? Thanks for pointing all of these out. I'm going to trace the failure code path anyway since there seem to be a couple of loose ends here and there, so I'll probably roll in a fix for this anyway. Thierry