Re: [PATCH 12/13] drm/i915: Manage HSW/BDW LCPLLs with the shared dpll interface

Maarten Lankhorst
2016-03-08T11:05:17Z

Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira:
> Manage the LCPLLs used with DisplayPort, so that all the HSW/BDW DPLLs
> are managed by the shared dpll code.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 18 ++++----
> drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++----
> drivers/gpu/drm/i915/intel_dp.c | 23 +---------
> drivers/gpu/drm/i915/intel_dp_mst.c | 4 --
> drivers/gpu/drm/i915/intel_dpll_mgr.c | 79 +++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_dpll_mgr.h | 5 ++-
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> 7 files changed, 110 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ad7888c..3cb9f36 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -992,17 +992,13 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> {
> struct intel_shared_dpll *pll;
>
> - if (intel_encoder->type == INTEL_OUTPUT_HDMI ||
> - intel_encoder->type == INTEL_OUTPUT_ANALOG) {
> - pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> - intel_encoder);
> - if (!pll)
> - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> - pipe_name(intel_crtc->pipe));
> - return pll;
> - } else {
> - return true;
> - }
> + pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> + intel_encoder);
> + if (!pll)
> + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> + pipe_name(intel_crtc->pipe));
> +
> + return pll;
> }
Eventually the reliance on intel_encoder->type should end here, and based on the connector.
It would fix some kms tests, but that would be better to do in a future patch.
> ...
>
> +static bool hsw_ddi_lcpll_get_hw_state(struct drm_i915_private *dev_priv,
> + struct intel_shared_dpll *pll,
> + struct intel_dpll_hw_state *hw_state)
> +{
> + /*
> + * LC PLL is kept enabled all the time since it drives CDCLK. The
> + * state checker still expects it to be disabled when it is not used
> + * by any crtc. To avoid adding a case to LC PLL, just tell the
> + * state checker what it expects.
> + */
> + if (pll->config.crtc_mask)
> + return true;
> + else
> + return false;
> +}
Wouldn't it be better to return true or the real hardware state then, and set the ALWAYS_ON flag from the next patch?
> +static const struct intel_shared_dpll_funcs hsw_ddi_lcpll_funcs = {
> + .enable = hsw_ddi_lcpll_enable,
> + .disable = hsw_ddi_lcpll_disable,
> + .get_hw_state = hsw_ddi_lcpll_get_hw_state,
> +};
> +
> struct skl_dpll_regs {
> i915_reg_t ctl, cfgcr1, cfgcr2;
> };
> @@ -1559,9 +1617,12 @@ static const struct intel_dpll_mgr pch_pll_mgr = {
> };
>
> static const struct dpll_info hsw_plls[] = {
> - { "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs },
> - { "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs },
> - { "SPLL", DPLL_ID_SPLL, &hsw_ddi_spll_funcs },
> + { "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs },
> + { "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs },
> + { "SPLL", DPLL_ID_SPLL, &hsw_ddi_spll_funcs },
> + { "LCPLL 810", DPLL_ID_LCPLL_810, &hsw_ddi_lcpll_funcs },
> + { "LCPLL 1350", DPLL_ID_LCPLL_1350, &hsw_ddi_lcpll_funcs },
> + { "LCPLL 2700", DPLL_ID_LCPLL_2700, &hsw_ddi_lcpll_funcs },
> { NULL, -1, NULL, },
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 82e53f5..872878e 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -49,13 +49,16 @@ enum intel_dpll_id {
> DPLL_ID_WRPLL1 = 0,
> DPLL_ID_WRPLL2 = 1,
> DPLL_ID_SPLL = 2,
> + DPLL_ID_LCPLL_810 = 3,
> + DPLL_ID_LCPLL_1350 = 4,
> + DPLL_ID_LCPLL_2700 = 5,
>
> /* skl */
> DPLL_ID_SKL_DPLL1 = 0,
> DPLL_ID_SKL_DPLL2 = 1,
> DPLL_ID_SKL_DPLL3 = 2,
> };
> -#define I915_NUM_PLLS 3
> +#define I915_NUM_PLLS 6
>
> struct intel_dpll_hw_state {
> /* i9xx, pch plls */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c9e5030..18aa287 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1258,7 +1258,6 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
> void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
> bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> struct intel_digital_port *port);
> -void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
>
> void
> intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,