[PATCH 1/2] drm/i915: fix up locking inconsistency around gem_do_init

[PATCH 1/2] drm/i915: fix up locking inconsistency around gem_do_init

[Daniel Vetter]

The locking in our setup and teardown paths is rather arbitrary, but
generally we try to protect gem stuff with dev->struct_mutex. Further,
the ums/gem ioctl to setup gem _does_ take the look. So fix up this
benign inconsistency.

Noticed while reading through code.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 448848c..51bbd03 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1196,6 +1196,7 @@ static int i915_load_gem_init(struct drm_device *dev)
 	/* Basic memrange allocator for stolen space */
 	drm_mm_init(&dev_priv->mm.stolen, 0, prealloc_size);
 
+	mutex_lock(&dev->struct_mutex);
 	/* Let GEM Manage all of the aperture.
 	 *
 	 * However, leave one page at the end still bound to the scratch page.
@@ -1207,7 +1208,6 @@ static int i915_load_gem_init(struct drm_device *dev)
 	 */
 	i915_gem_do_init(dev, 0, mappable_size, gtt_size - PAGE_SIZE);
 
-	mutex_lock(&dev->struct_mutex);
 	ret = i915_gem_init_ringbuffer(dev);
 	mutex_unlock(&dev->struct_mutex);
 	if (ret)
-- 
1.7.8.3

[PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

Chris Wilson reported that with a bunch of patches to no longer force
batchbuffer (in most cases at least) into the mappable part of gtt
that his Pineview died while prefetching the last page of the gtt.

Turns out that since my intel-gtt rewrite we don't actually put a
dummy pte in that last page anymore. Which left me rather puzzled
because Chris' error_state was the first ever since that rewrite that
indicated a dead gpu due to prefetch.

So I've gone ahead and created the gem_cs_prefetch testcase which
reliably manages to execute a batch on the last page. Of all the
machines Chris and I have tried this on only his Pineview fell over,
all the others handled the invalid pte right after the end of the
batch correctly.

The other thing is that due to my intel-gtt we've also started to use
the non-mappable part of the gtt. So my theory is that this guard page
was only necessary when we didn't use and hence also didn't set up any
dummy ptes to the scratch page in that area. In that case the cs would
prefetch into the unmappable gtt area and fail over on the invalid
pte.

So given that this guard page smells like it just duct-taped over an
issue in our code I've simply removed it. And this seems to work!

Now we always run the risk that an odd machine we don't have in our
test labs needs this, but
- we now have an excellent testcase to diagnose any such issues
- and the patch is easy to revert.
Hence I prefer if we try to get rid of this.

Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44748
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 51bbd03..6f918be 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1197,16 +1197,7 @@ static int i915_load_gem_init(struct drm_device *dev)
 	drm_mm_init(&dev_priv->mm.stolen, 0, prealloc_size);
 
 	mutex_lock(&dev->struct_mutex);
-	/* Let GEM Manage all of the aperture.
-	 *
-	 * However, leave one page at the end still bound to the scratch page.
-	 * There are a number of places where the hardware apparently
-	 * prefetches past the end of the object, and we've seen multiple
-	 * hangs with the GPU head pointer stuck in a batchbuffer bound
-	 * at the last page of the aperture.  One page should be enough to
-	 * keep any prefetching inside of the aperture.
-	 */
-	i915_gem_do_init(dev, 0, mappable_size, gtt_size - PAGE_SIZE);
+	i915_gem_do_init(dev, 0, mappable_size, gtt_size);
 
 	ret = i915_gem_init_ringbuffer(dev);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.7.8.3

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1839 bytes --]

On Mon, 30 Jan 2012 16:55:49 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Chris Wilson reported that with a bunch of patches to no longer force
> batchbuffer (in most cases at least) into the mappable part of gtt
> that his Pineview died while prefetching the last page of the gtt.
> 
> Turns out that since my intel-gtt rewrite we don't actually put a
> dummy pte in that last page anymore. Which left me rather puzzled
> because Chris' error_state was the first ever since that rewrite that
> indicated a dead gpu due to prefetch.
> 
> So I've gone ahead and created the gem_cs_prefetch testcase which
> reliably manages to execute a batch on the last page. Of all the
> machines Chris and I have tried this on only his Pineview fell over,
> all the others handled the invalid pte right after the end of the
> batch correctly.
> 
> The other thing is that due to my intel-gtt we've also started to use
> the non-mappable part of the gtt. So my theory is that this guard page
> was only necessary when we didn't use and hence also didn't set up any
> dummy ptes to the scratch page in that area. In that case the cs would
> prefetch into the unmappable gtt area and fail over on the invalid
> pte.
> 
> So given that this guard page smells like it just duct-taped over an
> issue in our code I've simply removed it. And this seems to work!
> 
> Now we always run the risk that an odd machine we don't have in our
> test labs needs this, but
> - we now have an excellent testcase to diagnose any such issues
> - and the patch is easy to revert.
> Hence I prefer if we try to get rid of this.

I like the theory about invalid ptes -- makes sense to me.  I'm still a
little confused: Does the pineview still fail after this patch, and
would just making the hunk do what it meant to again fix it?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

On Mon, Jan 30, 2012 at 11:09:01AM -0800, Eric Anholt wrote:
> On Mon, 30 Jan 2012 16:55:49 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Chris Wilson reported that with a bunch of patches to no longer force
> > batchbuffer (in most cases at least) into the mappable part of gtt
> > that his Pineview died while prefetching the last page of the gtt.
> > 
> > Turns out that since my intel-gtt rewrite we don't actually put a
> > dummy pte in that last page anymore. Which left me rather puzzled
> > because Chris' error_state was the first ever since that rewrite that
> > indicated a dead gpu due to prefetch.
> > 
> > So I've gone ahead and created the gem_cs_prefetch testcase which
> > reliably manages to execute a batch on the last page. Of all the
> > machines Chris and I have tried this on only his Pineview fell over,
> > all the others handled the invalid pte right after the end of the
> > batch correctly.
> > 
> > The other thing is that due to my intel-gtt we've also started to use
> > the non-mappable part of the gtt. So my theory is that this guard page
> > was only necessary when we didn't use and hence also didn't set up any
> > dummy ptes to the scratch page in that area. In that case the cs would
> > prefetch into the unmappable gtt area and fail over on the invalid
> > pte.
> > 
> > So given that this guard page smells like it just duct-taped over an
> > issue in our code I've simply removed it. And this seems to work!
> > 
> > Now we always run the risk that an odd machine we don't have in our
> > test labs needs this, but
> > - we now have an excellent testcase to diagnose any such issues
> > - and the patch is easy to revert.
> > Hence I prefer if we try to get rid of this.
> 
> I like the theory about invalid ptes -- makes sense to me.  I'm still a
> little confused: Does the pineview still fail after this patch, and
> would just making the hunk do what it meant to again fix it?

The pineview happily works when we use the entire gtt and doesn't fall
over when executing a batch in the very last page (as opposed to a batch
in the 2nd last page with the last one having an invalid pte). Just making
the code do again what it claims to do would do the trick, too.

But I'd like to get rid of the scratch ptes long-term so that we can make
good use of the resulting pagefaults for debugging stray writes (at least
on newer generations). This here is one step - but the big part isn't
really the patch but creating a test-case for the cs prefetcher so that we
can actually check things in a reliable, reproducible and quick way.

I'll clarify the commit msg to say more explicitly that pnv works with
this.
-Daniel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

On Mon, Jan 30, 2012 at 08:14:21PM +0100, Daniel Vetter wrote:
> On Mon, Jan 30, 2012 at 11:09:01AM -0800, Eric Anholt wrote:
> > On Mon, 30 Jan 2012 16:55:49 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > Chris Wilson reported that with a bunch of patches to no longer force
> > > batchbuffer (in most cases at least) into the mappable part of gtt
> > > that his Pineview died while prefetching the last page of the gtt.
> > > 
> > > Turns out that since my intel-gtt rewrite we don't actually put a
> > > dummy pte in that last page anymore. Which left me rather puzzled
> > > because Chris' error_state was the first ever since that rewrite that
> > > indicated a dead gpu due to prefetch.
> > > 
> > > So I've gone ahead and created the gem_cs_prefetch testcase which
> > > reliably manages to execute a batch on the last page. Of all the
> > > machines Chris and I have tried this on only his Pineview fell over,
> > > all the others handled the invalid pte right after the end of the
> > > batch correctly.
> > > 
> > > The other thing is that due to my intel-gtt we've also started to use
> > > the non-mappable part of the gtt. So my theory is that this guard page
> > > was only necessary when we didn't use and hence also didn't set up any
> > > dummy ptes to the scratch page in that area. In that case the cs would
> > > prefetch into the unmappable gtt area and fail over on the invalid
> > > pte.
> > > 
> > > So given that this guard page smells like it just duct-taped over an
> > > issue in our code I've simply removed it. And this seems to work!
> > > 
> > > Now we always run the risk that an odd machine we don't have in our
> > > test labs needs this, but
> > > - we now have an excellent testcase to diagnose any such issues
> > > - and the patch is easy to revert.
> > > Hence I prefer if we try to get rid of this.
> > 
> > I like the theory about invalid ptes -- makes sense to me.  I'm still a
> > little confused: Does the pineview still fail after this patch, and
> > would just making the hunk do what it meant to again fix it?
> 
> The pineview happily works when we use the entire gtt and doesn't fall
> over when executing a batch in the very last page (as opposed to a batch
> in the 2nd last page with the last one having an invalid pte). Just making
> the code do again what it claims to do would do the trick, too.
> 
> But I'd like to get rid of the scratch ptes long-term so that we can make
> good use of the resulting pagefaults for debugging stray writes (at least
> on newer generations). This here is one step - but the big part isn't
> really the patch but creating a test-case for the cs prefetcher so that we
> can actually check things in a reliable, reproducible and quick way.
> 
> I'll clarify the commit msg to say more explicitly that pnv works with
> this.

Might as well clarify completely on the testing this got: Tested with
i-g-t/tests/gem_cs_prefetch on ivb, snb, ilk, g33, i915g, i855gm by me.
I'll also test this on a freshly ebayed gm45 which should arrive this week
and my i945gme netbook as soon as I've resurrected grub on that one.
Tested by Chris on his pnv and i965gm. That's only about half of all the
chip versions we support, but about as good as it gets.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

On Mon, Jan 30, 2012 at 08:22:29PM +0100, Daniel Vetter wrote:
> On Mon, Jan 30, 2012 at 08:14:21PM +0100, Daniel Vetter wrote:
> > On Mon, Jan 30, 2012 at 11:09:01AM -0800, Eric Anholt wrote:
> > > On Mon, 30 Jan 2012 16:55:49 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > Chris Wilson reported that with a bunch of patches to no longer force
> > > > batchbuffer (in most cases at least) into the mappable part of gtt
> > > > that his Pineview died while prefetching the last page of the gtt.
> > > > 
> > > > Turns out that since my intel-gtt rewrite we don't actually put a
> > > > dummy pte in that last page anymore. Which left me rather puzzled
> > > > because Chris' error_state was the first ever since that rewrite that
> > > > indicated a dead gpu due to prefetch.
> > > > 
> > > > So I've gone ahead and created the gem_cs_prefetch testcase which
> > > > reliably manages to execute a batch on the last page. Of all the
> > > > machines Chris and I have tried this on only his Pineview fell over,
> > > > all the others handled the invalid pte right after the end of the
> > > > batch correctly.
> > > > 
> > > > The other thing is that due to my intel-gtt we've also started to use
> > > > the non-mappable part of the gtt. So my theory is that this guard page
> > > > was only necessary when we didn't use and hence also didn't set up any
> > > > dummy ptes to the scratch page in that area. In that case the cs would
> > > > prefetch into the unmappable gtt area and fail over on the invalid
> > > > pte.
> > > > 
> > > > So given that this guard page smells like it just duct-taped over an
> > > > issue in our code I've simply removed it. And this seems to work!
> > > > 
> > > > Now we always run the risk that an odd machine we don't have in our
> > > > test labs needs this, but
> > > > - we now have an excellent testcase to diagnose any such issues
> > > > - and the patch is easy to revert.
> > > > Hence I prefer if we try to get rid of this.
> > > 
> > > I like the theory about invalid ptes -- makes sense to me.  I'm still a
> > > little confused: Does the pineview still fail after this patch, and
> > > would just making the hunk do what it meant to again fix it?
> > 
> > The pineview happily works when we use the entire gtt and doesn't fall
> > over when executing a batch in the very last page (as opposed to a batch
> > in the 2nd last page with the last one having an invalid pte). Just making
> > the code do again what it claims to do would do the trick, too.
> > 
> > But I'd like to get rid of the scratch ptes long-term so that we can make
> > good use of the resulting pagefaults for debugging stray writes (at least
> > on newer generations). This here is one step - but the big part isn't
> > really the patch but creating a test-case for the cs prefetcher so that we
> > can actually check things in a reliable, reproducible and quick way.
> > 
> > I'll clarify the commit msg to say more explicitly that pnv works with
> > this.
> 
> Might as well clarify completely on the testing this got: Tested with
> i-g-t/tests/gem_cs_prefetch on ivb, snb, ilk, g33, i915g, i855gm by me.
> I'll also test this on a freshly ebayed gm45 which should arrive this week
> and my i945gme netbook as soon as I've resurrected grub on that one.
> Tested by Chris on his pnv and i965gm. That's only about half of all the
> chip versions we support, but about as good as it gets.

Ok, I've botched up the testing on my snb, shame on me.

Without this patch my snb also dies with gem_cs_prefetch, but like Chris'
pnv, survives without a hitch. So it looks like I've lucked out.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

On Tue, Jan 31, 2012 at 02:24:36PM +0100, Daniel Vetter wrote:
> Ok, I've botched up the testing on my snb, shame on me.
> 
> Without this patch my snb also dies with gem_cs_prefetch, but like Chris'
> pnv, survives without a hitch. So it looks like I've lucked out.

This is getting funny: I've meant to say that _with_ this patch, my snb
survives without a hitch (like Chris' pnv).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: drop the guard page at the end of the gtt

[Daniel Vetter]

On Mon, Jan 30, 2012 at 04:55:49PM +0100, Daniel Vetter wrote:
> Chris Wilson reported that with a bunch of patches to no longer force
> batchbuffer (in most cases at least) into the mappable part of gtt
> that his Pineview died while prefetching the last page of the gtt.
> 
> Turns out that since my intel-gtt rewrite we don't actually put a
> dummy pte in that last page anymore. Which left me rather puzzled
> because Chris' error_state was the first ever since that rewrite that
> indicated a dead gpu due to prefetch.
> 
> So I've gone ahead and created the gem_cs_prefetch testcase which
> reliably manages to execute a batch on the last page. Of all the
> machines Chris and I have tried this on only his Pineview fell over,
> all the others handled the invalid pte right after the end of the
> batch correctly.
> 
> The other thing is that due to my intel-gtt we've also started to use
> the non-mappable part of the gtt. So my theory is that this guard page
> was only necessary when we didn't use and hence also didn't set up any
> dummy ptes to the scratch page in that area. In that case the cs would
> prefetch into the unmappable gtt area and fail over on the invalid
> pte.
> 
> So given that this guard page smells like it just duct-taped over an
> issue in our code I've simply removed it. And this seems to work!
> 
> Now we always run the risk that an odd machine we don't have in our
> test labs needs this, but
> - we now have an excellent testcase to diagnose any such issues
> - and the patch is easy to revert.
> Hence I prefer if we try to get rid of this.
> 
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44748
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, I've seriously botched things up at testing. This actually blows up my
g33 (which works fine without the patch), so we need that darn guard page.

I'll fix this mess up.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48