Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Arun R Murthy <arun.r.murthy@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 12/12] drm/i915: Disable DDR DVFS on CHV
Date: Mon, 9 Mar 2015 17:00:09 +0200	[thread overview]
Message-ID: <20150309150009.GT11371@intel.com> (raw)
In-Reply-To: <54FD2515.9080409@intel.com>

On Mon, Mar 09, 2015 at 10:14:05AM +0530, Arun R Murthy wrote:
> 
> On Friday 06 March 2015 12:49 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > DDR DVFS introduces massive memory latencies which can't be handled by
> > the PND deadline stuff. Instead the watermarks will need to be
> > programmed to compensate for the latency and the deadlines will need to
> > be programmed to tight fixed values. That means DDR DVFS can only be
> > enabled if the display FIFOs are large enough, and that pretty much
> > means we have to manually repartition them to suit the needs of the
> > moment.
> >
> > That's a lot of change, so in the meantime let's just disable DDR DVFS
> > to get the display(s) to be stable.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> 
> Since this patch disabled DDR DVFS in the driver, can this be done
> once. Here I assume that DDR DVFS gets disabled on each update
> watermark call.

Yeah it did occur to me that we could get away with disabling just once,
but once we get the DDR DVFS enabled we're going to be changing it more,
potentially at every watermark update (although having to do it every
time is rather unlikely).

In any case, you may have noticed that I skip the entire WM programming
if the calculated watermarks didn't change from the old values, so we
won't be frobbing the Punit unless something in the WM values has actually
changed. So I figured that's good enough for now.

My plan is to optimize away redundant Punit communication (for both PM5
and DDR DVFS) eventually, but that is part of the task to get DDR DVFS
enabled in the first place.

> Moreover enabling DDR DVFS should be followed with updating
> watermarks and display arbiter.
> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> >   drivers/gpu/drm/i915/i915_reg.h |  5 +++++
> >   drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 5a20f58..744d162 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -630,6 +630,11 @@ enum skl_disp_power_wells {
> >   #define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
> >   #define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
> >   
> > +#define PUNIT_REG_DDR_SETUP2			0x139
> > +#define   FORCE_DDR_FREQ_REQ_ACK		(1 << 8)
> > +#define   FORCE_DDR_LOW_FREQ			(1 << 1)
> > +#define   FORCE_DDR_HIGH_FREQ			(1 << 0)
> > +
> >   #define PUNIT_GPU_STATUS_REG			0xdb
> >   #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
> >   #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1dd82ec..fc03e24 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -235,6 +235,28 @@ static const struct cxsr_latency *intel_get_cxsr_latency(int is_desktop,
> >   	return NULL;
> >   }
> >   
> > +static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> > +{
> > +	u32 val;
> > +
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +
> > +	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> > +	if (enable)
> > +		val &= ~FORCE_DDR_HIGH_FREQ;
> > +	else
> > +		val |= FORCE_DDR_HIGH_FREQ;
> > +	val &= ~FORCE_DDR_LOW_FREQ;
> > +	val |= FORCE_DDR_FREQ_REQ_ACK;
> > +	vlv_punit_write(dev_priv, PUNIT_REG_DDR_SETUP2, val);
> > +
> > +	if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2) &
> > +		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> > +		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> > +
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +}
> > +
> >   static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >   {
> >   	u32 val;
> > @@ -282,6 +304,7 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
> >   		      enable ? "enabled" : "disabled");
> >   }
> >   
> > +
> >   /*
> >    * Latency for FIFO fetches is dependent on several factors:
> >    *   - memory configuration (speed, channels)
> > @@ -992,6 +1015,17 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> >   		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> >   		      wm.sr.plane, wm.sr.cursor);
> >   
> > +	/*
> > +	 * FIXME DDR DVFS introduces massive memory latencies which
> > +	 * are not known to system agent so any deadline specified
> > +	 * by the display may not be respected. To support DDR DVFS
> > +	 * the watermark code needs to be rewritten to essentially
> > +	 * bypass deadline mechanism and rely solely on the
> > +	 * watermarks. For now disable DDR DVFS.
> > +	 */
> > +	if (IS_CHERRYVIEW(dev_priv))
> > +		chv_set_memory_dvfs(dev_priv, false);
> > +
> >   	if (!cxsr_enabled)
> >   		intel_set_memory_cxsr(dev_priv, false);
> >   

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