On 09/01/2017 12:28, Chris Wilson wrote:
On Mon, Jan 09, 2017 at 12:23:37PM +0000, Tvrtko Ursulin wrote:
Hi,
On 20/12/2016 13:42, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Rather than freeing and re-allocating the pages when DMA mapping
in large chunks fails, we can just rebuild the sg table with no
coalescing.
Also change back the page counter to unsigned int because that
is what the sg API supports.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
Only compile tested!
---
drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5275f6248ce3..e73f9f5a5d23 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2340,12 +2340,36 @@ static void i915_sg_trim(struct sg_table *orig_st)
*orig_st = new_st;
}
+static void i915_sg_uncoalesce(struct sg_table *orig_st, unsigned long nents)
+{
+ struct sg_table new_st;
+ struct scatterlist *new_sg;
+ struct sgt_iter sgt_iter;
+ struct page *page;
+
+ if (sg_alloc_table(&new_st, nents, GFP_KERNEL))
+ return;
+
+ new_sg = new_st.sgl;
+ for_each_sgt_page(page, sgt_iter, orig_st) {
+ sg_set_page(new_sg, page, PAGE_SIZE, 0);
+ /* called before being DMA mapped, no need to copy sg->dma_* */
+ new_sg = sg_next(new_sg);
+ }
+
+ GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
+
+ sg_free_table(orig_st);
+
+ *orig_st = new_st;
+}
+
static struct sg_table *
i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
{
struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
- const unsigned long page_count = obj->base.size / PAGE_SIZE;
- unsigned long i;
+ const unsigned int page_count = obj->base.size / PAGE_SIZE;
+ unsigned int i;
struct address_space *mapping;
struct sg_table *st;
struct scatterlist *sg;
@@ -2371,7 +2395,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
if (st == NULL)
return ERR_PTR(-ENOMEM);
-rebuild_st:
if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
kfree(st);
return ERR_PTR(-ENOMEM);
@@ -2429,6 +2452,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
/* Trim unused sg entries to avoid wasting memory. */
i915_sg_trim(st);
+prepare_gtt:
ret = i915_gem_gtt_prepare_pages(obj, st);
if (ret) {
/* DMA remapping failed? One possible cause is that
@@ -2436,16 +2460,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
* for PAGE_SIZE chunks instead may be helpful.
*/
if (max_segment > PAGE_SIZE) {
- for_each_sgt_page(page, sgt_iter, st)
- put_page(page);
- sg_free_table(st);
-
+ i915_sg_uncoalesce(st, page_count);
max_segment = PAGE_SIZE;
- goto rebuild_st;
+ goto prepare_gtt;
} else {
dev_warn(&dev_priv->drm.pdev->dev,
- "Failed to DMA remap %lu pages\n",
- page_count);
+ "Failed to DMA remap %u pages\n", page_count);
goto err_pages;
}
}
Are you still against this? As a reminder it saves a
put_page/allocate page-from-shmemfs cycle on dma mapping failures.
I still regard it as incomplete. Why bother saving that trip and not the
actual page allocation itself.
Page allocation is exactly what it avoids re-doing!?
You meant sg_alloc_table maybe? We could move the trim to be last and do
the uncoalesce in place but it would be so much more complex that I am
not sure it is worth it for this edge case.
The patch above was fast and simple improvement to the dma remapping
retry you added.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
