[Intel-gfx] [PATCH] drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc

[Damien Lespiau]

On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing
> dpll state and recomputing it via crtc_compute_clock (and probably some
> other commit which triggered pipe config checking), modesetting is now
> constantly triggering warnings about dpll_hw_state.ctrl1 mismatch.
> 
> Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do
> anything for eDP, leaving the ctrl1 state at the default of zero.
> 
> This potentially hacky fix makes skl_ddi_pll_select call
> skl_edp_set_pll_config which fixes the problem for me.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>

Nop! (at least I really don't think so).

As discussed on IRC, on DDI platforms, the (e)DP compute_config() does
the private DPLL selection, and the ddi_pll_select() does the the same
for shared DPLLs (I'm not saying that the end result we want, just how
it works today). That split comes from the introduction of shared_dpll
in the DDI PLL selection last summer.

Anyway, this means that, for SKL, dpll_hw_state is touched by the
encoder's compute_config() for (e)DP.

I think we could just remove the memset() in 4978cc93 (maybe?), or try
to unify a bit better and only have one place where we do PLL selection
(which I assume is part of the bigger atomic plan). Not
skl_edp_set_pll_config() in both compute_config() and ddi_pll_select()
though.

-- 
Damien

> ---
> 
> Big CC list so no one misses the chance of saying how this is not the
> right fix. :) But that is OK, I was just annoyed by the constant stream
> of warnings obscuring real problems.
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 4 +++-
>  drivers/gpu/drm/i915/intel_dp.c  | 2 +-
>  drivers/gpu/drm/i915/intel_drv.h | 3 +++
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 807e15d..e5b7723 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1306,8 +1306,10 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		}
>  
>  		cfgcr1 = cfgcr2 = 0;
> -	} else /* eDP */
> +	} else /* eDP */ {
> +		skl_edp_set_pll_config(crtc_state, clock);
>  		return true;
> +	}
>  
>  	crtc_state->dpll_hw_state.ctrl1 = ctrl1;
>  	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 75bccd6..13b5b0e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1092,7 +1092,7 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector)
>  	intel_connector_unregister(intel_connector);
>  }
>  
> -static void
> +void
>  skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
>  {
>  	u32 ctrl1;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index aa77af7..6e26644 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1193,6 +1193,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>  		unsigned frontbuffer_bits);
>  void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
>  
> +void
> +skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock);
> +
>  /* intel_dp_mst.c */
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>  void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
> -- 
> 2.4.0
>