* [PATH] Correct GPU timestamp read
@ 2014-09-22 16:22 Jacek Danecki
2014-09-23 8:37 ` Daniel Vetter
2014-09-25 12:26 ` Chris Wilson
0 siblings, 2 replies; 6+ messages in thread
From: Jacek Danecki @ 2014-09-22 16:22 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATH] Correct GPU timestamp read
2014-09-22 16:22 [PATH] Correct GPU timestamp read Jacek Danecki
@ 2014-09-23 8:37 ` Daniel Vetter
2014-09-23 17:12 ` Jacek Danecki
2014-09-25 12:26 ` Chris Wilson
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-09-23 8:37 UTC (permalink / raw)
To: Jacek Danecki; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATH] Correct GPU timestamp read
2014-09-23 8:37 ` Daniel Vetter
@ 2014-09-23 17:12 ` Jacek Danecki
0 siblings, 0 replies; 6+ messages in thread
From: Jacek Danecki @ 2014-09-23 17:12 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATH] Correct GPU timestamp read
2014-09-22 16:22 [PATH] Correct GPU timestamp read Jacek Danecki
2014-09-23 8:37 ` Daniel Vetter
@ 2014-09-25 12:26 ` Chris Wilson
2014-09-25 13:00 ` Jacek Danecki
1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-09-25 12:26 UTC (permalink / raw)
To: Jacek Danecki; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATH] Correct GPU timestamp read
2014-09-25 12:26 ` Chris Wilson
@ 2014-09-25 13:00 ` Jacek Danecki
2014-09-25 13:09 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Jacek Danecki @ 2014-09-25 13:00 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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, ×tamp.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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATH] Correct GPU timestamp read
2014-09-25 13:00 ` Jacek Danecki
@ 2014-09-25 13:09 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2014-09-25 13:09 UTC (permalink / raw)
To: Jacek Danecki; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-25 13:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-22 16:22 [PATH] Correct GPU timestamp read Jacek Danecki
2014-09-23 8:37 ` Daniel Vetter
2014-09-23 17:12 ` Jacek Danecki
2014-09-25 12:26 ` Chris Wilson
2014-09-25 13:00 ` Jacek Danecki
2014-09-25 13:09 ` Chris Wilson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox