[Intel-gfx] [PATCH 1/2] drm/i915: move HSW DDI pll enabling to haswell_crtc_enable

[Daniel Vetter]

On Fri, May 23, 2014 at 4:51 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> Now that we moved all the register writing to haswell_crtc_enable, we
> can also move the PLL enabling there. This is the last step to do
> before allowing runtime PM on DPMS.
>
> Daniel has some patches to do this while also moving all the HSW PLL
> code to the shared DPLL structs, but while we still discuss the color
> of those patches, we can apply this quick one and at least get runtime
> PM on DPMS running in the meantime.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Nope. If you want to do this you need 2 refcounts per shared dpll:
- One that tracks whether a crtc has a mode set that needs a given
shared dpll, whether the crtc is actually enabled at the hw level or
no. You need to increment that refcount in ->crtc_mode_set and
decrement it in ->crtc_off or ->crtc_mode_set.
- A 2nd refcount which tracks active usage and gets incremented in
->crtc_enable and decremented in ->crtc_disable.

This need is the entire point of the shared dpll infrastructure. I
haven't tested you patch on actual hw but from reading through it you
still keep that single refcount and increment it in crtc_mode_set and
decrement it in crtc_disable. That means:
- Multiple dpms off/on cycles will underrun the refcount since it's unbalanced.
- Sharing is broken since if you dpms off a crtc its pll can be stolen
and reused, which means upon dpms on you get a completely wrong pll
setting.

I think the kms_flip/*dpms* and kms_flip/2x*dpms* tests should be able
to hit this. Wrt testing please also note that even for just testing
runtime pm running only pm_rpm isn't good enough - some of the new rpm
tests I've added are subtests of kms_flip (since I wanted to test
vblanks and similar stuff). So you really need to use piglits
filtering to get all the rpm tests.

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3da73ef..1ea4fbe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4052,6 +4052,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         if (intel_crtc->active)
>                 return;
>
> +       intel_ddi_pll_enable(intel_crtc);
> +
>         if (intel_crtc->config.has_dp_encoder)
>                 intel_dp_set_m_n(intel_crtc);
>
> @@ -4243,6 +4245,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>         intel_update_fbc(dev);
>         intel_edp_psr_update(dev);
>         mutex_unlock(&dev->struct_mutex);
> +
> +       intel_ddi_put_crtc_pll(crtc);
>  }
>
>  static void ironlake_crtc_off(struct drm_crtc *crtc)
> @@ -4253,7 +4257,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
>
>  static void haswell_crtc_off(struct drm_crtc *crtc)
>  {
> -       intel_ddi_put_crtc_pll(crtc);
>  }
>
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> @@ -7471,7 +7474,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>
>         if (!intel_ddi_pll_select(intel_crtc))
>                 return -EINVAL;
> -       intel_ddi_pll_enable(intel_crtc);
>
>         intel_crtc->lowfreq_avail = false;
>
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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