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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Cc: Weinan Li <weinan.z.li@intel.com>
Cc: Chris Wilson <chris@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))
 		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)) {
+		ASSIGN_WRITE_MMIO_VFUNCS(gen2);
+		ASSIGN_READ_MMIO_VFUNCS(gen2);
+	} else if (IS_GEN5(dev_priv)) {
+		ASSIGN_WRITE_MMIO_VFUNCS(gen5);
+		ASSIGN_READ_MMIO_VFUNCS(gen5);
+	} else if (IS_GEN(dev_priv, 6, 7)) {
+		ASSIGN_WRITE_MMIO_VFUNCS(gen6);
+
+		if (IS_VALLEYVIEW(dev_priv)) {
+			ASSIGN_FW_DOMAINS_TABLE(__vlv_fw_ranges);
+			ASSIGN_READ_MMIO_VFUNCS(fwtable);
+		} else {
+			ASSIGN_READ_MMIO_VFUNCS(gen6);
 		}
-		break;
-	case 8:
+	} else if (IS_GEN8(dev_priv)) {
 		if (IS_CHERRYVIEW(dev_priv)) {
 			ASSIGN_FW_DOMAINS_TABLE(__chv_fw_ranges);
 			ASSIGN_WRITE_MMIO_VFUNCS(fwtable);
@@ -1352,28 +1340,18 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 			ASSIGN_WRITE_MMIO_VFUNCS(gen8);
 			ASSIGN_READ_MMIO_VFUNCS(gen6);
 		}
-		break;
-	case 7:
-	case 6:
-		ASSIGN_WRITE_MMIO_VFUNCS(gen6);
-
-		if (IS_VALLEYVIEW(dev_priv)) {
-			ASSIGN_FW_DOMAINS_TABLE(__vlv_fw_ranges);
-			ASSIGN_READ_MMIO_VFUNCS(fwtable);
-		} else {
-			ASSIGN_READ_MMIO_VFUNCS(gen6);
+	} else {
+		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;
 		}
-		break;
-	case 5:
-		ASSIGN_WRITE_MMIO_VFUNCS(gen5);
-		ASSIGN_READ_MMIO_VFUNCS(gen5);
-		break;
-	case 4:
-	case 3:
-	case 2:
-		ASSIGN_WRITE_MMIO_VFUNCS(gen2);
-		ASSIGN_READ_MMIO_VFUNCS(gen2);
-		break;
 	}
 
 	iosf_mbi_register_pmic_bus_access_notifier(
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Chris Wilson]

On Fri, Mar 10, 2017 at 09:57:47AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Cc: Weinan Li <weinan.z.li@intel.com>
> Cc: Chris Wilson <chris@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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Patchwork]

== Series Details ==

Series: drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly
URL   : https://patchwork.freedesktop.org/series/21047/
State : success

== Summary ==

Series 21047v1 drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly
https://patchwork.freedesktop.org/api/1.0/series/21047/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 467s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 605s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 536s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 609s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 506s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 500s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 441s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 507s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 493s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 476s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 503s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 608s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 507s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 559s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 561s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 427s

d8d69f76555feef19e3e4b601378446604d90da5 drm-tip: 2017y-03m-10d-11h-50m-04s UTC integration manifest
212fdd9 drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4133/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Mar 10, 2017 at 09:57:47AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
>> Cc: Weinan Li <weinan.z.li@intel.com>
>> Cc: Chris Wilson <chris@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.

First I thought that he hates switches :) But yup, not bad.
-Mika

>
> Do the gvt-g hosts in CI now provide coverage for us of vgpu paths?
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Tvrtko Ursulin]

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@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@intel.com>
>> Cc: Weinan Li <weinan.z.li@intel.com>
>> Cc: Chris Wilson <chris@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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 4056 bytes --]

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@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@intel.com>
> > > Cc: Weinan Li <weinan.z.li@intel.com>
> > > Cc: Chris Wilson <chris@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.

About CI for gvt, I think Terrence is still working but don't know how far it goes now.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Tvrtko Ursulin]

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@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@intel.com>
>>>> Cc: Weinan Li <weinan.z.li@intel.com>
>>>> Cc: Chris Wilson <chris@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.

That would be good I think, thank you. When it has been cleared that it 
actually works and doesn't break anything we can then merge it.

Regards,

Tvrtko

> About CI for gvt, I think Terrence is still working but don't know how far it goes now.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Chris Wilson]

On Mon, Mar 13, 2017 at 09:47:15AM +0000, Tvrtko Ursulin wrote:
> 
> 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@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@intel.com>
> >>>>Cc: Weinan Li <weinan.z.li@intel.com>
> >>>>Cc: Chris Wilson <chris@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.
> 
> That would be good I think, thank you. When it has been cleared that
> it actually works and doesn't break anything we can then merge it.

For the record,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Tvrtko Ursulin]

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@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@intel.com>
>>>> Cc: Weinan Li <weinan.z.li@intel.com>
>>>> Cc: Chris Wilson <chris@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?

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Xu, Terrence]

>-----Original Message-----
>From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
>Sent: Friday, March 17, 2017 5:30 PM
>To: Zhenyu Wang <zhenyuw@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>; Tvrtko Ursulin
><tursulin@ursulin.net>; Intel-gfx@lists.freedesktop.org; Ursulin, Tvrtko
><tvrtko.ursulin@intel.com>; Li, Weinan Z <weinan.z.li@intel.com>; Xu,
>Terrence <terrence.xu@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@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@intel.com>
>>>>> Cc: Weinan Li <weinan.z.li@intel.com>
>>>>> Cc: Chris Wilson <chris@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@intel.com> 

>Regards,
>
>Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@linux.intel.com]
>> Sent: Friday, March 17, 2017 5:30 PM
>> To: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>; Tvrtko Ursulin
>> <tursulin@ursulin.net>; Intel-gfx@lists.freedesktop.org; Ursulin, Tvrtko
>> <tvrtko.ursulin@intel.com>; Li, Weinan Z <weinan.z.li@intel.com>; Xu,
>> Terrence <terrence.xu@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@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@intel.com>
>>>>>> Cc: Weinan Li <weinan.z.li@intel.com>
>>>>>> Cc: Chris Wilson <chris@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@intel.com>

Thanks!

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

[Tvrtko Ursulin]

On 10/03/2017 12:48, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly
> URL   : https://patchwork.freedesktop.org/series/21047/
> State : success
>
> == Summary ==
>
> Series 21047v1 drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly
> https://patchwork.freedesktop.org/api/1.0/series/21047/revisions/1/mbox/
>
> fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 467s
> fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 605s
> fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 536s
> fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 609s
> fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 506s
> fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 500s
> fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 441s
> fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
> fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 441s
> fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 507s
> fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 493s
> fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 476s
> fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 503s
> fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 608s
> fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 507s
> fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 559s
> fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 561s
> fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 427s
>
> d8d69f76555feef19e3e4b601378446604d90da5 drm-tip: 2017y-03m-10d-11h-50m-04s UTC integration manifest
> 212fdd9 drm/i915/vgpu: Neuter forcewakes for VGPU more thouroughly

Pushed, thanks for review and testing!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx