Re: [PATCH] drm/i915: Use the existing pages when retrying to DMA map

[Chris Wilson <chris@chris-wilson.co.uk>]

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@intel.com>
> >
> >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@intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx