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

[Chris Wilson]

Quoting Michal Wajdeczko (2018-03-06 09:48:14)
> On Mon, 05 Mar 2018 23:21:21 +0100, Daniele Ceraolo Spurio  
> > static __always_inline void dup_param(const char *type, void *x)
> > @@ -1749,13 +1744,12 @@ static int capture(void *data)
> >   capture_params(error);
> >   capture_uc_state(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(error);
> > + capture_reg_state(error);
> > + gem_record_fences(error);
> > + gem_record_rings(error);
> > + capture_active_buffers(error);
> > + capture_pinned_buffers(error);
> >   error->overlay = intel_overlay_capture_error_state(error->i915);
> >   error->display = intel_display_capture_error_state(error->i915);
> 
> also, it looks that we are not consistent in using capture vs. record  
> verb...

Once upon a time, I fancied using it for different effects. Record
HW register state, capture user buffers (capture requires allocation and
whatnot). Not that I have ever been consistent.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michal Wajdeczko]

On Mon, 05 Mar 2018 23:21:21 +0100, Daniele Ceraolo Spurio  
 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 by getting i915 from
error->i915. While at it, s/dev_priv/i915 in functions that don't
perform register reads.

v2: take i915 from error->i915 (Michal), s/dev_priv/i915,
update commit message

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Signed-off-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 84  
---

 1 file changed, 39 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c  
b/drivers/gpu/drm/i915/i915_gpu_error.c

index ef29fb48d6d9..9afb1b9674c0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1084,9 +1084,9 @@ 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 i915_gpu_state *error)


hmm, as we drop i915 prefix then maybe we should start all static functions
with the verb, so s/gem_record_fences/record_gem_fences ?


 {
+   struct drm_i915_private *dev_priv = error->i915;
int i;
if (INTEL_GEN(dev_priv) >= 6) {
@@ -1424,14 +1424,14 @@ 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 i915_gpu_state *error)
 {
-   struct i915_ggtt *ggtt = _priv->ggtt;
+   struct drm_i915_private *i915 = error->i915;
+   struct i915_ggtt *ggtt = >ggtt;
int i;
for (i = 0; i < I915_NUM_ENGINES; i++) {
-   struct intel_engine_cs *engine = dev_priv->engine[i];
+   struct intel_engine_cs *engine = i915->engine[i];
struct drm_i915_error_engine *ee = >engine[i];
struct i915_request *request;
@@ -1460,17 +1460,16 @@ static void i915_gem_record_rings(struct  
drm_i915_private *dev_priv,

 * by userspace.
 */
ee->batchbuffer =
-   i915_error_object_create(dev_priv,
-request->batch);
+   i915_error_object_create(i915, request->batch);
-   if (HAS_BROKEN_CS_TLB(dev_priv))
+   if (HAS_BROKEN_CS_TLB(i915))
ee->wa_batchbuffer =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
engine->scratch);
request_record_user_bo(request, ee);
ee->ctx =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
request->ctx->engine[i].state);
error->simulated |=
@@ -1484,27 +1483,24 @@ static void i915_gem_record_rings(struct  
drm_i915_private *dev_priv,

ee->cpu_ring_head = ring->head;
ee->cpu_ring_tail = ring->tail;
ee->ringbuffer =
-   i915_error_object_create(dev_priv, ring->vma);
+   i915_error_object_create(i915, ring->vma);
engine_record_requests(engine, request, ee);
}
ee->hws_page =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 engine->status_page.vma);
-   ee->wa_ctx =
-   i915_error_object_create(dev_priv, engine->wa_ctx.vma);
+   ee->wa_ctx = i915_error_object_create(i915, engine->wa_ctx.vma);
-   ee->default_state =
-   capture_object(dev_priv, engine->default_state);
+   ee->default_state = capture_object(i915, engine->default_state);
}
 }
-static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
-   struct i915_gpu_state *error,
-   struct i915_address_space *vm,
-   int idx)
+static void gem_capture_vm(struct i915_gpu_state *error,
+  

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

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2018-03-05 22:21:21)
> 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 by getting i915 from
> error->i915. While at it, s/dev_priv/i915 in functions that don't
> perform register reads.
> 
> v2: take i915 from error->i915 (Michal), s/dev_priv/i915,
> update commit message
> 
> Cc: Michal Wajdeczko 
> Cc: Chris Wilson 
> Signed-off-by: Daniele Ceraolo Spurio 
Reviewed-by: Chris Wilson 
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Michel Thierry]

On 3/5/2018 2:21 PM, Daniele Ceraolo Spurio 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 by getting i915 from
error->i915.


Following the fist part of this logic, i915_error_object_free could 
probably be just error_object_free, but this would never end...


Changes look OK to me, feel free to add my r-b if you need it.


While at it, s/dev_priv/i915 in functions that don't
perform register reads.

v2: take i915 from error->i915 (Michal), s/dev_priv/i915,
 update commit message

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Signed-off-by: Daniele Ceraolo Spurio 


Reviewed-by: Michel Thierry 


---
  drivers/gpu/drm/i915/i915_gpu_error.c | 84 ---
  1 file changed, 39 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
