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

[Tvrtko Ursulin]

On 03/07/2019 13:12, 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.

Although we still don't know if it is our caller who took the fw or some 
unrelated thread. Still, a more thorough check is better even if not 
perfect.

> 
> 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..bc25a6e51463 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_RUNTIME_PM))
> +		return;
> +

If uncore->funcs.force_wake_get is set why wouldn't we still want to run 
the asserts?

>   	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();

This part looks good. Let wait and see if CI calls me a fool.

Regards,

Tvrtko

>   }
>   
>   /* We give fast paths for the really cool registers */
>