Re: [PATCH 08/31] drm/i915: hw state readout for shared pch plls

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 05, 2013 at 01:34:10PM +0200, Daniel Vetter wrote:
> Well, the first step of a long road at least, it only reads out
> the pipe -> shared dpll association thus far. Other state which needs
> to follow:
> 
> - hw state of the dpll (on/off + dpll registers). Currently we just
>   read that out from the hw state, but that doesn't work too well when
>   the dpll is in use, but not yet fully enabled. We get away since
>   most likely it already has been enabled and so the correct state is
>   left behind in the registers. But that doesn't hold for atomic
>   modesets when we want to enable all pipes at once.
> 
> - Refcount reconstruction for each dpll.
> 
> - Cross-checking of all the above. For that we need to keep the dpll
>   register state both in the pipe and in the shared_dpll struct, so
>   that we can check that every pipe is still connected to a correctly
>   configured dpll.
> 
> Note that since the refcount resconstruction isn't done yet this will
> spill a few WARNs at boot-up while trying to disable pch plls which
> have bogus refcounts. But since there's still a pile of refactoring to
> do I'd like to lock down the state handling as soon as possible hence
> decided against reordering the patches to quiet these WARNs - after
> all the issues they're complaining about have existed since forever,
> as Jesse can testify by having pch pll states blow up consistently in
> his fastboot patches ...
> 
> v2: We need to preserve the old shared_dpll since currently the
> shared dpll refcount dropping/getting is done in ->mode_set. With
> the usual pipe_config infrastructure the old dpll id is already lost
> at that point, hence preserve it in the new config.

Ah so here is the missing link. The correct place for this would appear
to be patch 6.

> v3: Rebase on top of the ips patch from Paulo.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a44c43c..20a933f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4020,12 +4020,11 @@ static void hsw_compute_ips_config(struct intel_crtc *crtc,
>  				   pipe_config->pipe_bpp == 24;
>  }
>  
> -static int intel_crtc_compute_config(struct drm_crtc *crtc,
> +static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  				     struct intel_crtc_config *pipe_config)
>  {
> -	struct drm_device *dev = crtc->dev;
> +	struct drm_device *dev = crtc->base.dev;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  	if (HAS_PCH_SPLIT(dev)) {
>  		/* FDI link clock is fixed at 2.7G */
> @@ -4056,10 +4055,16 @@ static int intel_crtc_compute_config(struct drm_crtc *crtc,
>  	}
>  
>  	if (IS_HASWELL(dev))
> -		hsw_compute_ips_config(intel_crtc, pipe_config);
> +		hsw_compute_ips_config(crtc, pipe_config);
> +
> +	/* XXX: PCH clock sharing is done in ->mode_set, so make sure the old
> +	 * clock survives for now. */
> +	if (pipe_config->has_pch_encoder &&
> +	    (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)))
> +		pipe_config->shared_dpll = crtc->config.shared_dpll;
>  
>  	if (pipe_config->has_pch_encoder)
> -		return ironlake_fdi_compute_config(intel_crtc, pipe_config);
> +		return ironlake_fdi_compute_config(crtc, pipe_config);
>  
>  	return 0;
>  }
> @@ -4934,6 +4939,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	uint32_t tmp;
>  
>  	pipe_config->cpu_transcoder = crtc->pipe;
> +	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> @@ -5810,6 +5816,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	uint32_t tmp;
>  
>  	pipe_config->cpu_transcoder = crtc->pipe;
> +	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> @@ -5827,6 +5834,16 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  		/* XXX: Can't properly read out the pch dpll pixel multiplier
>  		 * since we don't have state tracking for pch clocks yet. */
>  		pipe_config->pixel_multiplier = 1;
> +
> +		if (HAS_PCH_IBX(dev_priv->dev)) {
> +			pipe_config->shared_dpll = crtc->pipe;

Slightly magic, but all right.

> +		} else {
> +			tmp = I915_READ(PCH_DPLL_SEL);
> +			if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
> +				pipe_config->shared_dpll = DPLL_ID_PCH_PLL_B;
> +			else
> +				pipe_config->shared_dpll = DPLL_ID_PCH_PLL_A;
> +		}
>  	} else {
>  		pipe_config->pixel_multiplier = 1;
>  	}
> @@ -5907,6 +5924,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	uint32_t tmp;
>  
>  	pipe_config->cpu_transcoder = crtc->pipe;
> +	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
> +
>  	tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
>  	if (tmp & TRANS_DDI_FUNC_ENABLE) {
>  		enum pipe trans_edp_pipe;
> @@ -7768,6 +7787,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	drm_mode_copy(&pipe_config->adjusted_mode, mode);
>  	drm_mode_copy(&pipe_config->requested_mode, mode);
>  	pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
> +	pipe_config->shared_dpll = DPLL_ID_PRIVATE;

Do we want to put this here, or should we have some DPLL_ID_INVALID, so
that we'd always catch cases where we forgot to really think about which
PLL should be used?

>  
>  	/* Compute a starting value for pipe_config->pipe_bpp taking the source
>  	 * plane pixel format and any sink constraints into account. Returns the
> @@ -7816,7 +7836,7 @@ encoder_retry:
>  	if (!pipe_config->port_clock)
>  		pipe_config->port_clock = pipe_config->adjusted_mode.clock;
>  
> -	ret = intel_crtc_compute_config(crtc, pipe_config);
> +	ret = intel_crtc_compute_config(to_intel_crtc(crtc), pipe_config);
>  	if (ret < 0) {
>  		DRM_DEBUG_KMS("CRTC fixup failed\n");
>  		goto fail;
> @@ -8079,6 +8099,8 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  	PIPE_CONF_CHECK_I(ips_enabled);
>  
> +	PIPE_CONF_CHECK_I(shared_dpll);
> +
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_FLAGS
>  
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC