[Intel-gfx] [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 at 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