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

Tvrtko Ursulin
2019-06-14T10:28:02Z

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