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

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

[Tvrtko Ursulin]

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;
 		}
 	}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Tue, Dec 20, 2016 at 01:42:47PM +0000, 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.

You are freeing and reallocating the pages - I thought you meant the
sg_table. The cost for recovering the shmemfs after allocation/swapin
already performed in the first pass should be a look up in the radix tree.
Not worrisome.
 
> Also change back the page counter to unsigned int because that
> is what the sg API supports.

Rather go the other way and start reporting an error when we overflow.
Being strict all page_counts should be u64. :|

> 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;

I was hoping for a way to use the pages already allocated. Feels
possible, just reversing the chain and walking backwards will be fun.

> +	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;
> +}

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

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

[Tvrtko Ursulin]

On 20/12/2016 14:14, Chris Wilson wrote:
> On Tue, Dec 20, 2016 at 01:42:47PM +0000, 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.
>
> You are freeing and reallocating the pages - I thought you meant the
> sg_table. The cost for recovering the shmemfs after allocation/swapin
> already performed in the first pass should be a look up in the radix tree.
> Not worrisome.

Maybe not, but looks a bit unsightly to me. This is simple enough and 
one re-allocation less. From both pages and sg entries, to just the sg 
entries.

>> Also change back the page counter to unsigned int because that
>> is what the sg API supports.
>
> Rather go the other way and start reporting an error when we overflow.
> Being strict all page_counts should be u64. :|

As I wrote in the other thread, that's already in 
i915_gem_object_create. Added there at the time when I spotted the 
sg_alloc_table unsigned int business.

>> 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;
>
> I was hoping for a way to use the pages already allocated. Feels
> possible, just reversing the chain and walking backwards will be fun.

For a rainy day as you said. :)

>
>> +	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;
>> +}
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Use the existing pages when retrying to DMA map

[Patchwork]

== Series Details ==

Series: drm/i915: Use the existing pages when retrying to DMA map
URL   : https://patchwork.freedesktop.org/series/17061/
State : failure

== Summary ==

Series 17061v1 drm/i915: Use the existing pages when retrying to DMA map
https://patchwork.freedesktop.org/api/1.0/series/17061/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (fi-skl-6260u)
Test kms_busy:
        Subgroup basic-flip-default-b:
                pass       -> INCOMPLETE (fi-bxt-j4205)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> PASS       (fi-skl-6260u)
        Subgroup basic-rte:
                fail       -> PASS       (fi-skl-6260u)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:170  pass:156  dwarn:0   dfail:0   fail:0   skip:13 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

7285204135ffcf095bea64196db6ef900d47e048 drm-tip: 2016y-12m-20d-14h-50m-18s UTC integration manifest
d4c3b77 drm/i915: Use the existing pages when retrying to DMA map

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3342/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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.


Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

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

[Tvrtko Ursulin]

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@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.

Page allocation is exactly what it avoids re-doing!?

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.

The patch above was fast and simple improvement to the dma remapping 
retry you added.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@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.
> 
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx