[PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate

[Tomeu Vizoso]

On Wed, 29 May 2019 at 11:18, Boris Brezillon
<boris.brezillon at collabora.com> wrote:
>
> mmu_ops->unmap() will fail when called on a BO that has not been
> previously mapped, and the error path in panfrost_ioctl_create_bo()
> can call drm_gem_object_put_unlocked() (which in turn calls
> panfrost_mmu_unmap()) on a BO that has not been mapped yet.
>
> Keep track of the mapped/unmapped state to avoid such issues.
>
> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.h | 1 +
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 ++++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 045000eb5fcf..6dbcaba020fc 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -11,6 +11,7 @@ struct panfrost_gem_object {
>         struct drm_gem_shmem_object base;
>
>         struct drm_mm_node node;
> +       bool is_mapped;
>  };
>
>  static inline
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 762b1bd2a8c2..fb556aa89203 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -156,6 +156,9 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>         struct sg_table *sgt;
>         int ret;
>
> +       if (bo->is_mapped)
> +               return 0;

In what circumstances we want to silently go on? I would expect that
for this function to be called when the BO has been mapped already
would mean that we have a bug in the kernel, so why not a WARN?

> +
>         sgt = drm_gem_shmem_get_pages_sgt(obj);
>         if (WARN_ON(IS_ERR(sgt)))
>                 return PTR_ERR(sgt);
> @@ -189,6 +192,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
>
>         pm_runtime_mark_last_busy(pfdev->dev);
>         pm_runtime_put_autosuspend(pfdev->dev);
> +       bo->is_mapped = true;
>
>         return 0;
>  }
> @@ -203,6 +207,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>         size_t unmapped_len = 0;
>         int ret;
>
> +       if (!bo->is_mapped)
> +               return;

Similarly, I think that what we should do is not to call
panfrost_mmu_unmap when a BO is freed if we know it isn't mapped. And
probably add a WARN here if it still gets called when the BO isn't
mapped.

> +
>         dev_dbg(pfdev->dev, "unmap: iova=%llx, len=%zx", iova, len);
>
>         ret = pm_runtime_get_sync(pfdev->dev);
> @@ -230,6 +237,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>
>         pm_runtime_mark_last_busy(pfdev->dev);
>         pm_runtime_put_autosuspend(pfdev->dev);
> +       bo->is_mapped = false;
>  }
>
>  static void mmu_tlb_inv_context_s1(void *cookie)
> --
> 2.20.1
>

Thanks for taking care of this!

Tomeu