[Intel-gfx] [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 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.
> 
> 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 at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre