From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains Date: Fri, 13 Sep 2013 23:49:21 +0300 Message-ID: <20130913204921.GF4531@intel.com> References: <1378995167-23445-1-git-send-email-ville.syrjala@linux.intel.com> <1378995167-23445-2-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 680EFE5DC5 for ; Fri, 13 Sep 2013 13:49:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Sep 13, 2013 at 05:05:59PM -0300, Paulo Zanoni wrote: > Hi > = > 2013/9/12 : > > From: Ville Syrj=E4l=E4 > > > > 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=E4l=E4 > > --- > > 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/in= tel_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_devi= ce *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 dom= ain); > > +extern void intel_display_power_put(struct drm_device *dev, > > + enum intel_display_power_domain dom= ain); > > 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/int= el_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_de= vice *dev, bool enable) > > } > > } > > > > +void intel_display_power_get(struct drm_device *dev, > > + enum intel_display_power_domain domain) > > +{ > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > + struct i915_power_well *power_well =3D &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 =3D dev->dev_private; > > + struct i915_power_well *power_well =3D &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, fals= e); > > + 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 *de= v, 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 =3D=3D enable) > > + goto out; > > + > > power_well->i915_request =3D 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 =3D 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=E4l=E4 Intel OTC