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

[Chris Wilson]

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