| Message ID | 1463489483-34903-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/05/16 13:51, Dave Gordon wrote: > The recently-added i915_gem_object_pin_map() can be further optimised > for "small" objects. To facilitate this, and simplify the error paths > before adding the new code, this patch pulls out the "mapping" part of > the operation (involving local allocations which must be undone before > return) into its own subfunction. > > The next patch will then insert the new optimisation into the middle of > the now-separated subfunction. > > This reorganisation will probably not affect the generated code, as the > compiler will most likely inline it anyway, but it makes the logical > structure a bit clearer and easier to modify. > > v2: > Restructure loop-over-pages & error check [Chris Wilson] > > v3: > Add page count to debug messages [Chris Wilson] > Convert WARN_ON() to GEM_BUG_ON() > > v4: > Drop the DEBUG messages [Tvrtko Ursulin] > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 54 +++++++++++++++++++++++++---------------- > 1 file changed, 33 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 24cab88..82a1bc4 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2398,6 +2398,38 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) > return 0; > } > > +/* The 'mapping' part of i915_gem_object_pin_map() below */ > +static void *i915_gem_object_map(const struct drm_i915_gem_object *obj) > +{ > + unsigned long n_pages = obj->base.size >> PAGE_SHIFT; > + struct sg_table *sgt = obj->pages; > + struct sg_page_iter sg_iter; > + struct page **pages; > + unsigned long i = 0; > + void *addr; > + > + /* A single page can always be kmapped */ > + if (n_pages == 1) > + return kmap(sg_page(sgt->sgl)); > + > + pages = drm_malloc_gfp(n_pages, sizeof(*pages), GFP_TEMPORARY); > + if (pages == NULL) > + return NULL; > + > + for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0) > + pages[i++] = sg_page_iter_page(&sg_iter); > + > + /* Check that we have the expected number of pages */ > + GEM_BUG_ON(i != n_pages); > + > + addr = vmap(pages, n_pages, 0, PAGE_KERNEL); > + > + drm_free_large(pages); > + > + return addr; > +} > + > +/* get, pin, and map the pages of the object into kernel space */ > void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj) > { > int ret; > @@ -2411,27 +2443,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj) > i915_gem_object_pin_pages(obj); > > if (obj->mapping == NULL) { > - struct page **pages; > - > - pages = NULL; > - if (obj->base.size == PAGE_SIZE) > - obj->mapping = kmap(sg_page(obj->pages->sgl)); > - else > - pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT, > - sizeof(*pages), > - GFP_TEMPORARY); > - if (pages != NULL) { > - struct sg_page_iter sg_iter; > - int n; > - > - n = 0; > - for_each_sg_page(obj->pages->sgl, &sg_iter, > - obj->pages->nents, 0) > - pages[n++] = sg_page_iter_page(&sg_iter); > - > - obj->mapping = vmap(pages, n, 0, PAGE_KERNEL); > - drm_free_large(pages); > - } > + obj->mapping = i915_gem_object_map(obj); > if (obj->mapping == NULL) { > i915_gem_object_unpin_pages(obj); > return ERR_PTR(-ENOMEM); > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On 17/05/16 14:23, Patchwork wrote: > == Series Details == > > Series: series starting with [v4,1/2] drm/i915: refactor i915_gem_object_pin_map() > URL : https://patchwork.freedesktop.org/series/7292/ > State : failure > > == Summary == > > Series 7292v1 Series without cover letter > http://patchwork.freedesktop.org/api/1.0/series/7292/revisions/1/mbox > > Test drv_hangman: > Subgroup error-state-basic: > pass -> FAIL (ro-ilk1-i5-650) https://bugs.freedesktop.org/show_bug.cgi?id=94305 Seems to only happen on that one machine? .Dave. > fi-bdw-i7-5557u total:219 pass:206 dwarn:0 dfail:0 fail:0 skip:13 > fi-bsw-n3050 total:218 pass:174 dwarn:0 dfail:0 fail:2 skip:42 > fi-byt-n2820 total:218 pass:175 dwarn:0 dfail:0 fail:2 skip:41 > fi-hsw-i7-4770k total:219 pass:198 dwarn:0 dfail:0 fail:0 skip:21 > fi-hsw-i7-4770r total:219 pass:193 dwarn:0 dfail:0 fail:0 skip:26 > fi-kbl-y total:219 pass:191 dwarn:1 dfail:0 fail:2 skip:25 > fi-skl-i7-6700k total:219 pass:191 dwarn:0 dfail:0 fail:0 skip:28 > ro-bdw-i5-5250u total:219 pass:181 dwarn:0 dfail:0 fail:0 skip:38 > ro-bdw-i7-5557U total:219 pass:206 dwarn:0 dfail:0 fail:0 skip:13 > ro-bdw-i7-5600u total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32 > ro-byt-n2820 total:218 pass:175 dwarn:0 dfail:0 fail:2 skip:41 > ro-hsw-i3-4010u total:218 pass:193 dwarn:0 dfail:0 fail:0 skip:25 > ro-hsw-i7-4770r total:219 pass:194 dwarn:0 dfail:0 fail:0 skip:25 > ro-ilk-i7-620lm total:219 pass:151 dwarn:0 dfail:0 fail:1 skip:67 > ro-ilk1-i5-650 total:214 pass:151 dwarn:0 dfail:0 fail:2 skip:61 > ro-ivb-i7-3770 total:219 pass:183 dwarn:0 dfail:0 fail:0 skip:36 > ro-ivb2-i7-3770 total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32 > ro-skl-i7-6700hq total:214 pass:189 dwarn:0 dfail:0 fail:0 skip:25 > ro-snb-i7-2620M total:219 pass:177 dwarn:0 dfail:0 fail:1 skip:41 > fi-snb-i7-2600 failed to connect after reboot > ro-bsw-n3050 failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_918/ > > 0d84413 drm-intel-nightly: 2016y-05m-17d-10h-26m-35s UTC integration manifest > cfa6373 drm/i915: optimise i915_gem_object_map() for small objects > 1864abb drm/i915: refactor i915_gem_object_pin_map()
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 24cab88..82a1bc4 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2398,6 +2398,38 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) return 0; } +/* The 'mapping' part of i915_gem_object_pin_map() below */ +static void *i915_gem_object_map(const struct drm_i915_gem_object *obj) +{ + unsigned long n_pages = obj->base.size >> PAGE_SHIFT; + struct sg_table *sgt = obj->pages; + struct sg_page_iter sg_iter; + struct page **pages; + unsigned long i = 0; + void *addr; + + /* A single page can always be kmapped */ + if (n_pages == 1) + return kmap(sg_page(sgt->sgl)); + + pages = drm_malloc_gfp(n_pages, sizeof(*pages), GFP_TEMPORARY); + if (pages == NULL) + return NULL; + + for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0) + pages[i++] = sg_page_iter_page(&sg_iter); + + /* Check that we have the expected number of pages */ + GEM_BUG_ON(i != n_pages); + + addr = vmap(pages, n_pages, 0, PAGE_KERNEL); + + drm_free_large(pages); + + return addr; +} + +/* get, pin, and map the pages of the object into kernel space */ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj) { int ret; @@ -2411,27 +2443,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj) i915_gem_object_pin_pages(obj); if (obj->mapping == NULL) { - struct page **pages; - - pages = NULL; - if (obj->base.size == PAGE_SIZE) - obj->mapping = kmap(sg_page(obj->pages->sgl)); - else - pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT, - sizeof(*pages), - GFP_TEMPORARY); - if (pages != NULL) { - struct sg_page_iter sg_iter; - int n; - - n = 0; - for_each_sg_page(obj->pages->sgl, &sg_iter, - obj->pages->nents, 0) - pages[n++] = sg_page_iter_page(&sg_iter); - - obj->mapping = vmap(pages, n, 0, PAGE_KERNEL); - drm_free_large(pages); - } + obj->mapping = i915_gem_object_map(obj); if (obj->mapping == NULL) { i915_gem_object_unpin_pages(obj); return ERR_PTR(-ENOMEM);
The recently-added i915_gem_object_pin_map() can be further optimised for "small" objects. To facilitate this, and simplify the error paths before adding the new code, this patch pulls out the "mapping" part of the operation (involving local allocations which must be undone before return) into its own subfunction. The next patch will then insert the new optimisation into the middle of the now-separated subfunction. This reorganisation will probably not affect the generated code, as the compiler will most likely inline it anyway, but it makes the logical structure a bit clearer and easier to modify. v2: Restructure loop-over-pages & error check [Chris Wilson] v3: Add page count to debug messages [Chris Wilson] Convert WARN_ON() to GEM_BUG_ON() v4: Drop the DEBUG messages [Tvrtko Ursulin] Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 54 +++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 21 deletions(-)