[PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Chris Wilson]

If we at first do not succeed with attempting to remap our physical
pages using a coalesced scattergather list, try again with one
scattergather entry per page. This should help with swiotlb as it uses a
limited buffer size and only searches for contiguous chunks within its
buffer aligned up to the next boundary - i.e. we may prematurely cause a
failure as we are unable to utilize the unused space between large
chunks and trigger an error such as:

	 i915 0000:00:02.0: swiotlb buffer is full (sz: 1630208 bytes)

Reported-by: Juergen Gross <jgross@suse.com>
Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: <drm-intel-fixes@lists.freedesktop.org>
---
 drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 412f3513f269..4e263df2afc3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2326,7 +2326,8 @@ 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);
-	int page_count, i;
+	const unsigned long page_count = obj->base.size / PAGE_SIZE;
+	unsigned long i;
 	struct address_space *mapping;
 	struct sg_table *st;
 	struct scatterlist *sg;
@@ -2352,7 +2353,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	if (st == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	page_count = obj->base.size / PAGE_SIZE;
+rebuild_st:
 	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
 		kfree(st);
 		return ERR_PTR(-ENOMEM);
@@ -2411,8 +2412,25 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	i915_sg_trim(st);
 
 	ret = i915_gem_gtt_prepare_pages(obj, st);
-	if (ret)
-		goto err_pages;
+	if (ret) {
+		/* DMA remapping failed? One possible cause is that
+		 * it could not reserve enough large entries, asking
+		 * 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);
+
+			max_segment = PAGE_SIZE;
+			goto rebuild_st;
+		} else {
+			dev_warn(&dev_priv->drm.pdev->dev,
+				 "Failed to DMA remap %lu pages\n",
+				 page_count);
+			goto err_pages;
+		}
+	}
 
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_do_bit_17_swizzle(obj, st);
-- 
2.11.0

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

[PATCH 2/2] drm/i915: Add a test that we terminate the trimmed sgtable as expected

[Chris Wilson]

In commit 0c40ce130e38 ("drm/i915: Trim the object sg table"), we expect
to copy exactly orig_st->nents across and allocate the table thusly.
The copy loop should therefore end with the new_sg being NULL.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4e263df2afc3..0a82cce5f731 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2316,6 +2316,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
 		/* 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);
 
-- 
2.11.0

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

Re: [PATCH 2/2] drm/i915: Add a test that we terminate the trimmed sgtable as expected

[Daniel Vetter]

On Mon, Dec 19, 2016 at 12:43:46PM +0000, Chris Wilson wrote:
> In commit 0c40ce130e38 ("drm/i915: Trim the object sg table"), we expect
> to copy exactly orig_st->nents across and allocate the table thusly.
> The copy loop should therefore end with the new_sg being NULL.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4e263df2afc3..0a82cce5f731 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2316,6 +2316,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
>  		/* 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 */

I wasn't sure this holds up vs. overallocation of sg tables, but the last
entry is marked and that mark seems to survive intact everywhere.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  	sg_free_table(orig_st);
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add a test that we terminate the trimmed sgtable as expected

[Tvrtko Ursulin]

On 19/12/2016 12:43, Chris Wilson wrote:
> In commit 0c40ce130e38 ("drm/i915: Trim the object sg table"), we expect
> to copy exactly orig_st->nents across and allocate the table thusly.
> The copy loop should therefore end with the new_sg being NULL.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4e263df2afc3..0a82cce5f731 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2316,6 +2316,7 @@ static void i915_sg_trim(struct sg_table *orig_st)
>  		/* 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);
>

I was sure I had this in the code back then, hm.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping
URL   : https://patchwork.freedesktop.org/series/16996/
State : success

== Summary ==

Series 16996v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16996/revisions/1/mbox/


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:247  pass:225  dwarn:1   dfail:0   fail:0   skip:21 
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 

cda2d70a4395323bcf064c81ee0f89d2de015544 drm-tip: 2016y-12m-19d-13h-00m-10s UTC integration manifest
441ca15 drm/i915: Add a test that we terminate the trimmed sgtable as expected
db8fb5f drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

== Logs ==

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

Re: [PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Daniel Vetter]

On Mon, Dec 19, 2016 at 12:43:45PM +0000, Chris Wilson wrote:
> If we at first do not succeed with attempting to remap our physical
> pages using a coalesced scattergather list, try again with one
> scattergather entry per page. This should help with swiotlb as it uses a
> limited buffer size and only searches for contiguous chunks within its
> buffer aligned up to the next boundary - i.e. we may prematurely cause a
> failure as we are unable to utilize the unused space between large
> chunks and trigger an error such as:
> 
> 	 i915 0000:00:02.0: swiotlb buffer is full (sz: 1630208 bytes)
> 
> Reported-by: Juergen Gross <jgross@suse.com>
> Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: <drm-intel-fixes@lists.freedesktop.org>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Feels a bit funny to call swiotlb_* functions, I'd kinda assume that we
could somehow figure this out from the dma limits instead of leaking
through the dma api abstraction. But that's already there, so meh.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 412f3513f269..4e263df2afc3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2326,7 +2326,8 @@ 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);
> -	int page_count, i;
> +	const unsigned long page_count = obj->base.size / PAGE_SIZE;
> +	unsigned long i;
>  	struct address_space *mapping;
>  	struct sg_table *st;
>  	struct scatterlist *sg;
> @@ -2352,7 +2353,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	if (st == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> -	page_count = obj->base.size / PAGE_SIZE;
> +rebuild_st:
>  	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
>  		kfree(st);
>  		return ERR_PTR(-ENOMEM);
> @@ -2411,8 +2412,25 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	i915_sg_trim(st);
>  
>  	ret = i915_gem_gtt_prepare_pages(obj, st);
> -	if (ret)
> -		goto err_pages;
> +	if (ret) {
> +		/* DMA remapping failed? One possible cause is that
> +		 * it could not reserve enough large entries, asking
> +		 * 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);
> +
> +			max_segment = PAGE_SIZE;
> +			goto rebuild_st;
> +		} else {
> +			dev_warn(&dev_priv->drm.pdev->dev,
> +				 "Failed to DMA remap %lu pages\n",
> +				 page_count);
> +			goto err_pages;
> +		}
> +	}
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
>  		i915_gem_object_do_bit_17_swizzle(obj, st);
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Tvrtko Ursulin]

On 19/12/2016 12:43, Chris Wilson wrote:
> If we at first do not succeed with attempting to remap our physical
> pages using a coalesced scattergather list, try again with one
> scattergather entry per page. This should help with swiotlb as it uses a
> limited buffer size and only searches for contiguous chunks within its
> buffer aligned up to the next boundary - i.e. we may prematurely cause a
> failure as we are unable to utilize the unused space between large
> chunks and trigger an error such as:
>
> 	 i915 0000:00:02.0: swiotlb buffer is full (sz: 1630208 bytes)
>
> Reported-by: Juergen Gross <jgross@suse.com>
> Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: <drm-intel-fixes@lists.freedesktop.org>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 412f3513f269..4e263df2afc3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2326,7 +2326,8 @@ 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);
> -	int page_count, i;
> +	const unsigned long page_count = obj->base.size / PAGE_SIZE;
> +	unsigned long i;
>  	struct address_space *mapping;
>  	struct sg_table *st;
>  	struct scatterlist *sg;
> @@ -2352,7 +2353,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	if (st == NULL)
>  		return ERR_PTR(-ENOMEM);
>
> -	page_count = obj->base.size / PAGE_SIZE;
> +rebuild_st:
>  	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
>  		kfree(st);
>  		return ERR_PTR(-ENOMEM);
> @@ -2411,8 +2412,25 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	i915_sg_trim(st);
>
>  	ret = i915_gem_gtt_prepare_pages(obj, st);
> -	if (ret)
> -		goto err_pages;
> +	if (ret) {
> +		/* DMA remapping failed? One possible cause is that
> +		 * it could not reserve enough large entries, asking
> +		 * 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);
> +
> +			max_segment = PAGE_SIZE;
> +			goto rebuild_st;
> +		} else {
> +			dev_warn(&dev_priv->drm.pdev->dev,
> +				 "Failed to DMA remap %lu pages\n",
> +				 page_count);
> +			goto err_pages;
> +		}
> +	}
>
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
>  		i915_gem_object_do_bit_17_swizzle(obj, st);
>

How much is the cost of freeing and re-acquiring pages in the fall back 
case? It could be avoidable by using the table and adding something like 
sgt = i915_sg_copy(sgt, table_max_segment). But it depends on how likely 
is this path to be hit on swiotlb platforms. I have no idea. Our 
datasets are much bigger than the swiotlb space - if that is true on 
such platforms?

Regards,

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

Re: [PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Chris Wilson]

On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
> 
> On 19/12/2016 12:43, Chris Wilson wrote:
> >If we at first do not succeed with attempting to remap our physical
> >pages using a coalesced scattergather list, try again with one
> >scattergather entry per page. This should help with swiotlb as it uses a
> >limited buffer size and only searches for contiguous chunks within its
> >buffer aligned up to the next boundary - i.e. we may prematurely cause a
> >failure as we are unable to utilize the unused space between large
> >chunks and trigger an error such as:
> >
> >	 i915 0000:00:02.0: swiotlb buffer is full (sz: 1630208 bytes)
> >
> >Reported-by: Juergen Gross <jgross@suse.com>
> >Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Imre Deak <imre.deak@intel.com>
> >Cc: <drm-intel-fixes@lists.freedesktop.org>
> >---
> > drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 412f3513f269..4e263df2afc3 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2326,7 +2326,8 @@ 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);
> >-	int page_count, i;
> >+	const unsigned long page_count = obj->base.size / PAGE_SIZE;
> >+	unsigned long i;
> > 	struct address_space *mapping;
> > 	struct sg_table *st;
> > 	struct scatterlist *sg;
> >@@ -2352,7 +2353,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > 	if (st == NULL)
> > 		return ERR_PTR(-ENOMEM);
> >
> >-	page_count = obj->base.size / PAGE_SIZE;
> >+rebuild_st:
> > 	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> > 		kfree(st);
> > 		return ERR_PTR(-ENOMEM);
> >@@ -2411,8 +2412,25 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > 	i915_sg_trim(st);
> >
> > 	ret = i915_gem_gtt_prepare_pages(obj, st);
> >-	if (ret)
> >-		goto err_pages;
> >+	if (ret) {
> >+		/* DMA remapping failed? One possible cause is that
> >+		 * it could not reserve enough large entries, asking
> >+		 * 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);
> >+
> >+			max_segment = PAGE_SIZE;
> >+			goto rebuild_st;
> >+		} else {
> >+			dev_warn(&dev_priv->drm.pdev->dev,
> >+				 "Failed to DMA remap %lu pages\n",
> >+				 page_count);
> >+			goto err_pages;
> >+		}
> >+	}
> >
> > 	if (i915_gem_object_needs_bit17_swizzle(obj))
> > 		i915_gem_object_do_bit_17_swizzle(obj, st);
> >
> 
> How much is the cost of freeing and re-acquiring pages in the fall
> back case? It could be avoidable by using the table and adding
> something like sgt = i915_sg_copy(sgt, table_max_segment). But it
> depends on how likely is this path to be hit on swiotlb platforms. I
> have no idea. Our datasets are much bigger than the swiotlb space -
> if that is true on such platforms?

It's below my level of care (atm). Platforms hitting this are using
swiotlb *bounce* buffers. They will not be able to support a full gfx
workload and be going through a copy. We could avoid the additional
work, the sg_table is large enough for a 1:1 copy if we do it before the
trim, but more importantly we need a simple fix for 4.10.
-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 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Chris Wilson]

On Tue, Dec 20, 2016 at 11:33:27AM +0000, Chris Wilson wrote:
> On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
> > How much is the cost of freeing and re-acquiring pages in the fall
> > back case? It could be avoidable by using the table and adding
> > something like sgt = i915_sg_copy(sgt, table_max_segment). But it
> > depends on how likely is this path to be hit on swiotlb platforms. I
> > have no idea. Our datasets are much bigger than the swiotlb space -
> > if that is true on such platforms?
> 
> It's below my level of care (atm). Platforms hitting this are using
> swiotlb *bounce* buffers. They will not be able to support a full gfx
> workload and be going through a copy. We could avoid the additional
> work, the sg_table is large enough for a 1:1 copy if we do it before the
> trim, but more importantly we need a simple fix for 4.10.

Pushed this pair as I think this is the safe course of action. Creating
i915_sg_expand() is a job for a rainy day.
-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 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Tvrtko Ursulin]

On 20/12/2016 12:36, Chris Wilson wrote:
> On Tue, Dec 20, 2016 at 11:33:27AM +0000, Chris Wilson wrote:
>> On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
>>> How much is the cost of freeing and re-acquiring pages in the fall
>>> back case? It could be avoidable by using the table and adding
>>> something like sgt = i915_sg_copy(sgt, table_max_segment). But it
>>> depends on how likely is this path to be hit on swiotlb platforms. I
>>> have no idea. Our datasets are much bigger than the swiotlb space -
>>> if that is true on such platforms?
>>
>> It's below my level of care (atm). Platforms hitting this are using
>> swiotlb *bounce* buffers. They will not be able to support a full gfx
>> workload and be going through a copy. We could avoid the additional
>> work, the sg_table is large enough for a 1:1 copy if we do it before the
>> trim, but more importantly we need a simple fix for 4.10.
>
> Pushed this pair as I think this is the safe course of action. Creating
> i915_sg_expand() is a job for a rainy day.

It would have been very simple and much more elegant in my opinion. But 
I understand Tested-by tag was precious to keep. I'll send a patch 
shortly but it won't be very tested due to time constraints.

Also I don't know why you changed page_count and i to unsigned long when 
the sg API can only handle unsigned int for that.

Regards,

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

Re: [PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Chris Wilson]

On Tue, Dec 20, 2016 at 01:38:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 20/12/2016 12:36, Chris Wilson wrote:
> >On Tue, Dec 20, 2016 at 11:33:27AM +0000, Chris Wilson wrote:
> >>On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
> >>>How much is the cost of freeing and re-acquiring pages in the fall
> >>>back case? It could be avoidable by using the table and adding
> >>>something like sgt = i915_sg_copy(sgt, table_max_segment). But it
> >>>depends on how likely is this path to be hit on swiotlb platforms. I
> >>>have no idea. Our datasets are much bigger than the swiotlb space -
> >>>if that is true on such platforms?
> >>
> >>It's below my level of care (atm). Platforms hitting this are using
> >>swiotlb *bounce* buffers. They will not be able to support a full gfx
> >>workload and be going through a copy. We could avoid the additional
> >>work, the sg_table is large enough for a 1:1 copy if we do it before the
> >>trim, but more importantly we need a simple fix for 4.10.
> >
> >Pushed this pair as I think this is the safe course of action. Creating
> >i915_sg_expand() is a job for a rainy day.
> 
> It would have been very simple and much more elegant in my opinion.

I'm ready to be impressed, in my head to do an inplace rewrite was tricky.
:)

> But I understand Tested-by tag was precious to keep. I'll send a
> patch shortly but it won't be very tested due to time constraints.
> 
> Also I don't know why you changed page_count and i to unsigned long
> when the sg API can only handle unsigned int for that.

Primary concern was moving them out of the way and worrying about our
own 64bit object size issues. Hmm, can we reuse

if (overflows_type(pgcount, unsigned int))
	return -E2BIG;

to catch the mismatch?
-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 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Tvrtko Ursulin]

On 20/12/2016 13:56, Chris Wilson wrote:
> On Tue, Dec 20, 2016 at 01:38:16PM +0000, Tvrtko Ursulin wrote:
>>
>> On 20/12/2016 12:36, Chris Wilson wrote:
>>> On Tue, Dec 20, 2016 at 11:33:27AM +0000, Chris Wilson wrote:
>>>> On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
>>>>> How much is the cost of freeing and re-acquiring pages in the fall
>>>>> back case? It could be avoidable by using the table and adding
>>>>> something like sgt = i915_sg_copy(sgt, table_max_segment). But it
>>>>> depends on how likely is this path to be hit on swiotlb platforms. I
>>>>> have no idea. Our datasets are much bigger than the swiotlb space -
>>>>> if that is true on such platforms?
>>>>
>>>> It's below my level of care (atm). Platforms hitting this are using
>>>> swiotlb *bounce* buffers. They will not be able to support a full gfx
>>>> workload and be going through a copy. We could avoid the additional
>>>> work, the sg_table is large enough for a 1:1 copy if we do it before the
>>>> trim, but more importantly we need a simple fix for 4.10.
>>>
>>> Pushed this pair as I think this is the safe course of action. Creating
>>> i915_sg_expand() is a job for a rainy day.
>>
>> It would have been very simple and much more elegant in my opinion.
>
> I'm ready to be impressed, in my head to do an inplace rewrite was tricky.
> :)

Maybe I've missed something then. We'll see. :)

>> But I understand Tested-by tag was precious to keep. I'll send a
>> patch shortly but it won't be very tested due to time constraints.
>>
>> Also I don't know why you changed page_count and i to unsigned long
>> when the sg API can only handle unsigned int for that.
>
> Primary concern was moving them out of the way and worrying about our
> own 64bit object size issues. Hmm, can we reuse
>
> if (overflows_type(pgcount, unsigned int))
> 	return -E2BIG;
>
> to catch the mismatch?

You have already added that to i915_gem_object_create some time ago! :)

Regards,

Tvrtko

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

Re: [PATCH 1/2] drm/i915: Fallback to single PAGE_SIZE segments for DMA remapping

[Chris Wilson]

On Tue, Dec 20, 2016 at 02:14:21PM +0000, Tvrtko Ursulin wrote:
> 
> On 20/12/2016 13:56, Chris Wilson wrote:
> >On Tue, Dec 20, 2016 at 01:38:16PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 20/12/2016 12:36, Chris Wilson wrote:
> >>>On Tue, Dec 20, 2016 at 11:33:27AM +0000, Chris Wilson wrote:
> >>>>On Tue, Dec 20, 2016 at 11:13:43AM +0000, Tvrtko Ursulin wrote:
> >>>>>How much is the cost of freeing and re-acquiring pages in the fall
> >>>>>back case? It could be avoidable by using the table and adding
> >>>>>something like sgt = i915_sg_copy(sgt, table_max_segment). But it
> >>>>>depends on how likely is this path to be hit on swiotlb platforms. I
> >>>>>have no idea. Our datasets are much bigger than the swiotlb space -
> >>>>>if that is true on such platforms?
> >>>>
> >>>>It's below my level of care (atm). Platforms hitting this are using
> >>>>swiotlb *bounce* buffers. They will not be able to support a full gfx
> >>>>workload and be going through a copy. We could avoid the additional
> >>>>work, the sg_table is large enough for a 1:1 copy if we do it before the
> >>>>trim, but more importantly we need a simple fix for 4.10.
> >>>
> >>>Pushed this pair as I think this is the safe course of action. Creating
> >>>i915_sg_expand() is a job for a rainy day.
> >>
> >>It would have been very simple and much more elegant in my opinion.
> >
> >I'm ready to be impressed, in my head to do an inplace rewrite was tricky.
> >:)
> 
> Maybe I've missed something then. We'll see. :)
> 
> >>But I understand Tested-by tag was precious to keep. I'll send a
> >>patch shortly but it won't be very tested due to time constraints.
> >>
> >>Also I don't know why you changed page_count and i to unsigned long
> >>when the sg API can only handle unsigned int for that.
> >
> >Primary concern was moving them out of the way and worrying about our
> >own 64bit object size issues. Hmm, can we reuse
> >
> >if (overflows_type(pgcount, unsigned int))
> >	return -E2BIG;
> >
> >to catch the mismatch?
> 
> You have already added that to i915_gem_object_create some time ago! :)

I know, I'm thinking ahead of documenting the types around the place so
that we can start actually preparing for huge objects.
-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