[Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
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
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
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
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
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
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
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Tue, 9 Oct 2012 19:59:02 +0200, Daniel Vetter 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 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, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter 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
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson wrote: > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > 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 Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper To: Daniel Vetter , Intel Graphics Development Cc: Daniel Vetter In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> References: <84c8a8$624...@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter 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 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, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote: > On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson > wrote: > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > > 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 > Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper > To: Daniel Vetter , Intel Graphics Development > > Cc: Daniel Vetter > In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > References: <84c8a8$624...@orsmga001.jf.intel.com> > <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > 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 > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Wed, Oct 10, 2012 at 10:01:47AM +0100, Chris Wilson wrote: > On Tue, 09 Oct 2012 23:16:02 +0100, Chris Wilson > wrote: > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > > 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 > Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper > To: Daniel Vetter , Intel Graphics Development > > Cc: Daniel Vetter > In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > References: <84c8a8$624...@orsmga001.jf.intel.com> > <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > 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 > > 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 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
On Wed, 10 Oct 2012 11:21:44 +0200, Daniel Vetter 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 > > wrote: > > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > > > 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 > > Subject: Re: [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper > > To: Daniel Vetter , Intel Graphics Development > > > > Cc: Daniel Vetter > > In-Reply-To: <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > > References: <84c8a8$624...@orsmga001.jf.intel.com> > > <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> > > > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > > 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 > > > > 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 > > 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 Subject: Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper To: Daniel Vetter Cc: Daniel Vetter , Intel Graphics Development In-Reply-To: <20121010091538.GB5533@phenom.ffwll.local> References: <84c8a8$624...@orsmga001.jf.intel.com> <1349815848-1824-1-git-send-email-daniel.vet...@ffwll.ch> <6c3329$6m5...@orsmga002.jf.intel.com> <453bf0$61a...@azsmga001.ch.intel.com> <20121010091538.GB5533@phenom.ffwll.local> On Wed, 10 Oct 2012 11:15:38 +0200, Daniel Vetter 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 > > wrote: > > > On Tue, 9 Oct 2012 22:50:48 +0200, Daniel Vetter > > > 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
Re: [Intel-gfx] [PATCH] drm/i915: fixup i915_gem_object_get_page inline helper
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 > 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 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
