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

Regards,

Tvrtko