* [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
@ 2012-10-09 17:59 Daniel Vetter
2012-10-09 20:41 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-10-09 17:59 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
The obj->pages to obj->pages->sgl rework introduced this helper, but
it doesn't actually work for n % SG_MAX_SINGLE_ALLOC == 0.
This is exercised by the improved hangman tests and the gem_exec_big
test in i-g-t.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4f2831a..d3dbd0f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1341,7 +1341,7 @@ int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
static inline struct page *i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
{
struct scatterlist *sg = obj->pages->sgl;
- while (n >= SG_MAX_SINGLE_ALLOC) {
+ while (n >= SG_MAX_SINGLE_ALLOC - 1) {
sg = sg_chain_ptr(sg + SG_MAX_SINGLE_ALLOC - 1);
n -= SG_MAX_SINGLE_ALLOC - 1;
}
--
1.7.11.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-09 17:59 [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper Daniel Vetter
@ 2012-10-09 20:41 ` Chris Wilson
2012-10-09 20:50 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-10-09 20:41 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Tue, 9 Oct 2012 19:59:02 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The obj->pages to obj->pages->sgl rework introduced this helper, but
> it doesn't actually work for n % SG_MAX_SINGLE_ALLOC == 0.
>
> This is exercised by the improved hangman tests and the gem_exec_big
> test in i-g-t.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I had to think about that harder than I should, to be sure we handled
the case where the last element was a page and not a chainptr correctly.
On hindsight, the code is clearly bogus for the n==SG_MAX_SINGLE_ALLOC
because n is an index. My original thought was to use 'nth' instead and
I believe that it would have helped prevent my confusion. Ah hindisght.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-09 20:41 ` Chris Wilson
@ 2012-10-09 20:50 ` Daniel Vetter
2012-10-09 22:16 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-10-09 20:50 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
The obj->pages to obj->pages->sgl rework introduced this helper, but
it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
For simplicity (and since right now I seem to be too stupid to see
the bug), let's just grab the right page with a for_each_sg loop.
This is exercised by the improved hangman tests and the gem_exec_big
test in i-g-t.
v2: Compared to v1, don't try to be clever since I seemingly only
manage to prove that I'm not clever.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4f2831a..32a2e47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1340,12 +1340,14 @@ void i915_gem_lastclose(struct drm_device *dev);
int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
static inline struct page *i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
{
- struct scatterlist *sg = obj->pages->sgl;
- while (n >= SG_MAX_SINGLE_ALLOC) {
- sg = sg_chain_ptr(sg + SG_MAX_SINGLE_ALLOC - 1);
- n -= SG_MAX_SINGLE_ALLOC - 1;
+ struct scatterlist *sg;
+ int i;
+
+ for_each_sg(obj->pages->sgl, sg, obj->pages->nents, i) {
+ if (i == n)
+ return sg_page(sg);
}
- return sg_page(sg+n);
+ return NULL;
}
static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
{
--
1.7.11.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-09 20:50 ` Daniel Vetter
@ 2012-10-09 22:16 ` Chris Wilson
2012-10-10 9:01 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-10-09 22:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The obj->pages to obj->pages->sgl rework introduced this helper, but
> it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
>
> For simplicity (and since right now I seem to be too stupid to see
> the bug), let's just grab the right page with a for_each_sg loop.
>
> This is exercised by the improved hangman tests and the gem_exec_big
> test in i-g-t.
>
> v2: Compared to v1, don't try to be clever since I seemingly only
> manage to prove that I'm not clever.
Only I expect that loop to show up on profiles even higher than the
sg_next() from pwrite. :|
I expect it to have a measureable impact upon relocation throughput,
so I should measure it...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-09 22:16 ` Chris Wilson
@ 2012-10-10 9:01 ` Chris Wilson
2012-10-10 9:15 ` Daniel Vetter
2012-10-10 9:21 ` Daniel Vetter
0 siblings, 2 replies; 10+ messages in thread
From: Chris Wilson @ 2012-10-10 9:01 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> >
> > For simplicity (and since right now I seem to be too stupid to see
> > the bug), let's just grab the right page with a for_each_sg loop.
> >
> > This is exercised by the improved hangman tests and the gem_exec_big
> > test in i-g-t.
> >
> > v2: Compared to v1, don't try to be clever since I seemingly only
> > manage to prove that I'm not clever.
>
> Only I expect that loop to show up on profiles even higher than the
> sg_next() from pwrite. :|
>
> I expect it to have a measureable impact upon relocation throughput,
> so I should measure it...
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The obj->pages to obj->pages->sgl rework introduced this helper, but
> it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
>
> For simplicity (and since right now I seem to be too stupid to see
> the bug), let's just grab the right page with a for_each_sg loop.
>
> This is exercised by the improved hangman tests and the gem_exec_big
> test in i-g-t.
>
> v2: Compared to v1, don't try to be clever since I seemingly only
> manage to prove that I'm not clever.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Looks like my worries are baseless. It can always be attacked latter if
need be. I'd still like to know what the mistake was...
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-10 9:01 ` Chris Wilson
@ 2012-10-10 9:15 ` Daniel Vetter
2012-10-10 9:21 ` Daniel Vetter
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-10 9:15 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development
On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote:
> On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > >
> > > For simplicity (and since right now I seem to be too stupid to see
> > > the bug), let's just grab the right page with a for_each_sg loop.
> > >
> > > This is exercised by the improved hangman tests and the gem_exec_big
> > > test in i-g-t.
> > >
> > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > manage to prove that I'm not clever.
> >
> > Only I expect that loop to show up on profiles even higher than the
> > sg_next() from pwrite. :|
> >
> > I expect it to have a measureable impact upon relocation throughput,
> > so I should measure it...
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
> To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
>
> On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> >
> > For simplicity (and since right now I seem to be too stupid to see
> > the bug), let's just grab the right page with a for_each_sg loop.
> >
> > This is exercised by the improved hangman tests and the gem_exec_big
> > test in i-g-t.
> >
> > v2: Compared to v1, don't try to be clever since I seemingly only
> > manage to prove that I'm not clever.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Looks like my worries are baseless. It can always be attacked latter if
> need be. I'd still like to know what the mistake was...
I think it was two mistakes:
- One was the off-by-one fixed in v1.
- Second seemed to be the special case that if the table fits exactly,
sg_alloc doesn't set a chain ptr with another sg table with just one
entry.
But like the commit message says, I've failed to be clever enough
yesterday to make it work.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-10 9:01 ` Chris Wilson
2012-10-10 9:15 ` Daniel Vetter
@ 2012-10-10 9:21 ` Daniel Vetter
2012-10-10 10:36 ` Chris Wilson
1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-10-10 9:21 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development
On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote:
> On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > >
> > > For simplicity (and since right now I seem to be too stupid to see
> > > the bug), let's just grab the right page with a for_each_sg loop.
> > >
> > > This is exercised by the improved hangman tests and the gem_exec_big
> > > test in i-g-t.
> > >
> > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > manage to prove that I'm not clever.
> >
> > Only I expect that loop to show up on profiles even higher than the
> > sg_next() from pwrite. :|
> >
> > I expect it to have a measureable impact upon relocation throughput,
> > so I should measure it...
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
> To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
>
> On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> >
> > For simplicity (and since right now I seem to be too stupid to see
> > the bug), let's just grab the right page with a for_each_sg loop.
> >
> > This is exercised by the improved hangman tests and the gem_exec_big
> > test in i-g-t.
> >
> > v2: Compared to v1, don't try to be clever since I seemingly only
> > manage to prove that I'm not clever.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Looks like my worries are baseless. It can always be attacked latter if
> need be. I'd still like to know what the mistake was...
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Merged to -fixes, with the missing regression-sha1 citation added.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-10 9:21 ` Daniel Vetter
@ 2012-10-10 10:36 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2012-10-10 10:36 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development
On Wed, 10 Oct 2012 11:21:44 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote:
> > On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > > >
> > > > For simplicity (and since right now I seem to be too stupid to see
> > > > the bug), let's just grab the right page with a for_each_sg loop.
> > > >
> > > > This is exercised by the improved hangman tests and the gem_exec_big
> > > > test in i-g-t.
> > > >
> > > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > > manage to prove that I'm not clever.
> > >
> > > Only I expect that loop to show up on profiles even higher than the
> > > sg_next() from pwrite. :|
> > >
> > > I expect it to have a measureable impact upon relocation throughput,
> > > so I should measure it...
> > > -Chris
> > >
> > > --
> > > Chris Wilson, Intel Open Source Technology Centre
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
> > To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> > References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> >
> > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > >
> > > For simplicity (and since right now I seem to be too stupid to see
> > > the bug), let's just grab the right page with a for_each_sg loop.
> > >
> > > This is exercised by the improved hangman tests and the gem_exec_big
> > > test in i-g-t.
> > >
> > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > manage to prove that I'm not clever.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Looks like my worries are baseless. It can always be attacked latter if
> > need be. I'd still like to know what the mistake was...
> >
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Merged to -fixes, with the missing regression-sha1 citation added.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
In-Reply-To: <20121010091538.GB5533@phenom.ffwll.local>
References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch> <6c3329$6m595h@orsmga002.jf.intel.com> <453bf0$61a4mf@azsmga001.ch.intel.com> <20121010091538.GB5533@phenom.ffwll.local>
On Wed, 10 Oct 2012 11:15:38 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote:
> > On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > > >
> > > > For simplicity (and since right now I seem to be too stupid to see
> > > > the bug), let's just grab the right page with a for_each_sg loop.
> > > >
> > > > This is exercised by the improved hangman tests and the gem_exec_big
> > > > test in i-g-t.
> > > >
> > > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > > manage to prove that I'm not clever.
> > >
> > > Only I expect that loop to show up on profiles even higher than the
> > > sg_next() from pwrite. :|
> > >
> > > I expect it to have a measureable impact upon relocation throughput,
> > > so I should measure it...
> > > -Chris
> > >
> > > --
> > > Chris Wilson, Intel Open Source Technology Centre
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
> > To: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> > References: <84c8a8$624sk4@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vetter@ffwll.ch>
> >
> > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > The obj->pages to obj->pages->sgl rework introduced this helper, but
> > > it doesn't actually work for n >= SG_MAX_SINGLE_ALLOC.
> > >
> > > For simplicity (and since right now I seem to be too stupid to see
> > > the bug), let's just grab the right page with a for_each_sg loop.
> > >
> > > This is exercised by the improved hangman tests and the gem_exec_big
> > > test in i-g-t.
> > >
> > > v2: Compared to v1, don't try to be clever since I seemingly only
> > > manage to prove that I'm not clever.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Looks like my worries are baseless. It can always be attacked latter if
> > need be. I'd still like to know what the mistake was...
>
> I think it was two mistakes:
> - One was the off-by-one fixed in v1.
> - Second seemed to be the special case that if the table fits exactly,
> sg_alloc doesn't set a chain ptr with another sg table with just one
> entry.
That oddity is the reason why the loop was structured so.
The mistake is that just because we have n == MAX, does not mean that
there are only n elements left in the sg! *hides*
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index d2dda78..e6707e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1383,9 +1383,14 @@ int __must_check i915_gem_object_get_pages(struct drm_i91
static inline struct page *i915_gem_object_get_page(struct drm_i915_gem_object
{
struct scatterlist *sg = obj->pages->sgl;
- while (n >= SG_MAX_SINGLE_ALLOC) {
+ int nents = obj->pages->orig;
+ while (nents > SG_MAX_SINGLE_ALLOC) {
+ if (n < SG_MAX_SINGLE_ALLOC - 1)
+ break;
+
sg = sg_chain_ptr(sg + SG_MAX_SINGLE_ALLOC - 1);
n -= SG_MAX_SINGLE_ALLOC - 1;
+ nents -= SG_MAX_SINGLE_ALLOC - 1;
}
return sg_page(sg+n);
}
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
2012-10-10 11:11 Chris Wilson
@ 2012-10-10 11:38 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-10-10 11:38 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, Oct 10, 2012 at 12:11:52PM +0100, Chris Wilson wrote:
> Note that just because we have n == MAX elements left, does not imply
> that there are only MAX elements left in the scatterlist and so we may
> not be on the last chain, and the nth element may in fact be a chain ptr.
>
> This is exercised by the improved hangman tests and the gem_exec_big
> test in i-g-t.
>
> This regression has been introduced in
>
> commit 9da3da660d8c19a54f6e93361d147509be3fff84
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Fri Jun 1 15:20:22 2012 +0100
>
> drm/i915: Replace the array of pages with a scatterlist
>
> v2: KISS, replace the direct lookup with a for_each_sg() [danvet]
> v3: Try to be clever again.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
I've merged this one and dropped v2 from -fixes, thanks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
@ 2012-10-10 11:11 Chris Wilson
2012-10-10 11:38 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-10-10 11:11 UTC (permalink / raw)
To: intel-gfx
Note that just because we have n == MAX elements left, does not imply
that there are only MAX elements left in the scatterlist and so we may
not be on the last chain, and the nth element may in fact be a chain ptr.
This is exercised by the improved hangman tests and the gem_exec_big
test in i-g-t.
This regression has been introduced in
commit 9da3da660d8c19a54f6e93361d147509be3fff84
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Fri Jun 1 15:20:22 2012 +0100
drm/i915: Replace the array of pages with a scatterlist
v2: KISS, replace the direct lookup with a for_each_sg() [danvet]
v3: Try to be clever again.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2dda78..babe43d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1383,9 +1383,14 @@ int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
static inline struct page *i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
{
struct scatterlist *sg = obj->pages->sgl;
- while (n >= SG_MAX_SINGLE_ALLOC) {
+ int nents = obj->pages->nents;
+ while (nents > SG_MAX_SINGLE_ALLOC) {
+ if (n < SG_MAX_SINGLE_ALLOC - 1)
+ break;
+
sg = sg_chain_ptr(sg + SG_MAX_SINGLE_ALLOC - 1);
n -= SG_MAX_SINGLE_ALLOC - 1;
+ nents -= SG_MAX_SINGLE_ALLOC - 1;
}
return sg_page(sg+n);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-10-10 11:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-09 17:59 [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper Daniel Vetter
2012-10-09 20:41 ` Chris Wilson
2012-10-09 20:50 ` Daniel Vetter
2012-10-09 22:16 ` Chris Wilson
2012-10-10 9:01 ` Chris Wilson
2012-10-10 9:15 ` Daniel Vetter
2012-10-10 9:21 ` Daniel Vetter
2012-10-10 10:36 ` Chris Wilson
2012-10-10 11:11 Chris Wilson
2012-10-10 11:38 ` Daniel Vetter
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.