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

[Tvrtko Ursulin]

On 17/03/2017 09:42, Xu, Terrence wrote:
>> -----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>

Thanks!

Regards,

Tvrtko