[PATH] Correct GPU timestamp read

[PATH] Correct GPU timestamp read

[Jacek Danecki]

Current implementation of reading GPU timestamp is broken.
It returns lower 32 bits shifted by 32 bits (XXXXXXXX00000000 instead of YYYYYYYYXXXXXXXX).
Below change is adding possibility to read hi part of that register separately.

Signed-off-by: Jacek Danecki jacek.danecki@intel.com
---

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 20673cc..5c87d92 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1089,6 +1089,7 @@ enum punit_power_well {
 #define RING_IMR(base)		((base)+0xa8)
 #define RING_HWSTAM(base)	((base)+0x98)
 #define RING_TIMESTAMP(base)	((base)+0x358)
+#define RING_TIMESTAMP_HI(base)	((base)+0x35C)
 #define   TAIL_ADDR		0x001FFFF8
 #define   HEAD_WRAP_COUNT	0xFFE00000
 #define   HEAD_WRAP_ONE		0x00200000
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e81bc3b..6fa4c86 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -969,6 +969,7 @@ static const struct register_whitelist {
 	uint32_t gen_bitmask;
 } whitelist[] = {
 	{ RING_TIMESTAMP(RENDER_RING_BASE), 8, GEN_RANGE(4, 8) },
+	{ RING_TIMESTAMP_HI(RENDER_RING_BASE), 4, GEN_RANGE(4, 8) },
 };

 int i915_reg_read_ioctl(struct drm_device *dev,
-- 1.8.3.1

-- 
jacek

Re: [PATH] Correct GPU timestamp read

[Daniel Vetter]

On Mon, Sep 22, 2014 at 06:22:53PM +0200, Jacek Danecki wrote:
> Current implementation of reading GPU timestamp is broken.
> It returns lower 32 bits shifted by 32 bits (XXXXXXXX00000000 instead of YYYYYYYYXXXXXXXX).
> Below change is adding possibility to read hi part of that register separately.
> 
> Signed-off-by: Jacek Danecki jacek.danecki@intel.com

Needs to come with corresponding userspace using this.
-Daniel

> ---
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 20673cc..5c87d92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1089,6 +1089,7 @@ enum punit_power_well {
>  #define RING_IMR(base)		((base)+0xa8)
>  #define RING_HWSTAM(base)	((base)+0x98)
>  #define RING_TIMESTAMP(base)	((base)+0x358)
> +#define RING_TIMESTAMP_HI(base)	((base)+0x35C)
>  #define   TAIL_ADDR		0x001FFFF8
>  #define   HEAD_WRAP_COUNT	0xFFE00000
>  #define   HEAD_WRAP_ONE		0x00200000
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e81bc3b..6fa4c86 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -969,6 +969,7 @@ static const struct register_whitelist {
>  	uint32_t gen_bitmask;
>  } whitelist[] = {
>  	{ RING_TIMESTAMP(RENDER_RING_BASE), 8, GEN_RANGE(4, 8) },
> +	{ RING_TIMESTAMP_HI(RENDER_RING_BASE), 4, GEN_RANGE(4, 8) },
>  };
> 
>  int i915_reg_read_ioctl(struct drm_device *dev,
> -- 1.8.3.1
> 
> -- 
> jacek
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATH] Correct GPU timestamp read

[Jacek Danecki]

On 09/23/14 10:37, Daniel Vetter wrote:
> On Mon, Sep 22, 2014 at 06:22:53PM +0200, Jacek Danecki wrote:
>> Current implementation of reading GPU timestamp is broken.
>> It returns lower 32 bits shifted by 32 bits (XXXXXXXX00000000 instead of YYYYYYYYXXXXXXXX).
>> Below change is adding possibility to read hi part of that register separately.
>>
>> Signed-off-by: Jacek Danecki jacek.danecki@intel.com
> 
> Needs to come with corresponding userspace using this.

Beignet can use this in below function. They are using only 32 bits from timestamp register, because of kernel bug.

* IVB and HSW's result MUST shift in x86_64 system */
static uint64_t
intel_gpgpu_read_ts_reg_gen7(drm_intel_bufmgr *bufmgr)
{
  uint64_t result = 0;
  drm_intel_reg_read(bufmgr, TIMESTAMP_ADDR, &result);
  /* In x86_64 system, the low 32bits of timestamp count are stored in the high 32 bits of
     result which got from drm_intel_reg_read, and 32-35 bits are lost; but match bspec in
     i386 system. It seems the kernel readq bug. So shift 32 bit in x86_64, and only remain
     32 bits data in i386.
  */
#ifdef __i386__
  return result & 0x0ffffffff;
#else
  return result >> 32;
#endif  /* __i386__  */
}


-- 
jacek

Re: [PATH] Correct GPU timestamp read

[Chris Wilson]

On Mon, Sep 22, 2014 at 06:22:53PM +0200, Jacek Danecki wrote:
> Current implementation of reading GPU timestamp is broken.
> It returns lower 32 bits shifted by 32 bits (XXXXXXXX00000000 instead of YYYYYYYYXXXXXXXX).
> Below change is adding possibility to read hi part of that register separately.
> 
> Signed-off-by: Jacek Danecki jacek.danecki@intel.com

The problem is that beignet already works around the broken hw read
whereas mesa does not. If we apply the fix in the kernel we break the
one user of it in beignet but fix all the existing users of mesa.

The userspace workaround is effectively:

  u64 v = reg_read(TIMESTAMP);
  if (lower_32_bits(v) == 0) {
	  v >>= 32;
	  v |= reg_read(TIMESTAMP + 4) << 32;
  }

Our ABI says read 8 bytes from this location. I am not sure if says
anything about what to do if the hardware is broken, or what that value
means. Already that value depends upon generation and architecture, e.g.
on x86-32 this is done as 2 readl, but on x86-64 a single readq.

The question comes down to fix mesa and break beignet, or do nothing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATH] Correct GPU timestamp read

[Jacek Danecki]

On 09/25/14 14:26, Chris Wilson wrote:
> The problem is that beignet already works around the broken hw read
> whereas mesa does not. 

There is workaround in mesa already:

static uint64_t
ilo_get_timestamp(struct pipe_screen *screen)
{
   struct ilo_screen *is = ilo_screen(screen);
   union {
      uint64_t val;
      uint32_t dw[2];
   } timestamp;

   intel_winsys_read_reg(is->winsys, GEN6_REG_TIMESTAMP, &timestamp.val);

   /*
    * From the Ivy Bridge PRM, volume 1 part 3, page 107:
    *
    *     "Note: This timestamp register reflects the value of the PCU TSC.
    *      The PCU TSC counts 10ns increments; this timestamp reflects bits
    *      38:3 of the TSC (i.e. 80ns granularity, rolling over every 1.5
    *      hours)."
    *
    * However, it seems dw[0] is garbage and dw[1] contains the lower 32 bits
    * of the timestamp.  We will have to live with a timestamp that rolls over
    * every ~343 seconds.
    *
    * See also brw_get_timestamp().
    */
   return (uint64_t) timestamp.dw[1] * 80;
}


> If we apply the fix in the kernel we break the
> one user of it in beignet but fix all the existing users of mesa.

Are you talking about fix in kernel which will provide 36 bits GPU timestamp,
or about applying patch I've proposed?
If we add new register to register_whitelist, we will probably not break anything,
but we'll allow UMD's to use whole TIMESTAMP.

-- 
jacek

Re: [PATH] Correct GPU timestamp read

[Chris Wilson]

On Thu, Sep 25, 2014 at 03:00:53PM +0200, Jacek Danecki wrote:
> On 09/25/14 14:26, Chris Wilson wrote:
> > If we apply the fix in the kernel we break the
> > one user of it in beignet but fix all the existing users of mesa.
> 
> Are you talking about fix in kernel which will provide 36 bits GPU timestamp,
> or about applying patch I've proposed?

A kernel fix would be to use I915_READ64_2x32 and am debating whether
the ABI says anything about what the hardware reports as opposed to the
read N bytes at offset A interface of the ioctl.

> If we add new register to register_whitelist, we will probably not break anything,
> but we'll allow UMD's to use whole TIMESTAMP.

Existing clients, as shipped, are broken because the value being
returned is not the one they were expecting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre