| Message ID | 20180302191930.15236-2-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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
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
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; }
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(-)