Re: [PATCH v6 5/5] drm/i915: Tidy up execbuffer command parsing code

["Michael H. Nguyen" <michael.h.nguyen@intel.com>]

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@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@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@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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx