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

[Pandiyan, Dhinakaran]

On Thu, 2017-12-21 at 13:37 +0100, Maarten Lankhorst wrote:
> 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 
> > Cc: Ville Syrjälä 
> > Cc: Rodrigo Vivi 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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(_domains->domain_use_count[power_domain]));
> > }
> >  
> > mutex_unlock(_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(_domains->domain_use_count[domain]);
> >  }
> >  
> >  /**
> > @@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private 
> > *dev_priv,
> >  
> > mutex_lock(_domains->lock);
> >  
> > -   WARN(!power_domains->domain_use_count[domain],
> > -"Use count on domain %s is already zero\n",
> > +   WARN(atomic_dec_return(_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(_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(_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..
> 

This won't fail until the next patch where it is read outside of the
mutex. And that patch reads these values within the new spin_lock. I was
trying to split the changes so that the next patch does not become too
heavy.

> 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..
> 
I see your point, I can squash them together.

> And instead 

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

[Maarten Lankhorst]

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 
> Cc: Ville Syrjälä 
> Cc: Rodrigo Vivi 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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(_domains->domain_use_count[power_domain]));
>   }
>  
>   mutex_unlock(_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(_domains->domain_use_count[domain]);
>  }
>  
>  /**
> @@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private 
> *dev_priv,
>  
>   mutex_lock(_domains->lock);
>  
> - WARN(!power_domains->domain_use_count[domain],
> -  "Use count on domain %s is already zero\n",
> + WARN(atomic_dec_return(_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(_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(_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

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

[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 
Cc: Ville Syrjälä 
Cc: Rodrigo Vivi 
Signed-off-by: Dhinakaran Pandiyan 
---
 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(_domains->domain_use_count[power_domain]));
}
 
mutex_unlock(_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(_domains->domain_use_count[domain]);
 }
 
 /**
@@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private 
*dev_priv,
 
mutex_lock(_domains->lock);
 
-   WARN(!power_domains->domain_use_count[domain],
-"Use count on domain %s is already zero\n",
+   WARN(atomic_dec_return(_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(_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(_domains->domain_use_count[domain]);
 
if (power_well->count != domains_count) {
DRM_ERROR("power well %s refcount/domain refcount 
mismatch "
-- 
2.11.0

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