[Intel-gfx] [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Xu, Terrence]

>-----Original Message-----
>From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
>Sent: Friday, March 17, 2017 5:30 PM
>To: Zhenyu Wang <zhenyuw at linux.intel.com>
>Cc: Chris Wilson <chris at chris-wilson.co.uk>; Tvrtko Ursulin
><tursulin at ursulin.net>; Intel-gfx at lists.freedesktop.org; Ursulin, Tvrtko
><tvrtko.ursulin at intel.com>; Li, Weinan Z <weinan.z.li at intel.com>; Xu,
>Terrence <terrence.xu at intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU
>more thouroughly
>
>
>Hi,
>
>On 13/03/2017 09:37, Zhenyu Wang wrote:
>> On 2017.03.13 09:26:26 +0000, Tvrtko Ursulin wrote:
>>>
>>> On 10/03/2017 10:09, Chris Wilson wrote:
>>>> On Fri, Mar 10, 2017 at 09:57:47AM +0000, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>
>>>>> If we avoid initializing forcewake domains when running as a guest,
>>>>> and also use gen2 mmio accessors in that case, we can avoid the
>>>>> timer traffic and any looping through the forcewake code which is
>>>>> currently just so it can end up in the no-op forcewake
>>>>> implementation.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>> Cc: Weinan Li <weinan.z.li at intel.com>
>>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_uncore.c | 76
>>>>> +++++++++++++------------------------
>>>>>  1 file changed, 27 insertions(+), 49 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>>>>> b/drivers/gpu/drm/i915/intel_uncore.c
>>>>> index 71b9b387ad04..09f5f02d7901 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>>>> @@ -138,13 +138,6 @@ fw_domains_put(struct drm_i915_private
>>>>> *dev_priv, enum forcewake_domains fw_doma  }
>>>>>
>>>>>  static void
>>>>> -vgpu_fw_domains_nop(struct drm_i915_private *dev_priv,
>>>>> -		    enum forcewake_domains fw_domains)
>>>>> -{
>>>>> -	/* Guest driver doesn't need to takes care forcewake. */
>>>>> -}
>>>>> -
>>>>> -static void
>>>>>  fw_domains_posting_read(struct drm_i915_private *dev_priv)  {
>>>>>  	struct intel_uncore_forcewake_domain *d; @@ -1187,7 +1180,7
>@@
>>>>> static void fw_domain_init(struct drm_i915_private *dev_priv,
>>>>>
>>>>>  static void intel_uncore_fw_domains_init(struct drm_i915_private
>>>>> *dev_priv)  {
>>>>> -	if (INTEL_INFO(dev_priv)->gen <= 5)
>>>>> +	if (INTEL_GEN(dev_priv) <= 5 || intel_vgpu_active(dev_priv))
>>>>
>>>> Make these separate ifs, they aren't semantically related so be verbose.
>>>>
>>>>>  		return;
>>>>>
>>>>>  	if (IS_GEN9(dev_priv)) {
>>>>> @@ -1273,11 +1266,6 @@ static void
>intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>>>>>  			       FORCEWAKE, FORCEWAKE_ACK);
>>>>>  	}
>>>>>
>>>>> -	if (intel_vgpu_active(dev_priv)) {
>>>>> -		dev_priv->uncore.funcs.force_wake_get =
>vgpu_fw_domains_nop;
>>>>> -		dev_priv->uncore.funcs.force_wake_put =
>vgpu_fw_domains_nop;
>>>>> -	}
>>>>> -
>>>>>  	/* All future platforms are expected to require complex power gating
>*/
>>>>>  	WARN_ON(dev_priv->uncore.fw_domains == 0);  } @@ -1327,22
>>>>> +1315,22 @@ void intel_uncore_init(struct drm_i915_private
>*dev_priv)
>>>>>  	dev_priv->uncore.pmic_bus_access_nb.notifier_call =
>>>>>  		i915_pmic_bus_access_notifier;
>>>>>
>>>>> -	switch (INTEL_INFO(dev_priv)->gen) {
>>>>> -	default:
>>>>> -	case 9:
>>>>> -		ASSIGN_FW_DOMAINS_TABLE(__gen9_fw_ranges);
>>>>> -		ASSIGN_WRITE_MMIO_VFUNCS(fwtable);
>>>>> -		ASSIGN_READ_MMIO_VFUNCS(fwtable);
>>>>> -		if (HAS_DECOUPLED_MMIO(dev_priv)) {
>>>>> -			dev_priv->uncore.funcs.mmio_readl =
>>>>> -						gen9_decoupled_read32;
>>>>> -			dev_priv->uncore.funcs.mmio_readq =
>>>>> -						gen9_decoupled_read64;
>>>>> -			dev_priv->uncore.funcs.mmio_writel =
>>>>> -						gen9_decoupled_write32;
>>>>> +	if (IS_GEN(dev_priv, 2, 4) || intel_vgpu_active(dev_priv)) {
>>>>
>>>> Ok, this doesn't look too bad.
>>>>
>>>> Do the gvt-g hosts in CI now provide coverage for us of vgpu paths?
>>>
>>> No idea.
>>>
>>> Adding Zhenyu. So this patch avoids burning CPU cycles in guests and
>>> scheduling timers when all of that ends up in the dummy/no-op
>>> forcewake implementation.
>>>
>>> If interesting to you, would it be easy for you to test it or how
>>> should we proceed?
>>>
>>
>> Patch looks fine to me. I can apply it for our QA testing if required.
>
>Were you perhaps able to smoke test this one?

Hi Ursulin, we have verified your patch in guest, no regression be found.

Tested-by: Terrence Xu <terrence.xu at intel.com> 

>Regards,
>
>Tvrtko