Re: [Intel-gfx] [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain

[Imre Deak]

On Thu, Feb 16, 2017 at 11:31:01AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > So far the sync_hw hook wasn't called for power wells not belonging to
> > any power domain, that is the GEN9 PW1 and MISC_IO power wells. This
> > wasn't a problem so far since the goal of the sync_hw hook - to clear
> > the corresponding BIOS request bit - was guaranteed by clearing the
> > whole BIOS request register elsewhere. This will change with the next
> > patch, so fix up the inconsistency.
> > 
> > Cc: Ander Conselvan de Oliveira 
> > Cc: David Weinehall 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 34 
> > +
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 0f64bc1..f9aff26 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,17 +49,24 @@
> >   * present for a given platform.
> >   */
> >  
> > -#define for_each_power_well(i, power_well, domain_mask, power_domains) 
> > \
> > -   for (i = 0; \
> > -i < (power_domains)->power_well_count &&   \
> > +#define for_each_power_well(i, power_well) \
> > +   for ((i) = 0;   \
> > +(i) < (power_domains)->power_well_count && \
> 
> This now requires that power_domains is in scope to work properly. Would it 
> make
> sense to still pass that as argument to the macro or, alternatively, pass
> dev_priv?

Yes, that was a copy/paste fail. Yep, dev_priv simplifies things, will
change to that.

> Could probably nuke the i parameter too, since no caller uses it?

Ok. I also moved these to i915_dev.h. I'll resend the patchset after a
trybot round.

> 
> But either way,
> 
> Reviewed-by: Ander Conselvan de Oliveira 

Thanks,
Imre

> 
> >  ((power_well) = &(power_domains)->power_wells[i]); \
> > -i++)   \
> > +(i)++)
> > +
> > +#define for_each_power_well_rev(i, power_well) 
> >  \
> > +   for ((i) = (power_domains)->power_well_count - 1;  \
> > +(i) >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > +(i)--)
> > +
> > +#define for_each_power_domain_well(i, power_well, domain_mask, 
> > power_domains) \
> > +   for_each_power_well(i, power_well)  \
> > for_each_if ((power_well)->domains & (domain_mask))
> >  
> > -#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) 
> > \
> > -   for (i = (power_domains)->power_well_count - 1;  \
> > -i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > -i--)\
> > +#define for_each_power_domain_well_rev(i, power_well, domain_mask, \
> > +  power_domains)   \
> > +   for_each_power_well_rev(i, power_well)  \
> > for_each_if ((power_well)->domains & (domain_mask))
> >  
> >  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> > @@ -210,7 +217,8 @@ bool __intel_display_power_is_enabled(struct 
> > drm_i915_private *dev_priv,
> >  
> > is_enabled = true;
> >  
> > -   for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains) {
> > +   for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> > +  power_domains) {
> > if (power_well->always_on)
> > continue;
> >  
> > @@ -1665,7 +1673,8 @@ __intel_display_power_get_domain(struct 
> > drm_i915_private *dev_priv,
> > struct i915_power_well *power_well;
> > int i;
> >  
> > -   for_each_power_well(i, power_well, BIT_ULL(domain), power_domains)
> > +   for_each_power_domain_well(i, power_well, BIT_ULL(domain),
> > +  power_domains)
> > intel_power_well_get(dev_priv, power_well);
> >  
> > power_domains->domain_use_count[domain]++;
> > @@ -1760,7 +1769,8 @@ void intel_display_power_put(struct drm_i915_private 
> > *dev_priv,
> >  intel_display_power_domain_str(domain));
> > power_domains->domain_use_count[domain]--;
> >  
> > -   for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains)
> > +   for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> > +  power_domains)
> > intel_power_well_put(dev_priv, power_well);
> >  
> > 

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain

[Ander Conselvan De Oliveira]

On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> So far the sync_hw hook wasn't called for power wells not belonging to
> any power domain, that is the GEN9 PW1 and MISC_IO power wells. This
> wasn't a problem so far since the goal of the sync_hw hook - to clear
> the corresponding BIOS request bit - was guaranteed by clearing the
> whole BIOS request register elsewhere. This will change with the next
> patch, so fix up the inconsistency.
> 
> Cc: Ander Conselvan de Oliveira 
> Cc: David Weinehall 
> Cc: Ville Syrjälä 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 34 
> +
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 0f64bc1..f9aff26 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,17 +49,24 @@
>   * present for a given platform.
>   */
>  
> -#define for_each_power_well(i, power_well, domain_mask, power_domains)   
> \
> - for (i = 0; \
> -  i < (power_domains)->power_well_count &&   \
> +#define for_each_power_well(i, power_well)   \
> + for ((i) = 0;   \
> +  (i) < (power_domains)->power_well_count && \

This now requires that power_domains is in scope to work properly. Would it make
sense to still pass that as argument to the macro or, alternatively, pass
dev_priv? Could probably nuke the i parameter too, since no caller uses it?

But either way,

Reviewed-by: Ander Conselvan de Oliveira 

>((power_well) = &(power_domains)->power_wells[i]); \
> -  i++)   \
> +  (i)++)
> +
> +#define for_each_power_well_rev(i, power_well)   
>  \
> + for ((i) = (power_domains)->power_well_count - 1;  \
> +  (i) >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> +  (i)--)
> +
> +#define for_each_power_domain_well(i, power_well, domain_mask, 
> power_domains) \
> + for_each_power_well(i, power_well)  \
>   for_each_if ((power_well)->domains & (domain_mask))
>  
> -#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> - for (i = (power_domains)->power_well_count - 1;  \
> -  i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> -  i--)\
> +#define for_each_power_domain_well_rev(i, power_well, domain_mask,   \
> +power_domains)   \
> + for_each_power_well_rev(i, power_well)  \
>   for_each_if ((power_well)->domains & (domain_mask))
>  
>  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> @@ -210,7 +217,8 @@ bool __intel_display_power_is_enabled(struct 
> drm_i915_private *dev_priv,
>  
>   is_enabled = true;
>  
> - for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains) {
> + for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> +power_domains) {
>   if (power_well->always_on)
>   continue;
>  
> @@ -1665,7 +1673,8 @@ __intel_display_power_get_domain(struct 
> drm_i915_private *dev_priv,
>   struct i915_power_well *power_well;
>   int i;
>  
> - for_each_power_well(i, power_well, BIT_ULL(domain), power_domains)
> + for_each_power_domain_well(i, power_well, BIT_ULL(domain),
> +power_domains)
>   intel_power_well_get(dev_priv, power_well);
>  
>   power_domains->domain_use_count[domain]++;
> @@ -1760,7 +1769,8 @@ void intel_display_power_put(struct drm_i915_private 
> *dev_priv,
>intel_display_power_domain_str(domain));
>   power_domains->domain_use_count[domain]--;
>  
> - for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains)
> + for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> +power_domains)
>   intel_power_well_put(dev_priv, power_well);
>  
>   mutex_unlock(_domains->lock);
> @@ -2427,7 +2437,7 @@ static void intel_power_domains_sync_hw(struct 
> drm_i915_private *dev_priv)
>   int i;
>  
>   mutex_lock(_domains->lock);
> - for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> + for_each_power_well(i, power_well) {
>   power_well->ops->sync_hw(dev_priv, power_well);
>