[PATCH] drm/i915/gvt: Skip writing 0 to HWSP during D3 resume

[Colin Xu]

On 2020-09-09 10:04, Zhenyu Wang wrote:
> On 2020.09.09 08:37:53 +0800, Colin Xu wrote:
>> On 2020-09-08 17:15, Zhenyu Wang wrote:
>>> On 2020.08.19 09:09:53 +0800, Colin Xu wrote:
>>>> Guest driver may reset HWSP to 0 as init value during D3->D0:
>>>> The full sequence is:
>>>> - Boot ->D0
>>>> - Update HWSP
>>>> - D0->D3
>>>> - ...In D3 state...
>>>> - D3->D0
>>>> - DMLR reset.
>>>> - Set engine HWSP to 0.
>>>> - Set engine ring mode to 0.
>>>> - Set engine HWSP to correct value.
>>>> - Set engine ring mode to correct value.
>>>> Ring mode is masked register so set 0 won't take effect.
>>>> However HWPS addr 0 is considered as invalid GGTT address which will
>>>> report error like:
>>>> gvt: vgpu 1: write invalid HWSP address, reg:0x2080, value:0x0
>>>>
>>> So looks this is to handle that wrong error message but function is
>>> still same, right?
>> Yes that's right. Current logic can guarantee that only correct HWSP write
>> from guest are accepted by GVT. Invalid HWSP address will be dropped and
>> won't be submitted to HW. During resume, correct HWSP will be updated
>> eventually updated. If pv_notified is set, the intermediate HWSP zero-out
>> can be ignored until hit next reset.
> Might just move that error into debug message instead of putting
> effort on tracking guest state without much gain..
Sometimes such invalid HWSP addr write msg may still be helpful. If put 
them into gvt_dbg_mmio without considering different case, may miss the 
information. So this change put real error into gvt_vgpu_err, but put 
expected zero out to gvt_dbg_mmio.
>
>>>> During vGPU in D3, per-engine HWSP gpa remains valid so we can skip
>>>> update HWSP in this case.
>>>> Check both pv_notified and previous engine HWSP gpa, if pv already
>>>> notified and previous HWSP gpa is valid, we skip this HWSP init and
>>>> let later HWSP write update the correct value. We also need zero out
>>>> per-engine HWSP gpa on engine reset to make sure hws_pga is valid.
>>>>
>>>> Signed-off-by: Colin Xu <colin.xu at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/handlers.c | 30 ++++++++++++++++++++---------
>>>>    drivers/gpu/drm/i915/gvt/vgpu.c     |  7 +++++++
>>>>    2 files changed, 28 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> index 840572add2d4..72860aaf1656 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> @@ -1489,12 +1489,6 @@ static int hws_pga_write(struct intel_vgpu *vgpu, unsigned int offset,
>>>>    	const struct intel_engine_cs *engine =
>>>>    		intel_gvt_render_mmio_to_engine(vgpu->gvt, offset);
>>>> -	if (!intel_gvt_ggtt_validate_range(vgpu, value, I915_GTT_PAGE_SIZE)) {
>>>> -		gvt_vgpu_err("write invalid HWSP address, reg:0x%x, value:0x%x\n",
>>>> -			      offset, value);
>>>> -		return -EINVAL;
>>>> -	}
>>>> -
>>>>    	/*
>>>>    	 * Need to emulate all the HWSP register write to ensure host can
>>>>    	 * update the VM CSB status correctly. Here listed registers can
>>>> @@ -1505,9 +1499,27 @@ static int hws_pga_write(struct intel_vgpu *vgpu, unsigned int offset,
>>>>    			     offset);
>>>>    		return -EINVAL;
>>>>    	}
>>>> -	vgpu->hws_pga[engine->id] = value;
>>>> -	gvt_dbg_mmio("VM(%d) write: 0x%x to HWSP: 0x%x\n",
>>>> -		     vgpu->id, value, offset);
>>>> +
>>>> +	if (!intel_gvt_ggtt_validate_range(vgpu, value, I915_GTT_PAGE_SIZE)) {
>>>> +		u32 old = vgpu->hws_pga[engine->id];
>>>> +
>>>> +		/* Skip zero out RING_HWS_PGA during D3 resume */
>>>> +		if (vgpu->pv_notified && value == 0 &&
>>>> +		    intel_gvt_ggtt_validate_range(vgpu, old,
>>>> +						  I915_GTT_PAGE_SIZE)) {
>>>> +			gvt_dbg_mmio("Skip zero out HWSP address, reg:0x%x, "
>>>> +				     "value:0x%x\n", offset, value);
>>>> +
>>>> +		} else {
>>>> +			gvt_vgpu_err("write invalid HWSP address, reg:0x%x, "
>>>> +				     "value:0x%x\n", offset, value);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	} else {
>>>> +		vgpu->hws_pga[engine->id] = value;
>>>> +		gvt_dbg_mmio("VM(%d) write: 0x%x to HWSP: 0x%x\n",
>>>> +			     vgpu->id, value, offset);
>>>> +	}
>>>>    	return intel_vgpu_default_mmio_write(vgpu, offset, &value, bytes);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> index 8fa9b31a2484..e0e073045d83 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> @@ -558,6 +558,9 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>>>    	intel_vgpu_reset_submission(vgpu, resetting_eng);
>>>>    	/* full GPU reset or device model level reset */
>>>>    	if (engine_mask == ALL_ENGINES || dmlr) {
>>>> +		struct intel_engine_cs *engine;
>>>> +		intel_engine_mask_t tmp;
>>>> +
>>>>    		intel_vgpu_select_submission_ops(vgpu, ALL_ENGINES, 0);
>>>>    		if (engine_mask == ALL_ENGINES)
>>>>    			intel_vgpu_invalidate_ppgtt(vgpu);
>>>> @@ -588,6 +591,10 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
>>>>    			else
>>>>    				vgpu->pv_notified = false;
>>>>    		}
>>>> +
>>>> +		for_each_engine_masked(engine, gvt->gt, engine_mask, tmp) {
>>>> +			vgpu->hws_pga[engine->id] = 0;
>>>> +		}
>>>>    	}
>>>>    	vgpu->resetting_eng = 0;
>>>> -- 
>>>> 2.28.0
>>>>
>>>> _______________________________________________
>>>> intel-gvt-dev mailing list
>>>> intel-gvt-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>> -- 
>> Best Regards,
>> Colin Xu
>>
-- 
Best Regards,
Colin Xu