Re: [PATCH v3 3/7] drm/i915: Require the vm mutex for i915_vma_bind()

2021-12-21 Thread Matthew Auld

On 17/12/2021 14:52, Thomas Hellström wrote:

Protect updates of struct i915_vma flags and async binding / unbinding
with the vm::mutex. This means that i915_vma_bind() needs to assert
vm::mutex held. In order to make that possible drop the caching of
kmap_atomic() maps around i915_vma_bind().

An alternative would be to use kmap_local() but since we block cpu
unplugging during sleeps inside kmap_local() sections this may have
unwanted side-effects. Particularly since we might wait for gpu while
holding the vm mutex.

This change may theoretically increase execbuf cpu-usage on snb, but
at least on non-highmem systems that increase should be very small.

Signed-off-by: Thomas Hellström 

Reviewed-by: Matthew Auld 



[PATCH v3 3/7] drm/i915: Require the vm mutex for i915_vma_bind()

2021-12-17 Thread Thomas Hellström
Protect updates of struct i915_vma flags and async binding / unbinding
with the vm::mutex. This means that i915_vma_bind() needs to assert
vm::mutex held. In order to make that possible drop the caching of
kmap_atomic() maps around i915_vma_bind().

An alternative would be to use kmap_local() but since we block cpu
unplugging during sleeps inside kmap_local() sections this may have
unwanted side-effects. Particularly since we might wait for gpu while
holding the vm mutex.

This change may theoretically increase execbuf cpu-usage on snb, but
at least on non-highmem systems that increase should be very small.

Signed-off-by: Thomas Hellström 
---
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 50 ++-
 drivers/gpu/drm/i915/i915_vma.c   |  1 +
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 2213f7b613da..6013f7e18f60 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1109,6 +1109,47 @@ static inline struct i915_ggtt *cache_to_ggtt(struct 
reloc_cache *cache)
return &i915->ggtt;
 }
 
+static void reloc_cache_unmap(struct reloc_cache *cache)
+{
+   void *vaddr;
+
+   if (!cache->vaddr)
+   return;
+
+   vaddr = unmask_page(cache->vaddr);
+   if (cache->vaddr & KMAP)
+   kunmap_atomic(vaddr);
+   else
+   io_mapping_unmap_atomic((void __iomem *)vaddr);
+}
+
+static void reloc_cache_remap(struct reloc_cache *cache,
+ struct drm_i915_gem_object *obj)
+{
+   void *vaddr;
+
+   if (!cache->vaddr)
+   return;
+
+   if (cache->vaddr & KMAP) {
+   struct page *page = i915_gem_object_get_page(obj, cache->page);
+
+   vaddr = kmap_atomic(page);
+   cache->vaddr = unmask_flags(cache->vaddr) |
+   (unsigned long)vaddr;
+   } else {
+   struct i915_ggtt *ggtt = cache_to_ggtt(cache);
+   unsigned long offset;
+
+   offset = cache->node.start;
+   if (!drm_mm_node_allocated(&cache->node))
+   offset += cache->page << PAGE_SHIFT;
+
+   cache->vaddr = (unsigned long)
+   io_mapping_map_atomic_wc(&ggtt->iomap, offset);
+   }
+}
+
 static void reloc_cache_reset(struct reloc_cache *cache, struct 
i915_execbuffer *eb)
 {
void *vaddr;
@@ -1373,10 +1414,17 @@ eb_relocate_entry(struct i915_execbuffer *eb,
 * batchbuffers.
 */
if (reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-   GRAPHICS_VER(eb->i915) == 6) {
+   GRAPHICS_VER(eb->i915) == 6 &&
+   !i915_vma_is_bound(target->vma, I915_VMA_GLOBAL_BIND)) {
+   struct i915_vma *vma = target->vma;
+
+   reloc_cache_unmap(&eb->reloc_cache);
+   mutex_lock(&vma->vm->mutex);
err = i915_vma_bind(target->vma,
target->vma->obj->cache_level,
PIN_GLOBAL, NULL);
+   mutex_unlock(&vma->vm->mutex);
+   reloc_cache_remap(&eb->reloc_cache, ev->vma->obj);
if (err)
return err;
}
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 927f0d4f8e11..d792a3d0da7a 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -398,6 +398,7 @@ int i915_vma_bind(struct i915_vma *vma,
u32 bind_flags;
u32 vma_flags;
 
+   lockdep_assert_held(&vma->vm->mutex);
GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
GEM_BUG_ON(vma->size > vma->node.size);
 
-- 
2.31.1