Re: [PATCH 2/3] drm/i915/error: standardize function style in error capture

["Michal Wajdeczko" <michal.wajdeczko@xxxxxxxxx>]

On Fri, 02 Mar 2018 20:19:29 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx