[Mesa-dev] [PATCH] i965: Use updated kernel interface for accurate TIMESTAMP reads

[Chris Forbes]

This fixes my HSW getting dropped back to 3.2 most of the time, and
seems like the reasonable thing to do.

Tested-and-acked-by: Chris Forbes <chrisf at ijw.co.nz>

On Tue, Jul 21, 2015 at 11:58 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> I was mistaken, I thought we already had fixed this in the kernel a
> couple of years ago. We had not, and the broken read (the hardware
> shifts the register output on 64bit kernels, but not on 32bit kernels) is
> now enshrined into the ABI. I also had the buggy architecture reversed,
> believing it to be 32bit that had the shifted results. On the basis of
> those mistakes, I wrote
>
> commit c8d3ebaffc0d7d915c1c19d54dba61fd1e57b338
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Wed Apr 29 13:32:38 2015 +0100
>
>     i965: Query whether we have kernel support for the TIMESTAMP register once
>
> Now that we do have an extended register read interface for always
> reporting the full 36bit TIMESTAMP (irrespective of whether the kernel
> is buggy or not), make use of it and in the process fix my reversed
> detection of the buggy reads for unpatched kernels.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Martin Peres <martin.peres at linux.intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> ---
>  src/mesa/drivers/dri/i965/brw_queryobj.c | 15 ++++++++--
>  src/mesa/drivers/dri/i965/intel_screen.c | 49 +++++++++++++++++++++++---------
>  src/mesa/drivers/dri/i965/intel_screen.h |  2 +-
>  3 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_queryobj.c b/src/mesa/drivers/dri/i965/brw_queryobj.c
> index aea4d9b..6f1accd 100644
> --- a/src/mesa/drivers/dri/i965/brw_queryobj.c
> +++ b/src/mesa/drivers/dri/i965/brw_queryobj.c
> @@ -497,13 +497,22 @@ brw_get_timestamp(struct gl_context *ctx)
>     struct brw_context *brw = brw_context(ctx);
>     uint64_t result = 0;
>
> -   drm_intel_reg_read(brw->bufmgr, TIMESTAMP, &result);
> +   switch (brw->intelScreen->hw_has_timestamp) {
> +   case 3: /* New kernel, always full 36bit accuracy */
> +      drm_intel_reg_read(brw->bufmgr, TIMESTAMP | 1, &result);
> +      break;
> +   case 2: /* 64bit kernel, result is right-shifted by 32bits, losing 4bits */
> +      drm_intel_reg_read(brw->bufmgr, TIMESTAMP, &result);
> +      result = result >> 32;
> +      break;
> +   case 1: /* 32bit kernel, result is 36bit wide but may be inaccurate! */
> +      drm_intel_reg_read(brw->bufmgr, TIMESTAMP, &result);
> +      break;
> +   }
>
>     /* See logic in brw_queryobj_get_results() */
> -   result = result >> 32;
>     result *= 80;
>     result &= (1ull << 36) - 1;
> -
>     return result;
>  }
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index 1470b05..65a1766 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -1123,25 +1123,48 @@ intel_detect_swizzling(struct intel_screen *screen)
>        return true;
>  }
>
> -static bool
> +static int
>  intel_detect_timestamp(struct intel_screen *screen)
>  {
> -   uint64_t dummy = 0;
> -   int loop = 10;
> +   uint64_t dummy = 0, last = 0;
> +   int upper, lower, loops;
>
> -   /*
> -    * On 32bit systems, some old kernels trigger a hw bug resulting in the
> -    * TIMESTAMP register being shifted and the low 32bits always zero. Detect
> -    * this by repeating the read a few times and check the register is
> -    * incrementing every 80ns as expected and not stuck on zero (as would be
> -    * the case with the buggy kernel/hw.).
> +   /* On 64bit systems, some old kernels trigger a hw bug resulting in the
> +    * TIMESTAMP register being shifted and the low 32bits always zero.
> +    *
> +    * More recent kernels offer an interface to read the full 36bits
> +    * everywhere.
>      */
> -   do {
> +   if (drm_intel_reg_read(screen->bufmgr, TIMESTAMP | 1, &dummy) == 0)
> +      return 3;
> +
> +   /* Determine if we have a 32bit or 64bit kernel by inspecting the
> +    * upper 32bits for a rapidly changing timestamp.
> +    */
> +   if (drm_intel_reg_read(screen->bufmgr, TIMESTAMP, &last))
> +      return 0;
> +
> +   upper = lower = 0;
> +   for (loops = 0; loops < 10; loops++) {
> +      /* The TIMESTAMP should change every 80ns, so several round trips
> +       * through the kernel should be enough to advance it.
> +       */
>        if (drm_intel_reg_read(screen->bufmgr, TIMESTAMP, &dummy))
> -        return false;
> -   } while ((dummy & 0xffffffff) == 0 && --loop);
> +         return 0;
> +
> +      upper += (dummy >> 32) != (last >> 32);
> +      if (upper > 1) /* beware 32bit counter overflow */
> +         return 2; /* upper dword holds the low 32bits of the timestamp */
> +
> +      lower += (dummy & 0xffffffff) != (last & 0xffffffff);
> +      if (lower > 1)
> +         return 1; /* timestamp is unshifted */
> +
> +      last = dummy;
> +   }
>
> -   return loop > 0;
> +   /* No advancement? No timestamp! */
> +   return 0;
>  }
>
>  /**
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.h b/src/mesa/drivers/dri/i965/intel_screen.h
> index a741c94..fd5143e 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.h
> +++ b/src/mesa/drivers/dri/i965/intel_screen.h
> @@ -52,7 +52,7 @@ struct intel_screen
>
>     bool hw_has_swizzling;
>
> -   bool hw_has_timestamp;
> +   int hw_has_timestamp;
>
>     /**
>      * Does the kernel support resource streamer?
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev