Re: [PATCH] drm/i915/vlv: modeset_global_* for VLV v6

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

On Mon, Nov 04, 2013 at 11:22:03PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 04, 2013 at 12:54:24PM -0800, Jesse Barnes wrote:
> > On VLV/BYT, we can adjust the CDclk frequency up or down based on the
> > max pixel clock we need to drive.  Lowering it can save power, while
> > raising it is necessary to support high resolution.
> > 
> > Add a new callback in modeset_affected_pipes and a
> > modeset_global_resources function to perform this adjustment as
> > necessary.
> > 
> > v2: use punit interface for 320 and 266 MHz CDclk adjustments (Ville)
> > v3: reset GMBUS dividers too, since we changed CDclk (Ville)
> > v4: jump to highest voltage when going to 400MHz CDclk (Jesse)
> > v5: drop duplicate define (Ville)
> >     use shifts by 1 for fixed point (Ville)
> >     drop new callback (Daniel)
> > v6: fixup adjusted_mode.clock -> adjusted_mode.crtc_clock again (Ville)
> >     document Bunit reg access better (Ville)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |   9 ++
> >  drivers/gpu/drm/i915/intel_display.c | 179 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_i2c.c     |   4 -
> >  3 files changed, 188 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 737d8a3..d644d75 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -360,9 +360,17 @@
> >  #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
> >  #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
> >  
> > +/* See configdb bunit SB addr map */
> > +#define BUNIT_REG_BISOC				0x11
> > +
> >  #define PUNIT_OPCODE_REG_READ			6
> >  #define PUNIT_OPCODE_REG_WRITE			7
> >  
> > +#define PUNIT_REG_DSPFREQ			0x36
> > +#define   DSPFREQSTAT_SHIFT			30
> > +#define   DSPFREQSTAT_MASK			(0x3 << DSPFREQSTAT_SHIFT)
> > +#define   DSPFREQGUAR_SHIFT			14
> > +#define   DSPFREQGUAR_MASK			(0x3 << DSPFREQGUAR_SHIFT)
> >  #define PUNIT_REG_PWRGT_CTRL			0x60
> >  #define PUNIT_REG_PWRGT_STATUS			0x61
> >  #define	  PUNIT_CLK_GATE			1
> > @@ -425,6 +433,7 @@
> >  #define  DSI_PLL_N1_DIV_MASK			(3 << 16)
> >  #define  DSI_PLL_M1_DIV_SHIFT			0
> >  #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
> > +#define CCK_DISPLAY_CLOCK_CONTROL		0x6b
> >  
> >  /*
> >   * DPIO - a special bus for various display related registers to hide behind
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 606a594..ae83f01 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3894,6 +3894,175 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
> >  	I915_WRITE(BCLRPAT(crtc->pipe), 0);
> >  }
> >  
> > +static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > +{
> > +	int vco;
> > +
> > +	switch (dev_priv->mem_freq) {
> > +	default:
> > +	case 800:
> > +		vco = 800;
> > +		break;
> > +	case 1066:
> > +		vco = 1600;
> > +		break;
> > +	case 1333:
> > +		vco = 2000;
> > +		break;
> > +	}
> > +
> > +	return vco;
> > +}
> > +
> > +/* Adjust CDclk dividers to allow high res or save power if possible */
> > +static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u32 val, cmd;
> > +
> > +	if (cdclk >= 320) /* jump to highest voltage for 400MHz too */
> > +		cmd = 2;
> > +	else if (cdclk == 266)
> > +		cmd = 1;
> > +	else
> > +		cmd = 0;
> > +
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> > +	val &= ~DSPFREQGUAR_MASK;
> > +	val |= (cmd << DSPFREQGUAR_SHIFT);
> > +	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> > +	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> > +		      DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> > +		     50)) {
> > +		DRM_ERROR("timed out waiting for CDclk change\n");
> > +	}
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +	if (cdclk == 400) {
> > +		u32 divider, vco;
> > +
> > +		vco = valleyview_get_vco(dev_priv);
> > +		divider = ((vco << 1) / cdclk) - 1;
> > +
> > +		mutex_lock(&dev_priv->dpio_lock);
> > +		/* adjust cdclk divider */
> > +		val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
> > +		val &= ~0xf;
> > +		val |= divider;
> > +		vlv_cck_write(dev_priv, CCK_DISPLAY_CLOCK_CONTROL, val);
> > +		mutex_unlock(&dev_priv->dpio_lock);
> > +	}
> > +
> > +	mutex_lock(&dev_priv->dpio_lock);
> > +	/* adjust self-refresh exit latency value */
> > +	val = vlv_bunit_read(dev_priv, BUNIT_REG_BISOC);
> > +	val &= ~0x7f;
> > +
> > +	/*
> > +	 * For high bandwidth configs, we set a higher latency in the bunit
> > +	 * so that the core display fetch happens in time to avoid underruns.
> > +	 */
> > +	if (cdclk == 400)
> > +		val |= 4500 / 250; /* 4.5 usec */
> > +	else
> > +		val |= 3000 / 250; /* 3.0 usec */
> > +	vlv_bunit_write(dev_priv, BUNIT_REG_BISOC, val);
> > +	mutex_unlock(&dev_priv->dpio_lock);
> > +
> > +	/* Since we changed the CDclk, we need to update the GMBUSFREQ too */
> > +	intel_i2c_reset(dev);
> > +}
> > +
> > +static int valleyview_cur_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > +	int cur_cdclk, vco;
> > +	int divider;
> > +
> > +	vco = valleyview_get_vco(dev_priv);
> > +
> > +	mutex_lock(&dev_priv->dpio_lock);
> > +	divider = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
> > +	mutex_unlock(&dev_priv->dpio_lock);
> > +
> > +	divider &= 0xf;
> > +
> > +	cur_cdclk = (vco << 1) / (divider + 1);
> > +
> > +	return cur_cdclk;
> > +}
> > +
> > +static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
> > +				 int max_pixclk)
> > +{
> > +	int cur_cdclk;
> > +
> > +	cur_cdclk = valleyview_cur_cdclk(dev_priv);
> > +
> > +	/*
> > +	 * Really only a few cases to deal with, as only 4 CDclks are supported:
> > +	 *   200MHz
> > +	 *   267MHz
> > +	 *   320MHz
> > +	 *   400MHz
> > +	 * So we check to see whether we're above 90% of the lower bin and
> > +	 * adjust if needed.
> > +	 */
> > +	if (max_pixclk > 288000) {
> > +		return 400;
> > +	} else if (max_pixclk > 240000) {
> > +		return 320;
> > +	} else
> > +		return 266;
> > +	/* Looks like the 200MHz CDclk freq doesn't work on some configs */
> > +}
> > +
> > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *intel_crtc;
> > +	int max_pixclk = 0;
> > +
> > +	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> > +			    base.head) {
> > +		if (!intel_crtc->base.enabled)
> > +			continue;
> > +
> > +		max_pixclk = max(max_pixclk,
> > +				 intel_crtc->config.adjusted_mode.crtc_clock);
> > +	}
> > +
> > +	return max_pixclk;
> > +}
> > +
> > +static void valleyview_modeset_global_pipes(struct drm_device *dev,
> > +					    unsigned *prepare_pipes)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc;
> > +	int max_pixclk = intel_mode_max_pixclk(dev_priv);
> > +	int cur_cdclk = valleyview_cur_cdclk(dev_priv);
> > +
> > +	if (valleyview_calc_cdclk(dev_priv, max_pixclk) == cur_cdclk)
> > +		return;
> > +
> > +	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> > +			    base.head)
> > +		if (intel_crtc->base.enabled)
> > +			*prepare_pipes |= (1 << intel_crtc->pipe);
> > +}
> > +
> >
> 
> 
> So now the problem is now that we have to calculate max_pixclk before
> intel_crtc->config has been update for modeset_pipes.
> 
> If we make intel_mode_max_pixclk() and valleyview_modeset_global_pipes() like so:
> 
> static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv,
> 				unsigned int modeset_pipes,
> 				struct intel_crtc_config *pipe_config)
> {
> 	struct drm_device *dev = dev_priv->dev;
> 	struct intel_crtc *intel_crtc;
> 	int max_pixclk = 0;
> 
> 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> 		if (!intel_crtc->base.enabled)
> 			continue;

Hmm. After a second though, this enabled check is still wrong. But we
know that if the pipe is in modeset_pipes it will be enabled, so we
could move the enabled check to the second branch like so:

if (modeset_pipes & (1 << pipe))
	max(pipe_config);
else if (crtc->enabled)
	max(intel_crtc->config);


> 
> 		if (modeset_pipes & (1 << intel_crtc_pipe))
> 			max_pixclk = max(max_pixclk, pipe_config->adjusted_mode.crtc_clock);
> 		else
> 			max_pixclk = max(max_pixclk, intel_crtc->config.adjusted_mode.crtc_clock);
> 	}
> 
> 	return max_pixclk;
> }
> 
> static void valleyview_modeset_global_pipes(struct drm_device *dev,
> 					    unsigned *prepare_pipes,
> 					    unsigned int modeset_pipes,
> 					    struct intel_crtc_config *pipe_config)
> {
> 	struct drm_i915_private *dev_priv = dev->dev_private;
> 	struct intel_crtc *intel_crtc;
> 	int max_pixclk = intel_mode_max_pixclk(dev_priv, modeset_pipes, pipe_config);
> 	int cur_cdclk = valleyview_cur_cdclk(dev_priv);
> 
> 	if (valleyview_calc_cdclk(dev_priv, max_pixclk) == cur_cdclk)
> 		return;
> 
> 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> 			    base.head)
> 		if (intel_crtc->base.enabled)
> 			*prepare_pipes |= (1 << intel_crtc->pipe);
> }
> 
> And then we calculate it after the new pipe config has been computed like so:
> 
> 	...
> 	if (modeset_pipes) {
> 		pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> 	        if (IS_ERR(pipe_config)) {
>                         ret = PTR_ERR(pipe_config);
>                         pipe_config = NULL;
> 
> 		        goto out;
>                 }
>                 intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> 		                       "[modeset]");
> 	}
> 
> +	if (IS_VALLEYVIEW(dev))
> +		valleyview_modeset_global_pipes(dev, &prepare_pipes, modeset_pipes, pipe_config);
> 
> 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>                 intel_crtc_disable(&intel_crtc->base);
> 	...
> 
> 
> Then things should just work (tm).
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC