[Intel-gfx] [PATCH] drm/i915: Simplify i915_gem_release_all_mmaps()

[Paulo Zanoni]

2014/1/16 Chris Wilson <chris at 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 at 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 at chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni