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

Submitted by Zhao Yakui on March 27, 2018, 1:19 a.m.

Details

Message ID 1522113543-31879-1-git-send-email-yakui.zhao@intel.com
State New
Headers show
Series "drm/i915/gvt: Add the support of HUC_STATUS2 reg emulation for Guest VGPU" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Zhao Yakui March 27, 2018, 1:19 a.m.
From: Zhao Yakui <yakui.zhao@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.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 drivers/gpu/drm/i915/gvt/handlers.c | 1 +
 drivers/gpu/drm/i915/gvt/mmio.c     | 6 ++++++
 2 files changed, 7 insertions(+)

Patch hide | download patch | download mbox

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);
 	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)) {
+		mmio_hw_access_pre(dev_priv);
+		vgpu_vreg(vgpu, HUC_STATUS2) = I915_READ(HUC_STATUS2);
+		mmio_hw_access_post(dev_priv);
+	}
 }
 
 /**

Comments

On 2018.03.27 09:19:03 +0800, yakui.zhao@intel.com wrote:
> From: Zhao Yakui <yakui.zhao@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.

> Signed-off-by: Zhao Yakui <yakui.zhao@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);
>  	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)) {
> +		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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
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@intel.com wrote:
> > > From: Zhao Yakui <yakui.zhao@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?

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@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.

> > >   	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.

> > > +		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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> >
On 2018年03月27日 11:18, Zhenyu Wang wrote:
> On 2018.03.27 09:19:03 +0800, yakui.zhao@intel.com wrote:
>> From: Zhao Yakui <yakui.zhao@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.

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@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);
>>   	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)) {
>> +		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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
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@intel.com wrote:
>>>> From: Zhao Yakui <yakui.zhao@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@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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>>>
> 
> 
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
On 2018.03.27 14:03:39 +0800, Zhao, Yakui wrote:
> > > > > 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?
> 

I think USES_HUC() just check if huc fw is available for load, which
should be the state that we want to check huc fw status. And looks
HAS_HUC_UCODE() has no user within i915 code at all? I'm not clear
on that history maybe we should not use it anymore.
On 2018年03月27日 15:27, Zhenyu Wang wrote:
> On 2018.03.27 14:03:39 +0800, Zhao, Yakui wrote:
>>>>>> 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?
>>
> 
> I think USES_HUC() just check if huc fw is available for load, which
> should be the state that we want to check huc fw status. And looks
> HAS_HUC_UCODE() has no user within i915 code at all? I'm not clear
> on that history maybe we should not use it anymore.
> 

The USES_HUC macro will use the module parameter of i915.enable_guc to 
check whether the HUC is needed. And now it is used to control whether 
the Huc FW is loaded.

It is also ok to use the USES_HUC in the function of intel_vgpu_reset_mmio.

I will use the macro of USES_HUC to update the patch.

BTW:
    For the real handler of HUC_STATUS2 reg: As it is one read-only 
register and not changed after initialization, I think that it can be 
initialized only once at the vgpu reset time.
    How do you think?

Thanks

> 
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
On 2018.03.28 08:49:48 +0800, Zhao, Yakui wrote:
> 
> 
> On 2018年03月27日 15:27, Zhenyu Wang wrote:
> > On 2018.03.27 14:03:39 +0800, Zhao, Yakui wrote:
> > > > > > > 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?
> > > 
> > 
> > I think USES_HUC() just check if huc fw is available for load, which
> > should be the state that we want to check huc fw status. And looks
> > HAS_HUC_UCODE() has no user within i915 code at all? I'm not clear
> > on that history maybe we should not use it anymore.
> > 
> 
> The USES_HUC macro will use the module parameter of i915.enable_guc to check
> whether the HUC is needed. And now it is used to control whether the Huc FW
> is loaded.
> 
> It is also ok to use the USES_HUC in the function of intel_vgpu_reset_mmio.
> 
> I will use the macro of USES_HUC to update the patch.
> 
> BTW:
>    For the real handler of HUC_STATUS2 reg: As it is one read-only register
> and not changed after initialization, I think that it can be initialized
> only once at the vgpu reset time.
>    How do you think?
> 

Still want a handler as looks that reg also contains some state info
that might need to be handled, as should not just expect what current
guest driver behavior is, but consider correct emulated state from hw.