[Intel-gfx] [PATCH v6 5/5] drm/i915: Tidy up execbuffer command parsing code
Michael H. Nguyen
michael.h.nguyen at intel.com
Thu Dec 11 10:56:09 PST 2014
On 12/11/2014 05:49 AM, Bloomfield, Jon wrote:
>
>
>> -----Original Message-----
>> From: Nguyen, Michael H
>> Sent: Monday, December 08, 2014 10:34 PM
>> To: intel-gfx at lists.freedesktop.org
>> Cc: Bloomfield, Jon; Brad Volkin
>> Subject: [PATCH v6 5/5] drm/i915: Tidy up execbuffer command parsing code
>>
>> From: Brad Volkin <bradley.d.volkin at intel.com>
>>
>> Move it to a separate function since the main do_execbuffer function
>> already has so much going on.
>>
>> v2:
>> - Move pin/unpin calls inside i915_parse_cmds() (Chris W, v4 7/7
>> feedback)
>>
>> Issue: VIZ-4719
>> Signed-off-by: Brad Volkin <bradley.d.volkin at intel.com>
>>
>> Conflicts:
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> ---
>> drivers/gpu/drm/i915/i915_cmd_parser.c | 8 ++
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 126 ++++++++++++++++---
>> ----------
>> 2 files changed, 77 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> index 118079d..80b1b28 100644
>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> @@ -1042,10 +1042,17 @@ int i915_parse_cmds(struct intel_engine_cs
>> *ring,
>> struct drm_i915_cmd_descriptor default_desc = { 0 };
>> bool oacontrol_set = false; /* OACONTROL tracking. See
>> check_cmd() */
>>
>> + ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>> + if (ret) {
>> + DRM_DEBUG_DRIVER("CMD: Failed to pin shadow batch\n");
>> + return -1;
>> + }
>> +
>> batch_base = copy_batch(shadow_batch_obj, batch_obj,
>> batch_start_offset, batch_len);
>> if (IS_ERR(batch_base)) {
>> DRM_DEBUG_DRIVER("CMD: Failed to copy batch\n");
>> + i915_gem_object_ggtt_unpin(shadow_batch_obj);
>> return PTR_ERR(batch_base);
>> }
>>
>> @@ -1116,6 +1123,7 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
>> }
>>
>> vunmap(batch_base);
>> + i915_gem_object_ggtt_unpin(shadow_batch_obj);
>>
>> return ret;
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 2071938..3d36465 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1069,6 +1069,65 @@ i915_emit_box(struct intel_engine_cs *ring,
>> return 0;
>> }
>>
>> +static struct drm_i915_gem_object*
>> +i915_gem_execbuffer_parse(struct intel_engine_cs *ring,
>> + struct drm_i915_gem_exec_object2
>> *shadow_exec_entry,
>> + struct eb_vmas *eb,
>> + struct drm_i915_gem_object *batch_obj,
>> + u32 batch_start_offset,
>> + u32 batch_len,
>> + bool is_master,
>> + u32 *flags)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(batch_obj->base.dev);
>> + struct drm_i915_gem_object *shadow_batch_obj;
>> + int ret;
>> +
>> + shadow_batch_obj = i915_gem_batch_pool_get(&dev_priv-
>>> mm.batch_pool,
>> + batch_obj->base.size);
>> + if (IS_ERR(shadow_batch_obj))
>> + return shadow_batch_obj;
>> +
>> + ret = i915_parse_cmds(ring,
>> + batch_obj,
>> + shadow_batch_obj,
>> + batch_start_offset,
>> + batch_len,
>> + is_master);
>> + if (ret) {
>> + if (ret == -EACCES)
>> + return batch_obj;
>
> Shouldn't this be returning an error code ?
Good question. -EACCESS tells the caller of i915_parse_cmds() of a
special case. There are some comments in its fnc header and implementation.
-EACCESS indicates that batch_obj contains a chained batch in which case
we dispatch batch_obj as non-secure. We return batch_obj here b/c the
logic below doesn't apply. I believe the reason we safely dispatch
chained batches as non-secure is b/c Brad and others probably determined
that there weren't any valid use cases where chained batches needed to
be dispatched as secure. Leaving them non-secure, HW will NOP bad cmds
and its not necessary for i915 to do parsing.
Thanks,
Mike
>
>
>
>> + } else {
>> + struct i915_vma *vma;
>> +
>> + memset(shadow_exec_entry, 0,
>> sizeof(*shadow_exec_entry));
>> +
>> + vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>> + vma->exec_entry = shadow_exec_entry;
>> + vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
>> + drm_gem_object_reference(&shadow_batch_obj->base);
>> + list_add_tail(&vma->exec_list, &eb->vmas);
>> +
>> + shadow_batch_obj->base.pending_read_domains =
>> + batch_obj->base.pending_read_domains;
>> +
>> + /*
>> + * Set the DISPATCH_SECURE bit to remove the
>> NON_SECURE
>> + * bit from MI_BATCH_BUFFER_START commands issued in
>> the
>> + * dispatch_execbuffer implementations. We specifically
>> + * don't want that set when the command parser is
>> + * enabled.
>> + *
>> + * FIXME: with aliasing ppgtt, buffers that should only
>> + * be in ggtt still end up in the aliasing ppgtt. remove
>> + * this check when that is fixed.
>> + */
>> + if (USES_FULL_PPGTT(dev))
>> + *flags |= I915_DISPATCH_SECURE;
>> + }
>> +
>> + return ret ? ERR_PTR(ret) : shadow_batch_obj; }
>>
>> int
>> i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file
>> *file, @@ -1286,7 +1345,6 @@ i915_gem_do_execbuffer(struct drm_device
>> *dev, void *data,
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct eb_vmas *eb;
>> struct drm_i915_gem_object *batch_obj;
>> - struct drm_i915_gem_object *shadow_batch_obj = NULL;
>> struct drm_i915_gem_exec_object2 shadow_exec_entry;
>> struct intel_engine_cs *ring;
>> struct intel_context *ctx;
>> @@ -1406,63 +1464,17 @@ i915_gem_do_execbuffer(struct drm_device
>> *dev, void *data,
>> }
>>
>> if (i915_needs_cmd_parser(ring)) {
>> - shadow_batch_obj =
>> - i915_gem_batch_pool_get(&dev_priv-
>>> mm.batch_pool,
>> - batch_obj->base.size);
>> - if (IS_ERR(shadow_batch_obj)) {
>> - ret = PTR_ERR(shadow_batch_obj);
>> - /* Don't try to clean up the obj in the error path */
>> - shadow_batch_obj = NULL;
>> - goto err;
>> - }
>> -
>> - shadow_batch_obj->madv = I915_MADV_WILLNEED;
>> -
>> - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>> - if (ret)
>> + batch_obj = i915_gem_execbuffer_parse(ring,
>> + &shadow_exec_entry,
>> + eb,
>> + batch_obj,
>> + args->batch_start_offset,
>> + args->batch_len,
>> + file->is_master,
>> + &flags);
>> + if (IS_ERR(batch_obj)) {
>> + ret = PTR_ERR(batch_obj);
>> goto err;
>> -
>> - ret = i915_parse_cmds(ring,
>> - batch_obj,
>> - shadow_batch_obj,
>> - args->batch_start_offset,
>> - args->batch_len,
>> - file->is_master);
>> - i915_gem_object_ggtt_unpin(shadow_batch_obj);
>> -
>> - if (ret) {
>> - if (ret != -EACCES)
>> - goto err;
>> - } else {
>> - struct i915_vma *vma;
>> -
>> - memset(&shadow_exec_entry, 0,
>> - sizeof(shadow_exec_entry));
>> -
>> - vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>> - vma->exec_entry = &shadow_exec_entry;
>> - vma->exec_entry->flags =
>> __EXEC_OBJECT_PURGEABLE;
>> - drm_gem_object_reference(&shadow_batch_obj-
>>> base);
>> - list_add_tail(&vma->exec_list, &eb->vmas);
>> -
>> - shadow_batch_obj->base.pending_read_domains =
>> - batch_obj->base.pending_read_domains;
>> -
>> - batch_obj = shadow_batch_obj;
>> -
>> - /*
>> - * Set the DISPATCH_SECURE bit to remove the
>> NON_SECURE
>> - * bit from MI_BATCH_BUFFER_START commands
>> issued in the
>> - * dispatch_execbuffer implementations. We
>> specifically
>> - * don't want that set when the command parser is
>> - * enabled.
>> - *
>> - * FIXME: with aliasing ppgtt, buffers that should only
>> - * be in ggtt still end up in the aliasing ppgtt. remove
>> - * this check when that is fixed.
>> - */
>> - if (USES_FULL_PPGTT(dev))
>> - flags |= I915_DISPATCH_SECURE;
>> }
>> }
>>
>> --
>> 1.9.1
>
More information about the Intel-gfx
mailing list