Re: [PATCH 1/4] drm/rockchip: Do not use DMA mapping API if attached to IOMMU domain

[Tomasz Figa]

Hi Mark,

Thanks for reviving this series and sorry for not taking care of it
myself. Please see some comments inline.

On Tue, Feb 7, 2017 at 3:09 PM, Mark Yao <mark.yao@xxxxxxxxxxxxxx> wrote:
> From: Tomasz Figa <tfiga@xxxxxxxxxxxx>
>
> The API is not suitable for subsystems consisting of multiple devices
> and requires severe hacks to use it. To mitigate this, this patch
> implements allocation and address space management locally by using
> helpers provided by DRM framework, like other DRM drivers do, e.g.
> Tegra.
>
> This patch should not introduce any functional changes until the driver
> is made to attach subdevices into an IOMMU domain with the generic IOMMU
> API, which will happen in following patch. Based heavily on GEM
> implementation of Tegra DRM driver.
>
> Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> Signed-off-by: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx>
> Acked-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>

I believe you need your Signed-off-by here.

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   4 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 217 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.h |   8 +
>  3 files changed, 219 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index fb6226c..7c123d9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -30,6 +30,7 @@
>
>  struct drm_device;
>  struct drm_connector;
> +struct iommu_domain;
>
>  /*
>   * Rockchip drm private crtc funcs.
> @@ -60,7 +61,8 @@ struct rockchip_drm_private {
>         struct drm_gem_object *fbdev_bo;
>         const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
>         struct drm_atomic_state *state;
> -
> +       struct iommu_domain *domain;
> +       struct drm_mm mm;
>         struct list_head psr_list;
>         spinlock_t psr_list_lock;
>  };
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index b70f942..5209392 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -16,11 +16,135 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_vma_manager.h>
> +#include <linux/iommu.h>
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_gem.h"
>
> -static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
> +static int rockchip_gem_iommu_map(struct rockchip_gem_object *rk_obj)
> +{
> +       struct drm_device *drm = rk_obj->base.dev;
> +       struct rockchip_drm_private *private = drm->dev_private;
> +       int prot = IOMMU_READ | IOMMU_WRITE;
> +       ssize_t ret;
> +
> +       ret = drm_mm_insert_node_generic(&private->mm, &rk_obj->mm,
> +                                        rk_obj->base.size, PAGE_SIZE,
> +                                        0, 0);
> +       if (ret < 0) {
> +               DRM_ERROR("out of I/O virtual memory: %zd\n", ret);
> +               return ret;
> +       }
> +
> +       rk_obj->dma_addr = rk_obj->mm.start;
> +
> +       ret = iommu_map_sg(private->domain, rk_obj->dma_addr, rk_obj->sgt->sgl,
> +                          rk_obj->sgt->nents, prot);
> +       if (ret < 0) {
> +               DRM_ERROR("failed to map buffer: %zd\n", ret);
> +               goto err_remove_node;
> +       }
> +
> +       rk_obj->size = ret;
> +
> +       return 0;
> +
> +err_remove_node:
> +       drm_mm_remove_node(&rk_obj->mm);
> +
> +       return ret;
> +}
> +
> +static int rockchip_gem_iommu_unmap(struct rockchip_gem_object *rk_obj)
> +{
> +       struct drm_device *drm = rk_obj->base.dev;
> +       struct rockchip_drm_private *private = drm->dev_private;
> +
> +       iommu_unmap(private->domain, rk_obj->dma_addr, rk_obj->size);
> +       drm_mm_remove_node(&rk_obj->mm);
> +
> +       return 0;
> +}
> +
> +static int rockchip_gem_get_pages(struct rockchip_gem_object *rk_obj)
> +{
> +       struct drm_device *drm = rk_obj->base.dev;
> +       int ret, i;
> +       struct scatterlist *s;
> +
> +       rk_obj->pages = drm_gem_get_pages(&rk_obj->base);
> +       if (IS_ERR(rk_obj->pages))
> +               return PTR_ERR(rk_obj->pages);
> +
> +       rk_obj->num_pages = rk_obj->base.size >> PAGE_SHIFT;
> +
> +       rk_obj->sgt = drm_prime_pages_to_sg(rk_obj->pages, rk_obj->num_pages);
> +       if (IS_ERR(rk_obj->sgt)) {
> +               ret = PTR_ERR(rk_obj->sgt);
> +               goto err_put_pages;
> +       }
> +
> +       /*
> +        * Fake up the SG table so that dma_sync_sg_for_device() can be used
> +        * to flush the pages associated with it.
> +        *
> +        * TODO: Replace this by drm_clflush_sg() once it can be implemented
> +        * without relying on symbols that are not exported.
> +        */
> +       for_each_sg(rk_obj->sgt->sgl, s, rk_obj->sgt->nents, i)
> +               sg_dma_address(s) = sg_phys(s);
> +
> +       dma_sync_sg_for_device(drm->dev, rk_obj->sgt->sgl, rk_obj->sgt->nents,
> +                              DMA_TO_DEVICE);
> +
> +       return 0;
> +
> +err_put_pages:
> +       drm_gem_put_pages(&rk_obj->base, rk_obj->pages, false, false);
> +       return ret;
> +}
> +
> +static void rockchip_gem_put_pages(struct rockchip_gem_object *rk_obj)
> +{
> +       sg_free_table(rk_obj->sgt);
> +       kfree(rk_obj->sgt);
> +       drm_gem_put_pages(&rk_obj->base, rk_obj->pages, false, false);

We need to call drm_gem_put_pages with two last arguments true, not
false, because we don't have any way of tracking the accesses, so we
need to assume the page was both accessed and dirty. See
https://chromium-review.googlesource.com/382934.

> +}
> +
> +static int rockchip_gem_alloc_iommu(struct rockchip_gem_object *rk_obj,
> +                                   bool alloc_kmap)
> +{
> +       int ret;
> +
> +       ret = rockchip_gem_get_pages(rk_obj);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = rockchip_gem_iommu_map(rk_obj);
> +       if (ret < 0)
> +               goto err_free;
> +
> +       if (alloc_kmap) {
> +               rk_obj->kvaddr = vmap(rk_obj->pages, rk_obj->num_pages, VM_MAP,
> +                                     pgprot_writecombine(PAGE_KERNEL));
> +               if (!rk_obj->kvaddr) {
> +                       DRM_ERROR("failed to vmap() buffer\n");
> +                       ret = -ENOMEM;
> +                       goto err_unmap;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_unmap:
> +       rockchip_gem_iommu_unmap(rk_obj);
> +err_free:
> +       rockchip_gem_put_pages(rk_obj);
> +
> +       return ret;
> +}
> +
> +static int rockchip_gem_alloc_dma(struct rockchip_gem_object *rk_obj,
>                                   bool alloc_kmap)
>  {
>         struct drm_gem_object *obj = &rk_obj->base;
> @@ -42,7 +166,27 @@ static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
>         return 0;
>  }
>
> -static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
> +static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
> +                                 bool alloc_kmap)
> +{
> +       struct drm_gem_object *obj = &rk_obj->base;
> +       struct drm_device *drm = obj->dev;
> +       struct rockchip_drm_private *private = drm->dev_private;
> +
> +       if (private->domain)
> +               return rockchip_gem_alloc_iommu(rk_obj, alloc_kmap);
> +       else
> +               return rockchip_gem_alloc_dma(rk_obj, alloc_kmap);
> +}
> +
> +static void rockchip_gem_free_iommu(struct rockchip_gem_object *rk_obj)
> +{
> +       vunmap(rk_obj->kvaddr);
> +       rockchip_gem_iommu_unmap(rk_obj);
> +       rockchip_gem_put_pages(rk_obj);
> +}
> +
> +static void rockchip_gem_free_dma(struct rockchip_gem_object *rk_obj)
>  {
>         struct drm_gem_object *obj = &rk_obj->base;
>         struct drm_device *drm = obj->dev;
> @@ -51,23 +195,64 @@ static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
>                        rk_obj->dma_attrs);
>  }
>
> -static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
> -                                       struct vm_area_struct *vma)
> +static void rockchip_gem_free_buf(struct rockchip_gem_object *rk_obj)
> +{
> +       if (rk_obj->pages)
> +               rockchip_gem_free_iommu(rk_obj);
> +       else
> +               rockchip_gem_free_dma(rk_obj);
> +}
>
> +static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
> +                                             struct vm_area_struct *vma)
>  {
> +       struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
> +       unsigned int i, count = obj->size >> PAGE_SHIFT;
> +       unsigned long user_count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> +       unsigned long uaddr = vma->vm_start;
>         int ret;
> +
> +       if (user_count == 0 || user_count > count)
> +               return -ENXIO;
> +
> +       for (i = 0; i < user_count; i++) {

Even though DRM mmap doesn't support page offset, we need to respect
it here for PRIME/DMA-buf mmap. See
https://chromium-review.googlesource.com/386477.

> +               ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
> +               if (ret)
> +                       return ret;
> +               uaddr += PAGE_SIZE;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rockchip_drm_gem_object_mmap_dma(struct drm_gem_object *obj,
> +                                           struct vm_area_struct *vma)
> +{
>         struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
>         struct drm_device *drm = obj->dev;
>
> +       return dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
> +                             obj->size, rk_obj->dma_attrs);
> +}
> +
> +static int rockchip_drm_gem_object_mmap(struct drm_gem_object *obj,
> +                                       struct vm_area_struct *vma)
> +{
> +       int ret;
> +       struct rockchip_gem_object *rk_obj = to_rockchip_obj(obj);
> +
>         /*
> -        * dma_alloc_attrs() allocated a struct page table for rk_obj, so clear
> +        * We allocated a struct page table for rk_obj, so clear
>          * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
>          */
>         vma->vm_flags &= ~VM_PFNMAP;
>         vma->vm_pgoff = 0;
>
> -       ret = dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
> -                            obj->size, rk_obj->dma_attrs);
> +       if (rk_obj->pages)
> +               ret = rockchip_drm_gem_object_mmap_iommu(obj, vma);
> +       else
> +               ret = rockchip_drm_gem_object_mmap_dma(obj, vma);
> +
>         if (ret)
>                 drm_gem_vm_close(vma);
>
> @@ -117,7 +302,7 @@ struct rockchip_gem_object *
>
>         obj = &rk_obj->base;
>
> -       drm_gem_private_object_init(drm, obj, size);
> +       drm_gem_object_init(drm, obj, size);

In error path of this function and in all functions that destroy the
GEM we need to call drm_gem_object_release(), as it's the counterpart
to drm_gem_object_init(). See
https://chromium-review.googlesource.com/385456.

Best regards,
Tomasz