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
