On 10/10/2016 14:43, Chris Wilson wrote:
On Mon, Oct 10, 2016 at 02:30:40PM +0100, Tvrtko Ursulin wrote:
On 10/10/2016 12:49, Chris Wilson wrote:
commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
work with SWIOTLB backend") took a heavy handed approach to undo the
scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
whereby we tried to pass a segment larger than SWIOTLB could handle.) We
can be a little more intelligent and try compacting the scatterlist up
to the maximum SWIOTLB segment size (when using SWIOTLB).
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
CC: Imre Deak <imre.deak@xxxxxxxxx>
CC: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca1a5a5c6f19..8b3474d215a5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
struct sgt_iter sgt_iter;
struct page *page;
unsigned long last_pfn = 0; /* suppress gcc warning */
+ unsigned long max_segment;
unsigned int would be enough.
Current maximum object size >> PAGE_SHIFT is 36 bits. We don't impose
any other restriction that would limit a sg chunk.
My bad, for some reason I thought it is used only under the
CONFIG_SWIOTLB guard.
int ret;
gfp_t gfp;
@@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
+ max_segment = obj->base.size;
+#ifdef CONFIG_SWIOTLB
+ if (swiotlb_nr_tbl())
+ max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
+#endif
+
Do you want to use IS_ENABLED here?
The symbol swiotlb_nr_tbl() is absent unless SWIOTLB is enabled at compile
time. So we need the cpp guard, or do you mean switch to
#if IS_ENABLED(CONFIG_SWIOTLB)
which we probably should indeed.
Ok, doesn't matter then.
st = kmalloc(sizeof(*st), GFP_KERNEL);
if (st == NULL)
return ERR_PTR(-ENOMEM);
@@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
goto err_pages;
}
}
-#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->length >= max_segment ||
I think this can overflow by a page, should be "sg->length >=
(max_segment - PAGE_SIZE)", or alternatively substract one page at
the max_segment assignment.
We are looking at the previous sg, right? (and we only ever increment by
PAGE_SIZE).
So: when the previous sg reaches the maximum length, start a new sg
element. Otherwise we extend the previous sg element by a PAGE, so on
the else branch the maximum of sg->length after the increment is
max_segment.
Yes I've corrected myself shortly after posting.
+ page_to_pfn(page) != last_pfn + 1) {
if (i)
sg = sg_next(sg);
st->nents++;
@@ -2273,9 +2274,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
/* Check that the i965g/gm workaround works. */
WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
}
-#ifdef CONFIG_SWIOTLB
- if (!swiotlb_nr_tbl())
-#endif
+ if (st->nents < st->orig_nents)
sg_mark_end(sg);
I wondered a few times that we could just terminate the table
unconditionally.
The caveat being that if we do insert orig_nents, then sg at this point
is NULL. Which is clearer:
if (st->nents < st->orig_nents) sg_mark_end(sg);
or
if (sg) sg_mark_end(sg); /* coalesced sg table */
?
I missed that as well (that it can be NULL here). Sorry for the noise
then. "if (sg)" looks somewhat better to me FWIW.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
