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