[PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[Chris Wilson]

An object can only have an active gtt mapping if it is currently bound
into the global gtt. Therefore we can simply walk the list of all bound
objects and check the flag upon those for an active gtt mapping.

>From commit 48018a57a8f5900e7e53ffaa0adeb784095accfb
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Dec 13 15:22:31 2013 -0200

    drm/i915: release the GTT mmaps when going into D3

Also note that the WARN is inappropriate for this function as GPU
activity is orthogonal to GTT mmap status. Rather it is the caller that
relies upon this condition and so it should assert that the GPU is idle
itself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 39c25ebc36b7..74f583fcab3e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1562,22 +1562,6 @@ out:
 	return ret;
 }
 
-void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
-{
-	struct i915_vma *vma;
-
-	/*
-	 * Only the global gtt is relevant for gtt memory mappings, so restrict
-	 * list traversal to objects bound into the global address space. Note
-	 * that the active list should be empty, but better safe than sorry.
-	 */
-	WARN_ON(!list_empty(&dev_priv->gtt.base.active_list));
-	list_for_each_entry(vma, &dev_priv->gtt.base.active_list, mm_list)
-		i915_gem_release_mmap(vma->obj);
-	list_for_each_entry(vma, &dev_priv->gtt.base.inactive_list, mm_list)
-		i915_gem_release_mmap(vma->obj);
-}
-
 /**
  * i915_gem_release_mmap - remove physical page mappings
  * @obj: obj in question
@@ -1602,6 +1586,15 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
+void
+i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
+{
+	struct drm_i915_gem_object *obj;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		i915_gem_release_mmap(obj);
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
-- 
1.8.5.2

Re: [PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[Paulo Zanoni]

2014/1/16 Chris Wilson <chris@chris-wilson.co.uk>:
> An object can only have an active gtt mapping if it is currently bound
> into the global gtt. Therefore we can simply walk the list of all bound
> objects and check the flag upon those for an active gtt mapping.
>
> From commit 48018a57a8f5900e7e53ffaa0adeb784095accfb
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Dec 13 15:22:31 2013 -0200
>
>     drm/i915: release the GTT mmaps when going into D3
>
> Also note that the WARN is inappropriate for this function as GPU
> activity is orthogonal to GTT mmap status. Rather it is the caller that
> relies upon this condition and so it should assert that the GPU is idle
> itself.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 39c25ebc36b7..74f583fcab3e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1562,22 +1562,6 @@ out:
>         return ret;
>  }
>
> -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> -{
> -       struct i915_vma *vma;
> -
> -       /*
> -        * Only the global gtt is relevant for gtt memory mappings, so restrict
> -        * list traversal to objects bound into the global address space. Note
> -        * that the active list should be empty, but better safe than sorry.
> -        */
> -       WARN_ON(!list_empty(&dev_priv->gtt.base.active_list));
> -       list_for_each_entry(vma, &dev_priv->gtt.base.active_list, mm_list)
> -               i915_gem_release_mmap(vma->obj);
> -       list_for_each_entry(vma, &dev_priv->gtt.base.inactive_list, mm_list)
> -               i915_gem_release_mmap(vma->obj);
> -}
> -
>  /**
>   * i915_gem_release_mmap - remove physical page mappings
>   * @obj: obj in question
> @@ -1602,6 +1586,15 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>         obj->fault_mappable = false;
>  }
>
> +void
> +i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_i915_gem_object *obj;
> +
> +       list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> +               i915_gem_release_mmap(obj);
> +}
> +

This was exactly what I wrote in the first version:
http://lists.freedesktop.org/archives/intel-gfx/2013-November/036310.html
I had written this based on a description by Daniel. I asked his help
due to my lack of experience with this code.

But then later he changed his mind and guided me to implement what is
currently merged today:
http://lists.freedesktop.org/archives/intel-gfx/2013-December/037263.html

So your patch does work and it does pass the pm_pc8.c test suite,
because I tested it in the past. Based on that, you have my
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I also counted the amount of times we call i915_gem_release_mmap() on
both versions, and for the pm_pc8.c test suite, as far as I remember,
it was the same. But, of course, pm_pc8.c doesn't really reflect a
real use-case environment.

But I think you and Daniel should discuss and really decide which
version you want merged. I don't have any strong opinions here.

Thanks,
Paulo

>  uint32_t
>  i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
>  {
> --
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[Chris Wilson]

An object can only have an active gtt mapping if it is currently bound
into the global gtt. Therefore we can simply walk the list of all bound
objects and check the flag upon those for an active gtt mapping.

>From commit 48018a57a8f5900e7e53ffaa0adeb784095accfb
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Dec 13 15:22:31 2013 -0200

    drm/i915: release the GTT mmaps when going into D3

Also note that the WARN is inappropriate for this function as GPU
activity is orthogonal to GTT mmap status. Rather it is the caller that
relies upon this condition and so it should assert that the GPU is idle
itself.

References: https://bugs.freedesktop.org/show_bug.cgi?id=80081
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1d7893c982a8..f671216031e7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1862,22 +1862,6 @@ out:
 	return ret;
 }
 
-void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
-{
-	struct i915_vma *vma;
-
-	/*
-	 * Only the global gtt is relevant for gtt memory mappings, so restrict
-	 * list traversal to objects bound into the global address space. Note
-	 * that the active list should be empty, but better safe than sorry.
-	 */
-	WARN_ON(!list_empty(&dev_priv->gtt.base.active_list));
-	list_for_each_entry(vma, &dev_priv->gtt.base.active_list, mm_list)
-		i915_gem_release_mmap(vma->obj);
-	list_for_each_entry(vma, &dev_priv->gtt.base.inactive_list, mm_list)
-		i915_gem_release_mmap(vma->obj);
-}
-
 /**
  * i915_gem_release_mmap - remove physical page mappings
  * @obj: obj in question
@@ -1903,6 +1887,15 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	obj->fault_mappable = false;
 }
 
+void
+i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
+{
+	struct drm_i915_gem_object *obj;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		i915_gem_release_mmap(obj);
+}
+
 uint32_t
 i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
 {
-- 
2.0.0

Re: [PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[Daniel Vetter]

On Mon, Jun 16, 2014 at 08:57:44AM +0100, Chris Wilson wrote:
> An object can only have an active gtt mapping if it is currently bound
> into the global gtt. Therefore we can simply walk the list of all bound
> objects and check the flag upon those for an active gtt mapping.
> 
> From commit 48018a57a8f5900e7e53ffaa0adeb784095accfb
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Dec 13 15:22:31 2013 -0200
> 
>     drm/i915: release the GTT mmaps when going into D3
> 
> Also note that the WARN is inappropriate for this function as GPU
> activity is orthogonal to GTT mmap status. Rather it is the caller that
> relies upon this condition and so it should assert that the GPU is idle
> itself.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=80081
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1d7893c982a8..f671216031e7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1862,22 +1862,6 @@ out:
>  	return ret;
>  }
>  
> -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> -{
> -	struct i915_vma *vma;
> -
> -	/*
> -	 * Only the global gtt is relevant for gtt memory mappings, so restrict
> -	 * list traversal to objects bound into the global address space. Note
> -	 * that the active list should be empty, but better safe than sorry.
> -	 */
> -	WARN_ON(!list_empty(&dev_priv->gtt.base.active_list));
> -	list_for_each_entry(vma, &dev_priv->gtt.base.active_list, mm_list)
> -		i915_gem_release_mmap(vma->obj);
> -	list_for_each_entry(vma, &dev_priv->gtt.base.inactive_list, mm_list)
> -		i915_gem_release_mmap(vma->obj);
> -}
> -
>  /**
>   * i915_gem_release_mmap - remove physical page mappings
>   * @obj: obj in question
> @@ -1903,6 +1887,15 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>  	obj->fault_mappable = false;
>  }
>  
> +void
> +i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_i915_gem_object *obj;
> +
> +	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> +		i915_gem_release_mmap(obj);
> +}
> +
>  uint32_t
>  i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode)
>  {
> -- 
> 2.0.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch