[Intel-gfx] [PATCH 11/42] drm/i915: Introduce an internal allocator for disposable private objects
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Oct 7 16:52:47 UTC 2016
On 07/10/2016 10:46, Chris Wilson wrote:
> Quite a few of our objects used for internal hardware programming do not
> benefit from being swappable or from being zero initialised. As such
> they do not benefit from using a shmemfs backing storage and since they
> are internal and never directly exposed to the user, we do not need to
> worry about providing a filp. For these we can use an
> drm_i915_gem_object wrapper around a sg_table of plain struct page. They
> are not swap backed and not automatically pinned. If they are reaped
> by the shrinker, the pages are released and the contents discarded. For
> the internal use case, this is fine as for example, ringbuffers are
> pinned from being written by a request to be read by the hardware. Once
> they are idle, they can be discarded entirely. As such they are a good
> match for execlist ringbuffers and a small variety of other internal
> objects.
>
> In the first iteration, this is limited to the scratch batch buffers we
> use (for command parsing and state initialisation).
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_drv.h | 5 +
> drivers/gpu/drm/i915/i915_gem_batch_pool.c | 28 ++---
> drivers/gpu/drm/i915/i915_gem_internal.c | 161 +++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
> drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++-
> 7 files changed, 189 insertions(+), 24 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/i915_gem_internal.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index a998c2bce70a..b94a90f34d2d 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -35,6 +35,7 @@ i915-y += i915_cmd_parser.o \
> i915_gem_execbuffer.o \
> i915_gem_fence.o \
> i915_gem_gtt.o \
> + i915_gem_internal.o \
> i915_gem.o \
> i915_gem_render_state.o \
> i915_gem_request.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fee5cc92e2f2..bad97f1e5265 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3538,6 +3538,11 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> u32 gtt_offset,
> u32 size);
>
> +/* i915_gem_internal.c */
> +struct drm_i915_gem_object *
> +i915_gem_object_create_internal(struct drm_device *dev,
> + unsigned int size);
> +
Wasn't size_t our convention for GEM objects?
> /* i915_gem_shrinker.c */
> unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> unsigned long target,
> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> index cb25cad3318c..3934c9103cf2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> @@ -97,9 +97,9 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
> size_t size)
> {
> struct drm_i915_gem_object *obj = NULL;
> - struct drm_i915_gem_object *tmp, *next;
> + struct drm_i915_gem_object *tmp;
> struct list_head *list;
> - int n;
> + int n, ret;
>
> lockdep_assert_held(&pool->engine->i915->drm.struct_mutex);
>
> @@ -112,19 +112,12 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
> n = ARRAY_SIZE(pool->cache_list) - 1;
> list = &pool->cache_list[n];
>
> - list_for_each_entry_safe(tmp, next, list, batch_pool_link) {
> + list_for_each_entry(tmp, list, batch_pool_link) {
> /* The batches are strictly LRU ordered */
> if (!i915_gem_active_is_idle(&tmp->last_read[pool->engine->id],
> &tmp->base.dev->struct_mutex))
> break;
>
> - /* While we're looping, do some clean up */
> - if (tmp->madv == __I915_MADV_PURGED) {
> - list_del(&tmp->batch_pool_link);
> - i915_gem_object_put(tmp);
> - continue;
> - }
> -
> if (tmp->base.size >= size) {
> obj = tmp;
> break;
> @@ -132,19 +125,16 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
> }
>
> if (obj == NULL) {
> - int ret;
> -
> - obj = i915_gem_object_create(&pool->engine->i915->drm, size);
> + obj = i915_gem_object_create_internal(&pool->engine->i915->drm,
> + size);
> if (IS_ERR(obj))
> return obj;
> -
> - ret = i915_gem_object_get_pages(obj);
> - if (ret)
> - return ERR_PTR(ret);
> -
> - obj->madv = I915_MADV_DONTNEED;
> }
>
> + ret = i915_gem_object_get_pages(obj);
> + if (ret)
> + return ERR_PTR(ret);
> +
> list_move_tail(&obj->batch_pool_link, list);
> i915_gem_object_pin_pages(obj);
> return obj;
> diff --git a/drivers/gpu/drm/i915/i915_gem_internal.c b/drivers/gpu/drm/i915/i915_gem_internal.c
> new file mode 100644
> index 000000000000..534a61c1aba2
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_internal.c
> @@ -0,0 +1,161 @@
> +/*
> + * Copyright © 2014-2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/i915_drm.h>
> +#include "i915_drv.h"
> +
> +static void internal_free_pages(struct sg_table *st)
> +{
> + struct sg_page_iter sg_iter;
> +
> + for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
> + put_page(sg_page_iter_page(&sg_iter));
> +
> + sg_free_table(st);
> + kfree(st);
> +}
> +
> +static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
> +{
> + const unsigned int npages = obj->base.size / PAGE_SIZE;
> + struct sg_table *st;
> + struct scatterlist *sg;
> + unsigned long last_pfn = 0; /* suppress gcc warning */
> + gfp_t gfp;
> + int i;
> +
> + st = kmalloc(sizeof(*st), GFP_KERNEL);
> + if (!st)
> + return -ENOMEM;
> +
> + if (sg_alloc_table(st, npages, GFP_KERNEL)) {
> + kfree(st);
> + return -ENOMEM;
> + }
> +
> + sg = st->sgl;
> + st->nents = 0;
> +
> + gfp = GFP_KERNEL | __GFP_HIGHMEM | __GFP_RECLAIMABLE;
> + if (IS_CRESTLINE(obj->base.dev) || IS_BROADWATER(obj->base.dev)) {
/O\ Nooooo... dev_priv please! :)
> + /* 965gm cannot relocate objects above 4GiB. */
> + gfp &= ~__GFP_HIGHMEM;
> + gfp |= __GFP_DMA32;
> + }
> +
> + for (i = 0; i < npages; i++) {
> + struct page *page;
> +
> + page = alloc_page(gfp);
> + if (!page)
> + goto err;
> +
> +#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);
> + st->nents++;
> + sg_set_page(sg, page, PAGE_SIZE, 0);
> + } else {
> + sg->length += PAGE_SIZE;
> + }
> + last_pfn = page_to_pfn(page);
> + }
> +#ifdef CONFIG_SWIOTLB
> + if (!swiotlb_nr_tbl())
> +#endif
> + sg_mark_end(sg);
Looks like the loop above could be moved into a helper and shared with
i915_gem_object_get_pages_gtt. Maybe just a page-alloc and
page-alloc-error callbacks would be required.
> + obj->pages = st;
> +
> + if (i915_gem_gtt_prepare_object(obj)) {
> + obj->pages = NULL;
> + 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,
> + * and the caller is expected to repopulate - the contents of this
> + * object are only valid whilst active and pinned.
> + */
> + obj->madv = I915_MADV_DONTNEED;
> + return 0;
> +
> +err:
> + sg_mark_end(sg);
> + internal_free_pages(st);
> + return -ENOMEM;
> +}
> +
> +static void i915_gem_object_put_pages_internal(struct drm_i915_gem_object *obj)
> +{
> + internal_free_pages(obj->pages);
> +
> + obj->dirty = 0;
> + obj->madv = I915_MADV_WILLNEED;
> +}
> +
> +static const struct drm_i915_gem_object_ops i915_gem_object_internal_ops = {
> + .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
> + .get_pages = i915_gem_object_get_pages_internal,
> + .put_pages = i915_gem_object_put_pages_internal,
> +};
> +
> +/**
> + * Creates a new object that wraps some internal memory for private use.
> + * This object is not backed by swappable storage, and as such its contents
> + * are volatile and only valid whilst pinned. If the object is reaped by the
> + * shrinker, its pages and data will be discarded. Equally, it is not a full
> + * GEM object and so not valid for access from userspace. This makes it useful
> + * for hardware interfaces like ringbuffers (which are pinned from the time
> + * the request is written to the time the hardware stops accessing it), but
> + * not for contexts (which need to be preserved when not active for later
> + * reuse). Note that it is not cleared upon allocation.
> + */
> +struct drm_i915_gem_object *
> +i915_gem_object_create_internal(struct drm_device *dev,
> + unsigned int size)
size_t again.
> +{
> + struct drm_i915_gem_object *obj;
> +
> + obj = i915_gem_object_alloc(dev);
> + if (!obj)
> + return ERR_PTR(-ENOMEM);
> +
> + drm_gem_private_object_init(dev, &obj->base, size);
> + i915_gem_object_init(obj, &i915_gem_object_internal_ops);
> +
> + obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> + obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> + obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
At least to_i915 would be helpful.
> +
> + return obj;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 09cf4874c45f..c009ee1c27fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -187,7 +187,7 @@ int i915_gem_render_state_init(struct drm_i915_gem_request *req)
> if (so.rodata->batch_items * 4 > 4096)
> return -EINVAL;
>
> - obj = i915_gem_object_create(&req->i915->drm, 4096);
> + obj = i915_gem_object_create_internal(&req->i915->drm, 4096);
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 480584c09306..99737b842d73 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -251,7 +251,7 @@ int intel_engine_create_scratch(struct intel_engine_cs *engine, int size)
>
> obj = i915_gem_object_create_stolen(&engine->i915->drm, size);
> if (!obj)
> - obj = i915_gem_object_create(&engine->i915->drm, size);
> + obj = i915_gem_object_create_internal(&engine->i915->drm, size);
> if (IS_ERR(obj)) {
> DRM_ERROR("Failed to allocate scratch page\n");
> return PTR_ERR(obj);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index f3dfb7ca625d..b5d9c03f84ce 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1783,9 +1783,10 @@ static int init_status_page(struct intel_engine_cs *engine)
> struct drm_i915_gem_object *obj;
> struct i915_vma *vma;
> unsigned int flags;
> + void *vaddr;
> int ret;
>
> - obj = i915_gem_object_create(&engine->i915->drm, 4096);
> + obj = i915_gem_object_create_internal(&engine->i915->drm, 4096);
> if (IS_ERR(obj)) {
> DRM_ERROR("Failed to allocate status page\n");
> return PTR_ERR(obj);
> @@ -1818,15 +1819,22 @@ static int init_status_page(struct intel_engine_cs *engine)
> if (ret)
> goto err;
>
> + vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_unpin;
> + }
> +
Unrelated fix, best if you could afford to extract it.
> engine->status_page.vma = vma;
> engine->status_page.ggtt_offset = i915_ggtt_offset(vma);
> - engine->status_page.page_addr =
> - i915_gem_object_pin_map(obj, I915_MAP_WB);
> + engine->status_page.page_addr = memset(vaddr, 0, 4096);
>
Very unusual, at least for me. :)
> DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n",
> engine->name, i915_ggtt_offset(vma));
> return 0;
>
> +err_unpin:
> + i915_vma_unpin(vma);
> err:
> i915_gem_object_put(obj);
> return ret;
Regards,
Tvrtko
More information about the Intel-gfx
mailing list