Re: [PATCH v2 6/8] drm/i915: Use an atomic_t array to track power domain use count.

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	dri-devel@lists.freedesktop.org,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v2 6/8] drm/i915: Use an atomic_t array to track power domain use count.
Date: Thu, 21 Dec 2017 13:37:47 +0100	[thread overview]
Message-ID: <16c62533-9a67-8bda-74bf-9fa791fd64f1@linux.intel.com> (raw)
In-Reply-To: <20171219052659.2355-7-dhinakaran.pandiyan@intel.com>

Hey,

Op 19-12-17 om 06:26 schreef Dhinakaran Pandiyan:
> Convert the power_domains->domain_use_count array that tracks per-domain
> use count to atomic_t type. This is needed to be able to read/write the use
> counts outside of the power domain mutex.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++------
>  3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1a7b28f62570..1f1d9162f2c2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2764,7 +2764,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
>  		for_each_power_domain(power_domain, power_well->domains)
>  			seq_printf(m, "  %-23s %d\n",
>  				 intel_display_power_domain_str(power_domain),
> -				 power_domains->domain_use_count[power_domain]);
> +				 atomic_read(&power_domains->domain_use_count[power_domain]));
>  	}
>  
>  	mutex_unlock(&power_domains->lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e4e613e7b41..ddadeb9eaf49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1489,7 +1489,7 @@ struct i915_power_domains {
>  	int power_well_count;
>  
>  	struct mutex lock;
> -	int domain_use_count[POWER_DOMAIN_NUM];
> +	atomic_t domain_use_count[POWER_DOMAIN_NUM];
>  	struct i915_power_well *power_wells;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 96ab74f3d101..992caec1fbc4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1453,7 +1453,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
>  		intel_power_well_get(dev_priv, power_well);
>  
> -	power_domains->domain_use_count[domain]++;
> +	atomic_inc(&power_domains->domain_use_count[domain]);
>  }
>  
>  /**
> @@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -	WARN(!power_domains->domain_use_count[domain],
> -	     "Use count on domain %s is already zero\n",
> +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
> +	     "Use count on domain %s was already zero\n",
>  	     intel_display_power_domain_str(domain));
> -	power_domains->domain_use_count[domain]--;
>  
>  	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
>  		intel_power_well_put(dev_priv, power_well);
> @@ -3049,7 +3048,7 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
>  		for_each_power_domain(domain, power_well->domains)
>  			DRM_DEBUG_DRIVER("  %-23s %d\n",
>  					 intel_display_power_domain_str(domain),
> -					 power_domains->domain_use_count[domain]);
> +					 atomic_read(&power_domains->domain_use_count[domain]));
>  	}
>  }
>  
> @@ -3092,7 +3091,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  
>  		domains_count = 0;
>  		for_each_power_domain(domain, power_well->domains)
> -			domains_count += power_domains->domain_use_count[domain];
> +			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
>  
>  		if (power_well->count != domains_count) {
>  			DRM_ERROR("power well %s refcount/domain refcount mismatch "

I can imagine this will start failing really badly. The previous code assumed that
everything is protected by power_domains->lock, and now this changes makes it no
longer the case..

I see the rest of the code changes things even more, but it would be better if the
locking rework was done in a single patch, and not bolted on..

And instead of using atomic_t, there is a refcount implementation in refcount.h,
it could be used here for locking power wells only if it would drop to zero..

Cheers,
Maarten

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