* [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
* [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
* 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
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 -- strict thread matches above, loose matches on Subject: below -- 2012-10-10 11:11 Chris Wilson 2012-10-10 11:38 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox