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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 09, 2017 at 12:40:07PM +0000, Tvrtko Ursulin wrote:
> 
> On 09/01/2017 12:28, Chris Wilson wrote:
> >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.
> 
> Page allocation is exactly what it avoids re-doing!?

It is not. The shmemfs pages are not being freed nor reallocated, just
their use count being manipulated.
 
> You meant sg_alloc_table maybe? We could move the trim to be last
> and do the uncoalesce in place but it would be so much more complex
> that I am not sure it is worth it for this edge case.

Only the sg_table is being reallocated and doing actual kfree/kmalloc.
 
> The patch above was fast and simple improvement to the dma remapping
> retry you added.

I didn't value it as being a significant improvement and thought you
have a more complete solution in mind.

For starters we can reduce the duplication here with

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ed3cd1a5f9bb..1bfec811f9b7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2199,20 +2199,12 @@ void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
        invalidate_mapping_pages(mapping, 0, (loff_t)-1);
 }
 
-static void
-i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj,
-                             struct sg_table *pages)
+static void free_sg_pages(struct drm_i915_gem_object *obj,
+                         struct sg_table *pages)
 {
        struct sgt_iter sgt_iter;
        struct page *page;
 
-       __i915_gem_object_release_shmem(obj, pages, true);
-
-       i915_gem_gtt_finish_pages(obj, pages);
-
-       if (i915_gem_object_needs_bit17_swizzle(obj))
-               i915_gem_object_save_bit_17_swizzle(obj, pages);
-
        for_each_sgt_page(page, sgt_iter, pages) {
                if (obj->mm.dirty)
                        set_page_dirty(page);
@@ -2222,9 +2214,23 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj,
 
                put_page(page);
        }
+       sg_free_table(pages);
+
        obj->mm.dirty = false;
+}
 
-       sg_free_table(pages);
+static void
+i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj,
+                             struct sg_table *pages)
+{
+       __i915_gem_object_release_shmem(obj, pages, true);
+
+       i915_gem_gtt_finish_pages(obj, pages);
+
+       if (i915_gem_object_needs_bit17_swizzle(obj))
+               i915_gem_object_save_bit_17_swizzle(obj, pages);
+
+       free_sg_pages(obj, pages);
        kfree(pages);
 }
 
@@ -2322,7 +2328,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
        struct address_space *mapping;
        struct sg_table *st;
        struct scatterlist *sg;
-       struct sgt_iter sgt_iter;
        struct page *page;
        unsigned long last_pfn = 0;     /* suppress gcc warning */
        unsigned int max_segment;
@@ -2409,10 +2414,7 @@ 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);
-
+                       free_sg_pages(obj, st);
                        max_segment = PAGE_SIZE;
                        goto rebuild_st;
                } else {
@@ -2431,9 +2433,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 err_sg:
        sg_mark_end(sg);
 err_pages:
-       for_each_sgt_page(page, sgt_iter, st)
-               put_page(page);
-       sg_free_table(st);
+       free_sg_pages(obj, st);
        kfree(st);
 
        /* shmemfs first checks if there is enough memory to allocate the page


-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux