[PATCH] Bootup regression of v3.10-rc6 + SWIOTLB + Intel 4000.

[PATCH] Bootup regression of v3.10-rc6 + SWIOTLB + Intel 4000.

[Konrad Rzeszutek Wilk]

Hey Dave, Chris, Imre, 

Attached is a fix that makes v3.10-rc6 boot on Intel HD 4000 when SWIOTLB
bounce buffer is in usage. The SWIOTLB can only handle up to 512KB swath
of memory to create bounce buffers for and Imre's patch made it possible
to provide more than to the DMA API which caused it to fail with dma_map_sg.

Since this is rc7 time I did the less risky way of fixing it - by just
doing what said code did before 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
 ("drm/i915: create compact dma scatter lists for gem objects") was
introduced by using a check to see if SWIOTLB is enabled.

It is not the best fix but I figured the less risky.

 drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)


I think that a better approach (in v3.11?) would be to do some form of
retry mechanism: (not compile tested, not run at all):

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9d00dc..0f9079d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1110,8 +1110,12 @@ struct drm_i915_gem_object_ops {
 	 * will therefore most likely be called when the object itself is
 	 * being released or under memory pressure (where we attempt to
 	 * reap pages for the shrinker).
+	 *
+	 * max is the maximum size an sg entry can be. Usually it is
+	 * PAGE_SIZE but if the backend (IOMMU) can deal with larger
+	 * then a larger value might be used as well.
 	 */
-	int (*get_pages)(struct drm_i915_gem_object *);
+	int (*get_pages)(struct drm_i915_gem_object *, unsigned long max);
 	void (*put_pages)(struct drm_i915_gem_object *);
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7045f45..a29e7db 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1738,7 +1738,7 @@ i915_gem_shrink_all(struct drm_i915_private *dev_priv)
 }
 
 static int
-i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
+i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj, unsigned long max)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	int page_count, i;
@@ -1809,7 +1809,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			continue;
 		}
 #endif
-		if (!i || page_to_pfn(page) != last_pfn + 1) {
+		if ((!i || (page_to_pfn(page) != last_pfn + 1)) && (sg->length < max)) {
 			if (i)
 				sg = sg_next(sg);
 			st->nents++;
@@ -1847,7 +1847,7 @@ err_pages:
  * or as the object is itself released.
  */
 int
-i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
+i915_gem_object_get_pages(struct drm_i915_gem_object *obj, unsigned int max)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	const struct drm_i915_gem_object_ops *ops = obj->ops;
@@ -1863,7 +1863,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 
 	BUG_ON(obj->pages_pin_count);
 
-	ret = ops->get_pages(obj);
+	ret = ops->get_pages(obj, max);
 	if (ret)
 		return ret;
 
@@ -2942,7 +2942,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 	u32 size, fence_size, fence_alignment, unfenced_alignment;
 	bool mappable, fenceable;
 	int ret;
+	static unsigned int max_size = 4 * 1024 * 1024; /* 4MB */
 
+#ifdef CONFIG_SWIOTLB
+	if (swiotlb_nr_tbl())
+		max_size = PAGE_SIZE;
+#endif
 	fence_size = i915_gem_get_gtt_size(dev,
 					   obj->base.size,
 					   obj->tiling_mode);
@@ -2972,8 +2977,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		DRM_ERROR("Attempting to bind an object larger than the aperture\n");
 		return -E2BIG;
 	}
-
-	ret = i915_gem_object_get_pages(obj);
+ retry:
+	ret = i915_gem_object_get_pages(obj, max_size);
 	if (ret)
 		return ret;
 
@@ -3015,6 +3020,10 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 	if (ret) {
 		i915_gem_object_unpin_pages(obj);
 		drm_mm_put_block(node);
+		if (max_size > PAGE_SIZE) {
+			max_size >> 1;
+			goto retry;
+		}
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index dc53a52..8101387 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -230,7 +230,8 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 	return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags);
 }
 
-static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
+static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj,
+					   unsigned long max)
 {
 	struct sg_table *sg;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 130d1db..9077ea9 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -231,7 +231,8 @@ i915_pages_create_for_stolen(struct drm_device *dev,
 	return st;
 }
 
-static int i915_gem_object_get_pages_stolen(struct drm_i915_gem_object *obj)
+static int i915_gem_object_get_pages_stolen(struct drm_i915_gem_object *obj,
+					    unsigned long max)
 {
 	BUG();
 	return -EINVAL;

[PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Konrad Rzeszutek Wilk]

Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
("drm/i915: create compact dma scatter lists for gem objects") makes
certain assumptions about the under laying DMA API that are not always
correct.

On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
I see:

[drm:intel_pipe_set_base] *ERROR* pin & fence failed
[drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28

Bit of debugging traced it down to dma_map_sg failing (in
i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).

That unfortunately are sizes that the SWIOTLB is incapable of handling -
the maximum it can handle is a an entry of 512KB of virtual contiguous
memory for its bounce buffer. (See IO_TLB_SEGSIZE).

Previous to the above mention git commit the SG entries were of 4KB, and
the code introduced by above git commit squashed the CPU contiguous PFNs
in one big virtual address provided to DMA API.

This patch is a simple semi-revert - were we emulate the old behavior
if we detect that SWIOTLB is online. If it is not online then we continue
on with the new compact scatter gather mechanism.

An alternative solution would be for the the '.get_pages' and the
i915_gem_gtt_prepare_object to retry with smaller max gap of the
amount of PFNs that can be combined together - but with this issue
discovered during rc7 that might be too risky.

Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Imre Deak <imre.deak@intel.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
CC: David Airlie <airlied@linux.ie>
CC: <dri-devel@lists.freedesktop.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 970ad17..7045f45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
 			gfp &= ~(__GFP_IO | __GFP_WAIT);
 		}
-
+#ifdef CONFIG_SWIOTLB
+		if (swiotlb_nr_tbl()) {
+			st->nents++;
+			sg_set_page(sg, page, PAGE_SIZE, 0);
+			sg = sg_next(sg);
+			continue;
+		}
+#endif
 		if (!i || page_to_pfn(page) != last_pfn + 1) {
 			if (i)
 				sg = sg_next(sg);
@@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		}
 		last_pfn = page_to_pfn(page);
 	}
-
-	sg_mark_end(sg);
+#ifdef CONFIG_SWIOTLB
+	if (!swiotlb_nr_tbl())
+#endif
+		sg_mark_end(sg);
 	obj->pages = st;
 
 	if (i915_gem_object_needs_bit17_swizzle(obj))
-- 
1.8.1.4

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Daniel Vetter]

On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
> Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
> ("drm/i915: create compact dma scatter lists for gem objects") makes
> certain assumptions about the under laying DMA API that are not always
> correct.
> 
> On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
> I see:
> 
> [drm:intel_pipe_set_base] *ERROR* pin & fence failed
> [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
> 
> Bit of debugging traced it down to dma_map_sg failing (in
> i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
> 
> That unfortunately are sizes that the SWIOTLB is incapable of handling -
> the maximum it can handle is a an entry of 512KB of virtual contiguous
> memory for its bounce buffer. (See IO_TLB_SEGSIZE).
> 
> Previous to the above mention git commit the SG entries were of 4KB, and
> the code introduced by above git commit squashed the CPU contiguous PFNs
> in one big virtual address provided to DMA API.
> 
> This patch is a simple semi-revert - were we emulate the old behavior
> if we detect that SWIOTLB is online. If it is not online then we continue
> on with the new compact scatter gather mechanism.
> 
> An alternative solution would be for the the '.get_pages' and the
> i915_gem_gtt_prepare_object to retry with smaller max gap of the
> amount of PFNs that can be combined together - but with this issue
> discovered during rc7 that might be too risky.
> 
> Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Imre Deak <imre.deak@intel.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> CC: David Airlie <airlied@linux.ie>
> CC: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Two things:

- SWIOTLB usage should seriously blow up all over the place in drm/i915.
  We really rely on the everywhere else true fact that the pages and their
  dma mapping point at the same backing storage.

- How is this solved elsewhere when constructing sg tables? Or are we
  really the only guys who try to construct such big sg entries? I
  expected somewhat that the dma mapping backed would fill in the segment
  limits accordingly, but I haven't found anything really on a quick
  search.


Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 970ad17..7045f45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>  			gfp &= ~(__GFP_IO | __GFP_WAIT);
>  		}
> -
> +#ifdef CONFIG_SWIOTLB
> +		if (swiotlb_nr_tbl()) {
> +			st->nents++;
> +			sg_set_page(sg, page, PAGE_SIZE, 0);
> +			sg = sg_next(sg);
> +			continue;
> +		}
> +#endif
>  		if (!i || page_to_pfn(page) != last_pfn + 1) {
>  			if (i)
>  				sg = sg_next(sg);
> @@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  		}
>  		last_pfn = page_to_pfn(page);
>  	}
> -
> -	sg_mark_end(sg);
> +#ifdef CONFIG_SWIOTLB
> +	if (!swiotlb_nr_tbl())
> +#endif
> +		sg_mark_end(sg);
>  	obj->pages = st;
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
> -- 
> 1.8.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Konrad Rzeszutek Wilk]

On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
> > ("drm/i915: create compact dma scatter lists for gem objects") makes
> > certain assumptions about the under laying DMA API that are not always
> > correct.
> > 
> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
> > I see:
> > 
> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
> > 
> > Bit of debugging traced it down to dma_map_sg failing (in
> > i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
> > 
> > That unfortunately are sizes that the SWIOTLB is incapable of handling -
> > the maximum it can handle is a an entry of 512KB of virtual contiguous
> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
> > 
> > Previous to the above mention git commit the SG entries were of 4KB, and
> > the code introduced by above git commit squashed the CPU contiguous PFNs
> > in one big virtual address provided to DMA API.
> > 
> > This patch is a simple semi-revert - were we emulate the old behavior
> > if we detect that SWIOTLB is online. If it is not online then we continue
> > on with the new compact scatter gather mechanism.
> > 
> > An alternative solution would be for the the '.get_pages' and the
> > i915_gem_gtt_prepare_object to retry with smaller max gap of the
> > amount of PFNs that can be combined together - but with this issue
> > discovered during rc7 that might be too risky.
> > 
> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > CC: Imre Deak <imre.deak@intel.com>
> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> > CC: David Airlie <airlied@linux.ie>
> > CC: <dri-devel@lists.freedesktop.org>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Two things:

Hey Daniel,

> 
> - SWIOTLB usage should seriously blow up all over the place in drm/i915.
>   We really rely on the everywhere else true fact that the pages and their
>   dma mapping point at the same backing storage.

It works. As in, it seems to work for just a normal desktop user. I don't 
see much of dma_sync_* sprinkled around the drm/i915 so I would think that
there are some issues would be hit as well - but at the first glance
when using it on a laptop it looks OK.
 
> - How is this solved elsewhere when constructing sg tables? Or are we
>   really the only guys who try to construct such big sg entries? I
>   expected somewhat that the dma mapping backed would fill in the segment
>   limits accordingly, but I haven't found anything really on a quick
>   search.

The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which will
construct the dma mapped pages. That allows it to construct "SWIOTLB-approved"
pages that won't need to go through dma_map/dma_unmap as they are
already mapped and ready to go.

Coming back to your question - I think that i915 is the one that I've
encountered.
> 
> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 970ad17..7045f45 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  			gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
> >  			gfp &= ~(__GFP_IO | __GFP_WAIT);
> >  		}
> > -
> > +#ifdef CONFIG_SWIOTLB
> > +		if (swiotlb_nr_tbl()) {
> > +			st->nents++;
> > +			sg_set_page(sg, page, PAGE_SIZE, 0);
> > +			sg = sg_next(sg);
> > +			continue;
> > +		}
> > +#endif
> >  		if (!i || page_to_pfn(page) != last_pfn + 1) {
> >  			if (i)
> >  				sg = sg_next(sg);
> > @@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  		}
> >  		last_pfn = page_to_pfn(page);
> >  	}
> > -
> > -	sg_mark_end(sg);
> > +#ifdef CONFIG_SWIOTLB
> > +	if (!swiotlb_nr_tbl())
> > +#endif
> > +		sg_mark_end(sg);
> >  	obj->pages = st;
> >  
> >  	if (i915_gem_object_needs_bit17_swizzle(obj))
> > -- 
> > 1.8.1.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Daniel Vetter]

On Mon, Jun 24, 2013 at 7:32 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
>> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
>> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
>> > ("drm/i915: create compact dma scatter lists for gem objects") makes
>> > certain assumptions about the under laying DMA API that are not always
>> > correct.
>> >
>> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
>> > I see:
>> >
>> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
>> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
>> >
>> > Bit of debugging traced it down to dma_map_sg failing (in
>> > i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
>> >
>> > That unfortunately are sizes that the SWIOTLB is incapable of handling -
>> > the maximum it can handle is a an entry of 512KB of virtual contiguous
>> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
>> >
>> > Previous to the above mention git commit the SG entries were of 4KB, and
>> > the code introduced by above git commit squashed the CPU contiguous PFNs
>> > in one big virtual address provided to DMA API.
>> >
>> > This patch is a simple semi-revert - were we emulate the old behavior
>> > if we detect that SWIOTLB is online. If it is not online then we continue
>> > on with the new compact scatter gather mechanism.
>> >
>> > An alternative solution would be for the the '.get_pages' and the
>> > i915_gem_gtt_prepare_object to retry with smaller max gap of the
>> > amount of PFNs that can be combined together - but with this issue
>> > discovered during rc7 that might be too risky.
>> >
>> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > CC: Chris Wilson <chris@chris-wilson.co.uk>
>> > CC: Imre Deak <imre.deak@intel.com>
>> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > CC: David Airlie <airlied@linux.ie>
>> > CC: <dri-devel@lists.freedesktop.org>
>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Two things:
>
> Hey Daniel,
>
>>
>> - SWIOTLB usage should seriously blow up all over the place in drm/i915.
>>   We really rely on the everywhere else true fact that the pages and their
>>   dma mapping point at the same backing storage.
>
> It works. As in, it seems to work for just a normal desktop user. I don't
> see much of dma_sync_* sprinkled around the drm/i915 so I would think that
> there are some issues would be hit as well - but at the first glance
> when using it on a laptop it looks OK.

Yeah, we have a pretty serious case of "roll our own coherency stuff".
The biggest reason is that for a long time i915.ko didn't care one bit
about iommus, and the thing we care about (flushing cpu caches for
dma) isn't supported on x86 since x86 every dma is coherent (well, not
quite, but we don't have support for it). I think longer-term it would
make sense to move the clfushing we're doing into the dma layer.

>> - How is this solved elsewhere when constructing sg tables? Or are we
>>   really the only guys who try to construct such big sg entries? I
>>   expected somewhat that the dma mapping backed would fill in the segment
>>   limits accordingly, but I haven't found anything really on a quick
>>   search.
>
> The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which will
> construct the dma mapped pages. That allows it to construct "SWIOTLB-approved"
> pages that won't need to go through dma_map/dma_unmap as they are
> already mapped and ready to go.
>
> Coming back to your question - I think that i915 is the one that I've
> encountered.

That's a bit surprising. With dma_buf graphics people will use sg
tables much more (there's even a nice sg_alloc_table_from_pages helper
to construct them), and those sg tables tend to have large segments. I
guess we need some more generic solution here ...

For now I guess we can live with your CONFIG_SWIOTLB hack.
-Daniel



>>
>>
>> Cheers, Daniel
>>
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> > index 970ad17..7045f45 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem.c
>> > @@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>> >                     gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>> >                     gfp &= ~(__GFP_IO | __GFP_WAIT);
>> >             }
>> > -
>> > +#ifdef CONFIG_SWIOTLB
>> > +           if (swiotlb_nr_tbl()) {
>> > +                   st->nents++;
>> > +                   sg_set_page(sg, page, PAGE_SIZE, 0);
>> > +                   sg = sg_next(sg);
>> > +                   continue;
>> > +           }
>> > +#endif
>> >             if (!i || page_to_pfn(page) != last_pfn + 1) {
>> >                     if (i)
>> >                             sg = sg_next(sg);
>> > @@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>> >             }
>> >             last_pfn = page_to_pfn(page);
>> >     }
>> > -
>> > -   sg_mark_end(sg);
>> > +#ifdef CONFIG_SWIOTLB
>> > +   if (!swiotlb_nr_tbl())
>> > +#endif
>> > +           sg_mark_end(sg);
>> >     obj->pages = st;
>> >
>> >     if (i915_gem_object_needs_bit17_swizzle(obj))
>> > --
>> > 1.8.1.4
>> >
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Daniel Vetter]

