[Mesa-dev] [PATCH 01/18] i965: Query whether we have kernel support for the TIMESTAMP register once

[Chris Wilson]

On Mon, Jul 06, 2015 at 10:01:21AM -0700, Kenneth Graunke wrote:
> On Monday, July 06, 2015 05:12:10 PM Chris Wilson wrote:
> > On Mon, Jul 06, 2015 at 04:19:36PM +0300, Martin Peres wrote:
> > > 
> > > 
> > > On 06/07/15 16:15, Martin Peres wrote:
> > > >On 06/07/15 16:13, Chris Wilson wrote:
> > > >>On Mon, Jul 06, 2015 at 03:10:48PM +0300, Martin Peres wrote:
> > > >>>On 06/07/15 13:33, Chris Wilson wrote:
> > > >>>>Move the query for the TIMESTAMP register from context init to the
> > > >>>>screen, so that it is only queried once for all contexts.
> > > >>>>
> > > >>>>On 32bit systems, some old kernels trigger a hw bug resulting in the
> > > >>>>TIMESTAMP register being shifted and the low bits always zero. Detect
> > > >>>>this by repeating the read a few times and check the register is
> > > >>>>incrementing.
> > > >>>You do not do the latter. You only check for the low bits.
> > > >>>
> > > >>>I guess the counter is supposed to be monotonically increasing and
> > > >>>with a resolution of a few microseconds which would make this
> > > >>>perfectly valid. Could you confirm and make sure to add this
> > > >>>information in the commit message please?
> > > >>The counter should increment every 80ns. What's misleading in what I
> > > >>wrote? It describes the hw bug and how to detect it.
> > > >
> > > >Well, it is not misleading, it just lacks this information.
> > > >
> > > >If it incremented every seconds, the patch would be stupid because
> > > >the timestamp could be at 0 and polling 10 times at a few us of
> > > >interval would always yield the same result. That's all :)
> > > 
> > > Oh, forgot to say: With this information added in the commit message
> > > and the commit message duplicated as a comment in
> > > intel_detect_timestamp(), the patch is:
> > 
> > How about:
> > 
> > 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.).
> > -Chris
> > 
> > 
> 
> It would be great to put this in a comment above the loop rather than
> only in the commit message.
> 
> Also, moving the check to screen-time and fixing the check to use a loop
> are really two separate things - but they're so trivial I'm inclined to
> not be picky. :)
> 
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Pushed,
To ssh://git.freedesktop.org/git/mesa/mesa
   38c2ec5..c8d3eba  c8d3ebaffc0d7d915c1c19d54dba61fd1e57b338 -> master
Thanks for the review,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre