Re: [PATCH 11/17] drm/i915: Make sure DPLL is enabled when kicking the power sequencer on VLV/CHV

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

On Tue, Oct 28, 2014 at 09:22:12AM +0100, Daniel Vetter wrote:
> On Thu, Oct 16, 2014 at 09:29:45PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The power seqeuencer kick procedure requires the DPLL to be running
> > in order to complete succesfully. In case the DPLL isn't currently
> > running when we need to kick the power seqeuncer enable it
> > temporarily. This can happen eg. during ->detect() when the pipe is
> > not already active.
> > 
> > To avoid needlessly duplicating the DPLL programming re-use the already
> > existing functions by passing a temporary pipe config to them instead
> > of having them consult the current pipe config at crtc->config.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> [snip]
> 
> > +	/*
> > +	 * The DPLL for the pipe must be enabled for this to work.
> > +	 * So enable temporarily it if it's not already enabled.
> > +	 */
> > +	if (!pll_enabled) {
> > +		struct intel_crtc *crtc =
> > +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > +		struct intel_crtc_config pipe_config = {
> > +			.pixel_multiplier = 1,
> > +			.dpll = IS_CHERRYVIEW(dev) ?
> > +				chv_dpll[0].dpll : vlv_dpll[0].dpll,
> > +			.port_clock = 162000,
> > +			.clock_set = true,
> > +			.has_dp_encoder = true,
> > +		};
> > +
> > +		if (IS_CHERRYVIEW(dev)) {
> > +			chv_update_pll(crtc, &pipe_config);
> > +			chv_prepare_pll(crtc, &pipe_config);
> > +			chv_enable_pll(crtc, &pipe_config);
> > +		} else {
> > +			vlv_update_pll(crtc, &pipe_config);
> > +			vlv_prepare_pll(crtc, &pipe_config);
> > +			vlv_enable_pll(crtc, &pipe_config);
> 
> I'm not terribly happy with the massive list of non-static platform
> functions this exposes. And it's also fairly leaky since the minimal pipe
> config depends upon what exactly the vlv/chv pll functions need.
> 
> Could we instead have a wrapper function for both the enable and disable
> which sits right next to the vlv/chv dpll code in intel_display.c? E.g.
> valleyview_force_pll_on/off(dev, pipe) or something like that.

Sure. I'll respin this with that idea.

> -Daniel
> 
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * Similar magic as in intel_dp_enable_port().
> >  	 * We _must_ do this port enable + disable trick
> > @@ -365,6 +395,13 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
> >  
> >  	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
> >  	POSTING_READ(intel_dp->output_reg);
> > +
> > +	if (!pll_enabled) {
> > +		if (IS_CHERRYVIEW(dev))
> > +			chv_disable_pll(dev_priv, pipe);
> > +		else
> > +			vlv_disable_pll(dev_priv, pipe);
> > +	}
> >  }
> >  
> >  static enum pipe
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f7ba1fc..69c8c5f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -911,6 +911,21 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
> >  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc);
> >  void intel_put_shared_dpll(struct intel_crtc *crtc);
> >  
> > +void vlv_update_pll(struct intel_crtc *crtc,
> > +		    struct intel_crtc_config *pipe_config);
> > +void chv_update_pll(struct intel_crtc *crtc,
> > +		    struct intel_crtc_config *pipe_config);
> > +void vlv_prepare_pll(struct intel_crtc *crtc,
> > +		     const struct intel_crtc_config *pipe_config);
> > +void chv_prepare_pll(struct intel_crtc *crtc,
> > +		     const struct intel_crtc_config *pipe_config);
> > +void vlv_enable_pll(struct intel_crtc *crtc,
> > +		    const struct intel_crtc_config *pipe_config);
> > +void chv_enable_pll(struct intel_crtc *crtc,
> > +		    const struct intel_crtc_config *pipe_config);
> > +void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> > +void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe);
> > +
> >  /* modesetting asserts */
> >  void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> >  			   enum pipe pipe);
> > -- 
> > 2.0.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx