[PATCH 0/2] drm/i915/skl: Fix (most) pipe underruns, properly this time

[PATCH 0/2] drm/i915/skl: Fix (most) pipe underruns, properly this time

[Lyude]

Unfortunately as a few of you are aware, Skylake is still very prone to pipe
underruns. Most of this comes from not doing things atomically enough (e.g.
needing to ensure that we update watermarks with other plane attributes, not
forcefully flushing pipes until we need to, etc.). Now that I've finally got a
grasp on how double buffered registers, arming registers, etc. works on skl,
I've written up patches that fix most of the pipe underruns on Skylake.

When I say "most", I'm referring to the fact that I still seem to be able to
reproduce pipe underruns, but this seems to be strictly exclusive to when pipes
are being disabled. This means things *still* need to be made more atomic
(sigh), but at least this is a start.

Testing this series with a chamelium[1], mainly so I could heavily stress test
the hotplugging of displays, I'm no longer able to reproduce pipe underruns
when enabling another pipe. I've also tried reproducing underruns using xdotool
to move the cursor in X from one monitor to another as quickly as possible
(e.g. > 60 fps), and am no longer able to reproduce underruns there either with
this patch series.

Signed-off-by: Lyude Paul <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com> <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>

[1]: https://www.chromium.org/chromium-os/testing/chamelium

Lyude (2):
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Don't mark pipes as dirty unless we've added/removed
    pipes

 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 31 +++++++++---------------
 2 files changed, 59 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/2] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

Thanks to Ville for suggesting this as a potential solution to pipe
underruns on Skylake.

On Skylake all of the registers for configuring planes, including the
registers for configuring their watermarks, are double buffered. New
values written to them won't take effect until said registers are
"armed", which is done by writing to the PLANE_SURF (or in the case of
cursor planes, the CURBASE register) register.

With this in mind, up until now we've been updating watermarks on skl
like this:

 - Calculate new watermark values in skl_compute_wm()

 - Update non-watermark settings for each plane in
   skylake_update_primary_plane()/i9xx_update_cursor()

 - Arm plane registers to update at the start of the next vblank in
   skylake_update_primary_plane()/i9xx_update_cursor()

 - *Possibly underrun here, since the plane may now have updated it's
   settings using the old and insufficient watermark values*

 - Update watermark settings in skl_write_wm_values()

 - *Possibly underrun here as well if we've passed a vblank,
   causing the hardware to get stuck running on intermediate wm values*

 - Finally arm plane registers so they update to the correct values at
   the start of the next vblank in skl_wm_flush_pipe()

Now we update watermarks atomically like this:

 - Calculate new watermark values in skl_compute_wm()

 - Update watermark values for each plane in
   skylake_write_plane_wm()/skylake_write_cursor_wm()

 - Update all the other values for the plane (position, address, etc.)
   in skylake_update_primary_plane()/i9xx_update_cursor()

 - Arm plane registers (including the watermark settings) to update at
   the start of the next vblank in
   skylake_update_primary_plane()/i9xx_update_cursor()

Which is more of a step in the right direction to fixing all of the
underrun issues we're currently seeing with skl

Signed-off-by: Lyude Paul <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com> <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 15 +-----------
 2 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb7d8fc5..3d2c125 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2971,6 +2971,27 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
 	return 0;
 }
 
+static void skylake_write_plane_wm(struct intel_crtc *intel_crtc,
+				   int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+
+	for (level = 0; level < max_level; level++) {
+		I915_WRITE(PLANE_WM(pipe, plane, level),
+			   wm->plane[pipe][plane][level]);
+	}
+	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
 static void skylake_update_primary_plane(struct drm_plane *plane,
 					 const struct intel_crtc_state *crtc_state,
 					 const struct intel_plane_state *plane_state)
@@ -3031,6 +3052,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skylake_write_plane_wm(intel_crtc, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10233,6 +10256,27 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
 	}
 }
 
+static void skl_write_cursor_wm(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(CUR_WM(pipe, level),
+			   wm->plane[pipe][PLANE_CURSOR][level]);
+	}
+	I915_WRITE(CUR_WM_TRANS(pipe),
+		   wm->plane_trans[pipe][PLANE_CURSOR]);
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
 static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 			       const struct intel_plane_state *plane_state)
 {
@@ -10242,6 +10286,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (IS_SKYLAKE(dev_priv))
+		skl_write_cursor_wm(intel_crtc);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 213ad35..3a7709c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3788,7 +3788,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		skl_disable_sagv(dev_priv);
 
 	for_each_intel_crtc(dev, crtc) {
-		int i, level, max_level = ilk_wm_max_level(dev);
+		int i;
 		enum pipe pipe = crtc->pipe;
 
 		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
@@ -3798,19 +3798,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
 
-		for (level = 0; level <= max_level; level++) {
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_WM(pipe, i, level),
-					   new->plane[pipe][i][level]);
-			I915_WRITE(CUR_WM(pipe, level),
-				   new->plane[pipe][PLANE_CURSOR][level]);
-		}
-		for (i = 0; i < intel_num_planes(crtc); i++)
-			I915_WRITE(PLANE_WM_TRANS(pipe, i),
-				   new->plane_trans[pipe][i]);
-		I915_WRITE(CUR_WM_TRANS(pipe),
-			   new->plane_trans[pipe][PLANE_CURSOR]);
-
 		for (i = 0; i < intel_num_planes(crtc); i++) {
 			skl_ddb_entry_write(dev_priv,
 					    PLANE_BUF_CFG(pipe, i),
-- 
2.7.4

[PATCH 2/2] drm/i915/skl: Don't mark pipes as dirty unless we've added/removed pipes

[Lyude]

Now that we update the watermark values atomically, we still need to fix
the case of how we update watermarks when we haven't added or removed
pipes.

When we haven't added or removed any pipes, we don't need to worry about
the ddb allocation changing. As such there's no order of flushing pipes
we have to guarantee; e.g. each pipe can be flushed at the earliest
given oppurtunity. This means all we have to do is:

 - Calculate the new wm values
 - Update each plane's settings and wm values
 - Arm the plane's registers to update at the next vblank

Right now the watermark code takes an additional few steps:
 - Rewrite the ddb allocations
 - Arm all of the planes for another update at the start of the next
   vblank
 - *Potentially underrun later if we update the wm registers before the
   start of the next vblank*

This patch prevents us from marking pipes as dirty unless the number of
pipes, and with that the ddb allocation, has actually changed. This
results in us skipping the additional steps, preventing the hardware
from using any intermediate values during each wm update.

This also fixes cursors causing monitors to flicker on Skylake. Finally.

Signed-off-by: Lyude Paul <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3a7709c..92158e3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state)
 		if (ret)
 			return ret;
 
-		if (changed)
-			results->dirty_pipes |= drm_crtc_mask(crtc);
+		/*
+		 * We don't need to do any additional setup for the pipes if we
+		 * haven't changed the number of active crtcs
+		 */
+		if (intel_state->active_pipe_changes) {
+			if (changed)
+				results->dirty_pipes |= drm_crtc_mask(crtc);
 
-		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
-			/* This pipe's WM's did not change */
-			continue;
+			if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
+				/* This pipe's WM's did not change */
+				continue;
+		}
 
 		intel_cstate->update_wm_pre = true;
 		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
-- 
2.7.4

Re: [PATCH 1/2] drm/i915/skl: Update plane watermarks atomically during plane updates

[Matt Roper]

On Tue, Jul 19, 2016 at 12:30:55PM -0400, Lyude wrote:
> Thanks to Ville for suggesting this as a potential solution to pipe
> underruns on Skylake.
> 
> On Skylake all of the registers for configuring planes, including the
> registers for configuring their watermarks, are double buffered. New
> values written to them won't take effect until said registers are
> "armed", which is done by writing to the PLANE_SURF (or in the case of
> cursor planes, the CURBASE register) register.
> 
> With this in mind, up until now we've been updating watermarks on skl
> like this:
> 
>  - Calculate new watermark values in skl_compute_wm()
> 
>  - Update non-watermark settings for each plane in
>    skylake_update_primary_plane()/i9xx_update_cursor()
> 
>  - Arm plane registers to update at the start of the next vblank in
>    skylake_update_primary_plane()/i9xx_update_cursor()
> 
>  - *Possibly underrun here, since the plane may now have updated it's
>    settings using the old and insufficient watermark values*
> 
>  - Update watermark settings in skl_write_wm_values()
> 
>  - *Possibly underrun here as well if we've passed a vblank,
>    causing the hardware to get stuck running on intermediate wm values*
> 
>  - Finally arm plane registers so they update to the correct values at
>    the start of the next vblank in skl_wm_flush_pipe()

I think you're on the right track with this patch, but I don't think the
sequence of events is quite what you describe in the commit message
here.  The calls to skylake_update_primary_plane()/i9xx_update_cursor()
are while we're under vblank evasion, which happens during

  finish_atomic_commit() -> drm_atomic_helper_commit_planes_on_crtc()

Then gen9 watermarks get written during intel_pre_plane_update() for
non-modesets or during crtc_enable for modesets.  So I believe the
actual sequence of events is:

        non-modeset {
         - calculate (during atomic check phase)
         - finish_atomic_commit:
           - intel_pre_plane_update:
              - intel_update_watermarks()
           - {vblank happens; new watermarks + old plane values => underrun }
           - drm_atomic_helper_commit_planes_on_crtc:
              - start vblank evasion
              - write new plane registers
              - end vblank evasion
        }

        or

        modeset {
         - calculate (during atomic check phase)
         - finish_atomic_commit:
           - crtc_enable:
              - intel_update_watermarks()
           - {vblank happens; new watermarks + old plane values => underrun }
           - drm_atomic_helper_commit_planes_on_crtc:
              - start vblank evasion
              - write new plane registers
              - end vblank evasion
        }

So you're right that fundamentally the problem arises from the
watermarks not being updated under vblank evasion when the corresponding
plane updates happen (so we might wind up with new watermarks before
we've actually switched our plane registers if a vblank sneaks in in the
meantime).  The sequence is just slightly different from the way you'd
listed it out.

> 
> Now we update watermarks atomically like this:
> 
>  - Calculate new watermark values in skl_compute_wm()
> 
>  - Update watermark values for each plane in
>    skylake_write_plane_wm()/skylake_write_cursor_wm()
> 
>  - Update all the other values for the plane (position, address, etc.)
>    in skylake_update_primary_plane()/i9xx_update_cursor()

Do we also need it in skl_update_plane() (from intel_sprite.c)?

> 
>  - Arm plane registers (including the watermark settings) to update at
>    the start of the next vblank in
>    skylake_update_primary_plane()/i9xx_update_cursor()
> 
> Which is more of a step in the right direction to fixing all of the
> underrun issues we're currently seeing with skl
> 
> Signed-off-by: Lyude Paul <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com> <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 15 +-----------
>  2 files changed, 48 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fb7d8fc5..3d2c125 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2971,6 +2971,27 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
>  	return 0;
>  }
>  
> +static void skylake_write_plane_wm(struct intel_crtc *intel_crtc,
> +				   int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	mutex_lock(&dev_priv->wm.wm_mutex);

I don't think this mutex is actually protecting anything important here
is it?  We already have the crtc & plane mutexes and we're not really
using/touching any global state.  Actually, we're calling this while
under vblank evasion, so we're not allowed to sleep here anyway.

(same comment for the cursor version)


Matt

> +
> +	for (level = 0; level < max_level; level++) {
> +		I915_WRITE(PLANE_WM(pipe, plane, level),
> +			   wm->plane[pipe][plane][level]);
> +	}
> +	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
> +
> +	mutex_unlock(&dev_priv->wm.wm_mutex);
> +}
> +
>  static void skylake_update_primary_plane(struct drm_plane *plane,
>  					 const struct intel_crtc_state *crtc_state,
>  					 const struct intel_plane_state *plane_state)
> @@ -3031,6 +3052,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	skylake_write_plane_wm(intel_crtc, 0);
> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> @@ -10233,6 +10256,27 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
>  	}
>  }
>  
> +static void skl_write_cursor_wm(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	mutex_lock(&dev_priv->wm.wm_mutex);
> +
> +	for (level = 0; level <= max_level; level++) {
> +		I915_WRITE(CUR_WM(pipe, level),
> +			   wm->plane[pipe][PLANE_CURSOR][level]);
> +	}
> +	I915_WRITE(CUR_WM_TRANS(pipe),
> +		   wm->plane_trans[pipe][PLANE_CURSOR]);
> +
> +	mutex_unlock(&dev_priv->wm.wm_mutex);
> +}
> +
>  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  			       const struct intel_plane_state *plane_state)
>  {
> @@ -10242,6 +10286,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (IS_SKYLAKE(dev_priv))
> +		skl_write_cursor_wm(intel_crtc);
> +
>  	if (plane_state && plane_state->visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 213ad35..3a7709c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3788,7 +3788,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		skl_disable_sagv(dev_priv);
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		int i, level, max_level = ilk_wm_max_level(dev);
> +		int i;
>  		enum pipe pipe = crtc->pipe;
>  
>  		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
> @@ -3798,19 +3798,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  
>  		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
>  
> -		for (level = 0; level <= max_level; level++) {
> -			for (i = 0; i < intel_num_planes(crtc); i++)
> -				I915_WRITE(PLANE_WM(pipe, i, level),
> -					   new->plane[pipe][i][level]);
> -			I915_WRITE(CUR_WM(pipe, level),
> -				   new->plane[pipe][PLANE_CURSOR][level]);
> -		}
> -		for (i = 0; i < intel_num_planes(crtc); i++)
> -			I915_WRITE(PLANE_WM_TRANS(pipe, i),
> -				   new->plane_trans[pipe][i]);
> -		I915_WRITE(CUR_WM_TRANS(pipe),
> -			   new->plane_trans[pipe][PLANE_CURSOR]);
> -
>  		for (i = 0; i < intel_num_planes(crtc); i++) {
>  			skl_ddb_entry_write(dev_priv,
>  					    PLANE_BUF_CFG(pipe, i),
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/2] drm/i915/skl: Don't mark pipes as dirty unless we've added/removed pipes

[Matt Roper]

On Tue, Jul 19, 2016 at 12:30:56PM -0400, Lyude wrote:
> Now that we update the watermark values atomically, we still need to fix
> the case of how we update watermarks when we haven't added or removed
> pipes.
> 
> When we haven't added or removed any pipes, we don't need to worry about
> the ddb allocation changing. As such there's no order of flushing pipes
> we have to guarantee; e.g. each pipe can be flushed at the earliest
> given oppurtunity. This means all we have to do is:
> 
>  - Calculate the new wm values
>  - Update each plane's settings and wm values
>  - Arm the plane's registers to update at the next vblank
> 
> Right now the watermark code takes an additional few steps:
>  - Rewrite the ddb allocations
>  - Arm all of the planes for another update at the start of the next
>    vblank
>  - *Potentially underrun later if we update the wm registers before the
>    start of the next vblank*
> 
> This patch prevents us from marking pipes as dirty unless the number of
> pipes, and with that the ddb allocation, has actually changed. This
> results in us skipping the additional steps, preventing the hardware
> from using any intermediate values during each wm update.
> 
> This also fixes cursors causing monitors to flicker on Skylake. Finally.
> 

This doesn't look right to me.  It's true that we don't need to worry
about *inter*-pipe DDB allocation changing, but *intra*-pipe DDB
allocations can still change.  We may be resizing planes, changing
scalers, enabling/disabling new planes, etc.  Those operations change
the DDB allocations for the planes within the fixed pipe allocation.
The watermark values will also change accordingly in that case.


Matt

> Signed-off-by: Lyude Paul <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3a7709c..92158e3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>  		if (ret)
>  			return ret;
>  
> -		if (changed)
> -			results->dirty_pipes |= drm_crtc_mask(crtc);
> +		/*
> +		 * We don't need to do any additional setup for the pipes if we
> +		 * haven't changed the number of active crtcs
> +		 */
> +		if (intel_state->active_pipe_changes) {
> +			if (changed)
> +				results->dirty_pipes |= drm_crtc_mask(crtc);
>  
> -		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> -			/* This pipe's WM's did not change */
> -			continue;
> +			if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> +				/* This pipe's WM's did not change */
> +				continue;
> +		}
>  
>  		intel_cstate->update_wm_pre = true;
>  		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl: Don't mark pipes as dirty unless we've added/removed pipes

[Rob Clark]

On Tue, Jul 19, 2016 at 1:19 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Tue, Jul 19, 2016 at 12:30:56PM -0400, Lyude wrote:
>> Now that we update the watermark values atomically, we still need to fix
>> the case of how we update watermarks when we haven't added or removed
>> pipes.
>>
>> When we haven't added or removed any pipes, we don't need to worry about
>> the ddb allocation changing. As such there's no order of flushing pipes
>> we have to guarantee; e.g. each pipe can be flushed at the earliest
>> given oppurtunity. This means all we have to do is:
>>
>>  - Calculate the new wm values
>>  - Update each plane's settings and wm values
>>  - Arm the plane's registers to update at the next vblank
>>
>> Right now the watermark code takes an additional few steps:
>>  - Rewrite the ddb allocations
>>  - Arm all of the planes for another update at the start of the next
>>    vblank
>>  - *Potentially underrun later if we update the wm registers before the
>>    start of the next vblank*
>>
>> This patch prevents us from marking pipes as dirty unless the number of
>> pipes, and with that the ddb allocation, has actually changed. This
>> results in us skipping the additional steps, preventing the hardware
>> from using any intermediate values during each wm update.
>>
>> This also fixes cursors causing monitors to flicker on Skylake. Finally.
>>
>
> This doesn't look right to me.  It's true that we don't need to worry
> about *inter*-pipe DDB allocation changing, but *intra*-pipe DDB
> allocations can still change.  We may be resizing planes, changing
> scalers, enabling/disabling new planes, etc.  Those operations change
> the DDB allocations for the planes within the fixed pipe allocation.
> The watermark values will also change accordingly in that case.
>

fwiw, msm/mdp5 has potentially similar constraints w/ SMP block
allocation (which seems like basically the same thing as DDB)..  we
just disallow changing width/bpp/etc on an active plane, so userspace
would pick a different plane (or skip the plane for one frame of
composition)..

not sure if that would help.. maybe we are more limited in not
actually being able to safely change SMP allocation on an active
plane/pipe.  But treating it as a disable of current pipe and switch
to new pipe is convenient, rather than dealing with all the cases that
could effect SMP block assignment.

BR,
-R

>
> Matt
>
>> Signed-off-by: Lyude Paul <cpaul@redhat.com>
>> Cc: stable@vger.kernel.org
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 3a7709c..92158e3 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state)
>>               if (ret)
>>                       return ret;
>>
>> -             if (changed)
>> -                     results->dirty_pipes |= drm_crtc_mask(crtc);
>> +             /*
>> +              * We don't need to do any additional setup for the pipes if we
>> +              * haven't changed the number of active crtcs
>> +              */
>> +             if (intel_state->active_pipe_changes) {
>> +                     if (changed)
>> +                             results->dirty_pipes |= drm_crtc_mask(crtc);
>>
>> -             if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>> -                     /* This pipe's WM's did not change */
>> -                     continue;
>> +                     if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>> +                             /* This pipe's WM's did not change */
>> +                             continue;
>> +             }
>>
>>               intel_cstate->update_wm_pre = true;
>>               skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
>> --
>> 2.7.4
>>
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

Thanks to Ville for suggesting this as a potential solution to pipe
underruns on Skylake.

On Skylake all of the registers for configuring planes, including the
registers for configuring their watermarks, are double buffered. New
values written to them won't take effect until said registers are
"armed", which is done by writing to the PLANE_SURF (or in the case of
cursor planes, the CURBASE register) register.

With this in mind, up until now we've been updating watermarks on skl
like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

  or

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

Now we update watermarks atomically like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks() (wm values aren't written yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - write new wm values
        - end vblank evasion
  }

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks() (actual wm values aren't written
          yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
	- write new wm values
        - end vblank evasion
  }

Which is more of a step in the right direction to fixing all of the
underrun issues we're currently seeing with skl

Changes since v1:
- Remove mutex_lock/mutex_unlock since they don't do anything and we're
  not touching global state
- Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make
  externally visible
- Add skl_write_plane_wm calls to skl_update_plane
- Fix conditional for for loop in skl_write_plane_wm (level < max_level
  should be level <= max_level)
- Make diagram in commit more accurate to what's actually happening

Signed-off-by: Lyude Paul <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com> <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 48 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb7d8fc5..1481b2b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skl_write_plane_wm(intel_crtc, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10242,6 +10244,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (IS_SKYLAKE(dev_priv))
+		skl_write_cursor_wm(intel_crtc);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851..f1f54d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1709,6 +1709,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 213ad35..90c788d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3776,6 +3776,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 		I915_WRITE(reg, 0);
 }
 
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
+			int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(PLANE_WM(pipe, plane, level),
+			   wm->plane[pipe][plane][level]);
+	}
+	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+}
+
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(CUR_WM(pipe, level),
+			   wm->plane[pipe][PLANE_CURSOR][level]);
+	}
+	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3788,7 +3821,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		skl_disable_sagv(dev_priv);
 
 	for_each_intel_crtc(dev, crtc) {
-		int i, level, max_level = ilk_wm_max_level(dev);
+		int i;
 		enum pipe pipe = crtc->pipe;
 
 		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
@@ -3798,19 +3831,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
 
-		for (level = 0; level <= max_level; level++) {
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_WM(pipe, i, level),
-					   new->plane[pipe][i][level]);
-			I915_WRITE(CUR_WM(pipe, level),
-				   new->plane[pipe][PLANE_CURSOR][level]);
-		}
-		for (i = 0; i < intel_num_planes(crtc); i++)
-			I915_WRITE(PLANE_WM_TRANS(pipe, i),
-				   new->plane_trans[pipe][i]);
-		I915_WRITE(CUR_WM_TRANS(pipe),
-			   new->plane_trans[pipe][PLANE_CURSOR]);
-
 		for (i = 0; i < intel_num_planes(crtc); i++) {
 			skl_ddb_entry_write(dev_priv,
 					    PLANE_BUF_CFG(pipe, i),
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0de935a..50026f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
+
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 
2.7.4