diff mbox series

[RFC,20/31] drm/i915: Compartmentalize i915_gem_suspend/restore_gtt_mappings

Message ID 20190614151731.17608-21-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 14, 2019, 3:17 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Having made start to better code compartmentalization by introducing
struct intel_gt, continue the theme elsewhere in code by making functions
take parameters take what logically makes most sense for them instead of
the global struct drm_i915_private.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Chris Wilson June 14, 2019, 4:19 p.m. UTC | #1 
Quoting Tvrtko Ursulin (2019-06-14 16:17:20)
> +void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)

You could have just slipped in a s/dev_priv/i915/...

> +{
> +       ggtt_suspend_mappings(&dev_priv->ggtt);
> +}
Tvrtko Ursulin June 14, 2019, 4:22 p.m. UTC | #2 
On 14/06/2019 17:19, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-14 16:17:20)
>> +void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
> 
> You could have just slipped in a s/dev_priv/i915/...

In this instance I apparently forgot, but yes, marking as TODO.

Regards,

Tvrtko

>> +{
>> +       ggtt_suspend_mappings(&dev_priv->ggtt);
>> +}
>
Rodrigo Vivi June 14, 2019, 5:52 p.m. UTC | #3 
On Fri, Jun 14, 2019 at 04:17:20PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Having made start to better code compartmentalization by introducing
> struct intel_gt, continue the theme elsewhere in code by making functions
> take parameters take what logically makes most sense for them instead of
> the global struct drm_i915_private.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0bc75c963955..516ffc4a521a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2323,23 +2323,28 @@ static bool needs_idle_maps(struct drm_i915_private *dev_priv)
>  	return IS_GEN(dev_priv, 5) && IS_MOBILE(dev_priv) && intel_vtd_active();
>  }
>  
> -void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
> +static void ggtt_suspend_mappings(struct i915_ggtt *ggtt)
>  {
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct drm_i915_private *i915 = ggtt->vm.i915;
>  
>  	/* Don't bother messing with faults pre GEN6 as we have little
>  	 * documentation supporting that it's a good idea.
>  	 */
> -	if (INTEL_GEN(dev_priv) < 6)
> +	if (INTEL_GEN(i915) < 6)
>  		return;
>  
> -	intel_gt_check_and_clear_faults(&dev_priv->gt);
> +	intel_gt_check_and_clear_faults(ggtt->vm.gt);
>  
>  	ggtt->vm.clear_range(&ggtt->vm, 0, ggtt->vm.total);
>  
>  	ggtt->invalidate(ggtt);
>  }
>  
> +void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
> +{
> +	ggtt_suspend_mappings(&dev_priv->ggtt);
> +}
> +
>  int i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj,
>  			       struct sg_table *pages)
>  {
> @@ -3674,12 +3679,11 @@ void i915_ggtt_disable_guc(struct drm_i915_private *i915)
>  	ggtt->invalidate(ggtt);
>  }
>  
> -void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
> +static void ggtt_restore_mappings(struct i915_ggtt *ggtt)
>  {
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	struct i915_vma *vma, *vn;
>  
> -	intel_gt_check_and_clear_faults(&dev_priv->gt);
> +	intel_gt_check_and_clear_faults(ggtt->vm.gt);
>  
>  	mutex_lock(&ggtt->vm.mutex);
>  
> @@ -3716,6 +3720,11 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
>  	ggtt->invalidate(ggtt);
>  
>  	mutex_unlock(&ggtt->vm.mutex);
> +}
> +
> +void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)

I'd say maybe more places to s/dev_priv/i915, but I'm actually in favor
of the big sed so:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +{
> +	ggtt_restore_mappings(&dev_priv->ggtt);
>  
>  	if (INTEL_GEN(dev_priv) >= 8) {
>  		struct intel_ppat *ppat = &dev_priv->ppat;
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 0bc75c963955..516ffc4a521a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2323,23 +2323,28 @@  static bool needs_idle_maps(struct drm_i915_private *dev_priv)
 	return IS_GEN(dev_priv, 5) && IS_MOBILE(dev_priv) && intel_vtd_active();
 }
 
-void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
+static void ggtt_suspend_mappings(struct i915_ggtt *ggtt)
 {
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct drm_i915_private *i915 = ggtt->vm.i915;
 
 	/* Don't bother messing with faults pre GEN6 as we have little
 	 * documentation supporting that it's a good idea.
 	 */
-	if (INTEL_GEN(dev_priv) < 6)
+	if (INTEL_GEN(i915) < 6)
 		return;
 
-	intel_gt_check_and_clear_faults(&dev_priv->gt);
+	intel_gt_check_and_clear_faults(ggtt->vm.gt);
 
 	ggtt->vm.clear_range(&ggtt->vm, 0, ggtt->vm.total);
 
 	ggtt->invalidate(ggtt);
 }
 
+void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
+{
+	ggtt_suspend_mappings(&dev_priv->ggtt);
+}
+
 int i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj,
 			       struct sg_table *pages)
 {
@@ -3674,12 +3679,11 @@  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 	ggtt->invalidate(ggtt);
 }
 
-void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
+static void ggtt_restore_mappings(struct i915_ggtt *ggtt)
 {
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct i915_vma *vma, *vn;
 
-	intel_gt_check_and_clear_faults(&dev_priv->gt);
+	intel_gt_check_and_clear_faults(ggtt->vm.gt);
 
 	mutex_lock(&ggtt->vm.mutex);
 
@@ -3716,6 +3720,11 @@  void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
 	ggtt->invalidate(ggtt);
 
 	mutex_unlock(&ggtt->vm.mutex);
+}
+
+void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
+{
+	ggtt_restore_mappings(&dev_priv->ggtt);
 
 	if (INTEL_GEN(dev_priv) >= 8) {
 		struct intel_ppat *ppat = &dev_priv->ppat;