* [PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate
@ 2019-05-29 9:18 Boris Brezillon
2019-05-31 12:54 ` Tomeu Vizoso
0 siblings, 1 reply; 3+ messages in thread
From: Boris Brezillon @ 2019-05-29 9:18 UTC (permalink / raw)
To: Rob Herring, Tomeu Vizoso; +Cc: dri-devel, Boris Brezillon, stable
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@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@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;
+
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;
+
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate
2019-05-29 9:18 [PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate Boris Brezillon
@ 2019-05-31 12:54 ` Tomeu Vizoso
2019-06-01 8:55 ` Boris Brezillon
0 siblings, 1 reply; 3+ messages in thread
From: Tomeu Vizoso @ 2019-05-31 12:54 UTC (permalink / raw)
To: Boris Brezillon; +Cc: Rob Herring, dri-devel, stable
On Wed, 29 May 2019 at 11:18, Boris Brezillon
<boris.brezillon@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@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate
2019-05-31 12:54 ` Tomeu Vizoso
@ 2019-06-01 8:55 ` Boris Brezillon
0 siblings, 0 replies; 3+ messages in thread
From: Boris Brezillon @ 2019-06-01 8:55 UTC (permalink / raw)
To: Tomeu Vizoso; +Cc: Rob Herring, dri-devel, stable
On Fri, 31 May 2019 14:54:54 +0200
Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> On Wed, 29 May 2019 at 11:18, Boris Brezillon
> <boris.brezillon@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@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@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.
Okay, will add WARN_ON()s and add a check in the caller.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-01 8:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-29 9:18 [PATCH] drm/panfrost: Make sure a BO is only unmapped when appropriate Boris Brezillon
2019-05-31 12:54 ` Tomeu Vizoso
2019-06-01 8:55 ` Boris Brezillon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.