[Mesa-dev] [PATCH] i965: Allow a per gen timebase scale factor

[Robert Bragg]

On Fri, Jan 6, 2017 at 9:28 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Friday, January 6, 2017 1:17:39 PM PST Kenneth Graunke wrote:
>> From: Robert Bragg <robert at sixbynine.org>
>>
>> v2: (Ken) Update timebase_scale for platforms past Skylake/Broxton too.
>
> Hi Robert!
>
> Your patch had merge conflicts in gen_device_info.c at this point, so I
> fixed those and re-sent it.  It also looked like it didn't set
> timebase_scale for KBL, GLK, etc...it should now (via GEN9_FEATURES and
> GEN9_LP_FEATURES).
>
> [snip]
>
>> +/* As best we know currently, the Gen HW timestamps are 36bits across
>> + * all platforms, which we need to account for when calculating a
>> + * delta to measure elapsed time.
>> + *
>> + * The timestamps read via glGetTimestamp() / brw_get_timestamp() sometimes
>> + * only have 32bits due to a kernel bug and so in that case we make sure to
>> + * treat all raw timestamps as 32bits so they overflow consistently and remain
>> + * comparable.
>> + */
>> +uint64_t
>> +brw_raw_timestamp_delta(struct brw_context *brw, uint64_t time0, uint64_t time1)
>> +{
>> +   if (brw->screen->hw_has_timestamp == 2) {
>> +      /* Kernel clips timestamps to 32bits in this case */
>> +      return (uint32_t)time1 - (uint32_t)time0;
>
> Is this right?  intel_detect_timestamp() says
>
>          return 2; /* upper dword holds the low 32bits of the timestamp */
>
> but casting these to uint32_t should take the low DWords...

*very* late seeing this and following up, sorry.

I think this is ok. The timestamps we're dealing with here aren't from
the kernel, they are read via a PIPE_CONTROL. The point here is really
just being consistent with clipping raw timestamps to 32bits if we're
running on a buggy kernel. It looks like brw_get_timestamp() is doing
the right thing with reporting the upper dword when the timestamp is
queried from the kernel.

I've updated the comment above the function and above this specific
return statement to hopefully clarify this.

>
>> +   } else {
>> +      if (time0 > time1)
>> +         return (1ULL << 36) + time1 - time0;
>> +      else
>> +         return time1 - time0;
>> +   }
>> +}
>
> Otherwise, this looks good to me, and is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks, and to Matt: I've added extra braces with the nested
control-flow in brw_raw_timestamp_delta(), though I didn't add
trailing commas where noted. After seeing that the uses of the
_FEATURES macros are all followed by explicit commas it ends up
breaking compilation if the macros contain their own trailing comma.

Just pushed to master.

Br,
- Robert

>
> Could you try and hook up your fixed-point multiplier to fix query
> buffer objects as well?  I'd like to land these for the 17.0 release.
>
> Thanks for fixing this!
>
> --Ken