[PATCH 1/4] drm/i915: Don't propagate SAGV errnos in vain.

[PATCH 1/4] drm/i915: Don't propagate SAGV errnos in vain.

[Rodrigo Vivi]

We never used this information on upper level.
So let's just print the error and make the functions void.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c  | 26 +++++++++++---------------
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f733c5f0faf..2260aa1ecd8c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2004,8 +2004,8 @@ void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
 void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
 bool intel_can_enable_sagv(struct drm_atomic_state *state);
 u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv);
-int intel_enable_sagv(struct drm_i915_private *dev_priv);
-int intel_disable_sagv(struct drm_i915_private *dev_priv);
+void intel_enable_sagv(struct drm_i915_private *dev_priv);
+void intel_disable_sagv(struct drm_i915_private *dev_priv);
 bool skl_wm_level_equals(const struct skl_wm_level *l1,
 			 const struct skl_wm_level *l2);
 bool skl_ddb_allocation_overlaps(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 92a14cb7e674..2b6419b7bdd1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3606,16 +3606,15 @@ intel_has_sagv(struct drm_i915_private *dev_priv)
  *  - All planes can enable watermarks for latencies >= SAGV engine block time
  *  - We're not using an interlaced display configuration
  */
-int
-intel_enable_sagv(struct drm_i915_private *dev_priv)
+void intel_enable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
 	if (!intel_has_sagv(dev_priv))
-		return 0;
+		return;
 
 	if (dev_priv->sagv_status == I915_SAGV_ENABLED)
-		return 0;
+		return;
 
 	DRM_DEBUG_KMS("Enabling the SAGV\n");
 	mutex_lock(&dev_priv->pcu_lock);
@@ -3633,26 +3632,24 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
-		return 0;
+		return;
 	} else if (ret < 0) {
-		DRM_ERROR("Failed to enable the SAGV\n");
-		return ret;
+		DRM_ERROR("Failed to enable the SAGV (%d)\n", ret);
+		return;
 	}
 
 	dev_priv->sagv_status = I915_SAGV_ENABLED;
-	return 0;
 }
 
-int
-intel_disable_sagv(struct drm_i915_private *dev_priv)
+void intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
 	if (!intel_has_sagv(dev_priv))
-		return 0;
+		return;
 
 	if (dev_priv->sagv_status == I915_SAGV_DISABLED)
-		return 0;
+		return;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->pcu_lock);
@@ -3671,14 +3668,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
 		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
-		return 0;
+		return;
 	} else if (ret < 0) {
 		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
-		return ret;
+		return;
 	}
 
 	dev_priv->sagv_status = I915_SAGV_DISABLED;
-	return 0;
 }
 
 bool intel_can_enable_sagv(struct drm_atomic_state *state)
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Introduce SAGV mutex.

[Rodrigo Vivi]

Now that we are spreading the places we can manipulate
sagv status let's protect it.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 15 +++++++++------
 drivers/gpu/drm/i915/intel_pm.c | 41 ++++++++++++++++++++++++++---------------
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 82a106b1bdbc..4c5174ceab96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2122,12 +2122,15 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
-	enum {
-		I915_SAGV_UNKNOWN = 0,
-		I915_SAGV_DISABLED,
-		I915_SAGV_ENABLED,
-		I915_SAGV_NOT_CONTROLLED
-	} sagv_status;
+	struct {
+		enum {
+			I915_SAGV_UNKNOWN = 0,
+			I915_SAGV_DISABLED,
+			I915_SAGV_ENABLED,
+			I915_SAGV_NOT_CONTROLLED
+		} status;
+		struct mutex lock;
+	} sagv;
 
 	struct {
 		/*
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2b6419b7bdd1..e1ec9c2fd08a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3584,15 +3584,19 @@ static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
 static bool
 intel_has_sagv(struct drm_i915_private *dev_priv)
 {
+	bool ret = false;
+
 	if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
 	    IS_CANNONLAKE(dev_priv))
 		return true;
 
+	mutex_lock(&dev_priv->sagv.lock);
 	if (IS_SKYLAKE(dev_priv) &&
-	    dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED)
-		return true;
+	    dev_priv->sagv.status != I915_SAGV_NOT_CONTROLLED)
+		ret = true;
+	mutex_unlock(&dev_priv->sagv.lock);
 
-	return false;
+	return ret;
 }
 
 /*
@@ -3613,8 +3617,9 @@ void intel_enable_sagv(struct drm_i915_private *dev_priv)
 	if (!intel_has_sagv(dev_priv))
 		return;
 
-	if (dev_priv->sagv_status == I915_SAGV_ENABLED)
-		return;
+	mutex_lock(&dev_priv->sagv.lock);
+	if (dev_priv->sagv.status == I915_SAGV_ENABLED)
+		goto out;
 
 	DRM_DEBUG_KMS("Enabling the SAGV\n");
 	mutex_lock(&dev_priv->pcu_lock);
@@ -3631,14 +3636,16 @@ void intel_enable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
-		return;
+		dev_priv->sagv.status = I915_SAGV_NOT_CONTROLLED;
+		goto out;
 	} else if (ret < 0) {
 		DRM_ERROR("Failed to enable the SAGV (%d)\n", ret);
-		return;
+		goto out;
 	}
 
-	dev_priv->sagv_status = I915_SAGV_ENABLED;
+	dev_priv->sagv.status = I915_SAGV_ENABLED;
+out:
+	mutex_unlock(&dev_priv->sagv.lock);
 }
 
 void intel_disable_sagv(struct drm_i915_private *dev_priv)
@@ -3648,8 +3655,9 @@ void intel_disable_sagv(struct drm_i915_private *dev_priv)
 	if (!intel_has_sagv(dev_priv))
 		return;
 
-	if (dev_priv->sagv_status == I915_SAGV_DISABLED)
-		return;
+	mutex_lock(&dev_priv->sagv.lock);
+	if (dev_priv->sagv.status == I915_SAGV_DISABLED)
+		goto out;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->pcu_lock);
@@ -3667,14 +3675,16 @@ void intel_disable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
-		return;
+		dev_priv->sagv.status = I915_SAGV_NOT_CONTROLLED;
+		goto out;
 	} else if (ret < 0) {
 		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
-		return;
+		goto out;
 	}
 
-	dev_priv->sagv_status = I915_SAGV_DISABLED;
+	dev_priv->sagv.status = I915_SAGV_DISABLED;
+out:
+	mutex_unlock(&dev_priv->sagv.lock);
 }
 
 bool intel_can_enable_sagv(struct drm_atomic_state *state)
@@ -9046,6 +9056,7 @@ void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
 void intel_init_pm(struct drm_i915_private *dev_priv)
 {
 	intel_fbc_init(dev_priv);
+	mutex_init(&dev_priv->sagv.lock);
 
 	/* For cxsr */
 	if (IS_PINEVIEW(dev_priv))
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Introduce SAGV mutex.

[Chris Wilson]

Quoting Rodrigo Vivi (2018-02-26 20:53:07)
> Now that we are spreading the places we can manipulate
> sagv status let's protect it.

This needs a lot more information about the protection you need. "sagv
status" is too similar to sagv_status, so it seems like you are simply
talking about protecting access to that variable, which doesn't make
sense. So I presume you are talking about ordering enable/disable, which
opens up a heap of questions as to why, and what should be done about
the implied races in wanting to disable sagv
before/during/immediately-after enabling it. (Even accepting the race
conditions exist, you only needed to correct access to sagv; a mutex for
a single variable is massive overkill. So again, the reader is left
presuming you intend more.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Introduce SAGV mutex.

[Rodrigo Vivi]

On Mon, Feb 26, 2018 at 09:21:19PM +0000, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-02-26 20:53:07)
> > Now that we are spreading the places we can manipulate
> > sagv status let's protect it.
> 
> This needs a lot more information about the protection you need. "sagv
> status" is too similar to sagv_status, so it seems like you are simply
> talking about protecting access to that variable, which doesn't make
> sense. So I presume you are talking about ordering enable/disable, which
> opens up a heap of questions as to why, and what should be done about
> the implied races in wanting to disable sagv
> before/during/immediately-after enabling it. (Even accepting the race
> conditions exist, you only needed to correct access to sagv; a mutex for
> a single variable is massive overkill. So again, the reader is left
> presuming you intend more.)

Yeap... Many intentions missing here indeed... sorry.

The intention here is to disable SAGV on any first fifo underrun interrupt
that could happen at anytime. And keep that disabled forever.
I don't want the sagv enable during modeset to enable that again. Because
I don't know what is happening to the machine anymore. Better to waste some
power than having the risk of hanging the machine.

So the sagv status variable already contain the
SAGV_NOT_CONTROLLED state and I wanted to use that to block SAGV.

I also want to disable SAGV as needed on any atomic commit, not only on
full modeset. [1] (Main missing part on this series probably)

The concurrence on disable wouldn't be a problem because the disable
sequence itself is already protected by the pcu mutex. However
the end result of sagv.status would be problematic. Because if plane
update disable sagv leaving STATUS_DISABLED, the next modeset would
enable SAGV back.

[1] https://patchwork.freedesktop.org/series/38806/

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Move underrun work from fbc to fifo_underrun.

[Rodrigo Vivi]

This underrun work can be useful to disable more pm
function that can cause trouble on underrun situations,
like SAGV.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  7 +++-
 drivers/gpu/drm/i915/i915_irq.c            |  2 ++
 drivers/gpu/drm/i915/intel_drv.h           |  3 +-
 drivers/gpu/drm/i915/intel_fbc.c           | 38 +---------------------
 drivers/gpu/drm/i915/intel_fifo_underrun.c | 51 +++++++++++++++++++++++++++++-
 5 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c5174ceab96..1650dd5e5ffb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -658,7 +658,6 @@ struct intel_fbc {
 	bool active;
 
 	bool underrun_detected;
-	struct work_struct underrun_work;
 
 	/*
 	 * Due to the atomic rules we can't access some structures without the
@@ -2133,6 +2132,12 @@ struct drm_i915_private {
 	} sagv;
 
 	struct {
+		struct work_struct work;
+		spinlock_t lock;
+		bool detected;
+	} underrun;
+
+	struct {
 		/*
 		 * Raw watermark latency values:
 		 * in 0.1us units for WM0,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0a7ed990a8d1..ee9bd2d4ce34 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4045,6 +4045,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	intel_hpd_init_work(dev_priv);
 
+	intel_underrun_init_work(dev_priv);
+
 	INIT_WORK(&rps->work, gen6_pm_rps_work);
 
 	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2260aa1ecd8c..3ca27cc8700d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1317,6 +1317,7 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum pipe pch_transcoder);
 void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv);
 void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv);
+void intel_underrun_init_work(struct drm_i915_private *dev_priv);
 
 /* i915_irq.c */
 void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
@@ -1766,7 +1767,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
-void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
+void intel_fbc_underrun_detected(struct drm_i915_private *dev_priv);
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_i915_private *dev_priv, i915_reg_t hdmi_reg,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 38b036c499d9..c63568564b77 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1231,10 +1231,8 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 	cancel_work_sync(&fbc->work.work);
 }
 
-static void intel_fbc_underrun_work_fn(struct work_struct *work)
+void intel_fbc_underrun_detected(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, fbc.underrun_work);
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	mutex_lock(&fbc->lock);
@@ -1252,39 +1250,6 @@ static void intel_fbc_underrun_work_fn(struct work_struct *work)
 }
 
 /**
- * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
- * @dev_priv: i915 device instance
- *
- * Without FBC, most underruns are harmless and don't really cause too many
- * problems, except for an annoying message on dmesg. With FBC, underruns can
- * become black screens or even worse, especially when paired with bad
- * watermarks. So in order for us to be on the safe side, completely disable FBC
- * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
- * already suggests that watermarks may be bad, so try to be as safe as
- * possible.
- *
- * This function is called from the IRQ handler.
- */
-void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
-{
-	struct intel_fbc *fbc = &dev_priv->fbc;
-
-	if (!fbc_supported(dev_priv))
-		return;
-
-	/* There's no guarantee that underrun_detected won't be set to true
-	 * right after this check and before the work is scheduled, but that's
-	 * not a problem since we'll check it again under the work function
-	 * while FBC is locked. This check here is just to prevent us from
-	 * unnecessarily scheduling the work, and it relies on the fact that we
-	 * never switch underrun_detect back to false after it's true. */
-	if (READ_ONCE(fbc->underrun_detected))
-		return;
-
-	schedule_work(&fbc->underrun_work);
-}
-
-/**
  * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
  * @dev_priv: i915 device instance
  *
@@ -1352,7 +1317,6 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
 	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
-	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
 	mutex_init(&fbc->lock);
 	fbc->enabled = false;
 	fbc->active = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 77c123cc8817..6a290621177b 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -350,6 +350,55 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
 	return old;
 }
 
+static void intel_underrun_work_fn(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, underrun.work);
+
+	intel_fbc_underrun_detected(dev_priv);
+}
+
+/*
+ * intel_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
+ *
+ * Without FBC, most underruns are harmless and don't really cause too many
+ * problems, except for an annoying message on dmesg. With FBC, underruns can
+ * become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable FBC
+ * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
+ * already suggests that watermarks may be bad, so try to be as safe as
+ * possible.
+ *
+ */
+static void intel_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_FBC(dev_priv))
+		return;
+
+	spin_lock(&dev_priv->underrun.lock);
+
+	if (dev_priv->underrun.detected)
+		goto out;
+	dev_priv->underrun.detected = true;
+
+	schedule_work(&dev_priv->underrun.work);
+out:
+	spin_unlock(&dev_priv->underrun.lock);
+}
+
+/**
+ * intel_underrun_init_work - Handle underrun outside atomic context.
+ * @dev_priv: i915 device instance
+ *
+ * Some features needs to be disabled when underrun is detected.
+ * Let's use work queue to make it outside the atomic irq context.
+ */
+void intel_underrun_init_work(struct drm_i915_private *dev_priv)
+{
+	INIT_WORK(&dev_priv->underrun.work, intel_underrun_work_fn);
+	spin_lock_init(&dev_priv->underrun.lock);
+}
+
 /**
  * intel_cpu_fifo_underrun_irq_handler - handle CPU fifo underrun interrupt
  * @dev_priv: i915 device instance
@@ -379,7 +428,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 			  pipe_name(pipe));
 	}
 
-	intel_fbc_handle_fifo_underrun_irq(dev_priv);
+	intel_handle_fifo_underrun_irq(dev_priv);
 }
 
 /**
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Move underrun work from fbc to fifo_underrun.

[Chris Wilson]

Quoting Rodrigo Vivi (2018-02-26 20:53:08)
> -void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> -{
> -       struct intel_fbc *fbc = &dev_priv->fbc;
> -
> -       if (!fbc_supported(dev_priv))
> -               return;
> -
> -       /* There's no guarantee that underrun_detected won't be set to true
> -        * right after this check and before the work is scheduled, but that's
> -        * not a problem since we'll check it again under the work function
> -        * while FBC is locked. This check here is just to prevent us from
> -        * unnecessarily scheduling the work, and it relies on the fact that we
> -        * never switch underrun_detect back to false after it's true. */
> -       if (READ_ONCE(fbc->underrun_detected))
> -               return;
> -
> -       schedule_work(&fbc->underrun_work);
> -}

