diff mbox series

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

Message ID 20190613151904.16256-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Implicit dev_priv removal and GT compartmentalization | expand

Commit Message

Tvrtko Ursulin June 13, 2019, 3:19 p.m. UTC 
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(-)

Comments

Chris Wilson June 13, 2019, 3:36 p.m. UTC | #1 
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.

> +
>         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
Tvrtko Ursulin June 14, 2019, 10:28 a.m. UTC | #2 
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
diff mbox series

Patch

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);
+
 	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);
 
 	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);
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	arch_phys_wc_del(ggtt->mtrr);
-	io_mapping_fini(&ggtt->iomap);
-
-	i915_gem_cleanup_stolen(dev_priv);
+	i915_gem_cleanup_stolen(i915);
 }
 
 static unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
@@ -3582,11 +3595,6 @@  int i915_ggtt_probe_hw(struct drm_i915_private *i915)
 	return 0;
 }
 
-static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
-{
-	ggtt->vm.cleanup(&ggtt->vm);
-}
-
 static int ggtt_init_hw(struct i915_ggtt *ggtt)
 {
 	struct i915_address_space *vm = &ggtt->vm;
@@ -3608,7 +3616,7 @@  static int ggtt_init_hw(struct i915_ggtt *ggtt)
 	if (!io_mapping_init_wc(&ggtt->iomap,
 				ggtt->gmadr.start,
 				ggtt->mappable_end)) {
-		ggtt_cleanup_hw(ggtt);
+		vm->cleanup(vm);
 		ret = -EIO;
 		goto out;
 	}
@@ -3653,7 +3661,7 @@  int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
 	return 0;
 
 out_gtt_cleanup:
-	ggtt_cleanup_hw(&dev_priv->ggtt);
+	dev_priv->ggtt.vm.cleanup(&dev_priv->ggtt.vm);
 	return ret;
 }