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 <kenn...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
