* [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
@ 2014-03-13 1:40 Damien Lespiau
2014-03-13 2:21 ` Ben Widawsky
0 siblings, 1 reply; 6+ messages in thread
From: Damien Lespiau @ 2014-03-13 1:40 UTC (permalink / raw)
To: intel-gfx; +Cc: benjamin.widawsky
While wandering in the spec, I noticed that BDW removes those 2 bits
from INSTPM. I couldn't find any direct way to invalidate the TLB (ie
without the ring working already). Maybe someone will be more lucky.
At least, we now know we may be a problem.
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c50388a..4eb3e06 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -981,8 +981,14 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
I915_WRITE(mmio, (u32)ring->status_page.gfx_addr);
POSTING_READ(mmio);
- /* Flush the TLB for this page */
- if (INTEL_INFO(dev)->gen >= 6) {
+ /*
+ * Flush the TLB for this page
+ *
+ * FIXME: These two bits have disappeared on gen8, so a question
+ * arises: do we still need this and if so how should we go about
+ * invalidating the TLB?
+ */
+ if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) {
u32 reg = RING_INSTPM(ring->mmio_base);
/* ring should be idle before issuing a sync flush*/
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
2014-03-13 1:40 [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Damien Lespiau
@ 2014-03-13 2:21 ` Ben Widawsky
2014-03-13 2:52 ` Daniel Vetter
2014-03-13 7:51 ` Chris Wilson
0 siblings, 2 replies; 6+ messages in thread
From: Ben Widawsky @ 2014-03-13 2:21 UTC (permalink / raw)
To: Damien Lespiau; +Cc: intel-gfx
On Thu, Mar 13, 2014 at 01:40:28AM +0000, Damien Lespiau wrote:
> While wandering in the spec, I noticed that BDW removes those 2 bits
> from INSTPM. I couldn't find any direct way to invalidate the TLB (ie
> without the ring working already). Maybe someone will be more lucky.
>
> At least, we now know we may be a problem.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c50388a..4eb3e06 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -981,8 +981,14 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
> I915_WRITE(mmio, (u32)ring->status_page.gfx_addr);
> POSTING_READ(mmio);
>
> - /* Flush the TLB for this page */
> - if (INTEL_INFO(dev)->gen >= 6) {
> + /*
> + * Flush the TLB for this page
> + *
> + * FIXME: These two bits have disappeared on gen8, so a question
> + * arises: do we still need this and if so how should we go about
> + * invalidating the TLB?
> + */
> + if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) {
> u32 reg = RING_INSTPM(ring->mmio_base);
>
> /* ring should be idle before issuing a sync flush*/
I'm missing something on the original patch,
884020bf3d2a3787a1cc6df902e98e0eec60330b. How were we emitting
breadcrumbs without flushing the TLB? All bathcbuffers should be
bookended by a TLB invalidate already, so I'm not sure the logic holds.
Chris could explain that one a bit further?
The only reason I bring this up is I'd like to rip this out completely
and have Thiago retest, or at least change the comment/commit message to
be to reflect whatever light Chris sheds on the matter.
Anyway, the bits are definitely gone, and I also can't find a non-ring
based replacement.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
2014-03-13 2:21 ` Ben Widawsky
@ 2014-03-13 2:52 ` Daniel Vetter
2014-03-13 7:51 ` Chris Wilson
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-03-13 2:52 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Wed, Mar 12, 2014 at 07:21:45PM -0700, Ben Widawsky wrote:
> On Thu, Mar 13, 2014 at 01:40:28AM +0000, Damien Lespiau wrote:
> > While wandering in the spec, I noticed that BDW removes those 2 bits
> > from INSTPM. I couldn't find any direct way to invalidate the TLB (ie
> > without the ring working already). Maybe someone will be more lucky.
> >
> > At least, we now know we may be a problem.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index c50388a..4eb3e06 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -981,8 +981,14 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
> > I915_WRITE(mmio, (u32)ring->status_page.gfx_addr);
> > POSTING_READ(mmio);
> >
> > - /* Flush the TLB for this page */
> > - if (INTEL_INFO(dev)->gen >= 6) {
> > + /*
> > + * Flush the TLB for this page
> > + *
> > + * FIXME: These two bits have disappeared on gen8, so a question
> > + * arises: do we still need this and if so how should we go about
> > + * invalidating the TLB?
> > + */
> > + if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) {
> > u32 reg = RING_INSTPM(ring->mmio_base);
> >
> > /* ring should be idle before issuing a sync flush*/
>
> I'm missing something on the original patch,
> 884020bf3d2a3787a1cc6df902e98e0eec60330b. How were we emitting
> breadcrumbs without flushing the TLB? All bathcbuffers should be
> bookended by a TLB invalidate already, so I'm not sure the logic holds.
> Chris could explain that one a bit further?
>
> The only reason I bring this up is I'd like to rip this out completely
> and have Thiago retest, or at least change the comment/commit message to
> be to reflect whatever light Chris sheds on the matter.
>
> Anyway, the bits are definitely gone, and I also can't find a non-ring
> based replacement.
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
2014-03-13 2:21 ` Ben Widawsky
2014-03-13 2:52 ` Daniel Vetter
@ 2014-03-13 7:51 ` Chris Wilson
2014-03-13 15:19 ` Ben Widawsky
1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-03-13 7:51 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Wed, Mar 12, 2014 at 07:21:45PM -0700, Ben Widawsky wrote:
> I'm missing something on the original patch,
> 884020bf3d2a3787a1cc6df902e98e0eec60330b. How were we emitting
> breadcrumbs without flushing the TLB? All bathcbuffers should be
> bookended by a TLB invalidate already, so I'm not sure the logic holds.
> Chris could explain that one a bit further?
Upon resume, the hardware continues writing the breadcrumbs into the old
hws page (due to the stale TLB) and we try to read the seqno from the
new page, so as shown by the error-states it appears that the breadcrumb
writes are not happening. Since the hardware is writing to a random address,
we are now corrupting random memory.
Which is what I thought I said in the changelog.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
2014-03-13 7:51 ` Chris Wilson
@ 2014-03-13 15:19 ` Ben Widawsky
2014-03-13 15:23 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2014-03-13 15:19 UTC (permalink / raw)
To: Chris Wilson, Ben Widawsky, Damien Lespiau,
intel-gfx@lists.freedesktop.org
On Thu, Mar 13, 2014 at 12:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Upon resume, the hardware continues writing the breadcrumbs into the old
> hws page (due to the stale TLB) and we try to read the seqno from the
> new page, so as shown by the error-states it appears that the breadcrumb
> writes are not happening. Since the hardware is writing to a random address,
> we are now corrupting random memory.
>
> Which is what I thought I said in the changelog.
Yes, you did say that. However, we should be idling on freeze, so the
explanation
I was missing is how or why the HW is continuing to use the old status page even
though we've had to do a TLB flush when we emit the next batch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
2014-03-13 15:19 ` Ben Widawsky
@ 2014-03-13 15:23 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2014-03-13 15:23 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx@lists.freedesktop.org
On Thu, Mar 13, 2014 at 08:19:06AM -0700, Ben Widawsky wrote:
> On Thu, Mar 13, 2014 at 12:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Upon resume, the hardware continues writing the breadcrumbs into the old
> > hws page (due to the stale TLB) and we try to read the seqno from the
> > new page, so as shown by the error-states it appears that the breadcrumb
> > writes are not happening. Since the hardware is writing to a random address,
> > we are now corrupting random memory.
> >
> > Which is what I thought I said in the changelog.
>
> Yes, you did say that. However, we should be idling on freeze, so the
> explanation
> I was missing is how or why the HW is continuing to use the old status page even
> though we've had to do a TLB flush when we emit the next batch.
Different set of TLBs. This CS TLB appears to be unrelated to individual
set of cache TLBs we flush with pipecontrols or flush_dw.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-13 15:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-13 1:40 [PATCH] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Damien Lespiau
2014-03-13 2:21 ` Ben Widawsky
2014-03-13 2:52 ` Daniel Vetter
2014-03-13 7:51 ` Chris Wilson
2014-03-13 15:19 ` Ben Widawsky
2014-03-13 15:23 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.