[PATCH] drm/i915/gvt: move intel_runtime_pm_get out of spin_lock in stop_schedule

[Hang Yuan]

On 08/07/2018 10:56 AM, Zhenyu Wang wrote:
> On 2018.07.31 18:05:46 +0800, intel-gvt-dev-bounces at lists.freedesktop.org wrote:
>> From: Hang Yuan <hang.yuan at linux.intel.com>
>>
>> pm_runtime_get_sync in intel_runtime_pm_get might sleep if i915
>> device is not active. When stop vgpu schedule, the device may be
>> inactive. So need to move runtime_pm_get out of spin_lock/unlock.
>>
>> Fixes: b24881e0b0b6("drm/i915/gvt: Add runtime_pm_get/put into gvt_switch_mmio
>> Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
>> Signed-off-by: Xiong Zhang <xiong.y.zhang at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/mmio_context.c | 2 --
>>   drivers/gpu/drm/i915/gvt/sched_policy.c | 3 +++
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/mmio_context.c b/drivers/gpu/drm/i915/gvt/mmio_context.c
>> index 7e702c6..10e63ee 100644
>> --- a/drivers/gpu/drm/i915/gvt/mmio_context.c
>> +++ b/drivers/gpu/drm/i915/gvt/mmio_context.c
>> @@ -549,11 +549,9 @@ void intel_gvt_switch_mmio(struct intel_vgpu *pre,
>>   	 * performace for batch mmio read/write, so we need
>>   	 * handle forcewake mannually.
>>   	 */
>> -	intel_runtime_pm_get(dev_priv);
>>   	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>   	switch_mmio(pre, next, ring_id);
>>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> -	intel_runtime_pm_put(dev_priv);
>>   }
> 
> If removed this, What about other user calling it? Or we need a flag
> to tell if need to get runtime pm when switch mmio?
> 
Henry: What kind of flag do you mean? How about adding some code 
comments to remind this function will access HW so runtime_get is needed 
if device is not active? intel_gvt_switch_mmio is called in atomic 
notification handler as well. So I can't use mutex to protect it in my 
original thought. Then still use spin_lock to protect it, which makes 
intel_runtime_pm_get inside the function buggy when device is not active.

>>   
>>   /**
>> diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c b/drivers/gpu/drm/i915/gvt/sched_policy.c
>> index 09d7bb7..985fe81 100644
>> --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
>> +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
>> @@ -426,6 +426,7 @@ void intel_vgpu_stop_schedule(struct intel_vgpu *vgpu)
>>   		&vgpu->gvt->scheduler;
>>   	int ring_id;
>>   	struct vgpu_sched_data *vgpu_data = vgpu->sched_data;
>> +	struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>>   
>>   	if (!vgpu_data->active)
>>   		return;
>> @@ -444,6 +445,7 @@ void intel_vgpu_stop_schedule(struct intel_vgpu *vgpu)
>>   		scheduler->current_vgpu = NULL;
>>   	}
>>   
>> +	intel_runtime_pm_get(dev_priv);
>>   	spin_lock_bh(&scheduler->mmio_context_lock);
>>   	for (ring_id = 0; ring_id < I915_NUM_ENGINES; ring_id++) {
>>   		if (scheduler->engine_owner[ring_id] == vgpu) {
>> @@ -452,5 +454,6 @@ void intel_vgpu_stop_schedule(struct intel_vgpu *vgpu)
>>   		}
>>   	}
>>   	spin_unlock_bh(&scheduler->mmio_context_lock);
>> +	intel_runtime_pm_put(dev_priv);
>>   	mutex_unlock(&vgpu->gvt->sched_lock);
>>   }