2013/7/18 Ben Widawsky <ben@xxxxxxxxxxxx>:
> On Fri, Jul 12, 2013 at 02:19:42PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>
> 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).
Originally this code was part of other function. I agree that the way
things look now, it should be on the same patch.
>
>> ---
>> 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
The audio registers go away when we disable the power well, which is
part of the assertion.
> eDP HPD
> Other CPU/PCH interrupts
I guess these ones deserve to be part of the assertion, but checking
interrupts is dangerous and racy...I'll try to find a good solution.
>
> 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.
This function should should be the last thing on a big sequence that
involves disabling all the outputs, rearranging the interrupts, etc.
If we detect a problem here, we can't just return, the caller would
have to unwind everything it did. Anyway, this is really something we
don't expect and should never happen. I was kinda assuming the callers
would have to make sure they do everything before they call
hsw_diable_lcpll, and then these assertions are just double-checking.
Suggestions welcome :)
>
>> val = I915_READ(LCPLL_CTL);
>>
>> if (switch_to_fclk) {
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
