| Message ID | 1418129953-1505-6-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 09, 2014 at 12:59:08PM +0000, John.C.Harrison@Intel.com wrote: > From: Dave Gordon <david.s.gordon@intel.com> > > There is a workaround for a hardware bug when reading the seqno from the status > page. The bug does not exist on VLV however, the workaround was still being > applied. Given how much trouble the missed seqno fun cause us I'd like more justification. What kind of (stress)-testing has been done here? And you're sure you've never seen the little dmesg notice that the kernel switched to polling? -Daniel > > Change-Id: Ic781fdb31e1f794ce1fa8a6d0d5ee379756c5db6 > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 3887f1a..f990ce4 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2367,7 +2367,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev) > ring->irq_get = gen6_ring_get_irq; > ring->irq_put = gen6_ring_put_irq; > ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT; > - ring->get_seqno = gen6_ring_get_seqno; > + if (IS_VALLEYVIEW(dev)) > + ring->get_seqno = ring_get_seqno; > + else > + ring->get_seqno = gen6_ring_get_seqno; > ring->set_seqno = ring_set_seqno; > if (i915_semaphore_is_enabled(dev)) { > ring->semaphore.sync_to = gen6_ring_sync; > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/12/14 10:42, Daniel Vetter wrote: > On Tue, Dec 09, 2014 at 12:59:08PM +0000, John.C.Harrison@Intel.com wrote: >> From: Dave Gordon <david.s.gordon@intel.com> >> >> There is a workaround for a hardware bug when reading the seqno from the status >> page. The bug does not exist on VLV however, the workaround was still being >> applied. > > Given how much trouble the missed seqno fun cause us I'd like more > justification. What kind of (stress)-testing has been done here? And > you're sure you've never seen the little dmesg notice that the kernel > switched to polling? > -Daniel This was necessary during VLV scheduler/preemption work, where we stressed the command streamer and IRQ path probably more than any of the usual tests, since we were looking for out-of-sequence anomalies. The comments in gen6_ring_get_seqno() say it's to fix a bug on ivb/snb, and nothing in the BSpec suggests it's needed on later chips. I think the problem was really in the fact that the workaround implemented in gen6_ring_get_seqno() was reading a nonshadowed register, therefore triggering forcewake activity, which was pretty buggy; so we took out the gen6-specific code before finding that there were still other problems with forcewake. [Aside] The original intention appears to have been to generate a single I/O READ cycle, forcing all pending inbound DMA so be flushed to memory, so that the subsequent memory read would see the latest value. But now, it doesn't just generate a single cycle but a whole flurry of writes and polling reads, because reading ACTHD requires forcewake (unless that was wrong, and has now been fixed). So should gen6_ring_get_seqno() instead read a register that doesn't require forcewake? I wouldn't object to using it in perpetuity if it really only read one register! [/aside] .Dave. >> Change-Id: Ic781fdb31e1f794ce1fa8a6d0d5ee379756c5db6 >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 3887f1a..f990ce4 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -2367,7 +2367,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev) >> ring->irq_get = gen6_ring_get_irq; >> ring->irq_put = gen6_ring_put_irq; >> ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT; >> - ring->get_seqno = gen6_ring_get_seqno; >> + if (IS_VALLEYVIEW(dev)) >> + ring->get_seqno = ring_get_seqno; >> + else >> + ring->get_seqno = gen6_ring_get_seqno; >> ring->set_seqno = ring_set_seqno; >> if (i915_semaphore_is_enabled(dev)) { >> ring->semaphore.sync_to = gen6_ring_sync; >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Dec 10, 2014 at 05:11:09PM +0000, Dave Gordon wrote: > On 10/12/14 10:42, Daniel Vetter wrote: > > On Tue, Dec 09, 2014 at 12:59:08PM +0000, John.C.Harrison@Intel.com wrote: > >> From: Dave Gordon <david.s.gordon@intel.com> > >> > >> There is a workaround for a hardware bug when reading the seqno from the status > >> page. The bug does not exist on VLV however, the workaround was still being > >> applied. > > > > Given how much trouble the missed seqno fun cause us I'd like more > > justification. What kind of (stress)-testing has been done here? And > > you're sure you've never seen the little dmesg notice that the kernel > > switched to polling? > > -Daniel > > This was necessary during VLV scheduler/preemption work, where we > stressed the command streamer and IRQ path probably more than any of the > usual tests, since we were looking for out-of-sequence anomalies. > > The comments in gen6_ring_get_seqno() say it's to fix a bug on ivb/snb, > and nothing in the BSpec suggests it's needed on later chips. > > I think the problem was really in the fact that the workaround > implemented in gen6_ring_get_seqno() was reading a nonshadowed register, > therefore triggering forcewake activity, which was pretty buggy; so we > took out the gen6-specific code before finding that there were still > other problems with forcewake. > > [Aside] > The original intention appears to have been to generate a single I/O > READ cycle, forcing all pending inbound DMA so be flushed to memory, so > that the subsequent memory read would see the latest value. But now, it > doesn't just generate a single cycle but a whole flurry of writes and > polling reads, because reading ACTHD requires forcewake (unless that was > wrong, and has now been fixed). So should gen6_ring_get_seqno() instead > read a register that doesn't require forcewake? I wouldn't object to > using it in perpetuity if it really only read one register! > [/aside] The original intention was to work around missed interrupts. We still have them on pretty much everything gen6+. The workaround we've done for the performance issues this causes is the lazy_coherency trick where we only do this when waiting. It's all pretty nicely cargo-culted and not in Bspec, but it's also fairly real. We've given up on debugging, current kernels switch all wait_request calls to polling once the first missed interrupt happened. See the logic around gpu_error.missed_irq_rings. It might be that it's not real on vlv and only on big core machines, but then we don't have a lot of people using vlv on upstream and reporting bugs. For added insult we don't have any nice way to reproduce this really :( -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 3887f1a..f990ce4 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2367,7 +2367,10 @@ int intel_init_render_ring_buffer(struct drm_device *dev) ring->irq_get = gen6_ring_get_irq; ring->irq_put = gen6_ring_put_irq; ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT; - ring->get_seqno = gen6_ring_get_seqno; + if (IS_VALLEYVIEW(dev)) + ring->get_seqno = ring_get_seqno; + else + ring->get_seqno = gen6_ring_get_seqno; ring->set_seqno = ring_set_seqno; if (i915_semaphore_is_enabled(dev)) { ring->semaphore.sync_to = gen6_ring_sync;