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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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,

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx