diff mbox

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

Message ID 20180226205309.2438-3-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Feb. 26, 2018, 8:53 p.m. UTC 
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(-)

Comments

Chris Wilson Feb. 26, 2018, 9 p.m. UTC | #1 
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
Rodrigo Vivi Feb. 26, 2018, 10:23 p.m. UTC | #2 
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
diff mbox

Patch

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);
 }
 
 /**