[PATCH] drm/i915/gvt: Add the support of HUC_STATUS2 reg emulation for Guest VGPU

[Zhao, Yakui]

On 2018年03月27日 13:07, Zhenyu Wang wrote:
> On 2018.03.27 13:07:51 +0800, yzhao3 wrote:
>> On 2018年03月27日 11:18, Zhenyu Wang wrote:
>>> On 2018.03.27 09:19:03 +0800, yakui.zhao at intel.com wrote:
>>>> From: Zhao Yakui <yakui.zhao at intel.com>
>>>>
>>>> The HUC_STATUS2 reg is used to indicate whether the Huc FW is loaded. Only
>>>> when it is loaded successfully, the user-space driver on guest can use the
>>>> Huc to do the expected operation. This provides the support of HUC_STATUS2
>>>> trap for guest VGPU.
>>>>
>>>
>>> As we don't support huc for guest now, what scenario does this fix?
>>> And for future huc support, I think we can have some short-cut in guest
>>> driver e.g skip fw load, get status from pvinfo, etc.
>>>
>> The user-space driver only uses the I915_PARAM_HUC_STATUS ioctl to check
>> whether the Huc can be used(I915_PARAM_HUC_STATUS will read the HUC_STATUS2
>> register). Only when one bit is enabled, it can submit the GPU commands
>> related with Huc operation.
>>
>> The current issue is that the HUC_STATUS2 reg on guest doesn't report the
>> required bits and then it can't continue the Huc operation although the Huc
>> is supported on gvt-g. If it is reported as expected, the guest can send the
>> GPU commands related with Huc operation.
>>
> 
> ok, I was thinking huc commands require guc enabled, but seems not true,
> so we do can run huc command by execlist without guc. And given current way of
> i915 huc check, we do can expose huc capability. btw, besides HUC_STATUS2 are
> there other registers need to be handled for huc state?

Currently only the HUC_STATUS2 reg is needed for the querying Huc_state.

> 
> Some code comment below.
> 
>> For the future support of Huc/Guc, I agree with your point. The loading of
>> Guc/Huc FW should be skipped on guest VGPU. And based on the status of
>> Guc/Huc from gvt-g, the GPU commands related with Guc/Huc can be submitted
>> and then mediated to gvt-g.
>>
>>>> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/handlers.c | 1 +
>>>>    drivers/gpu/drm/i915/gvt/mmio.c     | 6 ++++++
>>>>    2 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> index a33c1c3e..465b7f6 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>>> @@ -2867,6 +2867,7 @@ static int init_skl_mmio_info(struct intel_gvt *gvt)
>>>>    	MMIO_D(_MMIO(0x4ab8), D_KBL);
>>>>    	MMIO_D(_MMIO(0x2248), D_SKL_PLUS | D_KBL);
>>>> +	MMIO_D(HUC_STATUS2, D_GEN9PLUS);
> 
> We'd better have a real handler for that instead of only reading at reset time,
> as we shouldn't depend on any code sequence for gvt reset/init vs. huc loading,
> but should always check actual state.
>

It is also ok to add one real handler for HUC_STATUS2 reg.

But in fact as the Huc FW is initialized/loaded only once in course of 
driver loading and the HUC_STATUS2 reg is read-only driver for the 
driver, so it is initialized for vgpu reset/init.
If the handler is preferred, I will add it.

 >
>>>>    	return 0;
>>>>    }
>>>> diff --git a/drivers/gpu/drm/i915/gvt/mmio.c b/drivers/gpu/drm/i915/gvt/mmio.c
>>>> index 11b71b3..7cf972d 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/mmio.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/mmio.c
>>>> @@ -235,6 +235,7 @@ void intel_vgpu_reset_mmio(struct intel_vgpu *vgpu, bool dmlr)
>>>>    	struct intel_gvt *gvt = vgpu->gvt;
>>>>    	const struct intel_gvt_device_info *info = &gvt->device_info;
>>>>    	void  *mmio = gvt->firmware.mmio;
>>>> +	struct drm_i915_private *dev_priv = gvt->dev_priv;
>>>>    	if (dmlr) {
>>>>    		memcpy(vgpu->mmio.vreg, mmio, info->mmio_size);
>>>> @@ -256,6 +257,11 @@ void intel_vgpu_reset_mmio(struct intel_vgpu *vgpu, bool dmlr)
>>>>    		memcpy(vgpu->mmio.sreg, mmio, GVT_GEN8_MMIO_RESET_OFFSET);
>>>>    	}
>>>> +	if (HAS_HUC_UCODE(dev_priv)) {
> 
> maybe use USES_HUC()? so only read hw value when huc is loaded, otherwise can just skip.

The status of Huc loading only depends on the value of HUC_STATUS2 reg. 
(The dev_priv->huc_fw.load_status can't reflect the status of Huc as it 
needs the Guc authentication).

So it is checked on the platforms that supports the Guc/Huc.
How do you think?

> 
>>>> +		mmio_hw_access_pre(dev_priv);
>>>> +		vgpu_vreg(vgpu, HUC_STATUS2) = I915_READ(HUC_STATUS2);
>>>> +		mmio_hw_access_post(dev_priv);
>>>> +	}
>>>>    }
>>>>    /**
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> intel-gvt-dev mailing list
>>>> intel-gvt-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>>>
> 
> 
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>