On Mon, Jan 09, 2017 at 12:23:37PM +0000, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 20/12/2016 13:42, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> >Rather than freeing and re-allocating the pages when DMA mapping
> >in large chunks fails, we can just rebuild the sg table with no
> >coalescing.
> >
> >Also change back the page counter to unsigned int because that
> >is what the sg API supports.
> >
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >---
> >Only compile tested!
> >---
> > drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 30 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 5275f6248ce3..e73f9f5a5d23 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2340,12 +2340,36 @@ static void i915_sg_trim(struct sg_table *orig_st)
> > *orig_st = new_st;
> > }
> >
> >+static void i915_sg_uncoalesce(struct sg_table *orig_st, unsigned long nents)
> >+{
> >+ struct sg_table new_st;
> >+ struct scatterlist *new_sg;
> >+ struct sgt_iter sgt_iter;
> >+ struct page *page;
> >+
> >+ if (sg_alloc_table(&new_st, nents, GFP_KERNEL))
> >+ return;
> >+
> >+ new_sg = new_st.sgl;
> >+ for_each_sgt_page(page, sgt_iter, orig_st) {
> >+ sg_set_page(new_sg, page, PAGE_SIZE, 0);
> >+ /* called before being DMA mapped, no need to copy sg->dma_* */
> >+ new_sg = sg_next(new_sg);
> >+ }
> >+
> >+ GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
> >+
> >+ sg_free_table(orig_st);
> >+
> >+ *orig_st = new_st;
> >+}
> >+
> > static struct sg_table *
> > i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > {
> > struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >- const unsigned long page_count = obj->base.size / PAGE_SIZE;
> >- unsigned long i;
> >+ const unsigned int page_count = obj->base.size / PAGE_SIZE;
> >+ unsigned int i;
> > struct address_space *mapping;
> > struct sg_table *st;
> > struct scatterlist *sg;
> >@@ -2371,7 +2395,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > if (st == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> >-rebuild_st:
> > if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> > kfree(st);
> > return ERR_PTR(-ENOMEM);
> >@@ -2429,6 +2452,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > /* Trim unused sg entries to avoid wasting memory. */
> > i915_sg_trim(st);
> >
> >+prepare_gtt:
> > ret = i915_gem_gtt_prepare_pages(obj, st);
> > if (ret) {
> > /* DMA remapping failed? One possible cause is that
> >@@ -2436,16 +2460,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > * for PAGE_SIZE chunks instead may be helpful.
> > */
> > if (max_segment > PAGE_SIZE) {
> >- for_each_sgt_page(page, sgt_iter, st)
> >- put_page(page);
> >- sg_free_table(st);
> >-
> >+ i915_sg_uncoalesce(st, page_count);
> > max_segment = PAGE_SIZE;
> >- goto rebuild_st;
> >+ goto prepare_gtt;
> > } else {
> > dev_warn(&dev_priv->drm.pdev->dev,
> >- "Failed to DMA remap %lu pages\n",
> >- page_count);
> >+ "Failed to DMA remap %u pages\n", page_count);
> > goto err_pages;
> > }
> > }
> >
>
> Are you still against this? As a reminder it saves a
> put_page/allocate page-from-shmemfs cycle on dma mapping failures.
I still regard it as incomplete. Why bother saving that trip and not the
actual page allocation itself.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
