[Intel-gfx] [RFC 24/28] drm/i915: Compartmentalize i915_ggtt_cleanup_hw

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-13 16:19:00)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Continuing on the theme of better logical organization of our code, make
> the first step towards making the ggtt code better isolated from wider
> struct drm_i915_private.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 54 +++++++++++++++++------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d09a4d9b71da..285a7a02c015 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2832,6 +2832,8 @@ static void fini_aliasing_ppgtt(struct drm_i915_private *i915)
>         struct i915_ggtt *ggtt = &i915->ggtt;
>         struct i915_ppgtt *ppgtt;
>  
> +       mutex_lock(&i915->drm.struct_mutex);

Do we still need to appease lockdep here? Hopefully not.

> +
>         ppgtt = fetch_and_zero(&i915->mm.aliasing_ppgtt);
>         if (!ppgtt)
>                 return;
> @@ -2840,6 +2842,8 @@ static void fini_aliasing_ppgtt(struct drm_i915_private *i915)
>  
>         ggtt->vm.vma_ops.bind_vma   = ggtt_bind_vma;
>         ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
> +
> +       mutex_unlock(&i915->drm.struct_mutex);
>  }
>  
>  static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
> @@ -2941,21 +2945,15 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>         return ret;
>  }
>  
> -/**
> - * i915_ggtt_cleanup_hw - Clean up GGTT hardware initialization
> - * @dev_priv: i915 device
> - */
> -void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
> +static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
>  {
> -       struct i915_ggtt *ggtt = &dev_priv->ggtt;
>         struct i915_address_space *vm = &ggtt->vm;
> +       struct drm_i915_private *i915 = vm->i915;
>         struct i915_vma *vma, *vn;
> -       struct pagevec *pvec;
>  
>         vm->closed = true;
>  
> -       mutex_lock(&dev_priv->drm.struct_mutex);
> -       fini_aliasing_ppgtt(dev_priv);
> +       mutex_lock(&i915->drm.struct_mutex);

As the lock here is just for the i915_vma_unbind and not
fini_aliasing_ppgtt.

>         list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link)
>                 WARN_ON(i915_vma_unbind(vma));
> @@ -2972,18 +2970,33 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv)
>  
>         vm->cleanup(vm);
>  
> -       pvec = &dev_priv->mm.wc_stash.pvec;
> +       mutex_unlock(&i915->drm.struct_mutex);
> +
> +       arch_phys_wc_del(ggtt->mtrr);
> +       io_mapping_fini(&ggtt->iomap);
> +}
> +
> +/**
> + * i915_ggtt_cleanup_hw - Clean up GGTT hardware initialization
> + * @dev_priv: i915 device
> + */
> +void i915_ggtt_cleanup_hw(struct drm_i915_private *i915)
> +{
> +       struct pagevec *pvec;
> +
> +       fini_aliasing_ppgtt(i915);
> +
> +       ggtt_cleanup_hw(&i915->ggtt);
> +
> +       mutex_lock(&i915->drm.struct_mutex);
> +       pvec = &i915->mm.wc_stash.pvec;
>         if (pvec->nr) {
>                 set_pages_array_wb(pvec->pages, pvec->nr);
>                 __pagevec_release(pvec);
>         }
> +       mutex_unlock(&i915->drm.struct_mutex);

The wc_stash doesn't use struct_mutex, so no lockdep appeasing required
here.

In general looks good, just too heavy handed on keeping struct_mutex
about.
-Chris