Re: [Intel-gfx] [PATCH 07/16] drm/i915: Tidy generation of the GTT mmap offset

[Joonas Lahtinen]

On to, 2016-08-04 at 08:30 +0100, Chris Wilson wrote:
> On Thu, Aug 04, 2016 at 10:25:42AM +0300, Joonas Lahtinen wrote:
> > 
> > On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > > 
> > > If we make the observation that mmap-offsets are only released when we
> > > free an object, we can then deduce that the shrinker only creates free
> > > space in the mmap arena indirectly by flushing the request list and
> > > freeing expired objects. If we combine this with the lockless
> > > vma-manager and lockless idling, we can avoid taking our big struct_mutex
> > > until we need to actually free the requests.
> > > 
> > > One side-effect is that we defer the madvise checking until we need the
> > > pages (i.e. the fault handler). This brings us into line with the other
> > > delayed checks (and madvise in general).
> > > 
> > > Signed-off-by: Chris Wilson 
> > Don't necessarily agree with all the "== 0", because it's in the
> > minority but;
> How about if I take this opportunity to start a new life using
> 
> int err;
> if (!err) ...

"int ret;" is quite de facto and if (!ret) too;

$ git grep '!ret\b' | wc -l
3826
$ git grep 'ret == 0\b' | wc -l
1869
$ git grep '!err\b' | wc -l
2064

So I'm inclined towards ret and !ret...

Regards, Joonas

> 
> ?
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/16] drm/i915: Tidy generation of the GTT mmap offset

[Chris Wilson]

On Thu, Aug 04, 2016 at 10:25:42AM +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> > If we make the observation that mmap-offsets are only released when we
> > free an object, we can then deduce that the shrinker only creates free
> > space in the mmap arena indirectly by flushing the request list and
> > freeing expired objects. If we combine this with the lockless
> > vma-manager and lockless idling, we can avoid taking our big struct_mutex
> > until we need to actually free the requests.
> > 
> > One side-effect is that we defer the madvise checking until we need the
> > pages (i.e. the fault handler). This brings us into line with the other
> > delayed checks (and madvise in general).
> > 
> > Signed-off-by: Chris Wilson 
> 
> Don't necessarily agree with all the "== 0", because it's in the
> minority but;

How about if I take this opportunity to start a new life using

int err;
if (!err) ...

?
-Chris

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

Re: [Intel-gfx] [PATCH 07/16] drm/i915: Tidy generation of the GTT mmap offset

[Joonas Lahtinen]

On ma, 2016-08-01 at 19:22 +0100, Chris Wilson wrote:
> If we make the observation that mmap-offsets are only released when we
> free an object, we can then deduce that the shrinker only creates free
> space in the mmap arena indirectly by flushing the request list and
> freeing expired objects. If we combine this with the lockless
> vma-manager and lockless idling, we can avoid taking our big struct_mutex
> until we need to actually free the requests.
> 
> One side-effect is that we defer the madvise checking until we need the
> pages (i.e. the fault handler). This brings us into line with the other
> delayed checks (and madvise in general).
> 
> Signed-off-by: Chris Wilson 

Don't necessarily agree with all the "== 0", because it's in the
minority but;

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 07/16] drm/i915: Tidy generation of the GTT mmap offset

[Chris Wilson]

If we make the observation that mmap-offsets are only released when we
free an object, we can then deduce that the shrinker only creates free
space in the mmap arena indirectly by flushing the request list and
freeing expired objects. If we combine this with the lockless
vma-manager and lockless idling, we can avoid taking our big struct_mutex
until we need to actually free the requests.

One side-effect is that we defer the madvise checking until we need the
pages (i.e. the fault handler). This brings us into line with the other
delayed checks (and madvise in general).

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 65 +
 1 file changed, 20 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8ac9605d5125..0c5d2872649c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1891,35 +1891,27 @@ u64 i915_gem_get_ggtt_alignment(struct drm_i915_private 
*dev_priv, u64 size,
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
 {
-   struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+   struct drm_device *dev = obj->base.dev;
int ret;
 
-   dev_priv->mm.shrinker_no_lock_stealing = true;
-
ret = drm_gem_create_mmap_offset(>base);
-   if (ret != -ENOSPC)
-   goto out;
+   if (ret == 0)
+   return 0;
 
-   /* Badly fragmented mmap space? The only way we can recover
-* space is by destroying unwanted objects. We can't randomly release
-* mmap_offsets as userspace expects them to be persistent for the
-* lifetime of the objects. The closest we can is to release the
-* offsets on purgeable objects by truncating it and marking it purged,
-* which prevents userspace from ever using that object again.
+   /* We can idle the GPU locklessly to flush stale objects, but in order
+* to claim that space for ourselves, we need to take the big
+* struct_mutex to free the requests+objects and allocate our slot.
 */
-   i915_gem_shrink(dev_priv,
-   obj->base.size >> PAGE_SHIFT,
-   I915_SHRINK_BOUND |
-   I915_SHRINK_UNBOUND |
-   I915_SHRINK_PURGEABLE);
-   ret = drm_gem_create_mmap_offset(>base);
-   if (ret != -ENOSPC)
-   goto out;
+   ret = i915_gem_wait_for_idle(to_i915(dev));
+   if (ret)
+   return ret;
 
-   i915_gem_shrink_all(dev_priv);
-   ret = drm_gem_create_mmap_offset(>base);
-out:
-   dev_priv->mm.shrinker_no_lock_stealing = false;
+   ret = i915_mutex_lock_interruptible(dev);
+   if (ret == 0) {
+   i915_gem_retire_requests(to_i915(dev));
+   ret = drm_gem_create_mmap_offset(>base);
+   mutex_unlock(>struct_mutex);
+   }
 
return ret;
 }
@@ -1938,32 +1930,15 @@ i915_gem_mmap_gtt(struct drm_file *file,
struct drm_i915_gem_object *obj;
int ret;
 
-   ret = i915_mutex_lock_interruptible(dev);
-   if (ret)
-   return ret;
-
obj = i915_gem_object_lookup(file, handle);
-   if (!obj) {
-   ret = -ENOENT;
-   goto unlock;
-   }
-
-   if (obj->madv != I915_MADV_WILLNEED) {
-   DRM_DEBUG("Attempting to mmap a purgeable buffer\n");
-   ret = -EFAULT;
-   goto out;
-   }
+   if (!obj)
+   return -ENOENT;
 
ret = i915_gem_object_create_mmap_offset(obj);
-   if (ret)
-   goto out;
+   if (ret == 0)
+   *offset = drm_vma_node_offset_addr(>base.vma_node);
 
-   *offset = drm_vma_node_offset_addr(>base.vma_node);
-
-out:
-   i915_gem_object_put(obj);
-unlock:
-   mutex_unlock(>struct_mutex);
+   i915_gem_object_put_unlocked(obj);
return ret;
 }
 
-- 
2.8.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx