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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 07, 2013 at 08:23:33PM +0300, Ville Syrjälä wrote:
> 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.

Yeah, I think you're right. I've botched this part a bit while developing
this series (those half-converted pipe config states are always a pain,
cpu_transcode was equally ugly) and squashed it into the wrong commit.

I'll fix this up.

> 
> > 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.

It's copied over from the get_shared_dpll code where we treat ilk like it
has a shared dpll, but it's fully fixed. Imo it's not worth it to change
that, the shared dpll code can cope with unshared dplls, too ;-)

I'll add a small comment here.

> 
> > +		} 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?

That's very much the idea behind INVALID, but with the currently still
crooked dpll selection in ->mode_set we can't do it. In the end I want the
sequence to be.

1. Set shared_dpll = DPLL_ID_INVALID for every pipe config we compute.
2. Let some platform code figure out which dpll we actually need (i.e.
PRIVATE, NONE or one of the platform specific shared dplls). This won't
yet do the get/put refcounting, since the modeset might still fail in the
compute config, so we shouldn't change state.
3. Wrap the get/put on shared dplls around the update_state code in the
middle of the modeset sequence. If we do the put still with the old pipe
config and the get after the new pipe config has been put into place it'll
all naturally work out.

Once that sequence is in place we can WARN_ON(shared_dpll == INVALID) and
also remove all the various hacks.
> 
> >  
> >  	/* 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch