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@xxxxxxxxx> > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html