> +static void intel_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_FBC(dev_priv))
> +               return;
> +
> +       spin_lock(&dev_priv->underrun.lock);
> +
> +       if (dev_priv->underrun.detected)
> +               goto out;
> +       dev_priv->underrun.detected = true;
> +
> +       schedule_work(&dev_priv->underrun.work);
> +out:
> +       spin_unlock(&dev_priv->underrun.lock);

This locking (or bool) isn't required by the following patch either. But
I presume the boolean is printed at some point, although that's probably
less useful than whatever FBC/SAGV would say about being disabled.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Move underrun work from fbc to fifo_underrun.

[Rodrigo Vivi]

On Mon, Feb 26, 2018 at 09:00:50PM +0000, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-02-26 20:53:08)
> > -void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> > -{
> > -       struct intel_fbc *fbc = &dev_priv->fbc;
> > -
> > -       if (!fbc_supported(dev_priv))
> > -               return;
> > -
> > -       /* There's no guarantee that underrun_detected won't be set to true
> > -        * right after this check and before the work is scheduled, but that's
> > -        * not a problem since we'll check it again under the work function
> > -        * while FBC is locked. This check here is just to prevent us from
> > -        * unnecessarily scheduling the work, and it relies on the fact that we
> > -        * never switch underrun_detect back to false after it's true. */
> > -       if (READ_ONCE(fbc->underrun_detected))
> > -               return;
> > -
> > -       schedule_work(&fbc->underrun_work);
> > -}
> 
> > +static void intel_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> > +{
> > +       if (!HAS_FBC(dev_priv))
> > +               return;
> > +
> > +       spin_lock(&dev_priv->underrun.lock);
> > +
> > +       if (dev_priv->underrun.detected)
> > +               goto out;
> > +       dev_priv->underrun.detected = true;
> > +
> > +       schedule_work(&dev_priv->underrun.work);
> > +out:
> > +       spin_unlock(&dev_priv->underrun.lock);
> 
> This locking (or bool) isn't required by the following patch either. But
> I presume the boolean is printed at some point, although that's probably
> less useful than whatever FBC/SAGV would say about being disabled.

I honestly didn't fully followed the initial goal of checking the detection
before scheduling the work.... So I assumed it was some underrun storm
and a mechanism to avoid multiple calls to fbc disable...

But yeap... I think we could live without it if no storm is possible
or if internal sagv and fbc are already handling those properly.

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Also disable SAGV on fifo underrun.

[Rodrigo Vivi]

On underrun situations and SAGV enabled we can face hard hangs.

So let's reuse the FBC workaround and expand that to SAGV
on the hope that it is not already too late for that.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ashar Shaikh <azhar.shaikh@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h           |  2 ++
 drivers/gpu/drm/i915/intel_fbc.c           |  3 ++
 drivers/gpu/drm/i915/intel_fifo_underrun.c | 17 +++++------
 drivers/gpu/drm/i915/intel_pm.c            | 45 ++++++++++++++++++++++++------
 4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3ca27cc8700d..169af853f6ff 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2003,10 +2003,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
 			      struct skl_pipe_wm *out);
 void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
 void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
+bool intel_has_sagv(struct drm_i915_private *dev_priv);
 bool intel_can_enable_sagv(struct drm_atomic_state *state);
 u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv);
 void intel_enable_sagv(struct drm_i915_private *dev_priv);
 void intel_disable_sagv(struct drm_i915_private *dev_priv);
+void intel_sagv_underrun_detected(struct drm_i915_private *dev_priv);
 bool skl_wm_level_equals(const struct skl_wm_level *l1,
 			 const struct skl_wm_level *l2);
 bool skl_ddb_allocation_overlaps(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index c63568564b77..a5d980227708 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1235,6 +1235,9 @@ void intel_fbc_underrun_detected(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
+	if (!fbc_supported(dev_priv))
+		return;
+
 	mutex_lock(&fbc->lock);
 
 	/* Maybe we were scheduled twice. */
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 6a290621177b..dea189b83827 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -356,23 +356,24 @@ static void intel_underrun_work_fn(struct work_struct *work)
 		container_of(work, struct drm_i915_private, underrun.work);
 
 	intel_fbc_underrun_detected(dev_priv);
+	intel_sagv_underrun_detected(dev_priv);
 }
 
 /*
- * intel_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
+ * intel_handle_fifo_underrun_irq - disable FBC and SAGV on a FIFO underrun
  *
- * Without FBC, most underruns are harmless and don't really cause too many
- * problems, except for an annoying message on dmesg. With FBC, underruns can
- * become black screens or even worse, especially when paired with bad
- * watermarks. So in order for us to be on the safe side, completely disable FBC
- * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
- * already suggests that watermarks may be bad, so try to be as safe as
+ * Without FBC or SAGV, most underruns are harmless and don't really cause too
+ * many problems, except for an annoying message on dmesg. With them, underruns
+ * can become black screens or even worse, especially when paired with bad
+ * watermarks. So in order for us to be on the safe side, completely disable
+ * them in case we ever detect a FIFO underrun on any pipe. An underrun on any
+ * pipe already suggests that watermarks may be bad, so try to be as safe as
  * possible.
  *
  */
 static void intel_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
 {
-	if (!HAS_FBC(dev_priv))
+	if (!HAS_FBC(dev_priv) && !intel_has_sagv(dev_priv))
 		return;
 
 	spin_lock(&dev_priv->underrun.lock);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e1ec9c2fd08a..84edaf161fe2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3581,8 +3581,7 @@ static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
 	return false;
 }
 
-static bool
-intel_has_sagv(struct drm_i915_private *dev_priv)
+bool intel_has_sagv(struct drm_i915_private *dev_priv)
 {
 	bool ret = false;
 
@@ -3648,6 +3647,22 @@ void intel_enable_sagv(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->sagv.lock);
 }
 
+static int __intel_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	mutex_lock(&dev_priv->pcu_lock);
+
+	/* bspec says to keep retrying for at least 1 ms */
+	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				GEN9_SAGV_DISABLE,
+				GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED,
+				1);
+	mutex_unlock(&dev_priv->pcu_lock);
+
+	return ret;
+}
+
 void intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
