From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 5/5] drm/i915: add HAS_LP_PCH check Date: Mon, 22 Jul 2013 19:44:51 -0300 Message-ID: References: <1374270968-2983-1-git-send-email-przanoni@gmail.com> <1374271137-3149-1-git-send-email-przanoni@gmail.com> <20130722171046.GA6850@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ob0-f172.google.com (mail-ob0-f172.google.com [209.85.214.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 17868E635B for ; Mon, 22 Jul 2013 15:44:52 -0700 (PDT) Received: by mail-ob0-f172.google.com with SMTP id wo10so8849911obc.31 for ; Mon, 22 Jul 2013 15:44:51 -0700 (PDT) In-Reply-To: <20130722171046.GA6850@bwidawsk.net> 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: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org 2013/7/22 Ben Widawsky : > On Fri, Jul 19, 2013 at 06:58:57PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni >> >> We have 2 possible LPT PCHs: the normal version, which contains the >> pixel path (FDI, transcoders, VGA, etc), and the LP version, which >> comes with ULT machines and doesn't contain the pixel path. Both >> models return true for HAS_PCH_LPT. >> >> We already have a few places where we explicitly check for LPT-LP, so >> add a HAS_LP_PCH check to simplify the code. >> >> Signed-off-by: Paulo Zanoni > > 1-4 is: > Reviewed-by: Ben Widawsky > > This patch I believe should have come first in the series (we discussed > on IRC), but meh. > > Also, I think the name HAS_LP_PCH is no good since the LP is "Low > Power." It really should be HAS_PCH_LPT_LP. I realize that's redundant > within the functions since they are lpt only, but this is defined in the > primary header file... I was thinking in the future where other PCHs may also have this LP/non-LP difference, so we'd use the HAS_LP_PCH macro for them. If we want the macro to be PCH-specific we should probably even drop the macro and keep using dev_priv->pch_id (drop this patch). > > With that this too is: > Reviewed-by: Ben Widawsky > >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_display.c | 9 +++------ >> drivers/gpu/drm/i915/intel_pm.c | 4 ++-- >> 3 files changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 2885265..262c3d4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1578,6 +1578,8 @@ struct drm_i915_file_private { >> #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX) >> #define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP) >> #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE) >> +#define HAS_LP_PCH(dev_priv) ((dev_priv)->pch_id == \ >> + INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) >> >> #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index c35a613..1d3c805 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5274,8 +5274,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread, >> >> if (WARN(with_fdi && !with_spread, "FDI requires downspread\n")) >> with_spread = true; >> - if (WARN(dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE && >> - with_fdi, "LP PCH doesn't have FDI\n")) >> + if (WARN(HAS_LP_PCH(dev_priv) && with_fdi, "LP PCH doesn't have FDI\n")) >> with_fdi = false; >> >> mutex_lock(&dev_priv->dpio_lock); >> @@ -5298,8 +5297,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread, >> } >> } >> >> - reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ? >> - SBI_GEN0 : SBI_DBUFF0; >> + reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0; >> tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK); >> tmp |= SBI_GEN0_CFG_BUFFENABLE_DISABLE; >> intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK); >> @@ -5315,8 +5313,7 @@ static void lpt_disable_clkout_dp(struct drm_device *dev) >> >> mutex_lock(&dev_priv->dpio_lock); >> >> - reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ? >> - SBI_GEN0 : SBI_DBUFF0; >> + reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0; >> tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK); >> tmp &= ~SBI_GEN0_CFG_BUFFENABLE_DISABLE; >> intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK); >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 43031ec..7643b16 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -4650,7 +4650,7 @@ static void lpt_init_clock_gating(struct drm_device *dev) >> * TODO: this bit should only be enabled when really needed, then >> * disabled when not needed anymore in order to save power. >> */ >> - if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) >> + if (HAS_LP_PCH(dev_priv)) >> I915_WRITE(SOUTH_DSPCLK_GATE_D, >> I915_READ(SOUTH_DSPCLK_GATE_D) | >> PCH_LP_PARTITION_LEVEL_DISABLE); >> @@ -4665,7 +4665,7 @@ static void lpt_suspend_hw(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> - if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) { >> + if (HAS_LP_PCH(dev_priv)) { >> uint32_t val = I915_READ(SOUTH_DSPCLK_GATE_D); >> >> val &= ~PCH_LP_PARTITION_LEVEL_DISABLE; >> -- >> 1.8.1.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ben Widawsky, Intel Open Source Technology Center -- Paulo Zanoni