[PATCH] drm/i915: avoid leaking DMA mappings

[PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

We have 3 types of DMA mappings for GEM objects:
1. physically contiguous for stolen and for objects needing contiguous
   memory
2. DMA-buf mappings imported via a DMA-buf attach operation
3. SG DMA mappings for shmem backed and userptr objects

For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
corresponding backing pages and so in practice we create/release the
mapping in the object's get_pages/put_pages callback.

For 3. the lifetime of the mapping matches that of any existing GPU binding
of the object, so we'll create the mapping when the object is bound to
the first vma and release the mapping when the object is unbound from its
last vma.

Since the object can be bound to multiple vmas, we can end up creating a
new DMA mapping in the 3. case even if the object already had one. This
is not allowed by the DMA API and can lead to leaked mapping data and
IOMMU memory space starvation in certain cases. For example HW IOMMU
drivers (intel_iommu) allocate a new range from their memory space
whenever a mapping is created, silently overriding a pre-existing
mapping.

Fix this by adding new callbacks to create/release the DMA mapping. This
way we can use the has_dma_mapping flag for objects of the 3. case also
(so far the flag was only used for the 1. and 2. case) and skip creating
a new mapping if one exists already.

Note that I also thought about simply creating/releasing the mapping
when get_pages/put_pages is called. However since creating a DMA mapping
may have associated resources (at least in case of HW IOMMU) it does
make sense to release these resources as early as possible. We can
release the DMA mapping as soon as the object is unbound from the last
vma, before we drop the backing pages, hence it's worth keeping the two
operations separate.

I noticed this issue by enabling DMA debugging, which got disabled after
a while due to its internal mapping tables getting full. It also reported
errors in connection to random other drivers that did a DMA mapping for
an address that was previously mapped by i915 but was never released.
Besides these diagnostic messages and the memory space starvation
problem for IOMMUs, I'm not aware of this causing a real issue.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 ++
 drivers/gpu/drm/i915/i915_gem.c     | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 15 ++++-----------
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1dbd957..64fd3f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1961,6 +1961,8 @@ struct drm_i915_gem_object_ops {
 	 */
 	int (*get_pages)(struct drm_i915_gem_object *);
 	void (*put_pages)(struct drm_i915_gem_object *);
+	int (*get_dma_mapping)(struct drm_i915_gem_object *);
+	void (*put_dma_mapping)(struct drm_i915_gem_object *);
 	int (*dmabuf_export)(struct drm_i915_gem_object *);
 	void (*release)(struct drm_i915_gem_object *);
 };
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e4d31fc..fe7020c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2349,6 +2349,30 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+static int i915_gem_object_get_dma_mapping_gtt(struct drm_i915_gem_object *obj)
+{
+	if (obj->has_dma_mapping)
+		return 0;
+
+	if (!dma_map_sg(&obj->base.dev->pdev->dev, obj->pages->sgl,
+			 obj->pages->nents, PCI_DMA_BIDIRECTIONAL))
+		return -ENOSPC;
+
+	obj->has_dma_mapping = true;
+
+	return 0;
+}
+
+static void i915_gem_object_put_dma_mapping_gtt(struct drm_i915_gem_object *obj)
+{
+	WARN_ON_ONCE(!obj->has_dma_mapping);
+
+	dma_unmap_sg(&obj->base.dev->pdev->dev, obj->pages->sgl,
+		     obj->pages->nents, PCI_DMA_BIDIRECTIONAL);
+
+	obj->has_dma_mapping = false;
+}
+
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct drm_i915_gem_request *req)
 {
@@ -4635,6 +4659,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.get_pages = i915_gem_object_get_pages_gtt,
 	.put_pages = i915_gem_object_put_pages_gtt,
+	.get_dma_mapping = i915_gem_object_get_dma_mapping_gtt,
+	.put_dma_mapping = i915_gem_object_put_dma_mapping_gtt,
 };
 
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b29b73f..56bc611 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1802,13 +1802,8 @@ void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
 
 int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 {
-	if (obj->has_dma_mapping)
-		return 0;
-
-	if (!dma_map_sg(&obj->base.dev->pdev->dev,
-			obj->pages->sgl, obj->pages->nents,
-			PCI_DMA_BIDIRECTIONAL))
-		return -ENOSPC;
+	if (obj->ops->get_dma_mapping)
+		return obj->ops->get_dma_mapping(obj);
 
 	return 0;
 }
@@ -2052,10 +2047,8 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
 
 	interruptible = do_idling(dev_priv);
 
-	if (!obj->has_dma_mapping)
-		dma_unmap_sg(&dev->pdev->dev,
-			     obj->pages->sgl, obj->pages->nents,
-			     PCI_DMA_BIDIRECTIONAL);
+	if (obj->ops->put_dma_mapping)
+		obj->ops->put_dma_mapping(obj);
 
 	undo_idling(dev_priv, interruptible);
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> We have 3 types of DMA mappings for GEM objects:
> 1. physically contiguous for stolen and for objects needing contiguous
>    memory
> 2. DMA-buf mappings imported via a DMA-buf attach operation
> 3. SG DMA mappings for shmem backed and userptr objects
> 
> For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> corresponding backing pages and so in practice we create/release the
> mapping in the object's get_pages/put_pages callback.
> 
> For 3. the lifetime of the mapping matches that of any existing GPU binding
> of the object, so we'll create the mapping when the object is bound to
> the first vma and release the mapping when the object is unbound from its
> last vma.
> 
> Since the object can be bound to multiple vmas, we can end up creating a
> new DMA mapping in the 3. case even if the object already had one. This
> is not allowed by the DMA API and can lead to leaked mapping data and
> IOMMU memory space starvation in certain cases. For example HW IOMMU
> drivers (intel_iommu) allocate a new range from their memory space
> whenever a mapping is created, silently overriding a pre-existing
> mapping.
> 
> Fix this by adding new callbacks to create/release the DMA mapping. This
> way we can use the has_dma_mapping flag for objects of the 3. case also
> (so far the flag was only used for the 1. and 2. case) and skip creating
> a new mapping if one exists already.
> 
> Note that I also thought about simply creating/releasing the mapping
> when get_pages/put_pages is called. However since creating a DMA mapping
> may have associated resources (at least in case of HW IOMMU) it does
> make sense to release these resources as early as possible. We can
> release the DMA mapping as soon as the object is unbound from the last
> vma, before we drop the backing pages, hence it's worth keeping the two
> operations separate.
> 
> I noticed this issue by enabling DMA debugging, which got disabled after
> a while due to its internal mapping tables getting full. It also reported
> errors in connection to random other drivers that did a DMA mapping for
> an address that was previously mapped by i915 but was never released.
> Besides these diagnostic messages and the memory space starvation
> problem for IOMMUs, I'm not aware of this causing a real issue.

Nope, it is much much simpler. Since we only do the dma prepare/finish
from inside get_pages/put_pages, we can put the calls there. The only
caveat there is userptr worker, but that can be easily fixed up.

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=f55727d7d6f76aeee687c1f2d31411662ff03b6f

Nak.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Tvrtko Ursulin]

Hi,

On 07/06/2015 03:50 PM, Imre Deak wrote:
> We have 3 types of DMA mappings for GEM objects:
> 1. physically contiguous for stolen and for objects needing contiguous
>     memory
> 2. DMA-buf mappings imported via a DMA-buf attach operation
> 3. SG DMA mappings for shmem backed and userptr objects
>
> For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> corresponding backing pages and so in practice we create/release the
> mapping in the object's get_pages/put_pages callback.
>
> For 3. the lifetime of the mapping matches that of any existing GPU binding
> of the object, so we'll create the mapping when the object is bound to
> the first vma and release the mapping when the object is unbound from its
> last vma.
>
> Since the object can be bound to multiple vmas, we can end up creating a
> new DMA mapping in the 3. case even if the object already had one. This
> is not allowed by the DMA API and can lead to leaked mapping data and
> IOMMU memory space starvation in certain cases. For example HW IOMMU
> drivers (intel_iommu) allocate a new range from their memory space
> whenever a mapping is created, silently overriding a pre-existing
> mapping.

