2013/9/13 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> On Fri, Sep 13, 2013 at 05:05:59PM -0300, Paulo Zanoni wrote:
>> Hi
>>
>> 2013/9/12 <ville.syrjala@xxxxxxxxxxxxxxx>:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxx>
>> > ---
>> > 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);
> }
Yes, looks good.
>
>>
>>
>> > + 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@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
