[Intel-gfx] [PATCH 40/66] drm/i915: Remove spll_refcount for hsw

[Paulo Zanoni]

2014-04-24 18:55 GMT-03:00 Daniel Vetter <daniel.vetter at ffwll.ch>:
> SPLL would be a reference clock we could potentially share,
> especially if we want to use the SSC mode. But currently we
> don't, so let's rip out this complexity for a simpler conversion
> to the new display pll framework.

I'm really not a fan of this patch, I think it goes in the opposite
way of where we want to be. We already talked a few times about adding
sharing support, and I even recently considered adding a
"i915.edp_use_ssc" module option and ask some bug reporters to test
them. Also, adding the SPLL support will be another way to prove that
your "shared DPLL" code is flexible enough to handle the all the
possibilities generated by the creative minds of the HW developers.
Can you please include SPLL in your conversion?

Thanks,
Paulo


>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_ddi.c | 41 +++++++++++++---------------------------
>  2 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e6c16068010f..b6eac92e0a22 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -221,7 +221,6 @@ void intel_link_compute_m_n(int bpp, int nlanes,
>                             struct intel_link_m_n *m_n);
>
>  struct intel_ddi_plls {
> -       int spll_refcount;
>         int wrpll1_refcount;
>         int wrpll2_refcount;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 271ce19ee880..16ec6aee3df7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -394,14 +394,11 @@ void intel_ddi_put_crtc_pll(struct drm_crtc *crtc)
>
>         switch (intel_crtc->ddi_pll_sel) {
>         case PORT_CLK_SEL_SPLL:
> -               plls->spll_refcount--;
> -               if (plls->spll_refcount == 0) {
> -                       DRM_DEBUG_KMS("Disabling SPLL\n");
> -                       val = I915_READ(SPLL_CTL);
> -                       WARN_ON(!(val & SPLL_PLL_ENABLE));
> -                       I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
> -                       POSTING_READ(SPLL_CTL);
> -               }
> +               DRM_DEBUG_KMS("Disabling SPLL\n");
> +               val = I915_READ(SPLL_CTL);
> +               WARN_ON(!(val & SPLL_PLL_ENABLE));
> +               I915_WRITE(SPLL_CTL, val & ~SPLL_PLL_ENABLE);
> +               POSTING_READ(SPLL_CTL);
>                 break;
>         case PORT_CLK_SEL_WRPLL1:
>                 plls->wrpll1_refcount--;
> @@ -425,7 +422,6 @@ void intel_ddi_put_crtc_pll(struct drm_crtc *crtc)
>                 break;
>         }
>
> -       WARN(plls->spll_refcount < 0, "Invalid SPLL refcount\n");
>         WARN(plls->wrpll1_refcount < 0, "Invalid WRPLL1 refcount\n");
>         WARN(plls->wrpll2_refcount < 0, "Invalid WRPLL2 refcount\n");
>
> @@ -821,16 +817,9 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>                 }
>
>         } else if (type == INTEL_OUTPUT_ANALOG) {
> -               if (plls->spll_refcount == 0) {
> -                       DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
> -                                     pipe_name(pipe));
> -                       plls->spll_refcount++;
> -                       intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
> -               } else {
> -                       DRM_ERROR("SPLL already in use\n");
> -                       return false;
> -               }
> -
> +               DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
> +                             pipe_name(pipe));
> +               intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>         } else {
>                 WARN(1, "Invalid DDI encoder type %d\n", type);
>                 return false;
> @@ -869,13 +858,13 @@ void intel_ddi_pll_enable(struct intel_crtc *crtc)
>                 return;
>
>         case PORT_CLK_SEL_SPLL:
> -               pll_name = "SPLL";
> -               reg = SPLL_CTL;
> -               refcount = plls->spll_refcount;
>                 new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz |
>                           SPLL_PLL_SSC;
> -               break;
> -
> +               WARN(I915_READ(SPLL_CTL) & enable_bit, "SPLL already enabled\n");
> +               I915_WRITE(SPLL_CTL, new_val);
> +               POSTING_READ(SPLL_CTL);
> +               udelay(20);
> +               return;
>         case PORT_CLK_SEL_WRPLL1:
>         case PORT_CLK_SEL_WRPLL2:
>                 if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) {
> @@ -1186,7 +1175,6 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>         enum pipe pipe;
>         struct intel_crtc *intel_crtc;
>
> -       dev_priv->ddi_plls.spll_refcount = 0;
>         dev_priv->ddi_plls.wrpll1_refcount = 0;
>         dev_priv->ddi_plls.wrpll2_refcount = 0;
>
> @@ -1203,9 +1191,6 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>                                                                  pipe);
>
>                 switch (intel_crtc->ddi_pll_sel) {
> -               case PORT_CLK_SEL_SPLL:
> -                       dev_priv->ddi_plls.spll_refcount++;
> -                       break;
>                 case PORT_CLK_SEL_WRPLL1:
>                         dev_priv->ddi_plls.wrpll1_refcount++;
>                         break;
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni