Re: [PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 13, 2013 at 05:05:59PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add APIs to get/put power well references for specific purposes.
> >
> > Also reorganize the internal i915_request power well handling to use the
> > reference count just like everyone else. This way all we need to do is
> > check the reference count and we know whether the power well needs to be
> > enabled of disabled.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 87 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 774ebb6..2ecd3d2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
> >
> >  extern bool intel_display_power_enabled(struct drm_device *dev,
> >                                         enum intel_display_power_domain domain);
> > +extern void intel_display_power_get(struct drm_device *dev,
> > +                                   enum intel_display_power_domain domain);
> > +extern void intel_display_power_put(struct drm_device *dev,
> > +                                   enum intel_display_power_domain domain);
> >  extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8cffef4..4962303 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> >         }
> >  }
> >
> > +void intel_display_power_get(struct drm_device *dev,
> > +                            enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return;
> > +
> > +       switch (domain) {
> > +       case POWER_DOMAIN_PIPE_A:
> > +       case POWER_DOMAIN_TRANSCODER_EDP:
> > +               return;
> > +       case POWER_DOMAIN_PIPE_B:
> > +       case POWER_DOMAIN_PIPE_C:
> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> > +       case POWER_DOMAIN_TRANSCODER_A:
> > +       case POWER_DOMAIN_TRANSCODER_B:
> > +       case POWER_DOMAIN_TRANSCODER_C:
> 
> I know I'm the one who added all these domains, but I have to say I
> only did this because of the reviewers, I don't really like the
> interface. With your addition there's a new problem: you can get the
> POWER_DOMAIN_PIPE_B interface and then put the
> POWER_COMAIN_PIPE_C_PANEL_FITTER and no one will notice. I really
> think the power well itself should be the domain. Also, in cases like
> the suspend/resume code we don't have any domain that makes sense. But
> what's *not* ugly about the power well code?
> 
> I'm not suggesting you to fix that, I'm more kinda asking for ideas, I
> may want to reorganize this code yet again when doing the D3 feature.
> (Just because every single time we touch the power well code we have
> to refactor it!)

In other platforms we're going to have totally different mix of
functional blocks vs. power wells. So assuming we want to deal with those
using a unified API we do need something like this. But maybe there's a
better way to go, haven't really thought about it.

> 
> 
> > +               spin_lock_irq(&power_well->lock);
> > +               if (!power_well->count++)
> > +                       __intel_set_power_well(power_well->device, true);
> > +               spin_unlock_irq(&power_well->lock);
> > +               return;
> > +       default:
> > +               BUG();
> > +       }
> > +}
> > +
> > +void intel_display_power_put(struct drm_device *dev,
> > +                            enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return;
> > +
> > +       switch (domain) {
> > +       case POWER_DOMAIN_PIPE_A:
> > +       case POWER_DOMAIN_TRANSCODER_EDP:
> > +               return;
> > +       case POWER_DOMAIN_PIPE_B:
> > +       case POWER_DOMAIN_PIPE_C:
> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> > +       case POWER_DOMAIN_TRANSCODER_A:
> > +       case POWER_DOMAIN_TRANSCODER_B:
> > +       case POWER_DOMAIN_TRANSCODER_C:
> > +               spin_lock_irq(&power_well->lock);
> > +               WARN_ON(!power_well->count);
> > +               if (!--power_well->count)
> > +                       __intel_set_power_well(power_well->device, false);
> > +               spin_unlock_irq(&power_well->lock);
> > +               return;
> > +       default:
> > +               BUG();
> > +       }
> > +}
> > +
> >  static struct i915_power_well *hsw_pwr;
> >
> >  /* Display audio driver power well request */
> > @@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
> >                 return;
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> > -       if (!hsw_pwr->count++ &&
> > -                       !hsw_pwr->i915_request)
> > +       if (!hsw_pwr->count++)
> >                 __intel_set_power_well(hsw_pwr->device, true);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> >         WARN_ON(!hsw_pwr->count);
> > -       if (!--hsw_pwr->count &&
> > -                      !hsw_pwr->i915_request)
> > +       if (!--hsw_pwr->count)
> >                 __intel_set_power_well(hsw_pwr->device, false);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> >                 return;
> >
> >         spin_lock_irq(&power_well->lock);
> > +
> > +       /*
> > +        * This function will only ever contribute one
> > +        * to the power well reference count. i915_request
> > +        * is what tracks whether we have or have not
> > +        * added the one to the reference count.
> > +        */
> > +       if (power_well->i915_request == enable)
> > +               goto out;
> > +
> >         power_well->i915_request = enable;
> >
> > -       /* only reject "disable" power well request */
> > -       if (power_well->count && !enable) {
> > -               spin_unlock_irq(&power_well->lock);
> > -               return;
> 
> And now to the real problem of the patch: previously whenever we got a
> call to "enable" we'd call __intel_set_power_well and certainly write
> the register. Now with this patch we may not do this due to
> i915_request and the count. This breaks suspend/resume where just
> after we resume we call intel_set_power_well(dev, true) but then the
> new code doesn't really writes the register since i915_request is
> already true. As a consequence, we see "unclaimed register" messages
> complaining about registers 70008, 71008 and 72008. Perhaps in the
> resume path we should fix our tracking and force the "enable" somehow.

Hmm. I guess we anyway want to force the power well to be active during
resume regardless of where the refcount was left.

So maybe just a resume power well func or something:

intel_resume_power_well()
{
	if (!i915_request) {
		i915_request = true;
		count++;
	}
	__set_power_well(true);
}

> 
> 
> > +       if (enable) {
> > +               if (!power_well->count++)
> > +                       __intel_set_power_well(dev, true);
> > +       } else {
> > +               WARN_ON(!power_well->count);
> > +               if (!--power_well->count)
> > +                       __intel_set_power_well(dev, false);
> >         }
> >
> > -       __intel_set_power_well(dev, enable);
> > + out:
> >         spin_unlock_irq(&power_well->lock);
> >  }
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC