[Intel-gfx] [PATCH 10/21] drm/i915: introduce lpt_enable_pch and cpt_enable_pch

[Paulo Zanoni]

Hi

I certainly like this function split. Reviews inline:

2012/6/28 Eugeni Dodonov <eugeni.dodonov at intel.com>:
> CPT/PPT and LPT have different functionality. So we introduce specific
> functions to handle each of them instead of using multiple if..
> statements.
>
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++++++++++++++++----
>  1 file changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d7b337b..9a695ab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2968,6 +2968,51 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
>         I915_WRITE(PIXCLK_GATE, PIXCLK_GATE_UNGATE);
>  }
>
> +/* On Lynx Point, the PCH part is different, as it has one FDI RX, one
> + * transcoder, and the clock is driven by iCLKIP.
> + */
> +static void lpt_pch_enable(struct drm_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       int pipe = intel_crtc->pipe;
> +
> +       /* On LPT, there is only one FDI RX and transcoder, which can be driven
> +        * by any pipe. To simplify, we consider that only pipeA can be used in
> +        * such mode.
> +        */
> +       if (HAS_PCH_LPT(dev_priv->dev)) {
> +               if (pipe > 0) {
> +                       DRM_ERROR("Attempting to enable PCH port with pipe > 0\n");
> +                       return;
> +               }
> +       }

It looks like the check above could go on a separate commit. The first
commit just moves code around, the second one adds the check.

I may have misunderstood something about this code which is not
familiar to me, but instead of limiting everything to pipe 0, can't we
just iterate through all pipes, read DDI_FUNC_CTL(pipe) and see if
there's any other enabled pipe on FDI mode?

> +
> +       assert_transcoder_disabled(dev_priv, pipe);
> +
> +       /* For PCH output, training FDI link */
> +       dev_priv->display.fdi_link_train(crtc);
> +
> +       intel_enable_pch_pll(intel_crtc);
> +
> +       DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");

This message could go inside lpt_program_iclkip :) We should also
paint it green.

> +       lpt_program_iclkip(crtc);
> +
> +       /* set transcoder timing, panel must allow it */
> +       assert_panel_unlocked(dev_priv, pipe);
> +       I915_WRITE(TRANS_HTOTAL(pipe), I915_READ(HTOTAL(pipe)));
> +       I915_WRITE(TRANS_HBLANK(pipe), I915_READ(HBLANK(pipe)));
> +       I915_WRITE(TRANS_HSYNC(pipe),  I915_READ(HSYNC(pipe)));
> +
> +       I915_WRITE(TRANS_VTOTAL(pipe), I915_READ(VTOTAL(pipe)));
> +       I915_WRITE(TRANS_VBLANK(pipe), I915_READ(VBLANK(pipe)));
> +       I915_WRITE(TRANS_VSYNC(pipe),  I915_READ(VSYNC(pipe)));
> +       I915_WRITE(TRANS_VSYNCSHIFT(pipe),  I915_READ(VSYNCSHIFT(pipe)));
> +
> +       intel_enable_transcoder(dev_priv, pipe);
> +}
> +
>  /*
>   * Enable PCH resources required for PCH ports:
>   *   - PCH PLLs
> @@ -2976,7 +3021,7 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
>   *   - DP transcoding bits
>   *   - transcoder
>   */
> -static void ironlake_pch_enable(struct drm_crtc *crtc)
> +static void cpt_pch_enable(struct drm_crtc *crtc)

Since this will run on both IBX and CPT, shouldn't we call it
ibx_pch_enable? At least on intel_hdmi.c, the functions are named
after the earliest generation that can run them.

>  {
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2991,10 +3036,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>
>         intel_enable_pch_pll(intel_crtc);
>
> -       if (HAS_PCH_LPT(dev)) {
> -               DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
> -               lpt_program_iclkip(crtc);
> -       } else if (HAS_PCH_CPT(dev)) {
> +       if (HAS_PCH_CPT(dev)) {
>                 u32 sel;
>
>                 temp = I915_READ(PCH_DPLL_SEL);
> @@ -3031,8 +3073,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>         I915_WRITE(TRANS_VSYNC(pipe),  I915_READ(VSYNC(pipe)));
>         I915_WRITE(TRANS_VSYNCSHIFT(pipe),  I915_READ(VSYNCSHIFT(pipe)));
>
> -       if (!IS_HASWELL(dev))
> -               intel_fdi_normal_train(crtc);
> +       intel_fdi_normal_train(crtc);
>
>         /* For PCH DP, enable TRANS_DP_CTL */
>         if (HAS_PCH_CPT(dev) &&
> @@ -3075,6 +3116,21 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>         intel_enable_transcoder(dev_priv, pipe);
>  }
>
> +/*
> + * When we need to use a PCH port, we can go through different paths on CPT/PPT
> + * and LPT chipsets.
> + */
> +static void ironlake_pch_enable(struct drm_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (HAS_PCH_LPT(dev_priv->dev))
> +               return lpt_pch_enable(crtc);
> +       else
> +               return cpt_pch_enable(crtc);
> +}
> +
>  static void intel_put_pch_pll(struct intel_crtc *intel_crtc)
>  {
>         struct intel_pch_pll *pll = intel_crtc->pch_pll;
> --
> 1.7.11.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni