Re: [PATCH v2] drm/i915: Fix forcewake active domain tracking

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/03/2017 08:50, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 07:32:51AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In commit 003342a50021 ("drm/i915: Keep track of active
>> forcewake domains in a bitmask") I forgot to adjust the
>> newly introduce fw_domains_active state across reset.
>>
>> This caused the assert_forcewakes_inactive to trigger
>> during suspend and resume if there were user held
>> forcewakes.
>>
>> v2: Bitmask checks are required since vfuncs are not
>>     always present.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
>> Testcase: igt/drv_suspend/forcewake
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: v4.10+ <stable@vger.kernel.org>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 2a3f35c30501..7efae77faca0 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -304,12 +304,14 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>>  	fw = dev_priv->uncore.fw_domains_active;
>>  	if (fw)
>>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
>> +	dev_priv->uncore.fw_domains_active = 0;
>
> Hmm, I think we would be happier with (think of a good name)
>
> static void __intel_uncore_force_wake_get(i915, fw_domains)
> {
> 	i915->uncore.funcs.force_wake_get(i915, fw_domains);
> 	i915->uncore.funcs.fw_domains_active |= fw_domain;
> }
>
> static void __intel_uncore_force_wake_put(i915, fw_domains)
> {
> 	i915->uncore.funcs.force_wake_put(i915, fw_domains);
> 	i915->uncore.funcs.fw_domains_active &= ~fw_domain;
> }
>
> Another alternative would be to move the bitops down to the callbacks.
> gcc might be happier, at the expense of some duplication and risk of
> forgetting.
>
> Anyway worth the effort?

Yes I agree, was considering the second option myself. Think I still 
prefer that one to keep the inseparable together.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx