| Message ID | 1360423656-10816-5-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 09, 2013 at 05:27:36PM +0200, Imre Deak wrote: > So far we created a sparse dma scatter list for gem objects, where each > scatter list entry represented only a single page. In the future we'll > have to handle compact scatter lists too where each entry can consist of > multiple pages, for example for objects imported through PRIME. > > The previous patches have already fixed up all other places where the > i915 driver _walked_ these lists. Here we have the corresponding fix to > _create_ compact lists. It's not a performance or memory footprint > improvement, but it helps to better exercise the new logic. > > Reference: http://www.spinics.net/lists/dri-devel/msg33917.html > Signed-off-by: Imre Deak <imre.deak@intel.com> Just a quick question: Have you checked with printks or so that we indeed create such coalesced sg entries every once in a while? -Daniel > --- > drivers/gpu/drm/i915/i915_gem.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 4a199e0..1028dd5 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1623,9 +1623,8 @@ i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj) > static void > i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) > { > - int page_count = obj->base.size / PAGE_SIZE; > - struct scatterlist *sg; > - int ret, i; > + struct drm_sg_iter sg_iter; > + int ret; > > BUG_ON(obj->madv == __I915_MADV_PURGED); > > @@ -1645,8 +1644,8 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) > if (obj->madv == I915_MADV_DONTNEED) > obj->dirty = 0; > > - for_each_sg(obj->pages->sgl, sg, page_count, i) { > - struct page *page = sg_page(sg); > + drm_for_each_sg_page(&sg_iter, obj->pages->sgl, 0) { > + struct page *page = sg_iter.page; > > if (obj->dirty) > set_page_dirty(page); > @@ -1740,7 +1739,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) > struct address_space *mapping; > struct sg_table *st; > struct scatterlist *sg; > + struct drm_sg_iter sg_iter; > struct page *page; > + unsigned long last_pfn = 0; /* suppress gcc warning */ > gfp_t gfp; > > /* Assert that the object is not currently in any GPU domain. As it > @@ -1770,7 +1771,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) > gfp = mapping_gfp_mask(mapping); > gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD; > gfp &= ~(__GFP_IO | __GFP_WAIT); > - for_each_sg(st->sgl, sg, page_count, i) { > + sg = st->sgl; > + st->nents = 0; > + for (i = 0; i < page_count; i++) { > page = shmem_read_mapping_page_gfp(mapping, i, gfp); > if (IS_ERR(page)) { > i915_gem_purge(dev_priv, page_count); > @@ -1793,9 +1796,18 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) > gfp &= ~(__GFP_IO | __GFP_WAIT); > } > > - sg_set_page(sg, page, PAGE_SIZE, 0); > + if (!i || page_to_pfn(page) != last_pfn + 1) { > + if (i) > + sg = sg_next(sg); > + st->nents++; > + sg_set_page(sg, page, PAGE_SIZE, 0); > + } else { > + sg->length += PAGE_SIZE; > + } > + last_pfn = page_to_pfn(page); > } > > + sg_mark_end(sg); > obj->pages = st; > > if (i915_gem_object_needs_bit17_swizzle(obj)) > @@ -1804,8 +1816,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) > return 0; > > err_pages: > - for_each_sg(st->sgl, sg, i, page_count) > - page_cache_release(sg_page(sg)); > + sg_mark_end(sg); > + drm_for_each_sg_page(&sg_iter, st->sgl, 0) > + page_cache_release(sg_iter.page); > sg_free_table(st); > kfree(st); > return PTR_ERR(page); > -- > 1.7.10.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, 2013-02-09 at 19:59 +0100, Daniel Vetter wrote: > On Sat, Feb 09, 2013 at 05:27:36PM +0200, Imre Deak wrote: > > So far we created a sparse dma scatter list for gem objects, where each > > scatter list entry represented only a single page. In the future we'll > > have to handle compact scatter lists too where each entry can consist of > > multiple pages, for example for objects imported through PRIME. > > > > The previous patches have already fixed up all other places where the > > i915 driver _walked_ these lists. Here we have the corresponding fix to > > _create_ compact lists. It's not a performance or memory footprint > > improvement, but it helps to better exercise the new logic. > > > > Reference: http://www.spinics.net/lists/dri-devel/msg33917.html > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Just a quick question: Have you checked with printks or so that we indeed > create such coalesced sg entries every once in a while? Yes, quite often that was the case, so we at least are really testing the thing..
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 4a199e0..1028dd5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1623,9 +1623,8 @@ i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj) static void i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) { - int page_count = obj->base.size / PAGE_SIZE; - struct scatterlist *sg; - int ret, i; + struct drm_sg_iter sg_iter; + int ret; BUG_ON(obj->madv == __I915_MADV_PURGED); @@ -1645,8 +1644,8 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) if (obj->madv == I915_MADV_DONTNEED) obj->dirty = 0; - for_each_sg(obj->pages->sgl, sg, page_count, i) { - struct page *page = sg_page(sg); + drm_for_each_sg_page(&sg_iter, obj->pages->sgl, 0) { + struct page *page = sg_iter.page; if (obj->dirty) set_page_dirty(page); @@ -1740,7 +1739,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) struct address_space *mapping; struct sg_table *st; struct scatterlist *sg; + struct drm_sg_iter sg_iter; struct page *page; + unsigned long last_pfn = 0; /* suppress gcc warning */ gfp_t gfp; /* Assert that the object is not currently in any GPU domain. As it @@ -1770,7 +1771,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) gfp = mapping_gfp_mask(mapping); gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD; gfp &= ~(__GFP_IO | __GFP_WAIT); - for_each_sg(st->sgl, sg, page_count, i) { + sg = st->sgl; + st->nents = 0; + for (i = 0; i < page_count; i++) { page = shmem_read_mapping_page_gfp(mapping, i, gfp); if (IS_ERR(page)) { i915_gem_purge(dev_priv, page_count); @@ -1793,9 +1796,18 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) gfp &= ~(__GFP_IO | __GFP_WAIT); } - sg_set_page(sg, page, PAGE_SIZE, 0); + if (!i || page_to_pfn(page) != last_pfn + 1) { + if (i) + sg = sg_next(sg); + st->nents++; + sg_set_page(sg, page, PAGE_SIZE, 0); + } else { + sg->length += PAGE_SIZE; + } + last_pfn = page_to_pfn(page); } + sg_mark_end(sg); obj->pages = st; if (i915_gem_object_needs_bit17_swizzle(obj)) @@ -1804,8 +1816,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) return 0; err_pages: - for_each_sg(st->sgl, sg, i, page_count) - page_cache_release(sg_page(sg)); + sg_mark_end(sg); + drm_for_each_sg_page(&sg_iter, st->sgl, 0) + page_cache_release(sg_iter.page); sg_free_table(st); kfree(st); return PTR_ERR(page);
So far we created a sparse dma scatter list for gem objects, where each scatter list entry represented only a single page. In the future we'll have to handle compact scatter lists too where each entry can consist of multiple pages, for example for objects imported through PRIME. The previous patches have already fixed up all other places where the i915 driver _walked_ these lists. Here we have the corresponding fix to _create_ compact lists. It's not a performance or memory footprint improvement, but it helps to better exercise the new logic. Reference: http://www.spinics.net/lists/dri-devel/msg33917.html Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)