[Intel-gfx] [PATCH 4/6] drm/i915: skip forcewake actions on forcewake-less uncore

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2019-06-18 19:40:40)
> 
> 
> On 6/18/19 3:22 AM, Chris Wilson wrote:
> > Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33)
> >> We always call some of the setup/cleanup functions for forcewake, even
> >> if the feature is not actually available. Skipping these operations if
> >> forcewake is not available saves us some operations on older gens and
> >> prepares us for having a forcewake-less display uncore.
> >> The suspend/resume functions have also been renamed to clearly indicate
> >> that they only operate on forcewake status.
> >>
> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.c     |  15 +--
> >>   drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++-----------
> >>   drivers/gpu/drm/i915/intel_uncore.h |   8 +-
> >>   3 files changed, 101 insertions(+), 69 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index d113296cbe34..95b36fe17f99 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
> >>   
> >>          intel_device_info_init_mmio(dev_priv);
> >>   
> >> -       intel_uncore_prune_mmio_domains(&dev_priv->uncore);
> >> +       intel_uncore_prune_forcewake_domains(&dev_priv->uncore);
> > 
> > The _prune_ was the exceptional case...
> > 
> >>          intel_uc_init_mmio(dev_priv);
> >>   
> >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >>   
> >>          i915_gem_suspend_late(dev_priv);
> >>   
> >> -       intel_uncore_suspend(&dev_priv->uncore);
> >> +       intel_uncore_forcewake_suspend(&dev_priv->uncore);
> > 
> > But are you sure you want to delegate all the fw control to i915_drv.c,
> > and not keep this as a call into intel_uncore_suspend() ? It is meant to
> > be telling the uncore functionality that it is time to suspend, and it
> > has to work out what to save.
> > 
> > I am not sold on this yet. (One day this will just be a bunch of async
> > tasks plugged into signals with ordering determined by their
> > self-declared dependency tree. One day.)
> > 
> > So sell me on why we want an uncore_forcewake object, as currently you
> > are proposing a intel_uncore_suspend_forcewake().
> > -Chris
> > 
> 
> My aim was to make it clear that this call will not be required for 
> display_uncore since there is nothing to do on suspend/resume if there 
> is no forcewake (and you're right, intel_uncore_suspend_forcewake is a 
> better naming). Ultimately I'd like to transition the GT uncore inside 
> intel_gt and call this inside intel_gt_suspend(). However, I don't mind 
> keeping the current naming and calling it for display uncore as well to 
> be more generic, there are checks everywhere so the overhead should me 
> minimal. What's your preference?

I think, for the time being a sketch like
i915_drm_suspend:
	intel_uncore_suspend(i915->gt.uncore)
	intel_uncore_suspend(i915->display.uncore)

is preferable. Rather than pulling the knowlegde that we need only
suspend the gt.uncore because it has forcewake into the high level
i915_drm_suspend. A couple of ifs or a vfunc go a long way to keeping as
simple as it can be (and no simpler).

At the i915_drm_suspend level it is hard enough to manage a list of
components and the correct order in which to call them :)
-Chris