[Intel-gfx] [PATCH v4 7/7] drm/i915: Tidy up execbuffer command parsing code

[Michael H. Nguyen]

Hi Chris,

>>
>> +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;
>> +
>> +	shadow_batch_obj->madv = I915_MADV_WILLNEED;
>> +
>> +	ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>> +	if (ret)
>> +		goto err;
>
> Pardon? This feels an implementation issue of i915_parse_cmds() and should
> be resolved there. Presumably you are not actually reading back through
> the GTT? That would be insane...
>
>> +	ret = i915_parse_cmds(ring,
>> +			      batch_obj,
>> +			      shadow_batch_obj,
>> +			      batch_start_offset,
>> +			      batch_len,
>> +			      is_master);
>> +	i915_gem_object_ggtt_unpin(shadow_batch_obj);
>
> Yet pin+unpin around the parser seems to serve no other purpose.
Are you suggesting to remove the pin/unpin calls? If so, isn't pinning 
needed to ensure the backing store pages are available in vmap_batch()? 
i.e. obj->pages->sgl is populated w/ physical pages.

Or, are you suggesting to move the pin/unpin calls inside 
i915_parse_cmds() ?

Thx,
Mike
> -Chris
>