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

[Daniele Ceraolo Spurio]

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?

Daniele