Re: [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Jul 12, 2013 at 02:19:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Most of the hardware needs to be disabled before LCPLL is disabled, so
> let's add a function to assert some of items listed in the "Display
> Sequences for LCPLL disabling" documentation.
> 
> The idea is that hsw_disable_lcpll should not disable the hardware,
> the callers need to take care of calling hsw_disable_lcpll only once
> everything is already disabled.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I don't see a reason to separate this patch from the previous one. It
makes review easier, but it's weird bisect wise. The correct order, if
you wanted to do them as separate patches would be to introduce the
assertions, and then add the functionality (I think).

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e5a5ec..9556dff 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2195,6 +2195,8 @@
>  #define BLC_PWM_CPU_CTL2	0x48250
>  #define BLC_PWM_CPU_CTL		0x48254
>  
> +#define HSW_BLC_PWM2_CTL	0x48350
> +
>  /* PCH CTL1 is totally different, all but the below bits are reserved. CTL2 is
>   * like the normal CTL from gen4 and earlier. Hooray for confusing naming. */
>  #define BLC_PWM_PCH_CTL1	0xc8250
> @@ -2203,6 +2205,12 @@
>  #define   BLM_PCH_POLARITY			(1 << 29)
>  #define BLC_PWM_PCH_CTL2	0xc8254
>  
> +#define UTIL_PIN_CTL		0x48400
> +#define   UTIL_PIN_ENABLE	(1 << 31)
> +
> +#define PCH_GTC_CTL		0xe7000
> +#define   PCH_GTC_ENABLE	(1 << 31)
> +
>  /* TV port control */
>  #define TV_CTL			0x68000
>  /** Enables the TV encoder */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ffb08bf..9055f60 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5922,6 +5922,32 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	return true;
>  }
>  
> +static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
> +	struct intel_crtc *crtc;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
> +		WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
> +		     pipe_name(crtc->pipe));
> +
> +	WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> +	WARN(plls->spll_refcount, "SPLL enabled\n");
> +	WARN(plls->wrpll1_refcount, "WRPLL1 enabled\n");
> +	WARN(plls->wrpll2_refcount, "WRPLL2 enabled\n");
> +	WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
> +	WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
> +	     "CPU PWM1 enabled\n");
> +	WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
> +	     "CPU PWM2 enabled\n");
> +	WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
> +	     "PCH PWM1 enabled\n");
> +	WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
> +	     "Utility pin enabled\n");
> +	WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");

Looking at probably the same list you've looked at, I don't see:
Audio controller
eDP HPD
Other CPU/PCH interrupts

You've worked with this code longer than I have, so maybe you can
explain why you've skipped them.

> +}
> +
>  /*
>   * This function implements pieces of two sequences from BSpec:
>   * - Sequence for display software to disable LCPLL
> @@ -5935,6 +5961,8 @@ void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  {
>  	uint32_t val;
>  
> +	assert_can_disable_lcpll(dev_priv);
> +

Do we want to proceed if the above fails? I was sort of under the
impression that hard hangs can occur as a result of some functions still
being enabled when we change clocks. I'm fine with continuing on since
we have the WARNS, just wondering if you've thought about it.

>  	val = I915_READ(LCPLL_CTL);
>  
>  	if (switch_to_fclk) {
> -- 
> 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