Date:	Thu, 16 Sep 2010 20:07:39 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Vasily Khoruzhick <anarsoul@...il.com>
cc:	Sitsofe Wheeler <sitsofe@...oo.com>,
	Venkatesh Pallipadi <venki@...gle.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: Interrupt latency on some 945GM platforms

Vasily,

On Thu, 16 Sep 2010, Vasily Khoruzhick wrote:

> В сообщении от 15 of September 2010 01:41:11 автор Sitsofe Wheeler написал:
> > > >     processor.max_cstate=2
> > > 
> > > Nope, it doesn't work with max_cstate=2
> > 
> > Perhaps intel_idle is being used? Any mention of it in dmesg?
> 
> Sitsofe, maybe you misunderstood me, I mean with max_cstate=1 graphics is 
> smooth, with higher values (i.e. max_cstate=2) graphics is jerky.
> 
> Btw, Jesse, any comments/solutions/workarounds except one with 
> processor.max_cstate=1 in kernel commandline? Should I file a bug on fdo 
> bugzilla?

we fixed a HPET related bug a few days ago, which might be the cause
for your problem as well.

Thanks,

	tglx
---
commit 54ff7e595d763d894104d421b103a89f7becf47c
Author: Thomas Gleixner <tglx@...utronix.de>
Date:   Tue Sep 14 22:10:21 2010 +0200

    x86: hpet: Work around hardware stupidity
    
    This more or less reverts commits 08be979 (x86: Force HPET
    readback_cmp for all ATI chipsets) and 30a564be (x86, hpet: Restrict
    read back to affected ATI chipsets) to the status of commit 8da854c
    (x86, hpet: Erratum workaround for read after write of HPET
    comparator).
    
    The delta to commit 8da854c is mostly comments and the change from
    WARN_ONCE to printk_once as we know the call path of this function
    already.
    
    This needs really in depth explanation:
    
    First of all the HPET design is a complete failure. Having a counter
    compare register which generates an interrupt on matching values
    forces the software to do at least one superfluous readback of the
    counter register.
    
    While it is nice in theory to program "absolute" time events it is
    practically useless because the timer runs at some absurd frequency
    which can never be matched to real world units. So we are forced to
    calculate a relative delta and this forces a readout of the actual
    counter value, adding the delta and programming the compare
    register. When the delta is small enough we run into the danger that
    we program a compare value which is already in the past. Due to the
    compare for equal nature of HPET we need to read back the counter
    value after writing the compare rehgister (btw. this is necessary for
    absolute timeouts as well) to make sure that we did not miss the timer
    event. We try to work around that by setting the minimum delta to a
    value which is larger than the theoretical time which elapses between
    the counter readout and the compare register write, but that's only
    true in theory. A NMI or SMI which hits between the readout and the
    write can easily push us beyond that limit. This would result in
    waiting for the next HPET timer interrupt until the 32bit wraparound
    of the counter happens which takes about 306 seconds.
    
    So we designed the next event function to look like:
    
       match = read_cnt() + delta;
       write_compare_ref(match);
       return read_cnt() < match ? 0 : -ETIME;
    
    At some point we got into trouble with certain ATI chipsets. Even the
    above "safe" procedure failed. The reason was that the write to the
    compare register was delayed probably for performance reasons. The
    theory was that they wanted to avoid the synchronization of the write
    with the HPET clock, which is understandable. So the write does not
    hit the compare register directly instead it goes to some intermediate
    register which is copied to the real compare register in sync with the
    HPET clock. That opens another window for hitting the dreaded "wait
    for a wraparound" problem.
    
    To work around that "optimization" we added a read back of the compare
    register which either enforced the update of the just written value or
    just delayed the readout of the counter enough to avoid the issue. We
    unfortunately never got any affirmative info from ATI/AMD about this.
    
    One thing is sure, that we nuked the performance "optimization" that
    way completely and I'm pretty sure that the result is worse than
    before some HW folks came up with those.
    
    Just for paranoia reasons I added a check whether the read back
    compare register value was the same as the value we wrote right
    before. That paranoia check triggered a couple of years after it was
    added on an Intel ICH9 chipset. Venki added a workaround (commit
    8da854c) which was reading the compare register twice when the first
    check failed. We considered this to be a penalty in general and
    restricted the readback (thus the wasted CPU cycles) to the known to
    be affected ATI chipsets.
    
    This turned out to be a utterly wrong decision. 2.6.35 testers
    experienced massive problems and finally one of them bisected it down
    to commit 30a564be which spured some further investigation.
    
    Finally we got confirmation that the write to the compare register can
    be delayed by up to two HPET clock cycles which explains the problems
    nicely. All we can do about this is to go back to Venki's initial
    workaround in a slightly modified version.
    
    Just for the record I need to say, that all of this could have been
    avoided if hardware designers and of course the HPET committee would
    have thought about the consequences for a split second. It's out of my
    comprehension why designing a working timer is so hard. There are two
    ways to achieve it:
    
     1) Use a counter wrap around aware compare_reg <= counter_reg
        implementation instead of the easy compare_reg == counter_reg
    
        Downsides:
    
    	- It needs more silicon.
    
    	- It needs a readout of the counter to apply a relative
    	  timeout. This is necessary as the counter does not run in
    	  any useful (and adjustable) frequency and there is no
    	  guarantee that the counter which is used for timer events is
    	  the same which is used for reading the actual time (and
    	  therefor for calculating the delta)
    
        Upsides:
    
    	- None
    
      2) Use a simple down counter for relative timer events
    
        Downsides:
    
    	- Absolute timeouts are not possible, which is not a problem
    	  at all in the context of an OS and the expected
    	  max. latencies/jitter (also see Downsides of #1)
    
       Upsides:
    
    	- It needs less or equal silicon.
    
    	- It works ALWAYS
    
    	- It is way faster than a compare register based solution (One
    	  write versus one write plus at least one and up to four
    	  reads)
    
    I would not be so grumpy about all of this, if I would not have been
    ignored for many years when pointing out these flaws to various
    hardware folks. I really hate timers (at least those which seem to be
    designed by janitors).
    
    Though finally we got a reasonable explanation plus a solution and I
    want to thank all the folks involved in chasing it down and providing
    valuable input to this.
    
    Bisected-by: Nix <nix@...eri.org.uk>
    Reported-by: Artur Skawina <art.08.09@...il.com>
    Reported-by: Damien Wyart <damien.wyart@...e.fr>
    Reported-by: John Drescher <drescherjm@...il.com>
    Cc: Venkatesh Pallipadi <venki@...gle.com>
    Cc: Ingo Molnar <mingo@...e.hu>
    Cc: H. Peter Anvin <hpa@...or.com>
    Cc: Arjan van de Ven <arjan@...ux.intel.com>
    Cc: Andreas Herrmann <andreas.herrmann3@....com>
    Cc: Borislav Petkov <borislav.petkov@....com>
    Cc: stable@...nel.org
    Acked-by: Suresh Siddha <suresh.b.siddha@...el.com>
    Signed-off-by: Thomas Gleixner <tglx@...utronix.de>

diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index 004e6e2..1d5c08a 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -68,7 +68,6 @@ extern unsigned long force_hpet_address;
 extern u8 hpet_blockid;
 extern int hpet_force_user;
 extern u8 hpet_msi_disable;
-extern u8 hpet_readback_cmp;
 extern int is_hpet_enabled(void);
 extern int hpet_enable(void);
 extern void hpet_disable(void);
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index e5cc7e8..ebdb85c 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -18,7 +18,6 @@
 #include <asm/apic.h>
 #include <asm/iommu.h>
 #include <asm/gart.h>
-#include <asm/hpet.h>
 
 static void __init fix_hypertransport_config(int num, int slot, int func)
 {
@@ -192,21 +191,6 @@ static void __init ati_bugs_contd(int num, int slot, int func)
 }
 #endif
 
-/*
- * Force the read back of the CMP register in hpet_next_event()
- * to work around the problem that the CMP register write seems to be
- * delayed. See hpet_next_event() for details.
- *
- * We do this on all SMBUS incarnations for now until we have more
- * information about the affected chipsets.
- */
-static void __init ati_hpet_bugs(int num, int slot, int func)
-{
-#ifdef CONFIG_HPET_TIMER
-	hpet_readback_cmp = 1;
-#endif
-}
-
 #define QFLAG_APPLY_ONCE 	0x1
 #define QFLAG_APPLIED		0x2
 #define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
@@ -236,8 +220,6 @@ static struct chipset early_qrk[] __initdata = {
 	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
 	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs_contd },
-	{ PCI_VENDOR_ID_ATI, PCI_ANY_ID,
-	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_hpet_bugs },
 	{}
 };
 
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 351f9c0..410fdb3 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -35,7 +35,6 @@
 unsigned long				hpet_address;
 u8					hpet_blockid; /* OS timer block num */
 u8					hpet_msi_disable;
-u8					hpet_readback_cmp;
 
 #ifdef CONFIG_PCI_MSI
 static unsigned long			hpet_num_timers;
@@ -395,23 +394,27 @@ static int hpet_next_event(unsigned long delta,
 	 * at that point and we would wait for the next hpet interrupt
 	 * forever. We found out that reading the CMP register back
 	 * forces the transfer so we can rely on the comparison with
-	 * the counter register below.
+	 * the counter register below. If the read back from the
+	 * compare register does not match the value we programmed
+	 * then we might have a real hardware problem. We can not do
+	 * much about it here, but at least alert the user/admin with
+	 * a prominent warning.
 	 *
-	 * That works fine on those ATI chipsets, but on newer Intel
-	 * chipsets (ICH9...) this triggers due to an erratum: Reading
-	 * the comparator immediately following a write is returning
-	 * the old value.
+	 * An erratum on some chipsets (ICH9,..), results in
+	 * comparator read immediately following a write returning old
+	 * value. Workaround for this is to read this value second
+	 * time, when first read returns old value.
 	 *
-	 * We restrict the read back to the affected ATI chipsets (set
-	 * by quirks) and also run it with hpet=verbose for debugging
-	 * purposes.
+	 * In fact the write to the comparator register is delayed up
+	 * to two HPET cycles so the workaround we tried to restrict
+	 * the readback to those known to be borked ATI chipsets
+	 * failed miserably. So we give up on optimizations forever
+	 * and penalize all HPET incarnations unconditionally.
 	 */
-	if (hpet_readback_cmp || hpet_verbose) {
-		u32 cmp = hpet_readl(HPET_Tn_CMP(timer));
-
-		if (cmp != cnt)
+	if (unlikely((u32)hpet_readl(HPET_Tn_CMP(timer)) != cnt)) {
+		if (hpet_readl(HPET_Tn_CMP(timer)) != cnt)
 			printk_once(KERN_WARNING
-			    "hpet: compare register read back failed.\n");
+				"hpet: compare register read back failed.\n");
 	}
 
 	return (s32)(hpet_readl(HPET_COUNTER) - cnt) >= 0 ? -ETIME : 0;

Powered by blists - more mailing lists