[PATCH 1/2] drm/i915/gvt: Factor out audit and shadow from workload dispatch

[Gao, Ping A]

Thanks Zhi,  I will address your comments in the next version.

On 2017/6/5 16:33, Wang, Zhi A wrote:
> Better move context pin out of intel_gvt_audit_and_shadow_workload(). It 
> looks a bit weird that only context unpin is out there while context pin 
> is in another function.
>
> On 05/24/17 20:49, Ping Gao wrote:
>> The workload audit and shadow could execute in earlier ELSP
>> writing stage for performance consideration, it's need to factor
>> out it from workload dispatch first.
>>
> To perform the workload scan and shadow in ELSP writing stage for 
> performance consideration, the workload scan and shadow stuffs should be 
> factored out from dispatch_workload().
>
>> Signed-off-by: Ping Gao <ping.a.gao at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/gvt.h       |  2 ++
>>   drivers/gpu/drm/i915/gvt/scheduler.c | 44 ++++++++++++++++++++++++------------
>>   2 files changed, 32 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index 07ecd73..accd9c4 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -455,6 +455,8 @@ int intel_vgpu_init_opregion(struct intel_vgpu *vgpu, u32 gpa);
>>   int intel_vgpu_emulate_opregion_request(struct intel_vgpu *vgpu, u32 swsci);
>>   void populate_pvinfo_page(struct intel_vgpu *vgpu);
>>
>> +int intel_gvt_audit_and_shadow_workload(struct intel_vgpu_workload *workload);
>> +
> Looks we can figure out a better name here. The actual work (scan and 
> shadow) is done in intel_gvt_scan_and_shadow_workload()/cmd_parser.c. 
> Maybe we can rename it 
> (intel_gvt_scan_and_shadow_workload()/cmd_parser.c) to 
> intel_gvt_scan_and_shadow_ringbuffer()/cmd_parser.c, as its work is 
> actually in ring buffer level. While the work we did here should be 
> workload level (lrc context/wa context/ring buffer), you can use the 
> name (intel_gvt_scan_and_shadow_workload()/scheduler.c) here.
>
> Do not forget to add some special comments for doxygen while you expose 
> an API. :P
>
>>   struct intel_gvt_ops {
>>   	int (*emulate_cfg_read)(struct intel_vgpu *, unsigned int, void *,
>>   				unsigned int);
>> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
>> index e63b1d8..d6bfdfe 100644
>> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
>> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
>> @@ -172,7 +172,8 @@ static int shadow_context_status_change(struct notifier_block *nb,
>>   	return NOTIFY_OK;
>>   }
>>
>> -static int dispatch_workload(struct intel_vgpu_workload *workload)
>> +
>> +int intel_gvt_audit_and_shadow_workload(struct intel_vgpu_workload *workload)
>>   {
>>   	int ring_id = workload->ring_id;
>>   	struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>> @@ -183,15 +184,10 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>>   	struct intel_ring *ring;
>>   	int ret;
>>
>> -	gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
>> -		ring_id, workload);
>> -
>>   	shadow_ctx->desc_template &= ~(0x3 << GEN8_CTX_ADDRESSING_MODE_SHIFT);
>>   	shadow_ctx->desc_template |= workload->ctx_desc.addressing_mode <<
>>   				    GEN8_CTX_ADDRESSING_MODE_SHIFT;
>>
>> -	mutex_lock(&dev_priv->drm.struct_mutex);
>> -
>>   	/* pin shadow context by gvt even the shadow context will be pinned
>>   	 * when i915 alloc request. That is because gvt will update the guest
>>   	 * context from shadow context when workload is completed, and at that
>> @@ -234,6 +230,29 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>>   	if (ret)
>>   		goto out;
>>
>> +out:
>> +	workload->status = ret;
>> +	return ret;
>> +}
>> +
>> +
>> +static int dispatch_workload(struct intel_vgpu_workload *workload)
>> +{
>> +	int ring_id = workload->ring_id;
>> +	struct i915_gem_context *shadow_ctx = workload->vgpu->shadow_ctx;
>> +	struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
>> +	struct intel_engine_cs *engine = dev_priv->engine[ring_id];
>> +	int ret = 0;
>> +
>> +	gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
>> +		ring_id, workload);
>> +
>> +	mutex_lock(&dev_priv->drm.struct_mutex);
>> +
>> +	ret = intel_gvt_audit_and_shadow_workload(workload);
>> +	if (ret)
>> +		goto out;
>> +
>>   	if (workload->prepare) {
>>   		ret = workload->prepare(workload);
>>   		if (ret)
>> @@ -243,16 +262,13 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>>   	gvt_dbg_sched("ring id %d submit workload to i915 %p\n",
>>   			ring_id, workload->req);
>>
>> -	ret = 0;
>> -	workload->dispatched = true;
>>   out:
>> -	if (ret)
>> -		workload->status = ret;
>> -
>> -	if (!IS_ERR_OR_NULL(rq))
>> -		i915_add_request(rq);
>> -	else
>> +	if (ret) {
>>   		engine->context_unpin(engine, shadow_ctx);
>> +	} else {
>> +		i915_add_request(workload->req);
>> +		workload->dispatched = true;
>> +	}
>>
>>   	mutex_unlock(&dev_priv->drm.struct_mutex);
>>   	return ret;
>>