Re: [RFC 24/28] drm/i915: Compartmentalize i915_ggtt_cleanup_hw

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 13/06/2019 16:36, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-13 16:19:00)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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.

Maybe not in your tree :) but in drm-tip AFAICS:

   fini_aliasing_ppgtt
     -> i915_vm_put
       -> i915_vm_release
         -> ppgtt_destroy_vma
          -> i915_vma_destroy

>> +
>>          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.

True, removed.

Regards,

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