b/drivers/gpu/drm/i915/i915_gpu_error.c
index ef29fb48d6d9..9afb1b9674c0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1084,9 +1084,9 @@ 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 i915_gpu_state *error)
  {
+   struct drm_i915_private *dev_priv = error->i915;
int i;
  
  	if (INTEL_GEN(dev_priv) >= 6) {

@@ -1424,14 +1424,14 @@ 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 i915_gpu_state *error)
  {
-   struct i915_ggtt *ggtt = _priv->ggtt;
+   struct drm_i915_private *i915 = error->i915;
+   struct i915_ggtt *ggtt = >ggtt;
int i;
  
  	for (i = 0; i < I915_NUM_ENGINES; i++) {

-   struct intel_engine_cs *engine = dev_priv->engine[i];
+   struct intel_engine_cs *engine = i915->engine[i];
struct drm_i915_error_engine *ee = >engine[i];
struct i915_request *request;
  
@@ -1460,17 +1460,16 @@ static void i915_gem_record_rings(struct drm_i915_private *dev_priv,

 * by userspace.
 */
ee->batchbuffer =
-   i915_error_object_create(dev_priv,
-request->batch);
+   i915_error_object_create(i915, request->batch);
  
-			if (HAS_BROKEN_CS_TLB(dev_priv))

+   if (HAS_BROKEN_CS_TLB(i915))
ee->wa_batchbuffer =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
engine->scratch);
request_record_user_bo(request, ee);
  
  			ee->ctx =

-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
request->ctx->engine[i].state);
  
  			error->simulated |=

@@ -1484,27 +1483,24 @@ static void i915_gem_record_rings(struct 
drm_i915_private *dev_priv,
ee->cpu_ring_head = ring->head;
ee->cpu_ring_tail = ring->tail;
ee->ringbuffer =
-   i915_error_object_create(dev_priv, ring->vma);
+   i915_error_object_create(i915, ring->vma);
  
  			engine_record_requests(engine, request, ee);

}
  
  		ee->hws_page =

-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 engine->status_page.vma);
  
-		ee->wa_ctx =

-   i915_error_object_create(dev_priv, engine->wa_ctx.vma);
+   ee->wa_ctx = i915_error_object_create(i915, engine->wa_ctx.vma);
  
-		ee->default_state =

-   capture_object(dev_priv, engine->default_state);
+   ee->default_state = capture_object(i915, engine->default_state);
}
  }
  
-static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,

-   struct i915_gpu_state *error,
-   struct i915_address_space *vm,
-   int idx)
+static void gem_capture_vm(struct i915_gpu_state *error,
+  struct 

[Intel-gfx] [PATCH v2 2/3] drm/i915/error: standardize function style in error capture

[Daniele Ceraolo Spurio]

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 by getting i915 from
error->i915. While at it, s/dev_priv/i915 in functions that don't
perform register reads.

v2: take i915 from error->i915 (Michal), s/dev_priv/i915,
update commit message

Cc: Michal Wajdeczko 
Cc: Chris Wilson 
Signed-off-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 84 ---
 1 file changed, 39 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
b/drivers/gpu/drm/i915/i915_gpu_error.c
index ef29fb48d6d9..9afb1b9674c0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1084,9 +1084,9 @@ 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 i915_gpu_state *error)
 {
+   struct drm_i915_private *dev_priv = error->i915;
int i;
 
if (INTEL_GEN(dev_priv) >= 6) {
@@ -1424,14 +1424,14 @@ 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 i915_gpu_state *error)
 {
-   struct i915_ggtt *ggtt = _priv->ggtt;
+   struct drm_i915_private *i915 = error->i915;
+   struct i915_ggtt *ggtt = >ggtt;
int i;
 
for (i = 0; i < I915_NUM_ENGINES; i++) {
-   struct intel_engine_cs *engine = dev_priv->engine[i];
+   struct intel_engine_cs *engine = i915->engine[i];
struct drm_i915_error_engine *ee = >engine[i];
struct i915_request *request;
 
@@ -1460,17 +1460,16 @@ static void i915_gem_record_rings(struct 
drm_i915_private *dev_priv,
 * by userspace.
 */
ee->batchbuffer =
-   i915_error_object_create(dev_priv,
-request->batch);
+   i915_error_object_create(i915, request->batch);
 
-   if (HAS_BROKEN_CS_TLB(dev_priv))
+   if (HAS_BROKEN_CS_TLB(i915))
ee->wa_batchbuffer =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
engine->scratch);
request_record_user_bo(request, ee);
 
ee->ctx =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 
request->ctx->engine[i].state);
 
error->simulated |=
@@ -1484,27 +1483,24 @@ static void i915_gem_record_rings(struct 
drm_i915_private *dev_priv,
ee->cpu_ring_head = ring->head;
ee->cpu_ring_tail = ring->tail;
ee->ringbuffer =
-   i915_error_object_create(dev_priv, ring->vma);
+   i915_error_object_create(i915, ring->vma);
 
engine_record_requests(engine, request, ee);
}
 
ee->hws_page =
-   i915_error_object_create(dev_priv,
+   i915_error_object_create(i915,
 engine->status_page.vma);
 
-   ee->wa_ctx =
-   i915_error_object_create(dev_priv, engine->wa_ctx.vma);
+   ee->wa_ctx = i915_error_object_create(i915, engine->wa_ctx.vma);
 
-   ee->default_state =
-   capture_object(dev_priv, engine->default_state);
+   ee->default_state = capture_object(i915, engine->default_state);
}
 }
 
-static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
-   struct i915_gpu_state *error,
-   struct i915_address_space *vm,
-   int idx)
+static void gem_capture_vm(struct i915_gpu_state *error,
+  struct i915_address_space *vm,
+  int idx)
 {
struct drm_i915_error_buffer *active_bo;
struct i915_vma *vma;
@@ -1527,8 +1523,7 @@ static void i915_gem_capture_vm(struct drm_i915_private