Ha.. back when I was adding multiple GGTT views I had this implemented 
by only calling i915_gem_gtt_prepare_object on first VMA being 
instantiated, and the same but opposite for last one going away. Someone 
told me it is not needed though and to rip it out. :) To be fair I had 
no clue so got it right just by being defensive.

> Fix this by adding new callbacks to create/release the DMA mapping. This
> way we can use the has_dma_mapping flag for objects of the 3. case also
> (so far the flag was only used for the 1. and 2. case) and skip creating
> a new mapping if one exists already.
>
> Note that I also thought about simply creating/releasing the mapping
> when get_pages/put_pages is called. However since creating a DMA mapping
> may have associated resources (at least in case of HW IOMMU) it does
> make sense to release these resources as early as possible. We can
> release the DMA mapping as soon as the object is unbound from the last
> vma, before we drop the backing pages, hence it's worth keeping the two
> operations separate.
>
> I noticed this issue by enabling DMA debugging, which got disabled after
> a while due to its internal mapping tables getting full. It also reported
> errors in connection to random other drivers that did a DMA mapping for
> an address that was previously mapped by i915 but was never released.
> Besides these diagnostic messages and the memory space starvation
> problem for IOMMUs, I'm not aware of this causing a real issue.

Out of interest how to enable DMA debugging?

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h     |  2 ++
>   drivers/gpu/drm/i915/i915_gem.c     | 26 ++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 15 ++++-----------
>   3 files changed, 32 insertions(+), 11 deletions(-)

Patch looks good to me but I have this gut feeling Daniel will say that 
function pointers are an overkill. Personally I think it is more 
readable than adding special casing to core GEM functions.

Regards,

Tvrtko



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 15:57 +0100, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > We have 3 types of DMA mappings for GEM objects:
> > 1. physically contiguous for stolen and for objects needing contiguous
> >    memory
> > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > 3. SG DMA mappings for shmem backed and userptr objects
> > 
> > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > corresponding backing pages and so in practice we create/release the
> > mapping in the object's get_pages/put_pages callback.
> > 
> > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > of the object, so we'll create the mapping when the object is bound to
> > the first vma and release the mapping when the object is unbound from its
> > last vma.
> > 
> > Since the object can be bound to multiple vmas, we can end up creating a
> > new DMA mapping in the 3. case even if the object already had one. This
> > is not allowed by the DMA API and can lead to leaked mapping data and
> > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > drivers (intel_iommu) allocate a new range from their memory space
> > whenever a mapping is created, silently overriding a pre-existing
> > mapping.
> > 
> > Fix this by adding new callbacks to create/release the DMA mapping. This
> > way we can use the has_dma_mapping flag for objects of the 3. case also
> > (so far the flag was only used for the 1. and 2. case) and skip creating
> > a new mapping if one exists already.
> > 
> > Note that I also thought about simply creating/releasing the mapping
> > when get_pages/put_pages is called. However since creating a DMA mapping
> > may have associated resources (at least in case of HW IOMMU) it does
> > make sense to release these resources as early as possible. We can
> > release the DMA mapping as soon as the object is unbound from the last
> > vma, before we drop the backing pages, hence it's worth keeping the two
> > operations separate.
> > 
> > I noticed this issue by enabling DMA debugging, which got disabled after
> > a while due to its internal mapping tables getting full. It also reported
> > errors in connection to random other drivers that did a DMA mapping for
> > an address that was previously mapped by i915 but was never released.
> > Besides these diagnostic messages and the memory space starvation
> > problem for IOMMUs, I'm not aware of this causing a real issue.
> 
> Nope, it is much much simpler. Since we only do the dma prepare/finish
> from inside get_pages/put_pages, we can put the calls there. The only
> caveat there is userptr worker, but that can be easily fixed up.
> 
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=f55727d7d6f76aeee687c1f2d31411662ff03b6f

Yes, that's what I meant by creating/releasing the mapping in the
get_pages/put_pages callbacks. It does have the disadvantage of keeping
on to IOMMU mapping resources longer than it's needed as I described
above.

> Nak.

Right. Your patch doesn't explicitly mention fixing the issues I tracked
down, but it does seem to fix them. It would make sens to add this fact
to the commit log.

--Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 16:11 +0100, Tvrtko Ursulin wrote:
> Hi,
> 
> On 07/06/2015 03:50 PM, Imre Deak wrote:
> > We have 3 types of DMA mappings for GEM objects:
> > 1. physically contiguous for stolen and for objects needing contiguous
> >     memory
> > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > 3. SG DMA mappings for shmem backed and userptr objects
> >
> > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > corresponding backing pages and so in practice we create/release the
> > mapping in the object's get_pages/put_pages callback.
> >
> > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > of the object, so we'll create the mapping when the object is bound to
> > the first vma and release the mapping when the object is unbound from its
> > last vma.
> >
> > Since the object can be bound to multiple vmas, we can end up creating a
> > new DMA mapping in the 3. case even if the object already had one. This
> > is not allowed by the DMA API and can lead to leaked mapping data and
> > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > drivers (intel_iommu) allocate a new range from their memory space
> > whenever a mapping is created, silently overriding a pre-existing
> > mapping.
> 
> Ha.. back when I was adding multiple GGTT views I had this implemented 
> by only calling i915_gem_gtt_prepare_object on first VMA being 
> instantiated, and the same but opposite for last one going away. Someone 
> told me it is not needed though and to rip it out. :) To be fair I had 
> no clue so got it right just by being defensive.
> 
> > Fix this by adding new callbacks to create/release the DMA mapping. This
> > way we can use the has_dma_mapping flag for objects of the 3. case also
> > (so far the flag was only used for the 1. and 2. case) and skip creating
> > a new mapping if one exists already.
> >
> > Note that I also thought about simply creating/releasing the mapping
> > when get_pages/put_pages is called. However since creating a DMA mapping
> > may have associated resources (at least in case of HW IOMMU) it does
> > make sense to release these resources as early as possible. We can
> > release the DMA mapping as soon as the object is unbound from the last
> > vma, before we drop the backing pages, hence it's worth keeping the two
> > operations separate.
> >
> > I noticed this issue by enabling DMA debugging, which got disabled after
> > a while due to its internal mapping tables getting full. It also reported
> > errors in connection to random other drivers that did a DMA mapping for
> > an address that was previously mapped by i915 but was never released.
> > Besides these diagnostic messages and the memory space starvation
> > problem for IOMMUs, I'm not aware of this causing a real issue.
> 
> Out of interest how to enable DMA debugging?

By adding CONFIG_DMA_API_DEBUG=y.

> 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h     |  2 ++
> >   drivers/gpu/drm/i915/i915_gem.c     | 26 ++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_gem_gtt.c | 15 ++++-----------
> >   3 files changed, 32 insertions(+), 11 deletions(-)
> 
> Patch looks good to me but I have this gut feeling Daniel will say that 
> function pointers are an overkill. Personally I think it is more 
> readable than adding special casing to core GEM functions.

Yea, imo it depends if want to keep the put_pages and release DMA
mapping separate operations. In that case we could move the relevant
code for DMA buf objects too into these new callbacks. But if that's
found to be not worth it then we can just create/release the mapping in
the get_pages/put_pages callbacks and so the new ones are not needed.

Thanks for your review,
Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Mon, Jul 06, 2015 at 06:11:40PM +0300, Imre Deak wrote:
> On ma, 2015-07-06 at 15:57 +0100, Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > We have 3 types of DMA mappings for GEM objects:
> > > 1. physically contiguous for stolen and for objects needing contiguous
> > >    memory
> > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > 3. SG DMA mappings for shmem backed and userptr objects
> > > 
> > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > corresponding backing pages and so in practice we create/release the
> > > mapping in the object's get_pages/put_pages callback.
> > > 
> > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > of the object, so we'll create the mapping when the object is bound to
> > > the first vma and release the mapping when the object is unbound from its
> > > last vma.
> > > 
> > > Since the object can be bound to multiple vmas, we can end up creating a
> > > new DMA mapping in the 3. case even if the object already had one. This
> > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > drivers (intel_iommu) allocate a new range from their memory space
> > > whenever a mapping is created, silently overriding a pre-existing
> > > mapping.
> > > 
> > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > a new mapping if one exists already.
> > > 
> > > Note that I also thought about simply creating/releasing the mapping
> > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > may have associated resources (at least in case of HW IOMMU) it does
> > > make sense to release these resources as early as possible. We can
> > > release the DMA mapping as soon as the object is unbound from the last
> > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > operations separate.
> > > 
> > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > a while due to its internal mapping tables getting full. It also reported
> > > errors in connection to random other drivers that did a DMA mapping for
> > > an address that was previously mapped by i915 but was never released.
> > > Besides these diagnostic messages and the memory space starvation
> > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > 
> > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > from inside get_pages/put_pages, we can put the calls there. The only
> > caveat there is userptr worker, but that can be easily fixed up.
> > 
> > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=f55727d7d6f76aeee687c1f2d31411662ff03b6f
> 
> Yes, that's what I meant by creating/releasing the mapping in the
> get_pages/put_pages callbacks. It does have the disadvantage of keeping
> on to IOMMU mapping resources longer than it's needed as I described
> above.

I don't think that is a disadvantage though. You haven't introduced a
dma shrinker which is what you need to handle a limited resource. So
it's a moot point as we don't handle the allocation failure smartly. By
moving the failure into get pages, at least it is tractable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Daniel Vetter]

On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > We have 3 types of DMA mappings for GEM objects:
> > 1. physically contiguous for stolen and for objects needing contiguous
> >    memory
> > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > 3. SG DMA mappings for shmem backed and userptr objects
> > 
> > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > corresponding backing pages and so in practice we create/release the
> > mapping in the object's get_pages/put_pages callback.
> > 
> > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > of the object, so we'll create the mapping when the object is bound to
> > the first vma and release the mapping when the object is unbound from its
> > last vma.
> > 
> > Since the object can be bound to multiple vmas, we can end up creating a
> > new DMA mapping in the 3. case even if the object already had one. This
> > is not allowed by the DMA API and can lead to leaked mapping data and
> > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > drivers (intel_iommu) allocate a new range from their memory space
> > whenever a mapping is created, silently overriding a pre-existing
> > mapping.

How does this happen? Essentially list_empty(obj->vmas) ==
!dma_mapping_exists should hold for objects of the 3rd type. I don't
understand how this is broken in the current code. There was definitely
versions of the ppgtt code where this wasn't working properly, but I
thought we've fixed that up again.

> > Fix this by adding new callbacks to create/release the DMA mapping. This
> > way we can use the has_dma_mapping flag for objects of the 3. case also
> > (so far the flag was only used for the 1. and 2. case) and skip creating
> > a new mapping if one exists already.
> > 
> > Note that I also thought about simply creating/releasing the mapping
> > when get_pages/put_pages is called. However since creating a DMA mapping
> > may have associated resources (at least in case of HW IOMMU) it does
> > make sense to release these resources as early as possible. We can
> > release the DMA mapping as soon as the object is unbound from the last
> > vma, before we drop the backing pages, hence it's worth keeping the two
> > operations separate.
> > 
> > I noticed this issue by enabling DMA debugging, which got disabled after
> > a while due to its internal mapping tables getting full. It also reported
> > errors in connection to random other drivers that did a DMA mapping for
> > an address that was previously mapped by i915 but was never released.
> > Besides these diagnostic messages and the memory space starvation
> > problem for IOMMUs, I'm not aware of this causing a real issue.
> 
> Nope, it is much much simpler. Since we only do the dma prepare/finish
> from inside get_pages/put_pages, we can put the calls there. The only
> caveat there is userptr worker, but that can be easily fixed up.

I do kinda like the distinction between just grabbing the backing storage
and making it accessible to the hw. Small one, but I think it does help if
we keep these two maps separate. Now the function names otoh are
super-confusing, that I agree with.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 17:29 +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > We have 3 types of DMA mappings for GEM objects:
> > > 1. physically contiguous for stolen and for objects needing contiguous
> > >    memory
> > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > 3. SG DMA mappings for shmem backed and userptr objects
> > > 
> > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > corresponding backing pages and so in practice we create/release the
> > > mapping in the object's get_pages/put_pages callback.
> > > 
> > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > of the object, so we'll create the mapping when the object is bound to
> > > the first vma and release the mapping when the object is unbound from its
> > > last vma.
> > > 
> > > Since the object can be bound to multiple vmas, we can end up creating a
> > > new DMA mapping in the 3. case even if the object already had one. This
> > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > drivers (intel_iommu) allocate a new range from their memory space
> > > whenever a mapping is created, silently overriding a pre-existing
> > > mapping.
> 
> How does this happen? Essentially list_empty(obj->vmas) ==
> !dma_mapping_exists should hold for objects of the 3rd type. I don't
> understand how this is broken in the current code. There was definitely
> versions of the ppgtt code where this wasn't working properly, but I
> thought we've fixed that up again.

When binding the object we don't check if it's already bound, just
create the mapping regardless. So if it was already bound (having a
mapping) we'll again create a new mapping overriding the old one.

> > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > a new mapping if one exists already.
> > > 
> > > Note that I also thought about simply creating/releasing the mapping
> > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > may have associated resources (at least in case of HW IOMMU) it does
> > > make sense to release these resources as early as possible. We can
> > > release the DMA mapping as soon as the object is unbound from the last
> > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > operations separate.
> > > 
> > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > a while due to its internal mapping tables getting full. It also reported
> > > errors in connection to random other drivers that did a DMA mapping for
> > > an address that was previously mapped by i915 but was never released.
> > > Besides these diagnostic messages and the memory space starvation
> > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > 
> > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > from inside get_pages/put_pages, we can put the calls there. The only
> > caveat there is userptr worker, but that can be easily fixed up.
> 
> I do kinda like the distinction between just grabbing the backing storage
> and making it accessible to the hw. Small one, but I think it does help if
> we keep these two maps separate. Now the function names otoh are
> super-confusing, that I agree with.

Well, please convince Chris :)


> -Daniel


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 16:28 +0100, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 06:11:40PM +0300, Imre Deak wrote:
> > On ma, 2015-07-06 at 15:57 +0100, Chris Wilson wrote:
> > > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > > We have 3 types of DMA mappings for GEM objects:
> > > > 1. physically contiguous for stolen and for objects needing contiguous
> > > >    memory
> > > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > > 3. SG DMA mappings for shmem backed and userptr objects
> > > > 
> > > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > > corresponding backing pages and so in practice we create/release the
> > > > mapping in the object's get_pages/put_pages callback.
> > > > 
> > > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > > of the object, so we'll create the mapping when the object is bound to
> > > > the first vma and release the mapping when the object is unbound from its
> > > > last vma.
> > > > 
> > > > Since the object can be bound to multiple vmas, we can end up creating a
> > > > new DMA mapping in the 3. case even if the object already had one. This
> > > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > > drivers (intel_iommu) allocate a new range from their memory space
> > > > whenever a mapping is created, silently overriding a pre-existing
> > > > mapping.
> > > > 
> > > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > > a new mapping if one exists already.
> > > > 
> > > > Note that I also thought about simply creating/releasing the mapping
> > > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > > may have associated resources (at least in case of HW IOMMU) it does
> > > > make sense to release these resources as early as possible. We can
> > > > release the DMA mapping as soon as the object is unbound from the last
> > > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > > operations separate.
> > > > 
> > > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > > a while due to its internal mapping tables getting full. It also reported
> > > > errors in connection to random other drivers that did a DMA mapping for
> > > > an address that was previously mapped by i915 but was never released.
> > > > Besides these diagnostic messages and the memory space starvation
> > > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > > 
> > > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > > from inside get_pages/put_pages, we can put the calls there. The only
> > > caveat there is userptr worker, but that can be easily fixed up.
> > > 
> > > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=f55727d7d6f76aeee687c1f2d31411662ff03b6f
> > 
> > Yes, that's what I meant by creating/releasing the mapping in the
> > get_pages/put_pages callbacks. It does have the disadvantage of keeping
> > on to IOMMU mapping resources longer than it's needed as I described
> > above.
> 
> I don't think that is a disadvantage though. You haven't introduced a
> dma shrinker which is what you need to handle a limited resource. So
> it's a moot point as we don't handle the allocation failure smartly. By
> moving the failure into get pages, at least it is tractable.

That's true, but we could do this in the future, if we had the new
callbacks.


> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Mon, Jul 06, 2015 at 05:29:39PM +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > We have 3 types of DMA mappings for GEM objects:
> > > 1. physically contiguous for stolen and for objects needing contiguous
> > >    memory
> > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > 3. SG DMA mappings for shmem backed and userptr objects
> > > 
> > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > corresponding backing pages and so in practice we create/release the
> > > mapping in the object's get_pages/put_pages callback.
> > > 
> > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > of the object, so we'll create the mapping when the object is bound to
> > > the first vma and release the mapping when the object is unbound from its
> > > last vma.
> > > 
> > > Since the object can be bound to multiple vmas, we can end up creating a
> > > new DMA mapping in the 3. case even if the object already had one. This
> > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > drivers (intel_iommu) allocate a new range from their memory space
> > > whenever a mapping is created, silently overriding a pre-existing
> > > mapping.
> 
> How does this happen? Essentially list_empty(obj->vmas) ==
> !dma_mapping_exists should hold for objects of the 3rd type. I don't
> understand how this is broken in the current code. There was definitely
> versions of the ppgtt code where this wasn't working properly, but I
> thought we've fixed that up again.

Every g/ppgtt binding remapped the obj->pages through the iommu. Even
with the DMAR disabled, we still pay the cpu cost of sw iommu (which is
itself an annoying kernel bug that you can't disable).
 
> > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > a new mapping if one exists already.
> > > 
> > > Note that I also thought about simply creating/releasing the mapping
> > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > may have associated resources (at least in case of HW IOMMU) it does
> > > make sense to release these resources as early as possible. We can
> > > release the DMA mapping as soon as the object is unbound from the last
> > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > operations separate.
> > > 
> > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > a while due to its internal mapping tables getting full. It also reported
> > > errors in connection to random other drivers that did a DMA mapping for
> > > an address that was previously mapped by i915 but was never released.
> > > Besides these diagnostic messages and the memory space starvation
> > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > 
> > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > from inside get_pages/put_pages, we can put the calls there. The only
> > caveat there is userptr worker, but that can be easily fixed up.
> 
> I do kinda like the distinction between just grabbing the backing storage
> and making it accessible to the hw. Small one, but I think it does help if
> we keep these two maps separate. Now the function names otoh are
> super-confusing, that I agree with.

But that is the raison-d'etre of get_pages(). We call it preciselly when
we want the backing storage available to the hw. We relaxed that for
set-domain to avoid one type of bug, and stolen/dma-buf have their own
notion of dma mapping. userptr is the odd one out due to its worker
asynchronously grabbing the pages.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 16:33 +0100, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 05:29:39PM +0200, Daniel Vetter wrote:
> > On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > > We have 3 types of DMA mappings for GEM objects:
> > > > 1. physically contiguous for stolen and for objects needing contiguous
> > > >    memory
> > > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > > 3. SG DMA mappings for shmem backed and userptr objects
> > > > 
> > > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > > corresponding backing pages and so in practice we create/release the
> > > > mapping in the object's get_pages/put_pages callback.
> > > > 
> > > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > > of the object, so we'll create the mapping when the object is bound to
> > > > the first vma and release the mapping when the object is unbound from its
> > > > last vma.
> > > > 
> > > > Since the object can be bound to multiple vmas, we can end up creating a
> > > > new DMA mapping in the 3. case even if the object already had one. This
> > > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > > drivers (intel_iommu) allocate a new range from their memory space
> > > > whenever a mapping is created, silently overriding a pre-existing
> > > > mapping.
> > 
> > How does this happen? Essentially list_empty(obj->vmas) ==
> > !dma_mapping_exists should hold for objects of the 3rd type. I don't
> > understand how this is broken in the current code. There was definitely
> > versions of the ppgtt code where this wasn't working properly, but I
> > thought we've fixed that up again.
> 
> Every g/ppgtt binding remapped the obj->pages through the iommu. Even
> with the DMAR disabled, we still pay the cpu cost of sw iommu (which is
> itself an annoying kernel bug that you can't disable).
>  
> > > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > > a new mapping if one exists already.
> > > > 
> > > > Note that I also thought about simply creating/releasing the mapping
> > > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > > may have associated resources (at least in case of HW IOMMU) it does
> > > > make sense to release these resources as early as possible. We can
> > > > release the DMA mapping as soon as the object is unbound from the last
> > > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > > operations separate.
> > > > 
> > > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > > a while due to its internal mapping tables getting full. It also reported
> > > > errors in connection to random other drivers that did a DMA mapping for
> > > > an address that was previously mapped by i915 but was never released.
> > > > Besides these diagnostic messages and the memory space starvation
> > > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > > 
> > > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > > from inside get_pages/put_pages, we can put the calls there. The only
> > > caveat there is userptr worker, but that can be easily fixed up.
> > 
> > I do kinda like the distinction between just grabbing the backing storage
> > and making it accessible to the hw. Small one, but I think it does help if
> > we keep these two maps separate. Now the function names otoh are
> > super-confusing, that I agree with.
> 
> But that is the raison-d'etre of get_pages(). We call it preciselly when
> we want the backing storage available to the hw. We relaxed that for
> set-domain to avoid one type of bug, and stolen/dma-buf have their own
> notion of dma mapping. userptr is the odd one out due to its worker
> asynchronously grabbing the pages.

Isn't the DMA mapping operation more tied to binding the object to a
VMA? As far as I can see we call put_pages only when destroying the
object (or attaching a physically contiguous mapping to it) and that's
because at that point we also give up on the content of the buffer.
Otherwise we just do unbinding when reclaiming memory. At this point it
make sense to release the DMA mapping independently of releasing the
buffer contents.

--Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Mon, Jul 06, 2015 at 06:56:00PM +0300, Imre Deak wrote:
> On ma, 2015-07-06 at 16:33 +0100, Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 05:29:39PM +0200, Daniel Vetter wrote:
> > > On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > > > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > > > We have 3 types of DMA mappings for GEM objects:
> > > > > 1. physically contiguous for stolen and for objects needing contiguous
> > > > >    memory
> > > > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > > > 3. SG DMA mappings for shmem backed and userptr objects
> > > > > 
> > > > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > > > corresponding backing pages and so in practice we create/release the
> > > > > mapping in the object's get_pages/put_pages callback.
> > > > > 
> > > > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > > > of the object, so we'll create the mapping when the object is bound to
> > > > > the first vma and release the mapping when the object is unbound from its
> > > > > last vma.
> > > > > 
> > > > > Since the object can be bound to multiple vmas, we can end up creating a
> > > > > new DMA mapping in the 3. case even if the object already had one. This
> > > > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > > > drivers (intel_iommu) allocate a new range from their memory space
> > > > > whenever a mapping is created, silently overriding a pre-existing
> > > > > mapping.
> > > 
> > > How does this happen? Essentially list_empty(obj->vmas) ==
> > > !dma_mapping_exists should hold for objects of the 3rd type. I don't
> > > understand how this is broken in the current code. There was definitely
> > > versions of the ppgtt code where this wasn't working properly, but I
> > > thought we've fixed that up again.
> > 
> > Every g/ppgtt binding remapped the obj->pages through the iommu. Even
> > with the DMAR disabled, we still pay the cpu cost of sw iommu (which is
> > itself an annoying kernel bug that you can't disable).
> >  
> > > > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > > > a new mapping if one exists already.
> > > > > 
> > > > > Note that I also thought about simply creating/releasing the mapping
> > > > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > > > may have associated resources (at least in case of HW IOMMU) it does
> > > > > make sense to release these resources as early as possible. We can
> > > > > release the DMA mapping as soon as the object is unbound from the last
> > > > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > > > operations separate.
> > > > > 
> > > > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > > > a while due to its internal mapping tables getting full. It also reported
> > > > > errors in connection to random other drivers that did a DMA mapping for
> > > > > an address that was previously mapped by i915 but was never released.
> > > > > Besides these diagnostic messages and the memory space starvation
> > > > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > > > 
> > > > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > > > from inside get_pages/put_pages, we can put the calls there. The only
> > > > caveat there is userptr worker, but that can be easily fixed up.
> > > 
> > > I do kinda like the distinction between just grabbing the backing storage
> > > and making it accessible to the hw. Small one, but I think it does help if
> > > we keep these two maps separate. Now the function names otoh are
> > > super-confusing, that I agree with.
> > 
> > But that is the raison-d'etre of get_pages(). We call it preciselly when
> > we want the backing storage available to the hw. We relaxed that for
> > set-domain to avoid one type of bug, and stolen/dma-buf have their own
> > notion of dma mapping. userptr is the odd one out due to its worker
> > asynchronously grabbing the pages.
> 
> Isn't the DMA mapping operation more tied to binding the object to a
> VMA? As far as I can see we call put_pages only when destroying the
> object (or attaching a physically contiguous mapping to it) and that's
> because at that point we also give up on the content of the buffer.
> Otherwise we just do unbinding when reclaiming memory. At this point it
> make sense to release the DMA mapping independently of releasing the
> buffer contents.

No. As proved above, it is not about each VMA, it about preparing the
object for access by the hw - i.e. a natural fit for the
get_pages/put_pages() greedy scheme, and if you look at the workloads
where we benefit from the current scheme, we also massively benefit from
avoiding the remapping. A dma shrinker would also simply call
i915_gem_shrink(), and we can do that today cf get_pages_gtt() and do
our own shrinking first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On ma, 2015-07-06 at 17:04 +0100, Chris Wilson wrote:
> On Mon, Jul 06, 2015 at 06:56:00PM +0300, Imre Deak wrote:
> > On ma, 2015-07-06 at 16:33 +0100, Chris Wilson wrote:
> > > On Mon, Jul 06, 2015 at 05:29:39PM +0200, Daniel Vetter wrote:
> > > > On Mon, Jul 06, 2015 at 03:57:44PM +0100, Chris Wilson wrote:
> > > > > On Mon, Jul 06, 2015 at 05:50:37PM +0300, Imre Deak wrote:
> > > > > > We have 3 types of DMA mappings for GEM objects:
> > > > > > 1. physically contiguous for stolen and for objects needing contiguous
> > > > > >    memory
> > > > > > 2. DMA-buf mappings imported via a DMA-buf attach operation
> > > > > > 3. SG DMA mappings for shmem backed and userptr objects
> > > > > > 
> > > > > > For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> > > > > > corresponding backing pages and so in practice we create/release the
> > > > > > mapping in the object's get_pages/put_pages callback.
> > > > > > 
> > > > > > For 3. the lifetime of the mapping matches that of any existing GPU binding
> > > > > > of the object, so we'll create the mapping when the object is bound to
> > > > > > the first vma and release the mapping when the object is unbound from its
> > > > > > last vma.
> > > > > > 
> > > > > > Since the object can be bound to multiple vmas, we can end up creating a
> > > > > > new DMA mapping in the 3. case even if the object already had one. This
> > > > > > is not allowed by the DMA API and can lead to leaked mapping data and
> > > > > > IOMMU memory space starvation in certain cases. For example HW IOMMU
> > > > > > drivers (intel_iommu) allocate a new range from their memory space
> > > > > > whenever a mapping is created, silently overriding a pre-existing
> > > > > > mapping.
> > > > 
> > > > How does this happen? Essentially list_empty(obj->vmas) ==
> > > > !dma_mapping_exists should hold for objects of the 3rd type. I don't
> > > > understand how this is broken in the current code. There was definitely
> > > > versions of the ppgtt code where this wasn't working properly, but I
> > > > thought we've fixed that up again.
> > > 
> > > Every g/ppgtt binding remapped the obj->pages through the iommu. Even
> > > with the DMAR disabled, we still pay the cpu cost of sw iommu (which is
> > > itself an annoying kernel bug that you can't disable).
> > >  
> > > > > > Fix this by adding new callbacks to create/release the DMA mapping. This
> > > > > > way we can use the has_dma_mapping flag for objects of the 3. case also
> > > > > > (so far the flag was only used for the 1. and 2. case) and skip creating
> > > > > > a new mapping if one exists already.
> > > > > > 
> > > > > > Note that I also thought about simply creating/releasing the mapping
> > > > > > when get_pages/put_pages is called. However since creating a DMA mapping
> > > > > > may have associated resources (at least in case of HW IOMMU) it does
> > > > > > make sense to release these resources as early as possible. We can
> > > > > > release the DMA mapping as soon as the object is unbound from the last
> > > > > > vma, before we drop the backing pages, hence it's worth keeping the two
> > > > > > operations separate.
> > > > > > 
> > > > > > I noticed this issue by enabling DMA debugging, which got disabled after
> > > > > > a while due to its internal mapping tables getting full. It also reported
> > > > > > errors in connection to random other drivers that did a DMA mapping for
> > > > > > an address that was previously mapped by i915 but was never released.
> > > > > > Besides these diagnostic messages and the memory space starvation
> > > > > > problem for IOMMUs, I'm not aware of this causing a real issue.
> > > > > 
> > > > > Nope, it is much much simpler. Since we only do the dma prepare/finish
> > > > > from inside get_pages/put_pages, we can put the calls there. The only
> > > > > caveat there is userptr worker, but that can be easily fixed up.
> > > > 
> > > > I do kinda like the distinction between just grabbing the backing storage
> > > > and making it accessible to the hw. Small one, but I think it does help if
> > > > we keep these two maps separate. Now the function names otoh are
> > > > super-confusing, that I agree with.
> > > 
> > > But that is the raison-d'etre of get_pages(). We call it preciselly when
> > > we want the backing storage available to the hw. We relaxed that for
> > > set-domain to avoid one type of bug, and stolen/dma-buf have their own
> > > notion of dma mapping. userptr is the odd one out due to its worker
> > > asynchronously grabbing the pages.
> > 
> > Isn't the DMA mapping operation more tied to binding the object to a
> > VMA? As far as I can see we call put_pages only when destroying the
> > object (or attaching a physically contiguous mapping to it) and that's
> > because at that point we also give up on the content of the buffer.
> > Otherwise we just do unbinding when reclaiming memory. At this point it
> > make sense to release the DMA mapping independently of releasing the
> > buffer contents.
> 
> No. As proved above, it is not about each VMA, it about preparing the
> object for access by the hw - i.e. a natural fit for the
> get_pages/put_pages() greedy scheme, and if you look at the workloads
> where we benefit from the current scheme, we also massively benefit from
> avoiding the remapping. A dma shrinker would also simply call
> i915_gem_shrink(), and we can do that today cf get_pages_gtt() and do
> our own shrinking first.

Right, misunderstood this. Adding new callbacks doesn't have a benefit
then.

--Imre

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: avoid leaking DMA mappings

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6732
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -4              302/302              298/302
SNB                 -4              312/316              308/316
IVB                 -5              345/345              340/345
BYT                 -1              289/289              288/289
HSW                 -5              382/382              377/382
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@gem_userptr_blits@dmabuf-sync      PASS(1)      FAIL(1)
*ILK  igt@gem_userptr_blits@dmabuf-unsync      PASS(1)      FAIL(1)
*ILK  igt@gem_userptr_blits@forked-access      PASS(1)      FAIL(1)
*ILK  igt@gem_userptr_blits@forked-sync-interruptible      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/i915_gem_userptr.c:#cancel_userptr[i915]()@WARNING:.* at .* cancel_userptr+0x
*SNB  igt@gem_userptr_blits@coherency-sync      PASS(1)      CRASH(1)
*SNB  igt@gem_userptr_blits@dmabuf-sync      PASS(1)      FAIL(1)
*SNB  igt@gem_userptr_blits@dmabuf-unsync      PASS(1)      FAIL(1)
*SNB  igt@gem_userptr_blits@forked-access      PASS(1)      FAIL(1)
*IVB  igt@gem_userptr_blits@coherency-sync      PASS(1)      CRASH(1)
*IVB  igt@gem_userptr_blits@coherency-unsync      PASS(1)      CRASH(1)
*IVB  igt@gem_userptr_blits@dmabuf-sync      PASS(1)      FAIL(1)
*IVB  igt@gem_userptr_blits@dmabuf-unsync      PASS(1)      FAIL(1)
*IVB  igt@gem_userptr_blits@forked-access      PASS(1)      FAIL(1)
*BYT  igt@gem_userptr_blits@forked-access      PASS(1)      FAIL(1)
*HSW  igt@gem_userptr_blits@coherency-sync      PASS(1)      FAIL(1)
*HSW  igt@gem_userptr_blits@coherency-unsync      PASS(1)      FAIL(1)
*HSW  igt@gem_userptr_blits@dmabuf-sync      PASS(1)      FAIL(1)
*HSW  igt@gem_userptr_blits@dmabuf-unsync      PASS(1)      FAIL(1)
*HSW  igt@gem_userptr_blits@forked-access      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915: avoid leaking DMA mappings

[Imre Deak]

We have 3 types of DMA mappings for GEM objects:
1. physically contiguous for stolen and for objects needing contiguous
   memory
2. DMA-buf mappings imported via a DMA-buf attach operation
3. SG DMA mappings for shmem backed and userptr objects

For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
corresponding backing pages and so in practice we create/release the
mapping in the object's get_pages/put_pages callback.

For 3. the lifetime of the mapping matches that of any existing GPU binding
of the object, so we'll create the mapping when the object is bound to
the first vma and release the mapping when the object is unbound from its
last vma.

Since the object can be bound to multiple vmas, we can end up creating a
new DMA mapping in the 3. case even if the object already had one. This
is not allowed by the DMA API and can lead to leaked mapping data and
IOMMU memory space starvation in certain cases. For example HW IOMMU
drivers (intel_iommu) allocate a new range from their memory space
whenever a mapping is created, silently overriding a pre-existing
mapping.

Fix this by moving the creation/removal of DMA mappings to the object's
get_pages/put_pages callbacks. These callbacks already check for and do
an early return in case of any nested calls. This way objects of the 3.
case also become more like the other object types.

I noticed this issue by enabling DMA debugging, which got disabled after
a while due to its internal mapping tables getting full. It also reported
errors in connection to random other drivers that did a DMA mapping for
an address that was previously mapped by i915 but was never released.
Besides these diagnostic messages and the memory space starvation
problem for IOMMUs, I'm not aware of this causing a real issue.

The fix is based on a patch from Chris.

v2:
- move the DMA mapping create/remove calls to the get_pages/put_pages
  callbacks instead of adding new callbacks for these (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         | 31 ++++++++++++++++---------------
 drivers/gpu/drm/i915/i915_gem_userptr.c | 27 +++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 425ced6..aa71067 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2146,6 +2146,8 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	i915_gem_gtt_finish_object(obj);
+
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_save_bit_17_swizzle(obj);
 
@@ -2206,6 +2208,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	struct sg_page_iter sg_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
+	int ret;
 	gfp_t gfp;
 
 	/* Assert that the object is not currently in any GPU domain. As it
@@ -2253,8 +2256,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			 */
 			i915_gem_shrink_all(dev_priv);
 			page = shmem_read_mapping_page(mapping, i);
-			if (IS_ERR(page))
+			if (IS_ERR(page)) {
+				ret = PTR_ERR(page);
 				goto err_pages;
+			}
 		}
 #ifdef CONFIG_SWIOTLB
 		if (swiotlb_nr_tbl()) {
@@ -2283,6 +2288,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		sg_mark_end(sg);
 	obj->pages = st;
 
+	ret = i915_gem_gtt_prepare_object(obj);
+	if (ret)
+		goto err_pages;
+
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_do_bit_17_swizzle(obj);
 
@@ -2307,10 +2316,10 @@ err_pages:
 	 * space and so want to translate the error from shmemfs back to our
 	 * usual understanding of ENOMEM.
 	 */
-	if (PTR_ERR(page) == -ENOSPC)
-		return -ENOMEM;
-	else
-		return PTR_ERR(page);
+	if (ret == -ENOSPC)
+		ret = -ENOMEM;
+
+	return ret;
 }
 
 /* Ensure that the associated pages are gathered from the backing storage
@@ -3288,10 +3297,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 
 	/* Since the unbound list is global, only move to that list if
 	 * no more VMAs exist. */
-	if (list_empty(&obj->vma_list)) {
-		i915_gem_gtt_finish_object(obj);
+	if (list_empty(&obj->vma_list))
 		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
-	}
 
 	/* And finally now the object is completely decoupled from this vma,
 	 * we can drop its hold on the backing storage and allow it to be
@@ -3819,22 +3826,16 @@ search_free:
 		goto err_remove_node;
 	}
 
-	ret = i915_gem_gtt_prepare_object(obj);
-	if (ret)
-		goto err_remove_node;
-
 	trace_i915_vma_bind(vma, flags);
 	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
-		goto err_finish_gtt;
+		goto err_remove_node;
 
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
 	return vma;
 
-err_finish_gtt:
-	i915_gem_gtt_finish_object(obj);
 err_remove_node:
 	drm_mm_remove_node(&vma->node);
 err_free_vma:
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 1f4e5a3..fcdeffc 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -545,6 +545,26 @@ err:
 	return ret;
 }
 
+static int
+__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
+			     struct page **pvec, int num_pages)
+{
+	int ret;
+
+	ret = st_set_pages(&obj->pages, pvec, num_pages);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_gtt_prepare_object(obj);
+	if (ret) {
+		sg_free_table(obj->pages);
+		kfree(obj->pages);
+		obj->pages = NULL;
+	}
+
+	return ret;
+}
+
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
@@ -584,7 +604,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (obj->userptr.work != &work->work) {
 		ret = 0;
 	} else if (pinned == num_pages) {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
 		if (ret == 0) {
 			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
 			pinned = 0;
@@ -693,7 +713,7 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 			}
 		}
 	} else {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
 		if (ret == 0) {
 			obj->userptr.work = NULL;
 			pinned = 0;
@@ -702,6 +722,7 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 
 	release_pages(pvec, pinned, 0);
 	drm_free_large(pvec);
+
 	return ret;
 }
 
@@ -715,6 +736,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 	if (obj->madv != I915_MADV_WILLNEED)
 		obj->dirty = 0;
 
+	i915_gem_gtt_finish_object(obj);
+
 	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
 		struct page *page = sg_page_iter_page(&sg_iter);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: remove unused has_dma_mapping flag

[Imre Deak]

After the previous patch this flag will check always clear, as it's
never set for shmem backed and userptr objects, so we can remove it.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        | 2 --
 drivers/gpu/drm/i915/i915_gem.c        | 3 ---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 2 --
 drivers/gpu/drm/i915/i915_gem_gtt.c    | 9 ++-------
 drivers/gpu/drm/i915/i915_gem_stolen.c | 1 -
 5 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 093d642..152eedf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2071,8 +2071,6 @@ struct drm_i915_gem_object {
 	unsigned int cache_level:3;
 	unsigned int cache_dirty:1;
 
-	unsigned int has_dma_mapping:1;
-
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
 
 	unsigned int pin_display;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index aa71067..219d4d5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -217,7 +217,6 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	sg_dma_len(sg) = obj->base.size;
 
 	obj->pages = st;
-	obj->has_dma_mapping = true;
 	return 0;
 }
 
@@ -269,8 +268,6 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj)
 
 	sg_free_table(obj->pages);
 	kfree(obj->pages);
-
-	obj->has_dma_mapping = false;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 7998da2..e9c2bfd 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -256,7 +256,6 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 		return PTR_ERR(sg);
 
 	obj->pages = sg;
-	obj->has_dma_mapping = true;
 	return 0;
 }
 
@@ -264,7 +263,6 @@ static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj)
 {
 	dma_buf_unmap_attachment(obj->base.import_attach,
 				 obj->pages, DMA_BIDIRECTIONAL);
-	obj->has_dma_mapping = false;
 }
 
 static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b29b73f..44255a8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1802,9 +1802,6 @@ void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
 
 int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 {
-	if (obj->has_dma_mapping)
-		return 0;
-
 	if (!dma_map_sg(&obj->base.dev->pdev->dev,
 			obj->pages->sgl, obj->pages->nents,
 			PCI_DMA_BIDIRECTIONAL))
@@ -2052,10 +2049,8 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
 
 	interruptible = do_idling(dev_priv);
 
-	if (!obj->has_dma_mapping)
-		dma_unmap_sg(&dev->pdev->dev,
-			     obj->pages->sgl, obj->pages->nents,
-			     PCI_DMA_BIDIRECTIONAL);
+	dma_unmap_sg(&dev->pdev->dev, obj->pages->sgl, obj->pages->nents,
+		     PCI_DMA_BIDIRECTIONAL);
 
 	undo_idling(dev_priv, interruptible);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index de76d88..ed682a9 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -316,7 +316,6 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 	if (obj->pages == NULL)
 		goto cleanup;
 
-	obj->has_dma_mapping = true;
 	i915_gem_object_pin_pages(obj);
 	obj->stolen = stolen;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Wed, Jul 08, 2015 at 07:18:58PM +0300, Imre Deak wrote:
> We have 3 types of DMA mappings for GEM objects:
> 1. physically contiguous for stolen and for objects needing contiguous
>    memory
> 2. DMA-buf mappings imported via a DMA-buf attach operation
> 3. SG DMA mappings for shmem backed and userptr objects
> 
> For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> corresponding backing pages and so in practice we create/release the
> mapping in the object's get_pages/put_pages callback.
> 
> For 3. the lifetime of the mapping matches that of any existing GPU binding
> of the object, so we'll create the mapping when the object is bound to
> the first vma and release the mapping when the object is unbound from its
> last vma.
> 
> Since the object can be bound to multiple vmas, we can end up creating a
> new DMA mapping in the 3. case even if the object already had one. This
> is not allowed by the DMA API and can lead to leaked mapping data and
> IOMMU memory space starvation in certain cases. For example HW IOMMU
> drivers (intel_iommu) allocate a new range from their memory space
> whenever a mapping is created, silently overriding a pre-existing
> mapping.
> 
> Fix this by moving the creation/removal of DMA mappings to the object's
> get_pages/put_pages callbacks. These callbacks already check for and do
> an early return in case of any nested calls. This way objects of the 3.
> case also become more like the other object types.
> 
> I noticed this issue by enabling DMA debugging, which got disabled after
> a while due to its internal mapping tables getting full. It also reported
> errors in connection to random other drivers that did a DMA mapping for
> an address that was previously mapped by i915 but was never released.
> Besides these diagnostic messages and the memory space starvation
> problem for IOMMUs, I'm not aware of this causing a real issue.
> 
> The fix is based on a patch from Chris.

You missed the bugfix from __i915_gem_object_init_pages()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: remove unused has_dma_mapping flag

[Chris Wilson]

On Wed, Jul 08, 2015 at 07:18:59PM +0300, Imre Deak wrote:
> After the previous patch this flag will check always clear, as it's
> never set for shmem backed and userptr objects, so we can remove it.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Mentioned a trivial obj->get_page bugfix for
__i915_gem_userptr_set_pages(), and then went on an archaelogical dig to
understand why I didn't think of this earlier. Apparently, it just never
occurred to me that I could remove this flag introduced for
i915_gem_prime when we did the get_pages/put_pages support for it.

Both patches (incl. the minor fix above for 1),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: remove unused has_dma_mapping flag

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6754
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                                  287/287              287/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/2] drm/i915: avoid leaking DMA mappings

[Imre Deak]

We have 3 types of DMA mappings for GEM objects:
1. physically contiguous for stolen and for objects needing contiguous
   memory
2. DMA-buf mappings imported via a DMA-buf attach operation
3. SG DMA mappings for shmem backed and userptr objects

For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
corresponding backing pages and so in practice we create/release the
mapping in the object's get_pages/put_pages callback.

For 3. the lifetime of the mapping matches that of any existing GPU binding
of the object, so we'll create the mapping when the object is bound to
the first vma and release the mapping when the object is unbound from its
last vma.

Since the object can be bound to multiple vmas, we can end up creating a
new DMA mapping in the 3. case even if the object already had one. This
is not allowed by the DMA API and can lead to leaked mapping data and
IOMMU memory space starvation in certain cases. For example HW IOMMU
drivers (intel_iommu) allocate a new range from their memory space
whenever a mapping is created, silently overriding a pre-existing
mapping.

Fix this by moving the creation/removal of DMA mappings to the object's
get_pages/put_pages callbacks. These callbacks already check for and do
an early return in case of any nested calls. This way objects of the 3.
case also become more like the other object types.

I noticed this issue by enabling DMA debugging, which got disabled after
a while due to its internal mapping tables getting full. It also reported
errors in connection to random other drivers that did a DMA mapping for
an address that was previously mapped by i915 but was never released.
Besides these diagnostic messages and the memory space starvation
problem for IOMMUs, I'm not aware of this causing a real issue.

The fix is based on a patch from Chris.

v2:
- move the DMA mapping create/remove calls to the get_pages/put_pages
  callbacks instead of adding new callbacks for these (Chris)
v3:
- also fix the get_page cache logic on the userptr async path (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem.c         | 31 ++++++++++++++++---------------
 drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 425ced6..aa71067 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2146,6 +2146,8 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
 
+	i915_gem_gtt_finish_object(obj);
+
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_save_bit_17_swizzle(obj);
 
@@ -2206,6 +2208,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	struct sg_page_iter sg_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
+	int ret;
 	gfp_t gfp;
 
 	/* Assert that the object is not currently in any GPU domain. As it
@@ -2253,8 +2256,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			 */
 			i915_gem_shrink_all(dev_priv);
 			page = shmem_read_mapping_page(mapping, i);
-			if (IS_ERR(page))
+			if (IS_ERR(page)) {
+				ret = PTR_ERR(page);
 				goto err_pages;
+			}
 		}
 #ifdef CONFIG_SWIOTLB
 		if (swiotlb_nr_tbl()) {
@@ -2283,6 +2288,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		sg_mark_end(sg);
 	obj->pages = st;
 
+	ret = i915_gem_gtt_prepare_object(obj);
+	if (ret)
+		goto err_pages;
+
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_do_bit_17_swizzle(obj);
 
@@ -2307,10 +2316,10 @@ err_pages:
 	 * space and so want to translate the error from shmemfs back to our
 	 * usual understanding of ENOMEM.
 	 */
-	if (PTR_ERR(page) == -ENOSPC)
-		return -ENOMEM;
-	else
-		return PTR_ERR(page);
+	if (ret == -ENOSPC)
+		ret = -ENOMEM;
+
+	return ret;
 }
 
 /* Ensure that the associated pages are gathered from the backing storage
@@ -3288,10 +3297,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 
 	/* Since the unbound list is global, only move to that list if
 	 * no more VMAs exist. */
-	if (list_empty(&obj->vma_list)) {
-		i915_gem_gtt_finish_object(obj);
+	if (list_empty(&obj->vma_list))
 		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
-	}
 
 	/* And finally now the object is completely decoupled from this vma,
 	 * we can drop its hold on the backing storage and allow it to be
@@ -3819,22 +3826,16 @@ search_free:
 		goto err_remove_node;
 	}
 
-	ret = i915_gem_gtt_prepare_object(obj);
-	if (ret)
-		goto err_remove_node;
-
 	trace_i915_vma_bind(vma, flags);
 	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
-		goto err_finish_gtt;
+		goto err_remove_node;
 
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
 	return vma;
 
-err_finish_gtt:
-	i915_gem_gtt_finish_object(obj);
 err_remove_node:
 	drm_mm_remove_node(&vma->node);
 err_free_vma:
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 1f4e5a3..8fd431b 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -545,6 +545,26 @@ err:
 	return ret;
 }
 
+static int
+__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
+			     struct page **pvec, int num_pages)
+{
+	int ret;
+
+	ret = st_set_pages(&obj->pages, pvec, num_pages);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_gtt_prepare_object(obj);
+	if (ret) {
+		sg_free_table(obj->pages);
+		kfree(obj->pages);
+		obj->pages = NULL;
+	}
+
+	return ret;
+}
+
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
@@ -584,9 +604,12 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (obj->userptr.work != &work->work) {
 		ret = 0;
 	} else if (pinned == num_pages) {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
 		if (ret == 0) {
 			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
+			obj->get_page.sg = obj->pages->sgl;
+			obj->get_page.last = 0;
+
 			pinned = 0;
 		}
 	}
@@ -693,7 +716,7 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 			}
 		}
 	} else {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
 		if (ret == 0) {
 			obj->userptr.work = NULL;
 			pinned = 0;
@@ -715,6 +738,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 	if (obj->madv != I915_MADV_WILLNEED)
 		obj->dirty = 0;
 
+	i915_gem_gtt_finish_object(obj);
+
 	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
 		struct page *page = sg_page_iter_page(&sg_iter);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Thu, Jul 09, 2015 at 12:59:05PM +0300, Imre Deak wrote:
> We have 3 types of DMA mappings for GEM objects:
> 1. physically contiguous for stolen and for objects needing contiguous
>    memory
> 2. DMA-buf mappings imported via a DMA-buf attach operation
> 3. SG DMA mappings for shmem backed and userptr objects
> 
> For 1. and 2. the lifetime of the DMA mapping matches the lifetime of the
> corresponding backing pages and so in practice we create/release the
> mapping in the object's get_pages/put_pages callback.
> 
> For 3. the lifetime of the mapping matches that of any existing GPU binding
> of the object, so we'll create the mapping when the object is bound to
> the first vma and release the mapping when the object is unbound from its
> last vma.
> 
> Since the object can be bound to multiple vmas, we can end up creating a
> new DMA mapping in the 3. case even if the object already had one. This
> is not allowed by the DMA API and can lead to leaked mapping data and
> IOMMU memory space starvation in certain cases. For example HW IOMMU
> drivers (intel_iommu) allocate a new range from their memory space
> whenever a mapping is created, silently overriding a pre-existing
> mapping.
> 
> Fix this by moving the creation/removal of DMA mappings to the object's
> get_pages/put_pages callbacks. These callbacks already check for and do
> an early return in case of any nested calls. This way objects of the 3.
> case also become more like the other object types.
> 
> I noticed this issue by enabling DMA debugging, which got disabled after
> a while due to its internal mapping tables getting full. It also reported
> errors in connection to random other drivers that did a DMA mapping for
> an address that was previously mapped by i915 but was never released.
> Besides these diagnostic messages and the memory space starvation
> problem for IOMMUs, I'm not aware of this causing a real issue.
> 
> The fix is based on a patch from Chris.
> 
> v2:
> - move the DMA mapping create/remove calls to the get_pages/put_pages
>   callbacks instead of adding new callbacks for these (Chris)
> v3:
> - also fix the get_page cache logic on the userptr async path (Chris)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org

Note to future self, I still like adding __i915_gem_object_init_pages()
so that we can get that bit of incestrous knowledge out of userptr.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 1/2] drm/i915: avoid leaking DMA mappings

[Chris Wilson]

On Thu, Jul 09, 2015 at 12:59:05PM +0300, Imre Deak wrote:
> +static int
> +__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
> +			     struct page **pvec, int num_pages)
> +{
> +	int ret;
> +
> +	ret = st_set_pages(&obj->pages, pvec, num_pages);
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_gem_gtt_prepare_object(obj);
> +	if (ret) {
> +		sg_free_table(obj->pages);
> +		kfree(obj->pages);
> +		obj->pages = NULL;

Oh dear, we just leaked a ref one each page.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 1/2] drm/i915: avoid leaking DMA mappings

[Imre Deak]

On la, 2015-07-11 at 21:54 +0100, Chris Wilson wrote:
> On Thu, Jul 09, 2015 at 12:59:05PM +0300, Imre Deak wrote:
> > +static int
> > +__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj,
> > +			     struct page **pvec, int num_pages)
> > +{
> > +	int ret;
> > +
> > +	ret = st_set_pages(&obj->pages, pvec, num_pages);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i915_gem_gtt_prepare_object(obj);
> > +	if (ret) {
> > +		sg_free_table(obj->pages);
> > +		kfree(obj->pages);
> > +		obj->pages = NULL;
> 
> Oh dear, we just leaked a ref one each page.

To summarize the IRC discussion on this: it would be logical that
sg_set_page() takes a ref - and in that case this would result in
leaking those refs - but this is not so. Instead we rely on the GUP refs
which we keep in case of success by setting pinned=0 (release_pages will
be a nop) and drop in case of failure by passing the original pinned
value to release_pages(). So after checking both the sync and async
userptr paths this looks ok to me.

--Imre

Re: [PATCH 2/2] drm/i915: remove unused has_dma_mapping flag

[Daniel Vetter]

On Wed, Jul 08, 2015 at 06:43:23PM +0100, Chris Wilson wrote:
> On Wed, Jul 08, 2015 at 07:18:59PM +0300, Imre Deak wrote:
> > After the previous patch this flag will check always clear, as it's
> > never set for shmem backed and userptr objects, so we can remove it.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Mentioned a trivial obj->get_page bugfix for
> __i915_gem_userptr_set_pages(), and then went on an archaelogical dig to
> understand why I didn't think of this earlier. Apparently, it just never
> occurred to me that I could remove this flag introduced for
> i915_gem_prime when we did the get_pages/put_pages support for it.
> 
> Both patches (incl. the minor fix above for 1),
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Applied to -fixes (too lazy for a backmerge), thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx