[2/3] drm/i915/error: standardize function style in error capture
diff mbox

Message ID 20180302191930.15236-2-daniele.ceraolospurio@intel.com
State New
Headers show

Commit Message

Daniele Ceraolo Spurio March 2, 2018, 7:19 p.m. UTC
some of the static functions used from capture() have the "i915_"
prefix while other don't; most of them take i915 as a parameter, but one
of them derives it internally from error->i915. Let's be consistent by
avoiding prefix for static functions and always providing i915 as a
parameter.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 47 ++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 23 deletions(-)

Comments

Michal Wajdeczko March 2, 2018, 8:07 p.m. UTC | #1
On Fri, 02 Mar 2018 20:19:29 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> some of the static functions used from capture() have the "i915_"
> prefix while other don't; most of them take i915 as a parameter, but one
> of them derives it internally from error->i915. Let's be consistent by
> avoiding prefix for static functions and always providing i915 as a
> parameter.

Maybe this one static function that derived i915 from error->i915 is the
one that did it correctly? I see no point in passing dev_priv directly
as extra param as it is already attached to passed gpu error state.

/Michal

>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gpu_error.c | 47  
> ++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c  
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index ef29fb48d6d9..d1f96e6a723a 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1084,8 +1084,8 @@ static uint32_t i915_error_generate_code(struct  
> drm_i915_private *dev_priv,
>  	return error_code;
>  }
> -static void i915_gem_record_fences(struct drm_i915_private *dev_priv,
> -				   struct i915_gpu_state *error)
> +static void gem_record_fences(struct drm_i915_private *dev_priv,
> +			      struct i915_gpu_state *error)
>  {
>  	int i;
> @@ -1424,8 +1424,8 @@ capture_object(struct drm_i915_private *dev_priv,
>  	}
>  }
> -static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
> -				  struct i915_gpu_state *error)
> +static void gem_record_rings(struct drm_i915_private *dev_priv,
> +			     struct i915_gpu_state *error)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	int i;
> @@ -1527,8 +1527,8 @@ static void i915_gem_capture_vm(struct  
> drm_i915_private *dev_priv,
>  	error->active_bo_count[idx] = count;
>  }
> -static void i915_capture_active_buffers(struct drm_i915_private  
> *dev_priv,
> -					struct i915_gpu_state *error)
> +static void capture_active_buffers(struct drm_i915_private *dev_priv,
> +				   struct i915_gpu_state *error)
>  {
>  	int cnt = 0, i, j;
> @@ -1552,8 +1552,8 @@ static void i915_capture_active_buffers(struct  
> drm_i915_private *dev_priv,
>  	}
>  }
> -static void i915_capture_pinned_buffers(struct drm_i915_private  
> *dev_priv,
> -					struct i915_gpu_state *error)
> +static void capture_pinned_buffers(struct drm_i915_private *dev_priv,
> +				   struct i915_gpu_state *error)
>  {
>  	struct i915_address_space *vm = &dev_priv->ggtt.base;
>  	struct drm_i915_error_buffer *bo;
> @@ -1583,9 +1583,9 @@ static void i915_capture_pinned_buffers(struct  
> drm_i915_private *dev_priv,
>  	error->pinned_bo = bo;
>  }
> -static void capture_uc_state(struct i915_gpu_state *error)
> +static void capture_uc_state(struct drm_i915_private *i915,
> +			     struct i915_gpu_state *error)
>  {
> -	struct drm_i915_private *i915 = error->i915;
>  	struct i915_error_uc *error_uc = &error->uc;
> 	/* Capturing uC state won't be useful if there is no GuC */
> @@ -1605,8 +1605,8 @@ static void capture_uc_state(struct i915_gpu_state  
> *error)
>  }
> /* Capture all registers which don't fit into another category. */
> -static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
> -				   struct i915_gpu_state *error)
> +static void capture_reg_state(struct drm_i915_private *dev_priv,
> +			      struct i915_gpu_state *error)
>  {
>  	int i;
> @@ -1704,8 +1704,8 @@ static void i915_error_capture_msg(struct  
> drm_i915_private *dev_priv,
>  		  engine_mask ? "reset" : "continue");
>  }
> -static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
> -				   struct i915_gpu_state *error)
> +static void capture_gen_state(struct drm_i915_private *dev_priv,
> +			      struct i915_gpu_state *error)
>  {
>  	error->awake = dev_priv->gt.awake;
>  	error->wakelock = atomic_read(&dev_priv->runtime_pm.wakeref_count);
> @@ -1741,6 +1741,7 @@ static void capture_params(struct i915_gpu_state  
> *error)
>  static int capture(void *data)
>  {
>  	struct i915_gpu_state *error = data;
> +	struct drm_i915_private *i915 = error->i915;
> 	error->time = ktime_get_real();
>  	error->boottime = ktime_get_boottime();
> @@ -1748,17 +1749,17 @@ static int capture(void *data)
>  				  error->i915->gt.last_init_time);
> 	capture_params(error);
> -	capture_uc_state(error);
> +	capture_uc_state(i915, error);
> -	i915_capture_gen_state(error->i915, error);
> -	i915_capture_reg_state(error->i915, error);
> -	i915_gem_record_fences(error->i915, error);
> -	i915_gem_record_rings(error->i915, error);
> -	i915_capture_active_buffers(error->i915, error);
> -	i915_capture_pinned_buffers(error->i915, error);
> +	capture_gen_state(i915, error);
> +	capture_reg_state(i915, error);
> +	gem_record_fences(i915, error);
> +	gem_record_rings(i915, error);
> +	capture_active_buffers(i915, error);
> +	capture_pinned_buffers(i915, error);
> -	error->overlay = intel_overlay_capture_error_state(error->i915);
> -	error->display = intel_display_capture_error_state(error->i915);
> +	error->overlay = intel_overlay_capture_error_state(i915);
> +	error->display = intel_display_capture_error_state(i915);
> 	return 0;
>  }
Chris Wilson March 3, 2018, 9:54 a.m. UTC | #2
Quoting Michal Wajdeczko (2018-03-02 20:07:54)
> On Fri, 02 Mar 2018 20:19:29 +0100, Daniele Ceraolo Spurio  
> <daniele.ceraolospurio@intel.com> wrote:
> 
> > some of the static functions used from capture() have the "i915_"
> > prefix while other don't; most of them take i915 as a parameter, but one
> > of them derives it internally from error->i915. Let's be consistent by
> > avoiding prefix for static functions and always providing i915 as a
> > parameter.
> 
> Maybe this one static function that derived i915 from error->i915 is the
> one that did it correctly? I see no point in passing dev_priv directly
> as extra param as it is already attached to passed gpu error state.

Yeah, we'll take readability over saving an instruction or two as the
compiler should be clever enough to do the work for us... I wonder if a
flatten directive would help...
-Chris
Chris Wilson March 3, 2018, 9:59 a.m. UTC | #3
Quoting Chris Wilson (2018-03-03 09:54:02)
> Quoting Michal Wajdeczko (2018-03-02 20:07:54)
> > On Fri, 02 Mar 2018 20:19:29 +0100, Daniele Ceraolo Spurio  
> > <daniele.ceraolospurio@intel.com> wrote:
> > 
> > > some of the static functions used from capture() have the "i915_"
> > > prefix while other don't; most of them take i915 as a parameter, but one
> > > of them derives it internally from error->i915. Let's be consistent by
> > > avoiding prefix for static functions and always providing i915 as a
> > > parameter.
> > 
> > Maybe this one static function that derived i915 from error->i915 is the
> > one that did it correctly? I see no point in passing dev_priv directly
> > as extra param as it is already attached to passed gpu error state.
> 
> Yeah, we'll take readability over saving an instruction or two as the
> compiler should be clever enough to do the work for us... I wonder if a
> flatten directive would help...

add/remove: 0/3 grow/shrink: 1/0 up/down: 11381/-1525 (9856)
Function                                     old     new   delta
capture                                     6159   17540  +11381
capture_object                               135       -    -135
capture_error_bo                             493       -    -493
i915_error_object_create                     897       -    -897

Waa! That wasn't quite the effect I was expecting.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index ef29fb48d6d9..d1f96e6a723a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1084,8 +1084,8 @@  static uint32_t i915_error_generate_code(struct drm_i915_private *dev_priv,
 	return error_code;
 }
 
-static void i915_gem_record_fences(struct drm_i915_private *dev_priv,
-				   struct i915_gpu_state *error)
+static void gem_record_fences(struct drm_i915_private *dev_priv,
+			      struct i915_gpu_state *error)
 {
 	int i;
 
@@ -1424,8 +1424,8 @@  capture_object(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
-				  struct i915_gpu_state *error)
+static void gem_record_rings(struct drm_i915_private *dev_priv,
+			     struct i915_gpu_state *error)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	int i;
@@ -1527,8 +1527,8 @@  static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 	error->active_bo_count[idx] = count;
 }
 
-static void i915_capture_active_buffers(struct drm_i915_private *dev_priv,
-					struct i915_gpu_state *error)
+static void capture_active_buffers(struct drm_i915_private *dev_priv,
+				   struct i915_gpu_state *error)
 {
 	int cnt = 0, i, j;
 
@@ -1552,8 +1552,8 @@  static void i915_capture_active_buffers(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void i915_capture_pinned_buffers(struct drm_i915_private *dev_priv,
-					struct i915_gpu_state *error)
+static void capture_pinned_buffers(struct drm_i915_private *dev_priv,
+				   struct i915_gpu_state *error)
 {
 	struct i915_address_space *vm = &dev_priv->ggtt.base;
 	struct drm_i915_error_buffer *bo;
@@ -1583,9 +1583,9 @@  static void i915_capture_pinned_buffers(struct drm_i915_private *dev_priv,
 	error->pinned_bo = bo;
 }
 
-static void capture_uc_state(struct i915_gpu_state *error)
+static void capture_uc_state(struct drm_i915_private *i915,
+			     struct i915_gpu_state *error)
 {
-	struct drm_i915_private *i915 = error->i915;
 	struct i915_error_uc *error_uc = &error->uc;
 
 	/* Capturing uC state won't be useful if there is no GuC */
@@ -1605,8 +1605,8 @@  static void capture_uc_state(struct i915_gpu_state *error)
 }
 
 /* Capture all registers which don't fit into another category. */
-static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
-				   struct i915_gpu_state *error)
+static void capture_reg_state(struct drm_i915_private *dev_priv,
+			      struct i915_gpu_state *error)
 {
 	int i;
 
@@ -1704,8 +1704,8 @@  static void i915_error_capture_msg(struct drm_i915_private *dev_priv,
 		  engine_mask ? "reset" : "continue");
 }
 
-static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
-				   struct i915_gpu_state *error)
+static void capture_gen_state(struct drm_i915_private *dev_priv,
+			      struct i915_gpu_state *error)
 {
 	error->awake = dev_priv->gt.awake;
 	error->wakelock = atomic_read(&dev_priv->runtime_pm.wakeref_count);
@@ -1741,6 +1741,7 @@  static void capture_params(struct i915_gpu_state *error)
 static int capture(void *data)
 {
 	struct i915_gpu_state *error = data;
+	struct drm_i915_private *i915 = error->i915;
 
 	error->time = ktime_get_real();
 	error->boottime = ktime_get_boottime();
@@ -1748,17 +1749,17 @@  static int capture(void *data)
 				  error->i915->gt.last_init_time);
 
 	capture_params(error);
-	capture_uc_state(error);
+	capture_uc_state(i915, error);
 
-	i915_capture_gen_state(error->i915, error);
-	i915_capture_reg_state(error->i915, error);
-	i915_gem_record_fences(error->i915, error);
-	i915_gem_record_rings(error->i915, error);
-	i915_capture_active_buffers(error->i915, error);
-	i915_capture_pinned_buffers(error->i915, error);
+	capture_gen_state(i915, error);
+	capture_reg_state(i915, error);
+	gem_record_fences(i915, error);
+	gem_record_rings(i915, error);
+	capture_active_buffers(i915, error);
+	capture_pinned_buffers(i915, error);
 
-	error->overlay = intel_overlay_capture_error_state(error->i915);
-	error->display = intel_display_capture_error_state(error->i915);
+	error->overlay = intel_overlay_capture_error_state(i915);
+	error->display = intel_display_capture_error_state(i915);
 
 	return 0;
 }