@@ -3660,14 +3675,8 @@ void intel_disable_sagv(struct drm_i915_private *dev_priv)
 		goto out;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
-	mutex_lock(&dev_priv->pcu_lock);
 
-	/* bspec says to keep retrying for at least 1 ms */
-	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
-				GEN9_SAGV_DISABLE,
-				GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED,
-				1);
-	mutex_unlock(&dev_priv->pcu_lock);
+	ret = __intel_disable_sagv(dev_priv);
 
 	/*
 	 * Some skl systems, pre-release machines in particular,
@@ -3687,6 +3696,24 @@ void intel_disable_sagv(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->sagv.lock);
 }
 
+void intel_sagv_underrun_detected(struct drm_i915_private *dev_priv)
+{
+	if (!intel_has_sagv(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->sagv.lock);
+
+	if (dev_priv->sagv.status == I915_SAGV_NOT_CONTROLLED)
+		goto out;
+
+	DRM_DEBUG_KMS("Disabling SAGV due to FIFO underrun.\n");
+	__intel_disable_sagv(dev_priv);
+	dev_priv->sagv.status = I915_SAGV_NOT_CONTROLLED;
+
+out:
+	mutex_unlock(&dev_priv->sagv.lock);
+}
+
 bool intel_can_enable_sagv(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Don't propagate SAGV errnos in vain.

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Don't propagate SAGV errnos in vain.
URL   : https://patchwork.freedesktop.org/series/38987/
State : failure

== Summary ==

Applying: drm/i915: Don't propagate SAGV errnos in vain.
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_drv.h).
error: could not build fake ancestor
Patch failed at 0001 drm/i915: Don't propagate SAGV errnos in vain.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx