[Intel-gfx] [PATCH v6 1/5] drm/i915: Implement a framework for batch buffer pools
Michael H. Nguyen
michael.h.nguyen at intel.com
Tue Dec 9 11:42:40 PST 2014
On 12/09/2014 05:18 AM, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 02:33:46PM -0800, michael.h.nguyen at intel.com wrote:
>> From: Brad Volkin <bradley.d.volkin at intel.com>
>>
>> This adds a small module for managing a pool of batch buffers.
>> The only current use case is for the command parser, as described
>> in the kerneldoc in the patch. The code is simple, but separating
>> it out makes it easier to change the underlying algorithms and to
>> extend to future use cases should they arise.
>>
>> The interface is simple: init to create an empty pool, fini to
>> clean it up, get to obtain a new buffer. Note that all buffers are
>> expected to be inactive before cleaning up the pool.
>>
>> Locking is currently based on the caller holding the struct_mutex.
>> We already do that in the places where we will use the batch pool
>> for the command parser.
>>
>> v2:
>> - s/BUG_ON/WARN_ON/ for locking assertions
>> - Remove the cap on pool size
>> - Switch from alloc/free to init/fini
>>
>> v3:
>> - Idiomatic looping structure in _fini
>> - Correct handling of purged objects
>> - Don't return a buffer that's too much larger than needed
>>
>> v4:
>> - Rebased to latest -nightly
>>
>> v5:
>> - Remove _put() function and clean up comments to match
>>
>> v6:
>> - Move purged check inside the loop (danvet, from v4 1/7 feedback)
>>
>> v7:
>> - Use single list instead of two. (Chris W)
>> - s/active_list/cache_list
>> - Squashed in debug patches (Chris W)
>> drm/i915: Add a batch pool debugfs file
>>
>> It provides some useful information about the buffers in
>> the global command parser batch pool.
>>
>> v2: rebase on global pool instead of per-ring pools
>> v3: rebase
>>
>> drm/i915: Add batch pool details to i915_gem_objects debugfs
>>
>> To better account for the potentially large memory consumption
>> of the batch pool.
>>
>> Issue: VIZ-4719
>> Signed-off-by: Brad Volkin <bradley.d.volkin at intel.com>
>> ---
>> Documentation/DocBook/drm.tmpl | 5 ++
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/i915_debugfs.c | 71 ++++++++++++++--
>> drivers/gpu/drm/i915/i915_drv.h | 21 +++++
>> drivers/gpu/drm/i915/i915_gem.c | 1 +
>> drivers/gpu/drm/i915/i915_gem_batch_pool.c | 132 +++++++++++++++++++++++++++++
>> 6 files changed, 222 insertions(+), 9 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/i915_gem_batch_pool.c
>>
>> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> index 85287cb..022923a 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -4029,6 +4029,11 @@ int num_ioctls;</synopsis>
>> !Idrivers/gpu/drm/i915/i915_cmd_parser.c
>> </sect2>
>> <sect2>
>> + <title>Batchbuffer Pools</title>
>> +!Pdrivers/gpu/drm/i915/i915_gem_batch_pool.c batch pool
>> +!Idrivers/gpu/drm/i915/i915_gem_batch_pool.c
>> + </sect2>
>> + <sect2>
>> <title>Logical Rings, Logical Ring Contexts and Execlists</title>
>> !Pdrivers/gpu/drm/i915/intel_lrc.c Logical Rings, Logical Ring Contexts and Execlists
>> !Idrivers/gpu/drm/i915/intel_lrc.c
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e4083e4..c8dbb37d 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -19,6 +19,7 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
>>
>> # GEM code
>> i915-y += i915_cmd_parser.o \
>> + i915_gem_batch_pool.o \
>> i915_gem_context.o \
>> i915_gem_render_state.o \
>> i915_gem_debug.o \
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index d0e445e..3c3bf98 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -359,6 +359,33 @@ static int per_file_stats(int id, void *ptr, void *data)
>> return 0;
>> }
>>
>> +#define print_file_stats(m, name, stats) \
>> + seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu shared, %zu unbound)\n", \
>> + name, \
>> + stats.count, \
>> + stats.total, \
>> + stats.active, \
>> + stats.inactive, \
>> + stats.global, \
>> + stats.shared, \
>> + stats.unbound)
>> +
>> +static void print_batch_pool_stats(struct seq_file *m,
>> + struct drm_i915_private *dev_priv)
>> +{
>> + struct drm_i915_gem_object *obj;
>> + struct file_stats stats;
>> +
>> + memset(&stats, 0, sizeof(stats));
>> +
>> + list_for_each_entry(obj,
>> + &dev_priv->mm.batch_pool.cache_list,
>> + batch_pool_list)
>> + per_file_stats(0, obj, &stats);
>> +
>> + print_file_stats(m, "batch pool", stats);
>> +}
>> +
>> #define count_vmas(list, member) do { \
>> list_for_each_entry(vma, list, member) { \
>> size += i915_gem_obj_ggtt_size(vma->obj); \
>> @@ -441,6 +468,9 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>> dev_priv->gtt.mappable_end - dev_priv->gtt.base.start);
>>
>> seq_putc(m, '\n');
>> + print_batch_pool_stats(m, dev_priv);
>> +
>> + seq_putc(m, '\n');
>> list_for_each_entry_reverse(file, &dev->filelist, lhead) {
>> struct file_stats stats;
>> struct task_struct *task;
>> @@ -458,15 +488,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>> */
>> rcu_read_lock();
>> task = pid_task(file->pid, PIDTYPE_PID);
>> - seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu shared, %zu unbound)\n",
>> - task ? task->comm : "<unknown>",
>> - stats.count,
>> - stats.total,
>> - stats.active,
>> - stats.inactive,
>> - stats.global,
>> - stats.shared,
>> - stats.unbound);
>> + print_file_stats(m, task ? task->comm : "<unknown>", stats);
>> rcu_read_unlock();
>> }
>>
>> @@ -583,6 +605,36 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>> return 0;
>> }
>>
>> +static int i915_gem_batch_pool_info(struct seq_file *m, void *data)
>> +{
>> + struct drm_info_node *node = m->private;
>> + struct drm_device *dev = node->minor->dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct drm_i915_gem_object *obj;
>> + int count = 0;
>> + int ret;
>> +
>> + ret = mutex_lock_interruptible(&dev->struct_mutex);
>> + if (ret)
>> + return ret;
>> +
>> + seq_puts(m, "cache:\n");
>> + list_for_each_entry(obj,
>> + &dev_priv->mm.batch_pool.cache_list,
>> + batch_pool_list) {
>> + seq_puts(m, " ");
>> + describe_obj(m, obj);
>> + seq_putc(m, '\n');
>> + count++;
>> + }
>> +
>> + seq_printf(m, "total: %d\n", count);
>> +
>> + mutex_unlock(&dev->struct_mutex);
>> +
>> + return 0;
>> +}
>> +
>> static int i915_gem_request_info(struct seq_file *m, void *data)
>> {
>> struct drm_info_node *node = m->private;
>> @@ -4324,6 +4376,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>> {"i915_gem_hws_blt", i915_hws_info, 0, (void *)BCS},
>> {"i915_gem_hws_bsd", i915_hws_info, 0, (void *)VCS},
>> {"i915_gem_hws_vebox", i915_hws_info, 0, (void *)VECS},
>> + {"i915_gem_batch_pool", i915_gem_batch_pool_info, 0},
>> {"i915_frequency_info", i915_frequency_info, 0},
>> {"i915_drpc_info", i915_drpc_info, 0},
>> {"i915_emon_status", i915_emon_status, 0},
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 11e85cb..f3e27e9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1129,6 +1129,11 @@ struct intel_l3_parity {
>> int which_slice;
>> };
>>
>> +struct i915_gem_batch_pool {
>> + struct drm_device *dev;
>> + struct list_head cache_list;
>> +};
>> +
>> struct i915_gem_mm {
>> /** Memory allocator for GTT stolen memory */
>> struct drm_mm stolen;
>> @@ -1142,6 +1147,13 @@ struct i915_gem_mm {
>> */
>> struct list_head unbound_list;
>>
>> + /*
>> + * A pool of objects to use as shadow copies of client batch buffers
>> + * when the command parser is enabled. Prevents the client from
>> + * modifying the batch contents after software parsing.
>> + */
>> + struct i915_gem_batch_pool batch_pool;
>> +
>> /** Usable portion of the GTT for GEM */
>> unsigned long stolen_base; /* limited to low memory (32-bit) */
>>
>> @@ -1872,6 +1884,8 @@ struct drm_i915_gem_object {
>> /** Used in execbuf to temporarily hold a ref */
>> struct list_head obj_exec_link;
>>
>> + struct list_head batch_pool_list;
>> +
>> /**
>> * This is set if the object is on the active lists (has pending
>> * rendering and so a non-zero seqno), and is not set if it i s on
>> @@ -2876,6 +2890,13 @@ void i915_destroy_error_state(struct drm_device *dev);
>> void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone);
>> const char *i915_cache_level_str(struct drm_i915_private *i915, int type);
>>
>> +/* i915_gem_batch_pool.c */
>> +void i915_gem_batch_pool_init(struct drm_device *dev,
>> + struct i915_gem_batch_pool *pool);
>> +void i915_gem_batch_pool_fini(struct i915_gem_batch_pool *pool);
>> +struct drm_i915_gem_object*
>> +i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool, size_t size);
>> +
>> /* i915_cmd_parser.c */
>> int i915_cmd_parser_get_version(void);
>> int i915_cmd_parser_init_ring(struct intel_engine_cs *ring);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index de241eb..2f14ae1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4384,6 +4384,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>> INIT_LIST_HEAD(&obj->ring_list);
>> INIT_LIST_HEAD(&obj->obj_exec_link);
>> INIT_LIST_HEAD(&obj->vma_list);
>> + INIT_LIST_HEAD(&obj->batch_pool_list);
>>
>> obj->ops = ops;
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> new file mode 100644
>> index 0000000..e9349e3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Copyright © 2014 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 "i915_drv.h"
>> +
>> +/**
>> + * DOC: batch pool
>> + *
>> + * In order to submit batch buffers as 'secure', the software command parser
>> + * must ensure that a batch buffer cannot be modified after parsing. It does
>> + * this by copying the user provided batch buffer contents to a kernel owned
>> + * buffer from which the hardware will actually execute, and by carefully
>> + * managing the address space bindings for such buffers.
>> + *
>> + * The batch pool framework provides a mechanism for the driver to manage a
>> + * set of scratch buffers to use for this purpose. The framework can be
>> + * extended to support other uses cases should they arise.
>> + */
>> +
>> +/**
>> + * i915_gem_batch_pool_init() - initialize a batch buffer pool
>> + * @dev: the drm device
>> + * @pool: the batch buffer pool
>> + */
>> +void i915_gem_batch_pool_init(struct drm_device *dev,
>> + struct i915_gem_batch_pool *pool)
>> +{
>> + pool->dev = dev;
>> + INIT_LIST_HEAD(&pool->cache_list);
>> +}
>> +
>> +/**
>> + * i915_gem_batch_pool_fini() - clean up a batch buffer pool
>> + * @pool: the pool to clean up
>> + *
>> + * Note: Callers must hold the struct_mutex.
>> + */
>> +void i915_gem_batch_pool_fini(struct i915_gem_batch_pool *pool)
>> +{
>> + WARN_ON(!mutex_is_locked(&pool->dev->struct_mutex));
>> +
>> + while (!list_empty(&pool->cache_list)) {
>> + struct drm_i915_gem_object *obj =
>> + list_first_entry(&pool->cache_list,
>> + struct drm_i915_gem_object,
>> + batch_pool_list);
>> +
>> + WARN_ON(obj->active);
>> +
>> + list_del_init(&obj->batch_pool_list);
>> + drm_gem_object_unreference(&obj->base);
>> + }
>> +}
>> +
>> +/**
>> + * i915_gem_batch_pool_get() - select a buffer from the pool
>> + * @pool: the batch buffer pool
>> + * @size: the minimum desired size of the returned buffer
>> + *
>> + * Finds or allocates a batch buffer in the pool with at least the requested
>> + * size. The caller is responsible for any domain, active/inactive, or
>> + * purgeability management for the returned buffer.
>> + *
>> + * Note: Callers must hold the struct_mutex
>> + *
>> + * Return: the selected batch buffer object
>> + */
>> +struct drm_i915_gem_object *
>> +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;
>> +
>> + WARN_ON(!mutex_is_locked(&pool->dev->struct_mutex));
>> +
>> + list_for_each_entry_safe(tmp, next,
>> + &pool->cache_list, batch_pool_list) {
>> +
>> + if (tmp->active)
>> + continue;
>> +
>> + /* While we're looping, do some clean up */
>> + if (tmp->madv == __I915_MADV_PURGED) {
>> + list_del(&tmp->batch_pool_list);
>> + drm_gem_object_unreference(&tmp->base);
>> + continue;
>> + }
>> +
>> + /*
>> + * Select a buffer that is at least as big as needed
>> + * but not 'too much' bigger. A better way to do this
>> + * might be to bucket the pool objects based on size.
>> + */
>> + if (tmp->base.size >= size &&
>> + tmp->base.size <= (2 * size)) {
>> + obj = tmp;
>> + break;
>> + }
>> + }
>> +
>> + if (!obj) {
>> + obj = i915_gem_alloc_object(pool->dev, size);
>> + if (!obj)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + list_add_tail(&obj->batch_pool_list, &pool->cache_list);
>> + }
>
> Shouldn't we have a else list_move_tail here to keep the list in lru
> order?
Yes!
> -Daniel
>
>> +
>> + return obj;
>> +}
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list