[Intel-gfx] [PATCH 62/89] drm/i915/skl: Query DPLL attached to port on SKL

[Paulo Zanoni]

2014-09-04 8:27 GMT-03:00 Damien Lespiau <damien.lespiau at intel.com>:
> From: Satheeshakrishna M <satheeshakrishna.m at intel.com>
>
> Modify the implementation to query DPLL attached to a SKL port.
>
> v2: Rebase on top of the run-time PM on DPMS series (Damien)
>
> Signed-off-by: Satheeshakrishna M <satheeshakrishna.m at intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69e023a..6e71250 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7789,6 +7789,28 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         return 0;
>  }
>
> +static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv,
> +                               enum port port,
> +                               struct intel_crtc_config *pipe_config)
> +{
> +       u32 temp;
> +
> +       temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
> +       pipe_config->ddi_pll_sel = temp >> (port * 3 + 1);

Since we're relying on the fact that ddi_pll_sel is actually "enum
skl_dpll", I'd change the definition of the enum and explicitly
declare SKL_DPLL0 to be "0", and SKL_DPLL1 to be 1, and so on.

> +
> +       switch (pipe_config->ddi_pll_sel) {
> +       case SKL_DPLL1:
> +               pipe_config->shared_dpll = DPLL_ID_SKL_DPLL1;
> +               break;
> +       case SKL_DPLL2:
> +               pipe_config->shared_dpll = DPLL_ID_SKL_DPLL2;
> +               break;
> +       case SKL_DPLL3:
> +               pipe_config->shared_dpll = DPLL_ID_SKL_DPLL3;
> +               break;

Please also add a WARN() to the default case, especially since there
are 4 possible values and we're just checking 3.

> +       }
> +}
> +
>  static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>                                 enum port port,
>                                 struct intel_crtc_config *pipe_config)
> @@ -7818,7 +7840,10 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>
>         port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
>
> -       haswell_get_ddi_pll(dev_priv, port, pipe_config);
> +       if (IS_SKYLAKE(dev))

If you invert the check so that SKL is on the "else" part (as you did
in your previous patch), you'll make sure we're ready for the next
Gens in case they happen to be same as SKL. It's more likely they will
be the same as SKL instead of being the same as HSW :)


With at least the WARN added on the switch statement: Reviewed-by:
Paulo Zanoni <paulo.r.zanoni at intel.com>

> +               skylake_get_ddi_pll(dev_priv, port, pipe_config);
> +       else
> +               haswell_get_ddi_pll(dev_priv, port, pipe_config);
>
>         if (pipe_config->shared_dpll >= 0) {
>                 pll = &dev_priv->shared_dplls[pipe_config->shared_dpll];
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 559b747..9558f07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -323,7 +323,10 @@ struct intel_crtc_config {
>         /* Selected dpll when shared or DPLL_ID_PRIVATE. */
>         enum intel_dpll_id shared_dpll;
>
> -       /* PORT_CLK_SEL for DDI ports. */
> +       /*
> +        * - PORT_CLK_SEL for DDI ports on HSW/BDW.
> +        * - enum skl_dpll on SKL
> +        */
>         uint32_t ddi_pll_sel;
>
>         /* Actual register state of the dpll, for shared dpll cross-checking. */
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni