| Message ID | 20170202132721.12711-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/02/2017 13:27, Chris Wilson wrote: > If we fail to dma-map the object, the most common cause is lack of space > inside the SW-IOTLB due to fragmentation. If we recreate the_sg_table > using segments of PAGE_SIZE (and single page allocations), we may succeed > in remapping the scatterlist. > > First became a significant problem for the mock selftests after commit > 5584f1b1d73e ("drm/i915: fix i915 running as dom0 under Xen") increased > the max_order. There is still "max_order = min(max_order, ...)" in that patch, so I don't see how it increased it. I think you mean "drm/i915: Allow compaction upto SWIOTLB max segment size" ? Or no, that predates the internal object support. So just fixes on the patch which introduced the internal objects? > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_internal.c | 37 ++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_internal.c b/drivers/gpu/drm/i915/i915_gem_internal.c > index 2b9d5e94a8ae..fc950abbe400 100644 > --- a/drivers/gpu/drm/i915/i915_gem_internal.c > +++ b/drivers/gpu/drm/i915/i915_gem_internal.c > @@ -48,24 +48,12 @@ static struct sg_table * > i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > { > struct drm_i915_private *i915 = to_i915(obj->base.dev); > - unsigned int npages = obj->base.size / PAGE_SIZE; > struct sg_table *st; > struct scatterlist *sg; > + unsigned int npages; > int max_order; > gfp_t gfp; > > - st = kmalloc(sizeof(*st), GFP_KERNEL); > - if (!st) > - return ERR_PTR(-ENOMEM); > - > - if (sg_alloc_table(st, npages, GFP_KERNEL)) { > - kfree(st); > - return ERR_PTR(-ENOMEM); > - } > - > - sg = st->sgl; > - st->nents = 0; > - > max_order = MAX_ORDER; > #ifdef CONFIG_SWIOTLB > if (swiotlb_nr_tbl()) { > @@ -87,6 +75,20 @@ i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > gfp |= __GFP_DMA32; > } > > +create_st: > + st = kmalloc(sizeof(*st), GFP_KERNEL); > + if (!st) > + return ERR_PTR(-ENOMEM); > + > + npages = obj->base.size / PAGE_SIZE; > + if (sg_alloc_table(st, npages, GFP_KERNEL)) { > + kfree(st); > + return ERR_PTR(-ENOMEM); > + } > + > + sg = st->sgl; > + st->nents = 0; > + > do { > int order = min(fls(npages) - 1, max_order); > struct page *page; > @@ -114,8 +116,15 @@ i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > sg = __sg_next(sg); > } while (1); > > - if (i915_gem_gtt_prepare_pages(obj, st)) > + if (i915_gem_gtt_prepare_pages(obj, st)) { > + /* Failed to dma-map try again with single page sg segments */ > + if (get_order(st->sgl->length)) { > + internal_free_pages(st); > + max_order = 0; > + goto create_st; > + } > goto err; > + } > > /* Mark the pages as dontneed whilst they are still pinned. As soon > * as they are unpinned they are allowed to be reaped by the shrinker, > Looks correct, just the correct victim needs to be identified in the commit and fixes tag. With that fixed: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Fri, Feb 03, 2017 at 09:40:23AM +0000, Tvrtko Ursulin wrote: > > On 02/02/2017 13:27, Chris Wilson wrote: > >If we fail to dma-map the object, the most common cause is lack of space > >inside the SW-IOTLB due to fragmentation. If we recreate the_sg_table > >using segments of PAGE_SIZE (and single page allocations), we may succeed > >in remapping the scatterlist. > > > >First became a significant problem for the mock selftests after commit > >5584f1b1d73e ("drm/i915: fix i915 running as dom0 under Xen") increased > >the max_order. > > There is still "max_order = min(max_order, ...)" in that patch, so I > don't see how it increased it. > > I think you mean "drm/i915: Allow compaction upto SWIOTLB max > segment size" ? Or no, that predates the internal object support. So > just fixes on the patch which introduced the internal objects? The failure bisects to 5584f1b1d73e as ilog2(IO_TLB_SEGPAGES) is 6, but here I have ilog2(swiotlb_max_segment() >> PAGE_SHIFT) == 9. I did not say the bug was introduced in that patch, just that it made the underlying fragmentation issue more visible - and causing test failures. -Chris
On Fri, Feb 03, 2017 at 09:40:23AM +0000, Tvrtko Ursulin wrote: > Looks correct, just the correct victim needs to be identified in the > commit and fixes tag. With that fixed: Added the fixes tag, thought I'm sure if it merits applying to stable without a user bug report - that I leave up to Jani. > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Thanks, and pushed. -Chris
On Fri, Feb 03, 2017 at 09:59:45AM +0000, Chris Wilson wrote: > On Fri, Feb 03, 2017 at 09:40:23AM +0000, Tvrtko Ursulin wrote: > > Looks correct, just the correct victim needs to be identified in the > > commit and fixes tag. With that fixed: > > Added the fixes tag, thought I'm sure if it merits applying to stable *though I'm not sure if. One coffee in the morning is not enough any more. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_internal.c b/drivers/gpu/drm/i915/i915_gem_internal.c index 2b9d5e94a8ae..fc950abbe400 100644 --- a/drivers/gpu/drm/i915/i915_gem_internal.c +++ b/drivers/gpu/drm/i915/i915_gem_internal.c @@ -48,24 +48,12 @@ static struct sg_table * i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) { struct drm_i915_private *i915 = to_i915(obj->base.dev); - unsigned int npages = obj->base.size / PAGE_SIZE; struct sg_table *st; struct scatterlist *sg; + unsigned int npages; int max_order; gfp_t gfp; - st = kmalloc(sizeof(*st), GFP_KERNEL); - if (!st) - return ERR_PTR(-ENOMEM); - - if (sg_alloc_table(st, npages, GFP_KERNEL)) { - kfree(st); - return ERR_PTR(-ENOMEM); - } - - sg = st->sgl; - st->nents = 0; - max_order = MAX_ORDER; #ifdef CONFIG_SWIOTLB if (swiotlb_nr_tbl()) { @@ -87,6 +75,20 @@ i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) gfp |= __GFP_DMA32; } +create_st: + st = kmalloc(sizeof(*st), GFP_KERNEL); + if (!st) + return ERR_PTR(-ENOMEM); + + npages = obj->base.size / PAGE_SIZE; + if (sg_alloc_table(st, npages, GFP_KERNEL)) { + kfree(st); + return ERR_PTR(-ENOMEM); + } + + sg = st->sgl; + st->nents = 0; + do { int order = min(fls(npages) - 1, max_order); struct page *page; @@ -114,8 +116,15 @@ i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) sg = __sg_next(sg); } while (1); - if (i915_gem_gtt_prepare_pages(obj, st)) + if (i915_gem_gtt_prepare_pages(obj, st)) { + /* Failed to dma-map try again with single page sg segments */ + if (get_order(st->sgl->length)) { + internal_free_pages(st); + max_order = 0; + goto create_st; + } goto err; + } /* Mark the pages as dontneed whilst they are still pinned. As soon * as they are unpinned they are allowed to be reaped by the shrinker,
If we fail to dma-map the object, the most common cause is lack of space inside the SW-IOTLB due to fragmentation. If we recreate the_sg_table using segments of PAGE_SIZE (and single page allocations), we may succeed in remapping the scatterlist. First became a significant problem for the mock selftests after commit 5584f1b1d73e ("drm/i915: fix i915 running as dom0 under Xen") increased the max_order. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem_internal.c | 37 ++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 14 deletions(-)