On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
> Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
> ("drm/i915: create compact dma scatter lists for gem objects") makes
> certain assumptions about the under laying DMA API that are not always
> correct.
> 
> On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
> I see:
> 
> [drm:intel_pipe_set_base] *ERROR* pin & fence failed
> [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
> 
> Bit of debugging traced it down to dma_map_sg failing (in
> i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
> 
> That unfortunately are sizes that the SWIOTLB is incapable of handling -
> the maximum it can handle is a an entry of 512KB of virtual contiguous
> memory for its bounce buffer. (See IO_TLB_SEGSIZE).
> 
> Previous to the above mention git commit the SG entries were of 4KB, and
> the code introduced by above git commit squashed the CPU contiguous PFNs
> in one big virtual address provided to DMA API.
> 
> This patch is a simple semi-revert - were we emulate the old behavior
> if we detect that SWIOTLB is online. If it is not online then we continue
> on with the new compact scatter gather mechanism.
> 
> An alternative solution would be for the the '.get_pages' and the
> i915_gem_gtt_prepare_object to retry with smaller max gap of the
> amount of PFNs that can be combined together - but with this issue
> discovered during rc7 that might be too risky.
> 
> Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Imre Deak <imre.deak@intel.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> CC: David Airlie <airlied@linux.ie>
> CC: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Queued for -next (with cc: stable), thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Konrad Rzeszutek Wilk]

On Mon, Jun 24, 2013 at 08:26:18PM +0200, Daniel Vetter wrote:
> On Mon, Jun 24, 2013 at 7:32 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
> >> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
> >> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
> >> > ("drm/i915: create compact dma scatter lists for gem objects") makes
> >> > certain assumptions about the under laying DMA API that are not always
> >> > correct.
> >> >
> >> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
> >> > I see:
> >> >
> >> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
> >> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
> >> >
> >> > Bit of debugging traced it down to dma_map_sg failing (in
> >> > i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
> >> >
> >> > That unfortunately are sizes that the SWIOTLB is incapable of handling -
> >> > the maximum it can handle is a an entry of 512KB of virtual contiguous
> >> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
> >> >
> >> > Previous to the above mention git commit the SG entries were of 4KB, and
> >> > the code introduced by above git commit squashed the CPU contiguous PFNs
> >> > in one big virtual address provided to DMA API.
> >> >
> >> > This patch is a simple semi-revert - were we emulate the old behavior
> >> > if we detect that SWIOTLB is online. If it is not online then we continue
> >> > on with the new compact scatter gather mechanism.
> >> >
> >> > An alternative solution would be for the the '.get_pages' and the
> >> > i915_gem_gtt_prepare_object to retry with smaller max gap of the
> >> > amount of PFNs that can be combined together - but with this issue
> >> > discovered during rc7 that might be too risky.
> >> >
> >> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> >> > CC: Imre Deak <imre.deak@intel.com>
> >> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > CC: David Airlie <airlied@linux.ie>
> >> > CC: <dri-devel@lists.freedesktop.org>
> >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>
> >> Two things:
> >
> > Hey Daniel,
> >
> >>
> >> - SWIOTLB usage should seriously blow up all over the place in drm/i915.
> >>   We really rely on the everywhere else true fact that the pages and their
> >>   dma mapping point at the same backing storage.
> >
> > It works. As in, it seems to work for just a normal desktop user. I don't
> > see much of dma_sync_* sprinkled around the drm/i915 so I would think that
> > there are some issues would be hit as well - but at the first glance
> > when using it on a laptop it looks OK.
> 
> Yeah, we have a pretty serious case of "roll our own coherency stuff".
> The biggest reason is that for a long time i915.ko didn't care one bit
> about iommus, and the thing we care about (flushing cpu caches for
> dma) isn't supported on x86 since x86 every dma is coherent (well, not
> quite, but we don't have support for it). I think longer-term it would
> make sense to move the clfushing we're doing into the dma layer.
> 
> >> - How is this solved elsewhere when constructing sg tables? Or are we
> >>   really the only guys who try to construct such big sg entries? I
> >>   expected somewhat that the dma mapping backed would fill in the segment
> >>   limits accordingly, but I haven't found anything really on a quick
> >>   search.
> >
> > The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which will
> > construct the dma mapped pages. That allows it to construct "SWIOTLB-approved"
> > pages that won't need to go through dma_map/dma_unmap as they are
> > already mapped and ready to go.
> >
> > Coming back to your question - I think that i915 is the one that I've
> > encountered.
> 
> That's a bit surprising. With dma_buf graphics people will use sg
> tables much more (there's even a nice sg_alloc_table_from_pages helper
> to construct them), and those sg tables tend to have large segments. I
> guess we need some more generic solution here ...

Yes. I don't grok the full picture yet so I am not sure how to help with
this right now. Is there a roadmap or Wiki on how this was envisioned?
> 
> For now I guess we can live with your CONFIG_SWIOTLB hack.
> -Daniel

OK, I read that as an Ack-ed-by. Should I send the patch to Dave Airlie
in a GIT PULL or some other way to make it on the v3.10-rc7 train?

> 
> 
> 
> >>
> >>
> >> Cheers, Daniel
> >>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++---
> >> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> > index 970ad17..7045f45 100644
> >> > --- a/drivers/gpu/drm/i915/i915_gem.c
> >> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> > @@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >> >                     gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
> >> >                     gfp &= ~(__GFP_IO | __GFP_WAIT);
> >> >             }
> >> > -
> >> > +#ifdef CONFIG_SWIOTLB
> >> > +           if (swiotlb_nr_tbl()) {
> >> > +                   st->nents++;
> >> > +                   sg_set_page(sg, page, PAGE_SIZE, 0);
> >> > +                   sg = sg_next(sg);
> >> > +                   continue;
> >> > +           }
> >> > +#endif
> >> >             if (!i || page_to_pfn(page) != last_pfn + 1) {
> >> >                     if (i)
> >> >                             sg = sg_next(sg);
> >> > @@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >> >             }
> >> >             last_pfn = page_to_pfn(page);
> >> >     }
> >> > -
> >> > -   sg_mark_end(sg);
> >> > +#ifdef CONFIG_SWIOTLB
> >> > +   if (!swiotlb_nr_tbl())
> >> > +#endif
> >> > +           sg_mark_end(sg);
> >> >     obj->pages = st;
> >> >
> >> >     if (i915_gem_object_needs_bit17_swizzle(obj))
> >> > --
> >> > 1.8.1.4
> >> >
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Dave Airlie]

On Tue, Jun 25, 2013 at 4:34 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Jun 24, 2013 at 08:26:18PM +0200, Daniel Vetter wrote:
>> On Mon, Jun 24, 2013 at 7:32 PM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>> > On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
>> >> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk wrote:
>> >> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
>> >> > ("drm/i915: create compact dma scatter lists for gem objects") makes
>> >> > certain assumptions about the under laying DMA API that are not always
>> >> > correct.
>> >> >
>> >> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
>> >> > I see:
>> >> >
>> >> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
>> >> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3], err = -28
>> >> >
>> >> > Bit of debugging traced it down to dma_map_sg failing (in
>> >> > i915_gem_gtt_prepare_object) as some of the SG entries were huge (3MB).
>> >> >
>> >> > That unfortunately are sizes that the SWIOTLB is incapable of handling -
>> >> > the maximum it can handle is a an entry of 512KB of virtual contiguous
>> >> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
>> >> >
>> >> > Previous to the above mention git commit the SG entries were of 4KB, and
>> >> > the code introduced by above git commit squashed the CPU contiguous PFNs
>> >> > in one big virtual address provided to DMA API.
>> >> >
>> >> > This patch is a simple semi-revert - were we emulate the old behavior
>> >> > if we detect that SWIOTLB is online. If it is not online then we continue
>> >> > on with the new compact scatter gather mechanism.
>> >> >
>> >> > An alternative solution would be for the the '.get_pages' and the
>> >> > i915_gem_gtt_prepare_object to retry with smaller max gap of the
>> >> > amount of PFNs that can be combined together - but with this issue
>> >> > discovered during rc7 that might be too risky.
>> >> >
>> >> > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> >> > CC: Chris Wilson <chris@chris-wilson.co.uk>
>> >> > CC: Imre Deak <imre.deak@intel.com>
>> >> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >> > CC: David Airlie <airlied@linux.ie>
>> >> > CC: <dri-devel@lists.freedesktop.org>
>> >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> >>
>> >> Two things:
>> >
>> > Hey Daniel,
>> >
>> >>
>> >> - SWIOTLB usage should seriously blow up all over the place in drm/i915.
>> >>   We really rely on the everywhere else true fact that the pages and their
>> >>   dma mapping point at the same backing storage.
>> >
>> > It works. As in, it seems to work for just a normal desktop user. I don't
>> > see much of dma_sync_* sprinkled around the drm/i915 so I would think that
>> > there are some issues would be hit as well - but at the first glance
>> > when using it on a laptop it looks OK.
>>
>> Yeah, we have a pretty serious case of "roll our own coherency stuff".
>> The biggest reason is that for a long time i915.ko didn't care one bit
>> about iommus, and the thing we care about (flushing cpu caches for
>> dma) isn't supported on x86 since x86 every dma is coherent (well, not
>> quite, but we don't have support for it). I think longer-term it would
>> make sense to move the clfushing we're doing into the dma layer.
>>
>> >> - How is this solved elsewhere when constructing sg tables? Or are we
>> >>   really the only guys who try to construct such big sg entries? I
>> >>   expected somewhat that the dma mapping backed would fill in the segment
>> >>   limits accordingly, but I haven't found anything really on a quick
>> >>   search.
>> >
>> > The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which will
>> > construct the dma mapped pages. That allows it to construct "SWIOTLB-approved"
>> > pages that won't need to go through dma_map/dma_unmap as they are
>> > already mapped and ready to go.
>> >
>> > Coming back to your question - I think that i915 is the one that I've
>> > encountered.
>>
>> That's a bit surprising. With dma_buf graphics people will use sg
>> tables much more (there's even a nice sg_alloc_table_from_pages helper
>> to construct them), and those sg tables tend to have large segments. I
>> guess we need some more generic solution here ...
>
> Yes. I don't grok the full picture yet so I am not sure how to help with
> this right now. Is there a roadmap or Wiki on how this was envisioned?
>>
>> For now I guess we can live with your CONFIG_SWIOTLB hack.
>> -Daniel
>
> OK, I read that as an Ack-ed-by. Should I send the patch to Dave Airlie
> in a GIT PULL or some other way to make it on the v3.10-rc7 train?

I don't like this at all, I'll accept the patch on the condition you
investigate further :-)

If you are using swiotlb on i915 things should break, I know I've
investigated problems before where swiotlb was being incorrectly used
due to page masks or other issues. Shouldn't you be passing through
using the real iommu?

Dave.

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Konrad Rzeszutek Wilk]

Dave Airlie <airlied@gmail.com> wrote:

>On Tue, Jun 25, 2013 at 4:34 AM, Konrad Rzeszutek Wilk
><konrad.wilk@oracle.com> wrote:
>> On Mon, Jun 24, 2013 at 08:26:18PM +0200, Daniel Vetter wrote:
>>> On Mon, Jun 24, 2013 at 7:32 PM, Konrad Rzeszutek Wilk
>>> <konrad.wilk@oracle.com> wrote:
>>> > On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
>>> >> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk
>wrote:
>>> >> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
>>> >> > ("drm/i915: create compact dma scatter lists for gem objects")
>makes
>>> >> > certain assumptions about the under laying DMA API that are not
>always
>>> >> > correct.
>>> >> >
>>> >> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the
>bootup
>>> >> > I see:
>>> >> >
>>> >> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
>>> >> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on
>[CRTC:3], err = -28
>>> >> >
>>> >> > Bit of debugging traced it down to dma_map_sg failing (in
>>> >> > i915_gem_gtt_prepare_object) as some of the SG entries were
>huge (3MB).
>>> >> >
>>> >> > That unfortunately are sizes that the SWIOTLB is incapable of
>handling -
>>> >> > the maximum it can handle is a an entry of 512KB of virtual
>contiguous
>>> >> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
>>> >> >
>>> >> > Previous to the above mention git commit the SG entries were of
>4KB, and
>>> >> > the code introduced by above git commit squashed the CPU
>contiguous PFNs
>>> >> > in one big virtual address provided to DMA API.
>>> >> >
>>> >> > This patch is a simple semi-revert - were we emulate the old
>behavior
>>> >> > if we detect that SWIOTLB is online. If it is not online then
>we continue
>>> >> > on with the new compact scatter gather mechanism.
>>> >> >
>>> >> > An alternative solution would be for the the '.get_pages' and
>the
>>> >> > i915_gem_gtt_prepare_object to retry with smaller max gap of
>the
>>> >> > amount of PFNs that can be combined together - but with this
>issue
>>> >> > discovered during rc7 that might be too risky.
>>> >> >
>>> >> > Reported-and-Tested-by: Konrad Rzeszutek Wilk
><konrad.wilk@oracle.com>
>>> >> > CC: Chris Wilson <chris@chris-wilson.co.uk>
>>> >> > CC: Imre Deak <imre.deak@intel.com>
>>> >> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> >> > CC: David Airlie <airlied@linux.ie>
>>> >> > CC: <dri-devel@lists.freedesktop.org>
>>> >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> >>
>>> >> Two things:
>>> >
>>> > Hey Daniel,
>>> >
>>> >>
>>> >> - SWIOTLB usage should seriously blow up all over the place in
>drm/i915.
>>> >>   We really rely on the everywhere else true fact that the pages
>and their
>>> >>   dma mapping point at the same backing storage.
>>> >
>>> > It works. As in, it seems to work for just a normal desktop user.
>I don't
>>> > see much of dma_sync_* sprinkled around the drm/i915 so I would
>think that
>>> > there are some issues would be hit as well - but at the first
>glance
>>> > when using it on a laptop it looks OK.
>>>
>>> Yeah, we have a pretty serious case of "roll our own coherency
>stuff".
>>> The biggest reason is that for a long time i915.ko didn't care one
>bit
>>> about iommus, and the thing we care about (flushing cpu caches for
>>> dma) isn't supported on x86 since x86 every dma is coherent (well,
>not
>>> quite, but we don't have support for it). I think longer-term it
>would
>>> make sense to move the clfushing we're doing into the dma layer.
>>>
>>> >> - How is this solved elsewhere when constructing sg tables? Or
>are we
>>> >>   really the only guys who try to construct such big sg entries?
>I
>>> >>   expected somewhat that the dma mapping backed would fill in the
>segment
>>> >>   limits accordingly, but I haven't found anything really on a
>quick
>>> >>   search.
>>> >
>>> > The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which
>will
>>> > construct the dma mapped pages. That allows it to construct
>"SWIOTLB-approved"
>>> > pages that won't need to go through dma_map/dma_unmap as they are
>>> > already mapped and ready to go.
>>> >
>>> > Coming back to your question - I think that i915 is the one that
>I've
>>> > encountered.
>>>
>>> That's a bit surprising. With dma_buf graphics people will use sg
>>> tables much more (there's even a nice sg_alloc_table_from_pages
>helper
>>> to construct them), and those sg tables tend to have large segments.
>I
>>> guess we need some more generic solution here ...
>>
>> Yes. I don't grok the full picture yet so I am not sure how to help
>with
>> this right now. Is there a roadmap or Wiki on how this was
>envisioned?
>>>
>>> For now I guess we can live with your CONFIG_SWIOTLB hack.
>>> -Daniel
>>
>> OK, I read that as an Ack-ed-by. Should I send the patch to Dave
>Airlie
>> in a GIT PULL or some other way to make it on the v3.10-rc7 train?
>
>I don't like this at all, I'll accept the patch on the condition you
>investigate further :-)
>
>If you are using swiotlb on i915 things should break, I know I've
>investigated problems before where swiotlb was being incorrectly used
>due to page masks or other issues. Shouldn't you be passing through
>using the real iommu?
>
>Dave.

Hey Dave
Of course I will investigate. 

The SWIOTLB is unfortunately used because it is a fallback (and I am the maintainer of it) and if a real IOMMU is activated it can be mitigated differently. When you say 'passed through'  you mean in terms of an IOMMU in a guest? There are no IOMMU inside a guest when passing in a PCI device. 

Let me start on a new thread on this when I have gotten my head wrapped around dma  buf. 

Thanks and sorry for getting to this so late in the cycle.  New laptop and playing with it and that triggered me finding this. 
-- 
Sent from my Android phone. Please excuse my brevity.

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Dave Airlie]

On Tue, Jun 25, 2013 at 9:18 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> Dave Airlie <airlied@gmail.com> wrote:
>
>>On Tue, Jun 25, 2013 at 4:34 AM, Konrad Rzeszutek Wilk
>><konrad.wilk@oracle.com> wrote:
>>> On Mon, Jun 24, 2013 at 08:26:18PM +0200, Daniel Vetter wrote:
>>>> On Mon, Jun 24, 2013 at 7:32 PM, Konrad Rzeszutek Wilk
>>>> <konrad.wilk@oracle.com> wrote:
>>>> > On Mon, Jun 24, 2013 at 07:09:12PM +0200, Daniel Vetter wrote:
>>>> >> On Mon, Jun 24, 2013 at 11:47:48AM -0400, Konrad Rzeszutek Wilk
>>wrote:
>>>> >> > Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
>>>> >> > ("drm/i915: create compact dma scatter lists for gem objects")
>>makes
>>>> >> > certain assumptions about the under laying DMA API that are not
>>always
>>>> >> > correct.
>>>> >> >
>>>> >> > On a ThinkPad X230 with an Intel HD 4000 with Xen during the
>>bootup
>>>> >> > I see:
>>>> >> >
>>>> >> > [drm:intel_pipe_set_base] *ERROR* pin & fence failed
>>>> >> > [drm:intel_crtc_set_config] *ERROR* failed to set mode on
>>[CRTC:3], err = -28
>>>> >> >
>>>> >> > Bit of debugging traced it down to dma_map_sg failing (in
>>>> >> > i915_gem_gtt_prepare_object) as some of the SG entries were
>>huge (3MB).
>>>> >> >
>>>> >> > That unfortunately are sizes that the SWIOTLB is incapable of
>>handling -
>>>> >> > the maximum it can handle is a an entry of 512KB of virtual
>>contiguous
>>>> >> > memory for its bounce buffer. (See IO_TLB_SEGSIZE).
>>>> >> >
>>>> >> > Previous to the above mention git commit the SG entries were of
>>4KB, and
>>>> >> > the code introduced by above git commit squashed the CPU
>>contiguous PFNs
>>>> >> > in one big virtual address provided to DMA API.
>>>> >> >
>>>> >> > This patch is a simple semi-revert - were we emulate the old
>>behavior
>>>> >> > if we detect that SWIOTLB is online. If it is not online then
>>we continue
>>>> >> > on with the new compact scatter gather mechanism.
>>>> >> >
>>>> >> > An alternative solution would be for the the '.get_pages' and
>>the
>>>> >> > i915_gem_gtt_prepare_object to retry with smaller max gap of
>>the
>>>> >> > amount of PFNs that can be combined together - but with this
>>issue
>>>> >> > discovered during rc7 that might be too risky.
>>>> >> >
>>>> >> > Reported-and-Tested-by: Konrad Rzeszutek Wilk
>><konrad.wilk@oracle.com>
>>>> >> > CC: Chris Wilson <chris@chris-wilson.co.uk>
>>>> >> > CC: Imre Deak <imre.deak@intel.com>
>>>> >> > CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> >> > CC: David Airlie <airlied@linux.ie>
>>>> >> > CC: <dri-devel@lists.freedesktop.org>
>>>> >> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> >>
>>>> >> Two things:
>>>> >
>>>> > Hey Daniel,
>>>> >
>>>> >>
>>>> >> - SWIOTLB usage should seriously blow up all over the place in
>>drm/i915.
>>>> >>   We really rely on the everywhere else true fact that the pages
>>and their
>>>> >>   dma mapping point at the same backing storage.
>>>> >
>>>> > It works. As in, it seems to work for just a normal desktop user.
>>I don't
>>>> > see much of dma_sync_* sprinkled around the drm/i915 so I would
>>think that
>>>> > there are some issues would be hit as well - but at the first
>>glance
>>>> > when using it on a laptop it looks OK.
>>>>
>>>> Yeah, we have a pretty serious case of "roll our own coherency
>>stuff".
>>>> The biggest reason is that for a long time i915.ko didn't care one
>>bit
>>>> about iommus, and the thing we care about (flushing cpu caches for
>>>> dma) isn't supported on x86 since x86 every dma is coherent (well,
>>not
>>>> quite, but we don't have support for it). I think longer-term it
>>would
>>>> make sense to move the clfushing we're doing into the dma layer.
>>>>
>>>> >> - How is this solved elsewhere when constructing sg tables? Or
>>are we
>>>> >>   really the only guys who try to construct such big sg entries?
>>I
>>>> >>   expected somewhat that the dma mapping backed would fill in the
>>segment
>>>> >>   limits accordingly, but I haven't found anything really on a
>>quick
>>>> >>   search.
>>>> >
>>>> > The TTM layer (so radeon, nouveau) uses pci_alloc_coherent which
>>will
>>>> > construct the dma mapped pages. That allows it to construct
>>"SWIOTLB-approved"
>>>> > pages that won't need to go through dma_map/dma_unmap as they are
>>>> > already mapped and ready to go.
>>>> >
>>>> > Coming back to your question - I think that i915 is the one that
>>I've
>>>> > encountered.
>>>>
>>>> That's a bit surprising. With dma_buf graphics people will use sg
>>>> tables much more (there's even a nice sg_alloc_table_from_pages
>>helper
>>>> to construct them), and those sg tables tend to have large segments.
>>I
>>>> guess we need some more generic solution here ...
>>>
>>> Yes. I don't grok the full picture yet so I am not sure how to help
>>with
>>> this right now. Is there a roadmap or Wiki on how this was
>>envisioned?
>>>>
>>>> For now I guess we can live with your CONFIG_SWIOTLB hack.
>>>> -Daniel
>>>
>>> OK, I read that as an Ack-ed-by. Should I send the patch to Dave
>>Airlie
>>> in a GIT PULL or some other way to make it on the v3.10-rc7 train?
>>
>>I don't like this at all, I'll accept the patch on the condition you
>>investigate further :-)
>>
>>If you are using swiotlb on i915 things should break, I know I've
>>investigated problems before where swiotlb was being incorrectly used
>>due to page masks or other issues. Shouldn't you be passing through
>>using the real iommu?
>>
>>Dave.
>
> Hey Dave
> Of course I will investigate.
>
> The SWIOTLB is unfortunately used because it is a fallback (and I am the maintainer of it) and if a real IOMMU is activated it can be mitigated differently. When you say 'passed through'  you mean in terms of an IOMMU in a guest? There are no IOMMU inside a guest when passing in a PCI device.

I just don't understand why anyone would see swiotlb in this case, the
host would be using hw iommu, why would the guest then need to use a
bounce buffer?

>
> Let me start on a new thread on this when I have gotten my head wrapped around dma  buf.
>
> Thanks and sorry for getting to this so late in the cycle.  New laptop and playing with it and that triggered me finding this.

My main worry is this will regress things for people with swiotlb
enabled even if the gpu isn't using it, granted it won't be any slower
than before so probably not something I care about now if I know
you'll narrow down why all this is necessary later.

Dave.

Re: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.

[Konrad Rzeszutek Wilk]

> >>Dave.
> >
> > Hey Dave
> > Of course I will investigate.
> >
> > The SWIOTLB is unfortunately used because it is a fallback (and I am the maintainer of it) and if a real IOMMU is activated it can be mitigated differently. When you say 'passed through'  you mean in terms of an IOMMU in a guest? There are no IOMMU inside a guest when passing in a PCI device.
> 
> I just don't understand why anyone would see swiotlb in this case, the
> host would be using hw iommu, why would the guest then need to use a
> bounce buffer?

Hey Dave,

Sorry for the late response.

The guest has no concept of the HW IOMMU as it is not 'emulated' or there
are no plumbing for it to interface with the host's IOMMU. It means that if
it has more than 4GB it will automatically turn on SWIOTLB (b/c hey it
might have 32-bit capable devices and it needs to bounce buffer the data
to an area above 4GB).

Normally the SWIOTLB bounce buffers won't be used unless:
 a) the pages are not contingous. This is not a case for HVM guests (as
   it _thinks_ its PFN are always contingous - albeit in reality in might
   not be, but that is the job of the host EPT/IOMMU to construct this fake
   view), but for Xen PV - which has a mapping of the PFN -> machine addresses -
   it _knows_ that the real machine address of a PFN. And as guests are
   created from random swaths of memory - some of the PFNs might be
   contingous but some might not. In other words for RAM regions:

      pfn_to_mfn(pfn + 1) != (pfn_to_mfn(pfn) + 1)

   mfn is the real physical address bitshifted (PAGE_SHIFT). For HVM guest:
   (pfn_to_mfn returns the pfn value, so the above formula is):

       pfn+1 == pfn+1

   If this does not make any sense to you - that is OK :-) I can try to
   explain more but it might just put you to sleep - in which case just
   think: "Xen PV CPU physical addresses are not the same as the bus(DMA)
   addresses." - which means it is similar to Sparc platforms or other
   platforms where the IOMMU has no address CPU->PCI machinery.

 b) the pages are not page aligned. Less of an issue, but still can
   come up.

 c) the DMA mask of the PCI device is 32-bit (common with USB devices, not
   so often with graphic cards). But hey - there are quirks that sometimes
    make graphics card DMA up only to certain bitness.

 d). user provided 'swiotlb=force' and now everything is going through
   the bounce buffer.

The nice solution is to have a virtualization aware version of IOMMU in the
guest that will disable SWIOTLB (or use it only in fallback). The AMD folks
were thinking about that for KVM, but nothing came out of that. The Citrix
folks are looking at that for Xen, but nothing yet (thought I did see some
RFC patches).

> 
> >
> > Let me start on a new thread on this when I have gotten my head wrapped around dma  buf.

Hadn't gotten to that yet.
> >
> > Thanks and sorry for getting to this so late in the cycle.  New laptop and playing with it and that triggered me finding this.
> 
> My main worry is this will regress things for people with swiotlb
> enabled even if the gpu isn't using it, granted it won't be any slower
> than before so probably not something I care about now if I know
> you'll narrow down why all this is necessary later.

I am not sure how it would? The patch makes the i915 construct the
scatter gather list as it was in v3.9. So it _should_ not impact it
negatively. I was trying to follow the spirit of doing a partial revert
as close as possible so that the risk of regression would be nil.

To summarize, I think (and please correct me if I am mistaken):
 - You or Daniel are thinking to take this patch for v3.10 or v3.11
   (and if in v3.11 then tack on stable@vger.kernel.org).

 - You will tell defer all SWIOTLB related issues to me. In other words
   if you see something that is i915 and swiotlb, you will happily shout
   "Konrad! Tag!" and wash your hands. Hopefully you can also send me
   some of the past bugs that you suspect are SWIOTLB related.

 - You expect me to look at dma-buf and figure out how it can coexist with
   SWIOTLB.

Sounds about right?
> 
> Dave.