[12/41] drm/i915: Introduce an internal allocator for disposable private objects

Commit Message

Chris Wilson Oct. 20, 2016, 3:03 p.m. UTC

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).

v2: Allocate physically contiguous pages, where possible.
v3: Reduce maximum order on subsequent requests following an allocation
failure.

Signed-off-by: Chris Wilson <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>
---
 drivers/gpu/drm/i915/Makefile                |   1 +
 drivers/gpu/drm/i915/i915_drv.h              |   5 +
 drivers/gpu/drm/i915/i915_gem_batch_pool.c   |  27 ++---
 drivers/gpu/drm/i915/i915_gem_internal.c     | 167 +++++++++++++++++++++++++++
 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, 194 insertions(+), 24 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_gem_internal.c

Comments

Tvrtko Ursulin Oct. 20, 2016, 4:22 p.m. UTC | #1

On 20/10/2016 16:03, 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).

And the status page.

>
> v2: Allocate physically contiguous pages, where possible.
> v3: Reduce maximum order on subsequent requests following an allocation
> failure.
>
> Signed-off-by: Chris Wilson <[e-mail]>
> Cc: Tvrtko Ursulin <[e-mail]>
> ---
>  drivers/gpu/drm/i915/Makefile                |   1 +
>  drivers/gpu/drm/i915/i915_drv.h              |   5 +
>  drivers/gpu/drm/i915/i915_gem_batch_pool.c   |  27 ++---
>  drivers/gpu/drm/i915/i915_gem_internal.c     | 167 +++++++++++++++++++++++++++
>  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, 194 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 612340097f4b..7faa04c91e1a 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 4e93c3797d90..e267e20bdcdb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3543,6 +3543,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_i915_private *dev_priv,
> +				unsigned int size);
> +
>  /* 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..aa4e1e043b4e 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,15 @@ 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, 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..e871be419fbd
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_internal.c
> @@ -0,0 +1,167 @@
> +/*
> + * 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"
> +
> +#define QUIET (__GFP_NORETRY | __GFP_NOWARN)
> +
> +static void internal_free_pages(struct sg_table *st)
> +{
> +	struct scatterlist *sg;
> +
> +	for (sg = st->sgl; sg; sg = __sg_next(sg))
> +		__free_pages(sg_page(sg), get_order(sg->length));
> +
> +	sg_free_table(st);
> +	kfree(st);
> +}
> +
> +static int 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;
> +	int max_order;
> +	gfp_t gfp;
> +
> +	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;
> +
> +	max_order = MAX_ORDER;
> +#ifdef CONFIG_SWIOTLB
> +	if (swiotlb_nr_tbl())
> +		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
> +#endif

I couldn't figure out what IO_TLB_SEGSIZE actually is in some minutes of 
cross-referencing. Did not seem to be in units of bytes according to 
swiotlb.h.

In either case my question is - why use different parameters than 
swiotlb_max_size you recently added to i915_gem.c?

> +
> +	gfp = GFP_KERNEL | __GFP_HIGHMEM | __GFP_RECLAIMABLE;
> +	if (IS_CRESTLINE(i915) || IS_BROADWATER(i915)) {
> +		/* 965gm cannot relocate objects above 4GiB. */
> +		gfp &= ~__GFP_HIGHMEM;
> +		gfp |= __GFP_DMA32;
> +	}
> +
> +	do {
> +		int order = min(fls(npages) - 1, max_order);
> +		struct page *page;
> +
> +		do {
> +			page = alloc_pages(gfp | (order ? QUIET : 0), order);
> +			if (page)
> +				break;
> +			if (!order--)
> +				goto err;
> +
> +			/* Limit subsequent allocations as well */
> +			max_order = order;
> +		} while (1);
> +
> +		sg_set_page(sg, page, PAGE_SIZE << order, 0);
> +		st->nents++;
> +
> +		npages -= 1 << order;
> +		if (!npages) {
> +			sg_mark_end(sg);
> +			break;
> +		}
> +
> +		sg = __sg_next(sg);
> +	} while (1);
> +	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)
> +{
> +	i915_gem_gtt_finish_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_i915_private *i915,
> +				unsigned int size)
> +{
> +	struct drm_i915_gem_object *obj;
> +
> +	obj = i915_gem_object_alloc(&i915->drm);
> +	if (!obj)
> +		return ERR_PTR(-ENOMEM);
> +
> +	drm_gem_private_object_init(&i915->drm, &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(i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
> +
> +	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 e7c3dbcc6c81..217e0b58b930 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, 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 2dc94812bea5..c7ac7e26f2dc 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -264,7 +264,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, 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 e29e84813234..2ce28d41f405 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1784,9 +1784,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, 4096);
>  	if (IS_ERR(obj)) {
>  		DRM_ERROR("Failed to allocate status page\n");
>  		return PTR_ERR(obj);
> @@ -1819,15 +1820,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;
> +	}
> +
>  	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);
>
>  	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

Chris Wilson Oct. 20, 2016, 8:36 p.m. UTC | #2

On Thu, Oct 20, 2016 at 05:22:23PM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/2016 16:03, 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).
> 
> And the status page.

Yeah, I was just thinking of the runtime allocated blocks where the
change can be measured.

> >+	max_order = MAX_ORDER;
> >+#ifdef CONFIG_SWIOTLB
> >+	if (swiotlb_nr_tbl())
> >+		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
> >+#endif
> 
> I couldn't figure out what IO_TLB_SEGSIZE actually is in some
> minutes of cross-referencing. Did not seem to be in units of bytes
> according to swiotlb.h.

Pages.
 
> In either case my question is - why use different parameters than
> swiotlb_max_size you recently added to i915_gem.c?

I was trying to exploit the compile time constants, and I did not care
to grow the search for even higher orders.
-Chris

Tvrtko Ursulin Oct. 21, 2016, 7:21 a.m. UTC | #3

On 20/10/2016 21:36, Chris Wilson wrote:
> On Thu, Oct 20, 2016 at 05:22:23PM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/2016 16:03, 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).
>>
>> And the status page.
>
> Yeah, I was just thinking of the runtime allocated blocks where the
> change can be measured.
>
>>> +	max_order = MAX_ORDER;
>>> +#ifdef CONFIG_SWIOTLB
>>> +	if (swiotlb_nr_tbl())
>>> +		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
>>> +#endif
>>
>> I couldn't figure out what IO_TLB_SEGSIZE actually is in some
>> minutes of cross-referencing. Did not seem to be in units of bytes
>> according to swiotlb.h.
>
> Pages.
>
>> In either case my question is - why use different parameters than
>> swiotlb_max_size you recently added to i915_gem.c?
>
> I was trying to exploit the compile time constants, and I did not care
> to grow the search for even higher orders.

Sorry pages, but it is not AFAICS, but it is units of IO_TLB_SHIFT size 
which is 2k. Happens to be fine since it is smaller (5) than MAX_ORDER 
(11), however to me it still looks like a unrelated number.

ilog2(IO_TLB_SEGSIZE) + 1 would be the same units. It would still look 
arbitrary but I suppose it would be passable.

Regards,

Tvrtko

Chris Wilson Oct. 21, 2016, 7:50 a.m. UTC | #4

On Fri, Oct 21, 2016 at 08:21:08AM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/2016 21:36, Chris Wilson wrote:
> >On Thu, Oct 20, 2016 at 05:22:23PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 20/10/2016 16:03, 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).
> >>
> >>And the status page.
> >
> >Yeah, I was just thinking of the runtime allocated blocks where the
> >change can be measured.
> >
> >>>+	max_order = MAX_ORDER;
> >>>+#ifdef CONFIG_SWIOTLB
> >>>+	if (swiotlb_nr_tbl())
> >>>+		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
> >>>+#endif
> >>
> >>I couldn't figure out what IO_TLB_SEGSIZE actually is in some
> >>minutes of cross-referencing. Did not seem to be in units of bytes
> >>according to swiotlb.h.
> >
> >Pages.
> >
> >>In either case my question is - why use different parameters than
> >>swiotlb_max_size you recently added to i915_gem.c?
> >
> >I was trying to exploit the compile time constants, and I did not care
> >to grow the search for even higher orders.
> 
> Sorry pages, but it is not AFAICS, but it is units of IO_TLB_SHIFT
> size which is 2k. Happens to be fine since it is smaller (5) than
> MAX_ORDER (11), however to me it still looks like a unrelated
> number.
> 
> ilog2(IO_TLB_SEGSIZE) + 1 would be the same units. It would still
> look arbitrary but I suppose it would be passable.

Oh, no! I do remember that. I guess that shows the importance of
comments since I lost that fact during the chase to find a way of
writing fls() as a compiletime constant.

Let's

/* convert swiotlb segment size into sensible units! */ 
#define IO_TLB_SEGPAGES (IO_TLB_SEGSIZE << IO_TLB_SHIFT >> PAGE_SHIFT)

-Chris

Tvrtko Ursulin Oct. 21, 2016, 7:53 a.m. UTC | #5

On 21/10/2016 08:50, Chris Wilson wrote:
> On Fri, Oct 21, 2016 at 08:21:08AM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/2016 21:36, Chris Wilson wrote:
>>> On Thu, Oct 20, 2016 at 05:22:23PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 20/10/2016 16:03, 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).
>>>>
>>>> And the status page.
>>>
>>> Yeah, I was just thinking of the runtime allocated blocks where the
>>> change can be measured.
>>>
>>>>> +	max_order = MAX_ORDER;
>>>>> +#ifdef CONFIG_SWIOTLB
>>>>> +	if (swiotlb_nr_tbl())
>>>>> +		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
>>>>> +#endif
>>>>
>>>> I couldn't figure out what IO_TLB_SEGSIZE actually is in some
>>>> minutes of cross-referencing. Did not seem to be in units of bytes
>>>> according to swiotlb.h.
>>>
>>> Pages.
>>>
>>>> In either case my question is - why use different parameters than
>>>> swiotlb_max_size you recently added to i915_gem.c?
>>>
>>> I was trying to exploit the compile time constants, and I did not care
>>> to grow the search for even higher orders.
>>
>> Sorry pages, but it is not AFAICS, but it is units of IO_TLB_SHIFT
>> size which is 2k. Happens to be fine since it is smaller (5) than
>> MAX_ORDER (11), however to me it still looks like a unrelated
>> number.
>>
>> ilog2(IO_TLB_SEGSIZE) + 1 would be the same units. It would still
>> look arbitrary but I suppose it would be passable.
>
> Oh, no! I do remember that. I guess that shows the importance of
> comments since I lost that fact during the chase to find a way of
> writing fls() as a compiletime constant.
>
> Let's
>
> /* convert swiotlb segment size into sensible units! */
> #define IO_TLB_SEGPAGES (IO_TLB_SEGSIZE << IO_TLB_SHIFT >> PAGE_SHIFT)

Thats even better, R-b on that. :)

Regards,

Tvrtko

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 612340097f4b..7faa04c91e1a 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 4e93c3797d90..e267e20bdcdb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3543,6 +3543,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_i915_private *dev_priv,
+				unsigned int size);
+
 /* 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..aa4e1e043b4e 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,15 @@  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, 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..e871be419fbd
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gem_internal.c
@@ -0,0 +1,167 @@ 
+/*
+ * 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"
+
+#define QUIET (__GFP_NORETRY | __GFP_NOWARN)
+
+static void internal_free_pages(struct sg_table *st)
+{
+	struct scatterlist *sg;
+
+	for (sg = st->sgl; sg; sg = __sg_next(sg))
+		__free_pages(sg_page(sg), get_order(sg->length));
+
+	sg_free_table(st);
+	kfree(st);
+}
+
+static int 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;
+	int max_order;
+	gfp_t gfp;
+
+	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;
+
+	max_order = MAX_ORDER;
+#ifdef CONFIG_SWIOTLB
+	if (swiotlb_nr_tbl())
+		max_order = min(max_order, ilog2(IO_TLB_SEGSIZE));
+#endif
+
+	gfp = GFP_KERNEL | __GFP_HIGHMEM | __GFP_RECLAIMABLE;
+	if (IS_CRESTLINE(i915) || IS_BROADWATER(i915)) {
+		/* 965gm cannot relocate objects above 4GiB. */
+		gfp &= ~__GFP_HIGHMEM;
+		gfp |= __GFP_DMA32;
+	}
+
+	do {
+		int order = min(fls(npages) - 1, max_order);
+		struct page *page;
+
+		do {
+			page = alloc_pages(gfp | (order ? QUIET : 0), order);
+			if (page)
+				break;
+			if (!order--)
+				goto err;
+
+			/* Limit subsequent allocations as well */
+			max_order = order;
+		} while (1);
+
+		sg_set_page(sg, page, PAGE_SIZE << order, 0);
+		st->nents++;
+
+		npages -= 1 << order;
+		if (!npages) {
+			sg_mark_end(sg);
+			break;
+		}
+
+		sg = __sg_next(sg);
+	} while (1);
+	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)
+{
+	i915_gem_gtt_finish_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_i915_private *i915,
+				unsigned int size)
+{
+	struct drm_i915_gem_object *obj;
+
+	obj = i915_gem_object_alloc(&i915->drm);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	drm_gem_private_object_init(&i915->drm, &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(i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
+
+	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 e7c3dbcc6c81..217e0b58b930 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, 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 2dc94812bea5..c7ac7e26f2dc 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -264,7 +264,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, 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 e29e84813234..2ce28d41f405 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1784,9 +1784,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, 4096);
 	if (IS_ERR(obj)) {
 		DRM_ERROR("Failed to allocate status page\n");
 		return PTR_ERR(obj);
@@ -1819,15 +1820,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;
+	}
+
 	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);
 
 	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;