Re: [Intel-gfx] [PATCH] drm/i915: Hold rpm wakeref for printing the engine's register state

[Chris Wilson]

Quoting Mika Kuoppala (2018-02-12 13:28:54)
> Chris Wilson  writes:
> 
> > When dumping the engine, we print out the current register values. This
> > requires the rpm wakeref. If the device is alseep, we can assume the
> > engine is asleep (and the register state is uninteresting) so skip and
> > only acquire the rpm wakeref if the device is already awake.
> >
> > Reported-by: Tvrtko Ursulin 
> > Signed-off-by: Chris Wilson 
> > Cc: Tvrtko Ursulin 
> > Cc: Mika Kuoppala 
> > ---
> > + if (intel_runtime_pm_get_if_in_use(engine->i915)) {
> > + intel_engine_print_registers(engine, m);
> > + intel_runtime_pm_put(engine->i915);
> > + } else {
> > + drm_printf(m, "Device is alseep; skipping register dump\n");
> > + }
> 
> s/alseep/asleep
> 
> Reviewed-by: Mika Kuoppala 

Done, and both pushed. Now to try Tvrtko's warning patch on the new
baseline. Thanks,
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Hold rpm wakeref for printing the engine's register state

[Mika Kuoppala]

Chris Wilson  writes:

> When dumping the engine, we print out the current register values. This
> requires the rpm wakeref. If the device is alseep, we can assume the
> engine is asleep (and the register state is uninteresting) so skip and
> only acquire the rpm wakeref if the device is already awake.
>
> Reported-by: Tvrtko Ursulin 
> Signed-off-by: Chris Wilson 
> Cc: Tvrtko Ursulin 
> Cc: Mika Kuoppala 
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 162 
> ++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h |   6 +-
>  2 files changed, 94 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3efc589a7f37..2df9a2d038ee 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -691,7 +691,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs 
> *engine)
>   engine->context_unpin(engine, engine->i915->kernel_context);
>  }
>  
> -u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
> +u64 intel_engine_get_active_head(const struct intel_engine_cs *engine)
>  {
>   struct drm_i915_private *dev_priv = engine->i915;
>   u64 acthd;
> @@ -707,7 +707,7 @@ u64 intel_engine_get_active_head(struct intel_engine_cs 
> *engine)
>   return acthd;
>  }
>  
> -u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine)
> +u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine)
>  {
>   struct drm_i915_private *dev_priv = engine->i915;
>   u64 bbaddr;
> @@ -1705,73 +1705,20 @@ static void hexdump(struct drm_printer *m, const void 
> *buf, size_t len)
>   }
>  }
>  
> -void intel_engine_dump(struct intel_engine_cs *engine,
> -struct drm_printer *m,
> -const char *header, ...)
> +static void intel_engine_print_registers(const struct intel_engine_cs 
> *engine,
> +  struct drm_printer *m)
>  {
> - struct intel_breadcrumbs * const b = >breadcrumbs;
> - const struct intel_engine_execlists * const execlists = 
> >execlists;
> - struct i915_gpu_error * const error = >i915->gpu_error;
>   struct drm_i915_private *dev_priv = engine->i915;
> - struct drm_i915_gem_request *rq;
> - struct rb_node *rb;
> - char hdr[80];
> + const struct intel_engine_execlists * const execlists =
> + >execlists;
>   u64 addr;
>  
> - if (header) {
> - va_list ap;
> -
> - va_start(ap, header);
> - drm_vprintf(m, header, );
> - va_end(ap);
> - }
> -
> - if (i915_terminally_wedged(>i915->gpu_error))
> - drm_printf(m, "*** WEDGED ***\n");
> -
> - drm_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [%d ms], 
> inflight %d\n",
> -intel_engine_get_seqno(engine),
> -intel_engine_last_submit(engine),
> -engine->hangcheck.seqno,
> -jiffies_to_msecs(jiffies - 
> engine->hangcheck.action_timestamp),
> -engine->timeline->inflight_seqnos);
> - drm_printf(m, "\tReset count: %d (global %d)\n",
> -i915_reset_engine_count(error, engine),
> -i915_reset_count(error));
> -
> - rcu_read_lock();
> -
> - drm_printf(m, "\tRequests:\n");
> -
> - rq = list_first_entry(>timeline->requests,
> -   struct drm_i915_gem_request, link);
> - if (>link != >timeline->requests)
> - print_request(m, rq, "\t\tfirst  ");
> -
> - rq = list_last_entry(>timeline->requests,
> -  struct drm_i915_gem_request, link);
> - if (>link != >timeline->requests)
> - print_request(m, rq, "\t\tlast   ");
> -
> - rq = i915_gem_find_active_request(engine);
> - if (rq) {
> - print_request(m, rq, "\t\tactive ");
> - drm_printf(m,
> -"\t\t[head %04x, postfix %04x, tail %04x, batch 
> 0x%08x_%08x]\n",
> -rq->head, rq->postfix, rq->tail,
> -rq->batch ? upper_32_bits(rq->batch->node.start) : 
> ~0u,
> -rq->batch ? lower_32_bits(rq->batch->node.start) : 
> ~0u);
> - }
> -
> - drm_printf(m, "\tRING_START: 0x%08x [0x%08x]\n",
> -I915_READ(RING_START(engine->mmio_base)),
> -rq ? i915_ggtt_offset(rq->ring->vma) : 0);
> - drm_printf(m, "\tRING_HEAD:  0x%08x [0x%08x]\n",
> -I915_READ(RING_HEAD(engine->mmio_base)) & HEAD_ADDR,
> -rq ? rq->ring->head : 0);
> - drm_printf(m, "\tRING_TAIL:  0x%08x [0x%08x]\n",
> -I915_READ(RING_TAIL(engine->mmio_base)) & TAIL_ADDR,
> -rq ? rq->ring->tail : 0);
> + drm_printf(m, 

[Intel-gfx] [PATCH] drm/i915: Hold rpm wakeref for printing the engine's register state

[Chris Wilson]

When dumping the engine, we print out the current register values. This
requires the rpm wakeref. If the device is alseep, we can assume the
engine is asleep (and the register state is uninteresting) so skip and
only acquire the rpm wakeref if the device is already awake.

Reported-by: Tvrtko Ursulin 
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Mika Kuoppala 
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 162 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |   6 +-
 2 files changed, 94 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 3efc589a7f37..2df9a2d038ee 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -691,7 +691,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs 
*engine)
engine->context_unpin(engine, engine->i915->kernel_context);
 }
 
-u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
+u64 intel_engine_get_active_head(const struct intel_engine_cs *engine)
 {
struct drm_i915_private *dev_priv = engine->i915;
u64 acthd;
@@ -707,7 +707,7 @@ u64 intel_engine_get_active_head(struct intel_engine_cs 
*engine)
return acthd;
 }
 
-u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine)
+u64 intel_engine_get_last_batch_head(const struct intel_engine_cs *engine)
 {
struct drm_i915_private *dev_priv = engine->i915;
u64 bbaddr;
@@ -1705,73 +1705,20 @@ static void hexdump(struct drm_printer *m, const void 
*buf, size_t len)
}
 }
 
-void intel_engine_dump(struct intel_engine_cs *engine,
-  struct drm_printer *m,
-  const char *header, ...)
+static void intel_engine_print_registers(const struct intel_engine_cs *engine,
+struct drm_printer *m)
 {
-   struct intel_breadcrumbs * const b = >breadcrumbs;
-   const struct intel_engine_execlists * const execlists = 
>execlists;
-   struct i915_gpu_error * const error = >i915->gpu_error;
struct drm_i915_private *dev_priv = engine->i915;
-   struct drm_i915_gem_request *rq;
-   struct rb_node *rb;
-   char hdr[80];
+   const struct intel_engine_execlists * const execlists =
+   >execlists;
u64 addr;
 
-   if (header) {
-   va_list ap;
-
-   va_start(ap, header);
-   drm_vprintf(m, header, );
-   va_end(ap);
-   }
-
-   if (i915_terminally_wedged(>i915->gpu_error))
-   drm_printf(m, "*** WEDGED ***\n");
-
-   drm_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [%d ms], 
inflight %d\n",
-  intel_engine_get_seqno(engine),
-  intel_engine_last_submit(engine),
-  engine->hangcheck.seqno,
-  jiffies_to_msecs(jiffies - 
engine->hangcheck.action_timestamp),
-  engine->timeline->inflight_seqnos);
-   drm_printf(m, "\tReset count: %d (global %d)\n",
-  i915_reset_engine_count(error, engine),
-  i915_reset_count(error));
-
-   rcu_read_lock();
-
-   drm_printf(m, "\tRequests:\n");
-
-   rq = list_first_entry(>timeline->requests,
- struct drm_i915_gem_request, link);
-   if (>link != >timeline->requests)
-   print_request(m, rq, "\t\tfirst  ");
-
-   rq = list_last_entry(>timeline->requests,
-struct drm_i915_gem_request, link);
-   if (>link != >timeline->requests)
-   print_request(m, rq, "\t\tlast   ");
-
-   rq = i915_gem_find_active_request(engine);
-   if (rq) {
-   print_request(m, rq, "\t\tactive ");
-   drm_printf(m,
-  "\t\t[head %04x, postfix %04x, tail %04x, batch 
0x%08x_%08x]\n",
-  rq->head, rq->postfix, rq->tail,
-  rq->batch ? upper_32_bits(rq->batch->node.start) : 
~0u,
-  rq->batch ? lower_32_bits(rq->batch->node.start) : 
~0u);
-   }
-
-   drm_printf(m, "\tRING_START: 0x%08x [0x%08x]\n",
-  I915_READ(RING_START(engine->mmio_base)),
-  rq ? i915_ggtt_offset(rq->ring->vma) : 0);
-   drm_printf(m, "\tRING_HEAD:  0x%08x [0x%08x]\n",
-  I915_READ(RING_HEAD(engine->mmio_base)) & HEAD_ADDR,
-  rq ? rq->ring->head : 0);
-   drm_printf(m, "\tRING_TAIL:  0x%08x [0x%08x]\n",
-  I915_READ(RING_TAIL(engine->mmio_base)) & TAIL_ADDR,
-  rq ? rq->ring->tail : 0);
+   drm_printf(m, "\tRING_START: 0x%08x\n",
+  I915_READ(RING_START(engine->mmio_base)));
+   drm_printf(m, "\tRING_HEAD:  0x%08x\n",
+