Re: [PATCH] drm/i915: Correct CSB probing for engine state dumper

From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Correct CSB probing for engine state dumper
Date: Tue, 21 Aug 2018 10:58:26 +0300	[thread overview]
Message-ID: <878t506ti5.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <20180817085039.884-1-chris@chris-wilson.co.uk>

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Since we no longer maintain our read position in the CSB pointers
> register, it always returns 0 and not where we last read up to. As a
> result the CSB probing in the state dumper starts from 0, either missing
> entries or showing stale one.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 8628567d8f6e..49b580c188eb 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1345,20 +1345,20 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>  
>  	if (HAS_EXECLISTS(dev_priv)) {
>  		const u32 *hws = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX];
> -		u32 ptr, read, write;
>  		unsigned int idx;
> +		u8 read, write;
>  
>  		drm_printf(m, "\tExeclist status: 0x%08x %08x\n",
>  			   I915_READ(RING_EXECLIST_STATUS_LO(engine)),
>  			   I915_READ(RING_EXECLIST_STATUS_HI(engine)));
>  
> -		ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
> -		read = GEN8_CSB_READ_PTR(ptr);
> -		write = GEN8_CSB_WRITE_PTR(ptr);
> -		drm_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws], tasklet queued? %s (%s)\n",
> -			   read, execlists->csb_head,
> -			   write,
> -			   intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)),
> +		read = execlists->csb_head;
> +		write = intel_read_status_page(engine,
> +					       intel_hws_csb_write_index(dev_priv));
> +
> +		drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n",
> +			   read, write,

In here I was thinking that we want to keep the write as 32bit so it
would show weirdness in here with %d. But the odds of that to happen
with only in this status page entry...

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +			   GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(engine))),
>  			   yesno(test_bit(TASKLET_STATE_SCHED,
>  					  &engine->execlists.tasklet.state)),
>  			   enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
> @@ -1370,12 +1370,12 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>  			write += GEN8_CSB_ENTRIES;
>  		while (read < write) {
>  			idx = ++read % GEN8_CSB_ENTRIES;
> -			drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [0x%08x in hwsp], context: %d [%d in hwsp]\n",
> +			drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [mmio:0x%08x], context: %d [mmio:%d]\n",
>  				   idx,
> -				   I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)),
>  				   hws[idx * 2],
> -				   I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)),
> -				   hws[idx * 2 + 1]);
> +				   I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)),
> +				   hws[idx * 2 + 1],
> +				   I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)));
>  		}
>  
>  		rcu_read_lock();
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx