[Intel-gfx] [PATCH v2] drm/i915: Check caller held wakerefs in assert_forcewakes_active

[Tvrtko Ursulin]

On 04/07/2019 09:17, Chris Wilson wrote:
> The intent of the assert is to document that the caller took the
> appropriate wakerefs for the function. However, as Tvrtko pointed out,
> we simply check whether the fw_domains are active and may be confused by
> the auto wakeref which may be dropped between the check and use. Let's
> be more careful in the assert and check that each fw_domain has an
> explicit caller wakeref above and beyond the automatic wakeref.
> 
> v2: Fix spelling for config DRM_I915_DEBUG_RUNTIME_PM
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 68d54e126d79..1771e69dbf03 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -738,6 +738,12 @@ void assert_forcewakes_inactive(struct intel_uncore *uncore)
>   void assert_forcewakes_active(struct intel_uncore *uncore,
>   			      enum forcewake_domains fw_domains)
>   {
> +	struct intel_uncore_forcewake_domain *domain;
> +	unsigned int tmp;
> +
> +	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
> +		return;
> +
>   	if (!uncore->funcs.force_wake_get)
>   		return;
>   
> @@ -747,6 +753,24 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
>   	WARN(fw_domains & ~uncore->fw_domains_active,
>   	     "Expected %08x fw_domains to be active, but %08x are off\n",
>   	     fw_domains, fw_domains & ~uncore->fw_domains_active);
> +
> +	/*
> +	 * Check that the caller has an explicit wakeref and we don't mistake
> +	 * it for the auto wakeref.
> +	 */
> +	local_irq_disable();
> +	for_each_fw_domain_masked(domain, fw_domains, uncore, tmp) {
> +		unsigned int expect = 1;
> +
> +		if (hrtimer_active(&domain->timer))
> +			expect++;
> +
> +		if (WARN(domain->wake_count < expect,
> +			 "Expected domain %d to be held awake by caller\n",
> +			 domain->id))
> +			break;
> +	}
> +	local_irq_enable();
>   }
>   
>   /* We give fast paths for the really cool registers */
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko