drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.]
Subsystems (2)
PATCH 1/1 drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.]
fyi if you are using an Xen + v3.10 + with i915.
----- Forwarded message from Konrad Rzeszutek Wilk <[email protected]> -----
Date: Mon, 24 Jun 2013 11:47:48 -0400
From: Konrad Rzeszutek Wilk <[email protected]>
To: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Cc: [email protected], Konrad Rzeszutek Wilk <[email protected]>
Subject: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.
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.
--- drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +12 -3
@@ -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))
Signature · 4 lines
1.8.1.4 ----- End forwarded message -----
Quoted 22 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
drivers/gpu/drm/i915/i915_gem.c +8
-1@@ -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);
drivers/gpu/drm/i915/i915_gem.c i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) · line 1828
Quoted 10 lines
drivers/gpu/drm/i915/i915_gem.c +4
-2} - -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))
Out of curiosity - while I can see the point of the first hunk, why do you need to also suppress the setting of the list terminator?
Jan
Quoted 31 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
drivers/gpu/drm/i915/i915_gem.c +12
-3@@ -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;
Quoted 4 lines
drivers/gpu/drm/i915/i915_gem.c i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) · line 1828
Quoted 1 line
drivers/gpu/drm/i915/i915_gem.c +0
-0if (i915_gem_object_needs_bit17_swizzle (obj))Out of curiosity - while I can see the point of the first hunk, why do you need to also suppress the setting of the list terminator?
It was crashing for me as sg was NULL for one-item sg structures. I didn't dig deep in it, but the combination of 'sg = sg_nex(sg)' and then sg_mark_end(sg) ended up with sg being NULL.
I could have made the 'sg = sg_next(sg)' be wrapped with a 'if (i < page_count)' but figured it would be easier to reproduce the original code as faithfully as possible.
Quoted 3 lines
Jan
Quoted 40 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
Quoted 4 lines · Jan Beulich
drivers/gpu/drm/i915/i915_gem.c i915_gem_object_get_pages_gtt(struct
Quoted 4 lines
Quoted 1 line
Quoted 1 line · Konrad Rzeszutek Wilk
drivers/gpu/drm/i915/i915_gem.c +0
-0@@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct
drm_i915_gem_object*obj)Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/i915/i915_gem.c +4
-1gfp |= __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 );Quoted 8 lines
drivers/gpu/drm/i915/i915_gem.c i915_gem_object_get_pages_gtt(struct
Quoted 8 lines
drivers/gpu/drm/i915/i915_gem.c +4
-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)Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/i915_gem.c +4
-2} 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;Quoted 4 lines
drivers/gpu/drm/i915/i915_gem.c i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) · line 1828
Quoted 1 line
drivers/gpu/drm/i915/i915_gem.c +0
-0if (i915_gem_object_needs_bit17_swizzle (obj))Out of curiosity - while I can see the point of the first hunk, why do you need to also suppress the setting of the list terminator?
Quoted 8 lines
It was crashing for me as sg was NULL for one-item sg structures. I didn't dig deep in it, but the combination of 'sg =
sg_nex(sg)' and thensg_mark_end(sg)ended up with sg being NULL.I could have made the 'sg =
sg_next(sg)' be wrapped with a 'if (i <page_count)'
but figured it would be easier to reproduce the original code as faithfully as possible.
So that's because the first hunk is sort of backwards then:
#ifdef CONFIG_SWIOTLB
if (swiotlb_nr_tbl()) {
if (i)
sg = sg_next(sg);
st->nents++;
sg_set_page(sg, page, PAGE_SIZE, 0);
continue;
}#endif
i.e. you want to match how the code immediately following this addition works.
Once you do that, it also becomes obvious that you really would just need to extend that following if() condition, i.e. by latching the result of swiotlb_nr_tbl() into a local variable (storing zero if !SWIOTLB), and then simply doing
if (!i || page_to_pfn(page) != last_pfn + 1 || using_swiotlb) {Jan