[Intel-gfx] [PATCH 39/66] drm/i915: Check hw state in assert_can_disable_lcpll

[Paulo Zanoni]

2014-04-24 18:55 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> All the other checks also check hw state, so checking our software
> refcounts for the plls looks a bit odd.

As I mentioned before, this contradicts your own previous review of
the patch that added this code. In addition, you said many times that
we should do SW checks instead of HW checks when possible.

What should be looking odd here is that we check registers directly
for the other stuff, instead of looking at some struct :)

> Also this will simplify the
> conversion over to the shared dpll framework, which itself has massive
> amounts of checks to make sure that we never leave a display pll
> enabled when we shouldn't.

What you wrote above is a nice reason to check the new shared DPLL
structs instead of the registers directly: it has tons of WARNs, so
it's unlikely the structs will be wrong.

>
> So after that conversion we should stil have a good enough coverage of
> asserts for entering pc8/runtime pm on hsw/bdw.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1513d9fceebe..22b3d74f9ecc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6926,7 +6926,6 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  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)
> @@ -6934,9 +6933,9 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>                      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(SPLL_CTL) & SPLL_PLL_ENABLE, "SPLL enabled\n");
> +       WARN(I915_READ(WRPLL_CTL1) & WRPLL_PLL_ENABLE, "WRPLL1 enabled\n");
> +       WARN(I915_READ(WRPLL_CTL2) & WRPLL_PLL_ENABLE, "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");
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni