Re: [PATCH v2 09/12] drm/i915: Rewrite VLV/CHV watermark code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 06, 2015 at 09:31:20AM -0800, Jesse Barnes wrote:
> On 03/05/2015 11:19 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > Assuming the PND deadline mechanism works reasonably we should do
> > memory requests as early as possible so that PND has schedule the
> > requests more intelligently. Currently we're still calculating
> > the watermarks as if VLV/CHV are identical to g4x, which isn't
> > the case.
> > 
> > The current code also seems to calculate insufficient watermarks
> > and hence we're seeing some underruns, especially on high resolution
> > displays.
> > 
> > To fix it just rip out the current code and replace is with something
> > that tries to utilize PND as efficiently as possible.
> > 
> > We now calculate the WM watermark to trigger when the FIFO still has
> > 256us worth of data. 256us is the maximum deadline value supoorted by
> > PND, so issuing memory requests earlier would mean we probably couldn't
> > utilize the full FIFO as PND would attempt to return the data at
> > least in at least 256us. We also clamp the watermark to at least 8
> > cachelines as that's the magic watermark that enabling trickle feed
> > would also impose. I'm assuming it matches some burst size.
> > 
> > In theory we could just enable trickle feed and ignore the WM values,
> > except trickle feed doesn't work with max fifo mode anyway, so we'd
> > still need to calculate the SR watermarks. It seems cleaner to just
> > disable trickle feed and calculate all watermarks the same way. Also
> > trickle feed wouldn't account for the 256us max deadline value, thoguh
> > that may be a moot point in non-max fifo mode sicne the FIFOs are fairly
> > small.
> > 
> > On VLV max fifo mode can be used with either primary or sprite planes.
> > So the code now also checks all the planes (apart from the cursor)
> > when calculating the SR plane watermark.
> > 
> > We don't have to worry about the WM1 watermarks since we're using the
> > PND deadline scheme which means the hardware ignores WM1 values.
> > 
> > v2: Use plane->state->fb instead of plane->fb
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  11 ++
> >  drivers/gpu/drm/i915/i915_reg.h |   4 +-
> >  drivers/gpu/drm/i915/intel_pm.c | 330 +++++++++++++++++++++-------------------
> >  3 files changed, 188 insertions(+), 157 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5de69a0..b191b12 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1516,6 +1516,17 @@ struct ilk_wm_values {
> >  
> >  struct vlv_wm_values {
> >  	struct {
> > +		uint16_t primary;
> > +		uint16_t sprite[2];
> > +		uint8_t cursor;
> > +	} pipe[3];
> > +
> > +	struct {
> > +		uint16_t plane;
> > +		uint8_t cursor;
> > +	} sr;
> > +
> > +	struct {
> >  		uint8_t cursor;
> >  		uint8_t sprite[2];
> >  		uint8_t primary;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8178610..9f98384 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4127,7 +4127,7 @@ enum skl_disp_power_wells {
> >  /* vlv/chv high order bits */
> >  #define DSPHOWM			(VLV_DISPLAY_BASE + 0x70064)
> >  #define   DSPFW_SR_HI_SHIFT		24
> > -#define   DSPFW_SR_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_HI_MASK		(1<<23)
> >  #define   DSPFW_SPRITEE_HI_SHIFT	22
> > @@ -4148,7 +4148,7 @@ enum skl_disp_power_wells {
> >  #define   DSPFW_PLANEA_HI_MASK		(1<<0)
> >  #define DSPHOWM1		(VLV_DISPLAY_BASE + 0x70068)
> >  #define   DSPFW_SR_WM1_HI_SHIFT		24
> > -#define   DSPFW_SR_WM1_HI_MASK		(1<<24)
> > +#define   DSPFW_SR_WM1_HI_MASK		(3<<24) /* 2 bits for chv, 1 for vlv */
> >  #define   DSPFW_SPRITEF_WM1_HI_SHIFT	23
> >  #define   DSPFW_SPRITEF_WM1_HI_MASK	(1<<23)
> >  #define   DSPFW_SPRITEE_WM1_HI_SHIFT	22
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bdb0f5d..497847c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -778,6 +778,55 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
> >  		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
> >  
> > +	I915_WRITE(DSPFW1,
> > +		   ((wm->sr.plane << DSPFW_SR_SHIFT) & DSPFW_SR_MASK) |
> > +		   ((wm->pipe[PIPE_B].cursor << DSPFW_CURSORB_SHIFT) & DSPFW_CURSORB_MASK) |
> > +		   ((wm->pipe[PIPE_B].primary << DSPFW_PLANEB_SHIFT) & DSPFW_PLANEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].primary << DSPFW_PLANEA_SHIFT) & DSPFW_PLANEA_MASK_VLV));
> > +	I915_WRITE(DSPFW2,
> > +		   ((wm->pipe[PIPE_A].sprite[1] << DSPFW_SPRITEB_SHIFT) & DSPFW_SPRITEB_MASK_VLV) |
> > +		   ((wm->pipe[PIPE_A].cursor << DSPFW_CURSORA_SHIFT) & DSPFW_CURSORA_MASK) |
> > +		   ((wm->pipe[PIPE_A].sprite[0] << DSPFW_SPRITEA_SHIFT) & DSPFW_SPRITEA_MASK_VLV));
> > +	I915_WRITE(DSPFW3,
> > +		   ((wm->sr.cursor << DSPFW_CURSOR_SR_SHIFT) & DSPFW_CURSOR_SR_MASK));
> > +
> > +	if (IS_CHERRYVIEW(dev_priv)) {
> > +		I915_WRITE(DSPFW7_CHV,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPFW8_CHV,
> > +			   ((wm->pipe[PIPE_C].sprite[1] << DSPFW_SPRITEF_SHIFT) & DSPFW_SPRITEF_MASK) |
> > +			   ((wm->pipe[PIPE_C].sprite[0] << DSPFW_SPRITEE_SHIFT) & DSPFW_SPRITEE_MASK));
> > +		I915_WRITE(DSPFW9_CHV,
> > +			   ((wm->pipe[PIPE_C].primary << DSPFW_PLANEC_SHIFT) & DSPFW_PLANEC_MASK) |
> > +			   ((wm->pipe[PIPE_C].cursor << DSPFW_CURSORC_SHIFT) & DSPFW_CURSORC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[1] >> 8) << DSPFW_SPRITEF_HI_SHIFT) & DSPFW_SPRITEF_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].sprite[0] >> 8) << DSPFW_SPRITEE_HI_SHIFT) & DSPFW_SPRITEE_HI_MASK) |
> > +			   (((wm->pipe[PIPE_C].primary >> 8) << DSPFW_PLANEC_HI_SHIFT) & DSPFW_PLANEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> > +	} else {
> > +		I915_WRITE(DSPFW7,
> > +			   ((wm->pipe[PIPE_B].sprite[1] << DSPFW_SPRITED_SHIFT) & DSPFW_SPRITED_MASK) |
> > +			   ((wm->pipe[PIPE_B].sprite[0] << DSPFW_SPRITEC_SHIFT) & DSPFW_SPRITEC_MASK));
> > +		I915_WRITE(DSPHOWM,
> > +			   (((wm->sr.plane >> 9) << DSPFW_SR_HI_SHIFT) & DSPFW_SR_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[1] >> 8) << DSPFW_SPRITED_HI_SHIFT) & DSPFW_SPRITED_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].sprite[0] >> 8) << DSPFW_SPRITEC_HI_SHIFT) & DSPFW_SPRITEC_HI_MASK) |
> > +			   (((wm->pipe[PIPE_B].primary >> 8) << DSPFW_PLANEB_HI_SHIFT) & DSPFW_PLANEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[1] >> 8) << DSPFW_SPRITEB_HI_SHIFT) & DSPFW_SPRITEB_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].sprite[0] >> 8) << DSPFW_SPRITEA_HI_SHIFT) & DSPFW_SPRITEA_HI_MASK) |
> > +			   (((wm->pipe[PIPE_A].primary >> 8) << DSPFW_PLANEA_HI_SHIFT) & DSPFW_PLANEA_HI_MASK));
> > +	}
> > +
> > +	POSTING_READ(DSPFW1);
> > +
> >  	dev_priv->wm.vlv = *wm;
> >  }
> >  
> > @@ -822,169 +871,113 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
> >  				DDL_PRECISION_HIGH : DDL_PRECISION_LOW);
> >  }
> >  
> > -/*
> > - * Update drain latency registers of memory arbiter
> > - *
> > - * Valleyview SoC has a new memory arbiter and needs drain latency registers
> > - * to be programmed. Each plane has a drain latency multiplier and a drain
> > - * latency value.
> > - */
> > -
> > -static void vlv_update_drain_latency(struct drm_crtc *crtc)
> > +static int vlv_compute_wm(struct intel_crtc *crtc,
> > +			  struct intel_plane *plane,
> > +			  int fifo_size)
> > +{
> > +	int clock, entries, pixel_size;
> > +
> > +	/*
> > +	 * FIXME the plane might have an fb
> > +	 * but be invisible (eg. due to clipping)
> > +	 */
> > +	if (!crtc->active || !plane->base.state->fb)
> > +		return 0;
> > +
> > +	pixel_size = drm_format_plane_cpp(plane->base.state->fb->pixel_format, 0);
> > +	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > +
> > +	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> > +
> > +	/*
> > +	 * Set up the watermark such that we don't start issuing memory
> > +	 * requests until we are within PND's max deadline value (256us).
> > +	 * Idea being to be idle as long as possible while still taking
> > +	 * advatange of PND's deadline scheduling. The limit of 8
> > +	 * cachelines (used when the FIFO will anyway drain in less time
> > +	 * than 256us) should match what we would be done if trickle
> > +	 * feed were enabled.
> > +	 */
> > +	return fifo_size - clamp(DIV_ROUND_UP(256 * entries, 64), 0, fifo_size - 8);
> > +}
> > +
> > +static bool vlv_compute_sr_wm(struct drm_device *dev,
> > +			      struct vlv_wm_values *wm)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe = INVALID_PIPE;
> > +	int num_planes = 0;
> > +	int fifo_size = 0;
> > +	struct intel_plane *plane;
> > +
> > +	wm->sr.cursor = wm->sr.plane = 0;
> > +
> > +	crtc = single_enabled_crtc(dev);
> > +	/* maxfifo not supported on pipe C */
> > +	if (crtc && to_intel_crtc(crtc)->pipe != PIPE_C) {
> > +		pipe = to_intel_crtc(crtc)->pipe;
> > +		num_planes = !!wm->pipe[pipe].primary +
> > +			!!wm->pipe[pipe].sprite[0] +
> > +			!!wm->pipe[pipe].sprite[1];
> > +		fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
> > +	}
> > +
> > +	if (fifo_size == 0 || num_planes > 1)
> > +		return false;
> > +
> > +	wm->sr.cursor = vlv_compute_wm(to_intel_crtc(crtc),
> > +				       to_intel_plane(crtc->cursor), 0x3f);
> > +
> > +	list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> > +		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> > +			continue;
> > +
> > +		if (plane->pipe != pipe)
> > +			continue;
> > +
> > +		wm->sr.plane = vlv_compute_wm(to_intel_crtc(crtc),
> > +					      plane, fifo_size);
> > +		if (wm->sr.plane != 0)
> > +			break;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void valleyview_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> >  	wm.ddl[pipe].primary = vlv_compute_drain_latency(crtc, crtc->primary);
> > +	wm.pipe[pipe].primary = vlv_compute_wm(intel_crtc,
> > +					       to_intel_plane(crtc->primary),
> > +					       vlv_get_fifo_size(dev, pipe, 0));
> > +
> >  	wm.ddl[pipe].cursor = vlv_compute_drain_latency(crtc, crtc->cursor);
> > +	wm.pipe[pipe].cursor = vlv_compute_wm(intel_crtc,
> > +					      to_intel_plane(crtc->cursor),
> > +					      0x3f);
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > -}
> > -
> > -#define single_plane_enabled(mask) is_power_of_2(mask)
> > -
> > -static void valleyview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -
> > -	if (cxsr_enabled)
> > -		intel_set_memory_cxsr(dev_priv, true);
> > -}
> > -
> > -static void cherryview_update_wm(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	static const int sr_latency_ns = 12000;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int planea_wm, planeb_wm, planec_wm;
> > -	int cursora_wm, cursorb_wm, cursorc_wm;
> > -	int plane_sr, cursor_sr;
> > -	int ignore_plane_sr, ignore_cursor_sr;
> > -	unsigned int enabled = 0;
> > -	bool cxsr_enabled;
> > -
> > -	vlv_update_drain_latency(crtc);
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_A,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planea_wm, &cursora_wm))
> > -		enabled |= 1 << PIPE_A;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_B,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planeb_wm, &cursorb_wm))
> > -		enabled |= 1 << PIPE_B;
> > -
> > -	if (g4x_compute_wm0(dev, PIPE_C,
> > -			    &valleyview_wm_info, pessimal_latency_ns,
> > -			    &valleyview_cursor_wm_info, pessimal_latency_ns,
> > -			    &planec_wm, &cursorc_wm))
> > -		enabled |= 1 << PIPE_C;
> > -
> > -	if (single_plane_enabled(enabled) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &plane_sr, &ignore_cursor_sr) &&
> > -	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > -			     2*sr_latency_ns,
> > -			     &valleyview_wm_info,
> > -			     &valleyview_cursor_wm_info,
> > -			     &ignore_plane_sr, &cursor_sr)) {
> > -		cxsr_enabled = true;
> > -	} else {
> > -		cxsr_enabled = false;
> > -		intel_set_memory_cxsr(dev_priv, false);
> > -		plane_sr = cursor_sr = 0;
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > -		      "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> > -		      "SR: plane=%d, cursor=%d\n",
> > -		      planea_wm, cursora_wm,
> > -		      planeb_wm, cursorb_wm,
> > -		      planec_wm, cursorc_wm,
> > -		      plane_sr, cursor_sr);
> > -
> > -	I915_WRITE(DSPFW1,
> > -		   (plane_sr << DSPFW_SR_SHIFT) |
> > -		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > -		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > -		   (planea_wm << DSPFW_PLANEA_SHIFT));
> > -	I915_WRITE(DSPFW2,
> > -		   (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > -		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > -	I915_WRITE(DSPFW3,
> > -		   (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > -		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > -	I915_WRITE(DSPFW9_CHV,
> > -		   (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> > -					      DSPFW_CURSORC_MASK)) |
> > -		   (planec_wm << DSPFW_PLANEC_SHIFT) |
> > -		   (cursorc_wm << DSPFW_CURSORC_SHIFT));
> >  
> >  	if (cxsr_enabled)
> >  		intel_set_memory_cxsr(dev_priv, true);
> > @@ -1002,17 +995,44 @@ static void valleyview_update_sprite_wm(struct drm_plane *plane,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	int sprite = to_intel_plane(plane)->plane;
> > +	bool cxsr_enabled;
> >  	struct vlv_wm_values wm = dev_priv->wm.vlv;
> >  
> > -	if (enabled)
> > +	if (enabled) {
> >  		wm.ddl[pipe].sprite[sprite] =
> >  			vlv_compute_drain_latency(crtc, plane);
> > -	else
> > +
> > +		wm.pipe[pipe].sprite[sprite] =
> > +			vlv_compute_wm(intel_crtc,
> > +				       to_intel_plane(plane),
> > +				       vlv_get_fifo_size(dev, pipe, sprite+1));
> > +	} else {
> >  		wm.ddl[pipe].sprite[sprite] = 0;
> > +		wm.pipe[pipe].sprite[sprite] = 0;
> > +	}
> > +
> > +	cxsr_enabled = vlv_compute_sr_wm(dev, &wm);
> > +
> > +	if (memcmp(&wm, &dev_priv->wm.vlv, sizeof(wm)) == 0)
> > +		return;
> > +
> > +	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: sprite %c=%d, "
> > +		      "SR: plane=%d, cursor=%d\n", pipe_name(pipe),
> > +		      sprite_name(pipe, sprite),
> > +		      wm.pipe[pipe].sprite[sprite],
> > +		      wm.sr.plane, wm.sr.cursor);
> > +
> > +	if (!cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, false);
> >  
> >  	vlv_write_wm_values(intel_crtc, &wm);
> > +
> > +	if (cxsr_enabled)
> > +		intel_set_memory_cxsr(dev_priv, true);
> >  }
> >  
> > +#define single_plane_enabled(mask) is_power_of_2(mask)
> > +
> >  static void g4x_update_wm(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -6485,7 +6505,7 @@ void intel_init_pm(struct drm_device *dev)
> >  		else if (INTEL_INFO(dev)->gen == 8)
> >  			dev_priv->display.init_clock_gating = broadwell_init_clock_gating;
> >  	} else if (IS_CHERRYVIEW(dev)) {
> > -		dev_priv->display.update_wm = cherryview_update_wm;
> > +		dev_priv->display.update_wm = valleyview_update_wm;
> >  		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> >  		dev_priv->display.init_clock_gating =
> >  			cherryview_init_clock_gating;
> > 
> 
> I wonder if we should be warning if the wm values we end up with exceed
> the mask size (the fact that you write them with a shift and mask made
> me think of it), but that could be a follow on, or even put into the
> calc code instead.  Anyway that's something we can do later after all
> the atomic changes hit.

IIRC we always have enough bits up to any legal FIFO size, so the clamping
done by vlv_compute_wm() should be enough. I should double check that though
since that isn't the case on a bunch of the other platforms.

I think in general I'd really like magic register bitfield macros that
scream whenever we overflow something by accident. But that's a much
bigger topic. For one we'd have to parametrize all the macros rather than
using raw shifts.

> 
> Does this fix any of our underrun bugs?  Should it have any references:
> lines?

Those should probably be at the DDR DVFS disable patch since before that
pretty much anything can happen. I was too lazy to trawl bugzilla though.
Pretty much hoping QA can just go retest all display bugs once we get
this landed.

> 
> Either way:
> Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>

Thanks.

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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux