From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 03/15] drm/i915: add intel_power_well_is_down Date: Fri, 15 Mar 2013 11:31:17 -0700 Message-ID: <20130315183117.GD17773@bwidawsk.net> References: <1362611003-4823-1-git-send-email-przanoni@gmail.com> <1362611003-4823-4-git-send-email-przanoni@gmail.com> <20130306232623.GE9021@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.localdomain (209-20-75-48.static.cloud-ips.com [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id F1CFFE5CD7 for ; Fri, 15 Mar 2013 11:31:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130306232623.GE9021@phenom.ffwll.local> 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: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote: > On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > It returns true if we're not supposed to touch the registers on the > > power down well. > > > > For now there's just one caller, but I'm going to add more. > > > > Signed-off-by: Paulo Zanoni > > --- > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++++++ > > 3 files changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 502cb28..bd27336 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv, > > if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) > > state = true; > > > > - if (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP && > > - !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) { > > + if (intel_power_well_is_down(dev_priv->dev) && > > The name here feels a bit too generic given that we already have on hsw > different display c states with different requirements and different > pieces of hw not working. > > Can't thinkg of anything better than intel_display_power_well_is_down > though ... > -Daniel > > > + cpu_transcoder != TRANSCODER_EDP) { > > cur_state = false; > > } else { > > reg = PIPECONF(cpu_transcoder); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 010e998..28c4789 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -671,6 +671,7 @@ extern void intel_update_fbc(struct drm_device *dev); > > extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); > > extern void intel_gpu_ips_teardown(void); > > > > +extern bool intel_power_well_is_down(struct drm_device *dev); > > 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 5479363..90562bc 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev) > > dev_priv->display.init_clock_gating(dev); > > } > > > > +/** > > + * Returns true if we're not supposed to touch the registers on the power down > > + * well. Notice that we don't check whether the power well is actually off, we > > + * just check if our driver told the hardware that it doesn't need the power > > + * well enabled. > > + */ I agree with Denial that the name sucks because your comment clearly contradicts what the function is actually called. Can't think of anything better either. In the bikeshed realm, I think it makes more sense to do the IS_HASWELL check in your pipe assertion, but I'll assume that you have a good usage as you mention later. Reviewed-by: Ben Widawsky > > +bool intel_power_well_is_down(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + if (IS_HASWELL(dev)) > > + return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE); > > + else > > + return false; > > +} > > + > > void intel_set_power_well(struct drm_device *dev, bool enable) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > -- > > 1.7.10.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center