| Message ID | 20181217152055.12872-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] drm/i915: Apply missed interrupt after reset w/a to all ringbuffer gen | expand |
On Mon, Dec 17, 2018 at 03:20:55PM +0000, Chris Wilson wrote: > Having completed a test run of gem_eio across all machines in CI we also > observe the phenomenon (of lost interrupts after resetting the GPU) on > gen3 machines as well as the previously sighted gen6/gen7. Let's apply > the same HWSTAM workaround that was effective for gen6+ for all, as > although we haven't seen the same failure on gen4/5 it seems prudent to > keep the code the same. > > As a consequence we can remove the extra setting of HWSTAM and apply the > register from a single site. > > v2: Delazy and move the HWSTAM into its own function > v3: Mask off all HWSP writes on driver unload and engine cleanup. > v4: And what about the physical hwsp? > v5: No, engine->init_hw() is not called from driver_init_hw(), don't be > daft. Really scrub HWSTAM as early as we can in driver_init_mmio() > v6: Rename set_hwsp as it was setting the mask not the hwsp register. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=108735 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 9 -- > drivers/gpu/drm/i915/intel_engine_cs.c | 23 +++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 107 +++++++++++++++--------- > 3 files changed, 91 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index e2dac9b5f4ce..0c7fc9890891 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -3586,9 +3586,6 @@ static void ironlake_irq_reset(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > > - if (IS_GEN(dev_priv, 5)) > - I915_WRITE(HWSTAM, 0xffffffff); > - > GEN3_IRQ_RESET(DE); > if (IS_GEN(dev_priv, 7)) > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > @@ -4368,8 +4365,6 @@ static void i8xx_irq_reset(struct drm_device *dev) > > i9xx_pipestat_irq_reset(dev_priv); > > - I915_WRITE16(HWSTAM, 0xffff); > - > GEN2_IRQ_RESET(); > } > > @@ -4537,8 +4532,6 @@ static void i915_irq_reset(struct drm_device *dev) > > i9xx_pipestat_irq_reset(dev_priv); > > - I915_WRITE(HWSTAM, 0xffffffff); > - > GEN3_IRQ_RESET(); > } > > @@ -4648,8 +4641,6 @@ static void i965_irq_reset(struct drm_device *dev) > > i9xx_pipestat_irq_reset(dev_priv); > > - I915_WRITE(HWSTAM, 0xffffffff); > - > GEN3_IRQ_RESET(); > } > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 6f165f9ad2bf..f1a2aeb34e5e 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -261,6 +261,23 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) > info->instance) >= INTEL_ENGINE_CS_MAX_NAME); > } > > +static void set_hwsp_writemask(struct intel_engine_cs *engine, u32 mask) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); g4x/ilk don't have HWSTAM for the bsd. A quick test on my ilk shows that the register at that offset stays at 0x0 even if written, so it might even be safe. But maybe we shouldn't poke at things that aren't there? > + > + if (INTEL_GEN(dev_priv) >= 3) > + I915_WRITE(hwstam, mask); > + else > + I915_WRITE16(hwstam, mask); > +} > + > +static void intel_engine_sanitize_mmio(struct intel_engine_cs *engine) > +{ > + /* Mask off all writes into the unknown HWSP */ > + set_hwsp_writemask(engine, ~0u); > +} > + > static int > intel_engine_setup(struct drm_i915_private *dev_priv, > enum intel_engine_id id) > @@ -312,6 +329,9 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > > ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier); > > + /* Scrub mmio state on takeover */ > + intel_engine_sanitize_mmio(engine); > + > dev_priv->engine_class[info->class][info->instance] = engine; > dev_priv->engine[id] = engine; > return 0; > @@ -495,6 +515,9 @@ void intel_engine_setup_common(struct intel_engine_cs *engine) > > static void cleanup_status_page(struct intel_engine_cs *engine) > { > + /* Stop writing into the status page before returnig it to the system */ > + set_hwsp_writemask(engine, ~0u); > + > if (HWS_NEEDS_PHYSICAL(engine->i915)) { > void *addr = fetch_and_zero(&engine->status_page.page_addr); > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index fdeca2b877c9..88f50e30a0c4 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -379,11 +379,31 @@ gen7_render_ring_flush(struct i915_request *rq, u32 mode) > return 0; > } > > -static void ring_setup_phys_status_page(struct intel_engine_cs *engine) > +static void set_hwstam(struct intel_engine_cs *engine, u32 mask) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); > + Ditto. Otherwise this looks good to me. > + /* > + * Keep the render interrupt unmasked as this papers over > + * lost interrupts following a reset. > + */ > + if (engine->id == RCS) { > + if (INTEL_GEN(engine->i915) >= 6) > + mask &= ~BIT(0); > + else > + mask &= ~I915_USER_INTERRUPT; > + } > + > + if (INTEL_GEN(dev_priv) >= 3) > + I915_WRITE(hwstam, mask); > + else > + I915_WRITE16(hwstam, mask); > +} > + > +static void set_hws_pga(struct intel_engine_cs *engine, phys_addr_t phys) > { > struct drm_i915_private *dev_priv = engine->i915; > - struct page *page = virt_to_page(engine->status_page.page_addr); > - phys_addr_t phys = PFN_PHYS(page_to_pfn(page)); > u32 addr; > > addr = lower_32_bits(phys); > @@ -393,12 +413,22 @@ static void ring_setup_phys_status_page(struct intel_engine_cs *engine) > I915_WRITE(HWS_PGA, addr); > } > > -static void intel_ring_setup_status_page(struct intel_engine_cs *engine) > +static void ring_setup_phys_status_page(struct intel_engine_cs *engine) > +{ > + struct page *page = virt_to_page(engine->status_page.page_addr); > + phys_addr_t phys = PFN_PHYS(page_to_pfn(page)); > + > + set_hws_pga(engine, phys); > + set_hwstam(engine, ~0u); > +} > + > +static void set_hwsp(struct intel_engine_cs *engine, u32 offset) > { > struct drm_i915_private *dev_priv = engine->i915; > - i915_reg_t mmio; > + i915_reg_t hwsp; > > - /* The ring status page addresses are no longer next to the rest of > + /* > + * The ring status page addresses are no longer next to the rest of > * the ring registers as of gen7. > */ > if (IS_GEN(dev_priv, 7)) { > @@ -410,56 +440,55 @@ static void intel_ring_setup_status_page(struct intel_engine_cs *engine) > default: > GEM_BUG_ON(engine->id); > case RCS: > - mmio = RENDER_HWS_PGA_GEN7; > + hwsp = RENDER_HWS_PGA_GEN7; > break; > case BCS: > - mmio = BLT_HWS_PGA_GEN7; > + hwsp = BLT_HWS_PGA_GEN7; > break; > case VCS: > - mmio = BSD_HWS_PGA_GEN7; > + hwsp = BSD_HWS_PGA_GEN7; > break; > case VECS: > - mmio = VEBOX_HWS_PGA_GEN7; > + hwsp = VEBOX_HWS_PGA_GEN7; > break; > } > } else if (IS_GEN(dev_priv, 6)) { > - mmio = RING_HWS_PGA_GEN6(engine->mmio_base); > + hwsp = RING_HWS_PGA_GEN6(engine->mmio_base); > } else { > - mmio = RING_HWS_PGA(engine->mmio_base); > + hwsp = RING_HWS_PGA(engine->mmio_base); > } > > - if (INTEL_GEN(dev_priv) >= 6) { > - u32 mask = ~0u; > + I915_WRITE(hwsp, offset); > + POSTING_READ(hwsp); > +} > > - /* > - * Keep the render interrupt unmasked as this papers over > - * lost interrupts following a reset. > - */ > - if (engine->id == RCS) > - mask &= ~BIT(0); > +static void flush_cs_tlb(struct intel_engine_cs *engine) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + i915_reg_t instpm = RING_INSTPM(engine->mmio_base); > > - I915_WRITE(RING_HWSTAM(engine->mmio_base), mask); > - } > + if (!IS_GEN_RANGE(dev_priv, 6, 7)) > + return; > > - I915_WRITE(mmio, engine->status_page.ggtt_offset); > - POSTING_READ(mmio); > + /* ring should be idle before issuing a sync flush*/ > + WARN_ON((I915_READ_MODE(engine) & MODE_IDLE) == 0); > > - /* Flush the TLB for this page */ > - if (IS_GEN_RANGE(dev_priv, 6, 7)) { > - i915_reg_t reg = RING_INSTPM(engine->mmio_base); > + I915_WRITE(instpm, > + _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE | > + INSTPM_SYNC_FLUSH)); > + if (intel_wait_for_register(dev_priv, > + instpm, INSTPM_SYNC_FLUSH, 0, > + 1000)) > + DRM_ERROR("%s: wait for SyncFlush to complete for TLB invalidation timed out\n", > + engine->name); > +} > > - /* ring should be idle before issuing a sync flush*/ > - WARN_ON((I915_READ_MODE(engine) & MODE_IDLE) == 0); > +static void ring_setup_status_page(struct intel_engine_cs *engine) > +{ > + set_hwsp(engine, engine->status_page.ggtt_offset); > + set_hwstam(engine, ~0u); > > - I915_WRITE(reg, > - _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE | > - INSTPM_SYNC_FLUSH)); > - if (intel_wait_for_register(dev_priv, > - reg, INSTPM_SYNC_FLUSH, 0, > - 1000)) > - DRM_ERROR("%s: wait for SyncFlush to complete for TLB invalidation timed out\n", > - engine->name); > - } > + flush_cs_tlb(engine); > } > > static bool stop_ring(struct intel_engine_cs *engine) > @@ -529,7 +558,7 @@ static int init_ring_common(struct intel_engine_cs *engine) > if (HWS_NEEDS_PHYSICAL(dev_priv)) > ring_setup_phys_status_page(engine); > else > - intel_ring_setup_status_page(engine); > + ring_setup_status_page(engine); > > intel_engine_reset_breadcrumbs(engine); > > -- > 2.20.0
Quoting Ville Syrjälä (2018-12-17 18:02:25) > On Mon, Dec 17, 2018 at 03:20:55PM +0000, Chris Wilson wrote: > > Having completed a test run of gem_eio across all machines in CI we also > > observe the phenomenon (of lost interrupts after resetting the GPU) on > > gen3 machines as well as the previously sighted gen6/gen7. Let's apply > > the same HWSTAM workaround that was effective for gen6+ for all, as > > although we haven't seen the same failure on gen4/5 it seems prudent to > > keep the code the same. > > > > As a consequence we can remove the extra setting of HWSTAM and apply the > > register from a single site. > > > > v2: Delazy and move the HWSTAM into its own function > > v3: Mask off all HWSP writes on driver unload and engine cleanup. > > v4: And what about the physical hwsp? > > v5: No, engine->init_hw() is not called from driver_init_hw(), don't be > > daft. Really scrub HWSTAM as early as we can in driver_init_mmio() > > v6: Rename set_hwsp as it was setting the mask not the hwsp register. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=108735 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 9 -- > > drivers/gpu/drm/i915/intel_engine_cs.c | 23 +++++ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 107 +++++++++++++++--------- > > 3 files changed, 91 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index e2dac9b5f4ce..0c7fc9890891 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -3586,9 +3586,6 @@ static void ironlake_irq_reset(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > - if (IS_GEN(dev_priv, 5)) > > - I915_WRITE(HWSTAM, 0xffffffff); > > - > > GEN3_IRQ_RESET(DE); > > if (IS_GEN(dev_priv, 7)) > > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > > @@ -4368,8 +4365,6 @@ static void i8xx_irq_reset(struct drm_device *dev) > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > - I915_WRITE16(HWSTAM, 0xffff); > > - > > GEN2_IRQ_RESET(); > > } > > > > @@ -4537,8 +4532,6 @@ static void i915_irq_reset(struct drm_device *dev) > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > - I915_WRITE(HWSTAM, 0xffffffff); > > - > > GEN3_IRQ_RESET(); > > } > > > > @@ -4648,8 +4641,6 @@ static void i965_irq_reset(struct drm_device *dev) > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > - I915_WRITE(HWSTAM, 0xffffffff); > > - > > GEN3_IRQ_RESET(); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index 6f165f9ad2bf..f1a2aeb34e5e 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -261,6 +261,23 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) > > info->instance) >= INTEL_ENGINE_CS_MAX_NAME); > > } > > > > +static void set_hwsp_writemask(struct intel_engine_cs *engine, u32 mask) > > +{ > > + struct drm_i915_private *dev_priv = engine->i915; > > + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); > > g4x/ilk don't have HWSTAM for the bsd. A quick test on my ilk shows that > the register at that offset stays at 0x0 even if written, so it might > even be safe. But maybe we shouldn't poke at things that aren't there? Swines. Did they forget where the copy function was? Or did they use the one mask for both status pages? Probably just don't maintain a copy of the iir in hwsp/bsd. Oh well, another respin. -Chris
On Mon, Dec 17, 2018 at 06:07:45PM +0000, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-12-17 18:02:25) > > On Mon, Dec 17, 2018 at 03:20:55PM +0000, Chris Wilson wrote: > > > Having completed a test run of gem_eio across all machines in CI we also > > > observe the phenomenon (of lost interrupts after resetting the GPU) on > > > gen3 machines as well as the previously sighted gen6/gen7. Let's apply > > > the same HWSTAM workaround that was effective for gen6+ for all, as > > > although we haven't seen the same failure on gen4/5 it seems prudent to > > > keep the code the same. > > > > > > As a consequence we can remove the extra setting of HWSTAM and apply the > > > register from a single site. > > > > > > v2: Delazy and move the HWSTAM into its own function > > > v3: Mask off all HWSP writes on driver unload and engine cleanup. > > > v4: And what about the physical hwsp? > > > v5: No, engine->init_hw() is not called from driver_init_hw(), don't be > > > daft. Really scrub HWSTAM as early as we can in driver_init_mmio() > > > v6: Rename set_hwsp as it was setting the mask not the hwsp register. > > > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=108735 > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 9 -- > > > drivers/gpu/drm/i915/intel_engine_cs.c | 23 +++++ > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 107 +++++++++++++++--------- > > > 3 files changed, 91 insertions(+), 48 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index e2dac9b5f4ce..0c7fc9890891 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -3586,9 +3586,6 @@ static void ironlake_irq_reset(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > > > - if (IS_GEN(dev_priv, 5)) > > > - I915_WRITE(HWSTAM, 0xffffffff); > > > - > > > GEN3_IRQ_RESET(DE); > > > if (IS_GEN(dev_priv, 7)) > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > > > @@ -4368,8 +4365,6 @@ static void i8xx_irq_reset(struct drm_device *dev) > > > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > > > - I915_WRITE16(HWSTAM, 0xffff); > > > - > > > GEN2_IRQ_RESET(); > > > } > > > > > > @@ -4537,8 +4532,6 @@ static void i915_irq_reset(struct drm_device *dev) > > > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > > > - I915_WRITE(HWSTAM, 0xffffffff); > > > - > > > GEN3_IRQ_RESET(); > > > } > > > > > > @@ -4648,8 +4641,6 @@ static void i965_irq_reset(struct drm_device *dev) > > > > > > i9xx_pipestat_irq_reset(dev_priv); > > > > > > - I915_WRITE(HWSTAM, 0xffffffff); > > > - > > > GEN3_IRQ_RESET(); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > > index 6f165f9ad2bf..f1a2aeb34e5e 100644 > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > > @@ -261,6 +261,23 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) > > > info->instance) >= INTEL_ENGINE_CS_MAX_NAME); > > > } > > > > > > +static void set_hwsp_writemask(struct intel_engine_cs *engine, u32 mask) > > > +{ > > > + struct drm_i915_private *dev_priv = engine->i915; > > > + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); > > > > g4x/ilk don't have HWSTAM for the bsd. A quick test on my ilk shows that > > the register at that offset stays at 0x0 even if written, so it might > > even be safe. But maybe we shouldn't poke at things that aren't there? > > Swines. Did they forget where the copy function was? Or did they use the > one mask for both status pages? Probably just don't maintain a copy of > the iir in hwsp/bsd. Not sure. Morbid curiosity might force me to find out one day.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index e2dac9b5f4ce..0c7fc9890891 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3586,9 +3586,6 @@ static void ironlake_irq_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); - if (IS_GEN(dev_priv, 5)) - I915_WRITE(HWSTAM, 0xffffffff); - GEN3_IRQ_RESET(DE); if (IS_GEN(dev_priv, 7)) I915_WRITE(GEN7_ERR_INT, 0xffffffff); @@ -4368,8 +4365,6 @@ static void i8xx_irq_reset(struct drm_device *dev) i9xx_pipestat_irq_reset(dev_priv); - I915_WRITE16(HWSTAM, 0xffff); - GEN2_IRQ_RESET(); } @@ -4537,8 +4532,6 @@ static void i915_irq_reset(struct drm_device *dev) i9xx_pipestat_irq_reset(dev_priv); - I915_WRITE(HWSTAM, 0xffffffff); - GEN3_IRQ_RESET(); } @@ -4648,8 +4641,6 @@ static void i965_irq_reset(struct drm_device *dev) i9xx_pipestat_irq_reset(dev_priv); - I915_WRITE(HWSTAM, 0xffffffff); - GEN3_IRQ_RESET(); } diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 6f165f9ad2bf..f1a2aeb34e5e 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -261,6 +261,23 @@ static void __sprint_engine_name(char *name, const struct engine_info *info) info->instance) >= INTEL_ENGINE_CS_MAX_NAME); } +static void set_hwsp_writemask(struct intel_engine_cs *engine, u32 mask) +{ + struct drm_i915_private *dev_priv = engine->i915; + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); + + if (INTEL_GEN(dev_priv) >= 3) + I915_WRITE(hwstam, mask); + else + I915_WRITE16(hwstam, mask); +} + +static void intel_engine_sanitize_mmio(struct intel_engine_cs *engine) +{ + /* Mask off all writes into the unknown HWSP */ + set_hwsp_writemask(engine, ~0u); +} + static int intel_engine_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id) @@ -312,6 +329,9 @@ intel_engine_setup(struct drm_i915_private *dev_priv, ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier); + /* Scrub mmio state on takeover */ + intel_engine_sanitize_mmio(engine); + dev_priv->engine_class[info->class][info->instance] = engine; dev_priv->engine[id] = engine; return 0; @@ -495,6 +515,9 @@ void intel_engine_setup_common(struct intel_engine_cs *engine) static void cleanup_status_page(struct intel_engine_cs *engine) { + /* Stop writing into the status page before returnig it to the system */ + set_hwsp_writemask(engine, ~0u); + if (HWS_NEEDS_PHYSICAL(engine->i915)) { void *addr = fetch_and_zero(&engine->status_page.page_addr); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index fdeca2b877c9..88f50e30a0c4 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -379,11 +379,31 @@ gen7_render_ring_flush(struct i915_request *rq, u32 mode) return 0; } -static void ring_setup_phys_status_page(struct intel_engine_cs *engine) +static void set_hwstam(struct intel_engine_cs *engine, u32 mask) +{ + struct drm_i915_private *dev_priv = engine->i915; + i915_reg_t hwstam = RING_HWSTAM(engine->mmio_base); + + /* + * Keep the render interrupt unmasked as this papers over + * lost interrupts following a reset. + */ + if (engine->id == RCS) { + if (INTEL_GEN(engine->i915) >= 6) + mask &= ~BIT(0); + else + mask &= ~I915_USER_INTERRUPT; + } + + if (INTEL_GEN(dev_priv) >= 3) + I915_WRITE(hwstam, mask); + else + I915_WRITE16(hwstam, mask); +} + +static void set_hws_pga(struct intel_engine_cs *engine, phys_addr_t phys) { struct drm_i915_private *dev_priv = engine->i915; - struct page *page = virt_to_page(engine->status_page.page_addr); - phys_addr_t phys = PFN_PHYS(page_to_pfn(page)); u32 addr; addr = lower_32_bits(phys); @@ -393,12 +413,22 @@ static void ring_setup_phys_status_page(struct intel_engine_cs *engine) I915_WRITE(HWS_PGA, addr); } -static void intel_ring_setup_status_page(struct intel_engine_cs *engine) +static void ring_setup_phys_status_page(struct intel_engine_cs *engine) +{ + struct page *page = virt_to_page(engine->status_page.page_addr); + phys_addr_t phys = PFN_PHYS(page_to_pfn(page)); + + set_hws_pga(engine, phys); + set_hwstam(engine, ~0u); +} + +static void set_hwsp(struct intel_engine_cs *engine, u32 offset) { struct drm_i915_private *dev_priv = engine->i915; - i915_reg_t mmio; + i915_reg_t hwsp; - /* The ring status page addresses are no longer next to the rest of + /* + * The ring status page addresses are no longer next to the rest of * the ring registers as of gen7. */ if (IS_GEN(dev_priv, 7)) { @@ -410,56 +440,55 @@ static void intel_ring_setup_status_page(struct intel_engine_cs *engine) default: GEM_BUG_ON(engine->id); case RCS: - mmio = RENDER_HWS_PGA_GEN7; + hwsp = RENDER_HWS_PGA_GEN7; break; case BCS: - mmio = BLT_HWS_PGA_GEN7; + hwsp = BLT_HWS_PGA_GEN7; break; case VCS: - mmio = BSD_HWS_PGA_GEN7; + hwsp = BSD_HWS_PGA_GEN7; break; case VECS: - mmio = VEBOX_HWS_PGA_GEN7; + hwsp = VEBOX_HWS_PGA_GEN7; break; } } else if (IS_GEN(dev_priv, 6)) { - mmio = RING_HWS_PGA_GEN6(engine->mmio_base); + hwsp = RING_HWS_PGA_GEN6(engine->mmio_base); } else { - mmio = RING_HWS_PGA(engine->mmio_base); + hwsp = RING_HWS_PGA(engine->mmio_base); } - if (INTEL_GEN(dev_priv) >= 6) { - u32 mask = ~0u; + I915_WRITE(hwsp, offset); + POSTING_READ(hwsp); +} - /* - * Keep the render interrupt unmasked as this papers over - * lost interrupts following a reset. - */ - if (engine->id == RCS) - mask &= ~BIT(0); +static void flush_cs_tlb(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->i915; + i915_reg_t instpm = RING_INSTPM(engine->mmio_base); - I915_WRITE(RING_HWSTAM(engine->mmio_base), mask); - } + if (!IS_GEN_RANGE(dev_priv, 6, 7)) + return; - I915_WRITE(mmio, engine->status_page.ggtt_offset); - POSTING_READ(mmio); + /* ring should be idle before issuing a sync flush*/ + WARN_ON((I915_READ_MODE(engine) & MODE_IDLE) == 0); - /* Flush the TLB for this page */ - if (IS_GEN_RANGE(dev_priv, 6, 7)) { - i915_reg_t reg = RING_INSTPM(engine->mmio_base); + I915_WRITE(instpm, + _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE | + INSTPM_SYNC_FLUSH)); + if (intel_wait_for_register(dev_priv, + instpm, INSTPM_SYNC_FLUSH, 0, + 1000)) + DRM_ERROR("%s: wait for SyncFlush to complete for TLB invalidation timed out\n", + engine->name); +} - /* ring should be idle before issuing a sync flush*/ - WARN_ON((I915_READ_MODE(engine) & MODE_IDLE) == 0); +static void ring_setup_status_page(struct intel_engine_cs *engine) +{ + set_hwsp(engine, engine->status_page.ggtt_offset); + set_hwstam(engine, ~0u); - I915_WRITE(reg, - _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE | - INSTPM_SYNC_FLUSH)); - if (intel_wait_for_register(dev_priv, - reg, INSTPM_SYNC_FLUSH, 0, - 1000)) - DRM_ERROR("%s: wait for SyncFlush to complete for TLB invalidation timed out\n", - engine->name); - } + flush_cs_tlb(engine); } static bool stop_ring(struct intel_engine_cs *engine) @@ -529,7 +558,7 @@ static int init_ring_common(struct intel_engine_cs *engine) if (HWS_NEEDS_PHYSICAL(dev_priv)) ring_setup_phys_status_page(engine); else - intel_ring_setup_status_page(engine); + ring_setup_status_page(engine); intel_engine_reset_breadcrumbs(engine);
Having completed a test run of gem_eio across all machines in CI we also observe the phenomenon (of lost interrupts after resetting the GPU) on gen3 machines as well as the previously sighted gen6/gen7. Let's apply the same HWSTAM workaround that was effective for gen6+ for all, as although we haven't seen the same failure on gen4/5 it seems prudent to keep the code the same. As a consequence we can remove the extra setting of HWSTAM and apply the register from a single site. v2: Delazy and move the HWSTAM into its own function v3: Mask off all HWSP writes on driver unload and engine cleanup. v4: And what about the physical hwsp? v5: No, engine->init_hw() is not called from driver_init_hw(), don't be daft. Really scrub HWSTAM as early as we can in driver_init_mmio() v6: Rename set_hwsp as it was setting the mask not the hwsp register. References: https://bugs.freedesktop.org/show_bug.cgi?id=108735 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 9 -- drivers/gpu/drm/i915/intel_engine_cs.c | 23 +++++ drivers/gpu/drm/i915/intel_ringbuffer.c | 107 +++++++++++++++--------- 3 files changed, 91 insertions(+), 48 deletions(-)