All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
@ 2019-03-22 18:08 Ville Syrjala
  2019-03-22 18:08 ` [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2 Ville Syrjala
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ville Syrjala @ 2019-03-22 18:08 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The AGPBUSY thing doesn't work on i945gm anymore. This means
the gmch is incapable of waking the CPU from C3 when an interrupt
is generated. The interrupts just get postponed indefinitely until
something wakes up the CPU. This is rather annoying for vblank
interrupts as we are unable to maintain a steady framerate
unless the machine is sufficiently loaded to stay out of C3.

To combat this let's use pm_qos to prevent C3 whenever vblank
interrupts are enabled. To maintain reasonable amount of powersaving
we will attempt to limit this to C3 only while leaving C1 and C2
enabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30364
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  8 +++
 drivers/gpu/drm/i915/i915_irq.c | 88 +++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f723b15527f8..0c736f8ca1b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2042,6 +2042,14 @@ struct drm_i915_private {
 		struct i915_vma *scratch;
 	} gt;
 
+	/* For i945gm vblank irq vs. C3 workaround */
+	struct {
+		struct work_struct work;
+		struct pm_qos_request pm_qos;
+		u8 c3_disable_latency;
+		u8 enabled;
+	} i945gm_vblank;
+
 	/* perform PHY state sanity checks? */
 	bool chv_phy_assert[2];
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2f788291cfe0..33386f0acab3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -30,6 +30,7 @@
 
 #include <linux/sysrq.h>
 #include <linux/slab.h>
+#include <linux/cpuidle.h>
 #include <linux/circ_buf.h>
 #include <drm/drm_irq.h>
 #include <drm/drm_drv.h>
@@ -3131,6 +3132,16 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	return 0;
 }
 
+static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (dev_priv->i945gm_vblank.enabled++ == 0)
+		schedule_work(&dev_priv->i945gm_vblank.work);
+
+	return i8xx_enable_vblank(dev, pipe);
+}
+
 static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -3195,6 +3206,16 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
+static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	i8xx_disable_vblank(dev, pipe);
+
+	if (--dev_priv->i945gm_vblank.enabled == 0)
+		schedule_work(&dev_priv->i945gm_vblank.work);
+}
+
 static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -3228,6 +3249,60 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
+static void i945gm_vblank_work_func(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, i945gm_vblank.work);
+
+	/*
+	 * Vblank interrupts fail to wake up the device from C3,
+	 * hence we want to prevent C3 usage while vblank interrupts
+	 * are enabled.
+	 */
+	pm_qos_update_request(&dev_priv->i945gm_vblank.pm_qos,
+			      dev_priv->i945gm_vblank.enabled ?
+			      dev_priv->i945gm_vblank.c3_disable_latency :
+			      PM_QOS_DEFAULT_VALUE);
+}
+
+static int cstate_disable_latency(const char *name)
+{
+	const struct cpuidle_driver *drv;
+	int i;
+
+	drv = cpuidle_get_driver();
+	if (!drv)
+		return 0;
+
+	for (i = 0; i < drv->state_count; i++) {
+		const struct cpuidle_state *state = &drv->states[i];
+
+		if (!strcmp(state->name, name))
+			return state->exit_latency ?
+				state->exit_latency - 1 : 0;
+	}
+
+	return 0;
+}
+
+static void i945gm_vblank_work_init(struct drm_i915_private *dev_priv)
+{
+	INIT_WORK(&dev_priv->i945gm_vblank.work,
+		  i945gm_vblank_work_func);
+
+	dev_priv->i945gm_vblank.c3_disable_latency =
+		cstate_disable_latency("C3");
+	pm_qos_add_request(&dev_priv->i945gm_vblank.pm_qos,
+			   PM_QOS_CPU_DMA_LATENCY,
+			   PM_QOS_DEFAULT_VALUE);
+}
+
+static void i945gm_vblank_work_fini(struct drm_i915_private *dev_priv)
+{
+	cancel_work_sync(&dev_priv->i945gm_vblank.work);
+	pm_qos_remove_request(&dev_priv->i945gm_vblank.pm_qos);
+}
+
 static void ibx_irq_reset(struct drm_i915_private *dev_priv)
 {
 	if (HAS_PCH_NOP(dev_priv))
@@ -4525,6 +4600,9 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	struct intel_rps *rps = &dev_priv->gt_pm.rps;
 	int i;
 
+	if (IS_I945GM(dev_priv))
+		i945gm_vblank_work_init(dev_priv);
+
 	intel_hpd_init_work(dev_priv);
 
 	INIT_WORK(&rps->work, gen6_pm_rps_work);
@@ -4647,6 +4725,13 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 			dev->driver->irq_uninstall = i8xx_irq_reset;
 			dev->driver->enable_vblank = i8xx_enable_vblank;
 			dev->driver->disable_vblank = i8xx_disable_vblank;
+		} else if (IS_I945GM(dev_priv)) {
+			dev->driver->irq_preinstall = i915_irq_reset;
+			dev->driver->irq_postinstall = i915_irq_postinstall;
+			dev->driver->irq_uninstall = i915_irq_reset;
+			dev->driver->irq_handler = i915_irq_handler;
+			dev->driver->enable_vblank = i945gm_enable_vblank;
+			dev->driver->disable_vblank = i945gm_disable_vblank;
 		} else if (IS_GEN(dev_priv, 3)) {
 			dev->driver->irq_preinstall = i915_irq_reset;
 			dev->driver->irq_postinstall = i915_irq_postinstall;
@@ -4677,6 +4762,9 @@ void intel_irq_fini(struct drm_i915_private *i915)
 {
 	int i;
 
+	if (IS_I945GM(i915))
+		i945gm_vblank_work_fini(i915);
+
 	for (i = 0; i < MAX_L3_SLICES; ++i)
 		kfree(i915->l3_parity.remap_info[i]);
 }
-- 
2.19.2

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2
  2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
@ 2019-03-22 18:08 ` Ville Syrjala
  2019-03-22 21:09   ` Chris Wilson
  2019-03-22 19:53 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjala @ 2019-03-22 18:08 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The vblank timestamp->counter guesstimator seems to be
working sufficiently well, so there's no reason not to
disable vblank interrupts ASAP even on gen2.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 33386f0acab3..a5aff4de5fb0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4642,13 +4642,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	else if (INTEL_GEN(dev_priv) >= 3)
 		dev->driver->get_vblank_counter = i915_get_vblank_counter;
 
-	/*
-	 * Opt out of the vblank disable timer on everything except gen2.
-	 * Gen2 doesn't have a hardware frame counter and so depends on
-	 * vblank interrupts to produce sane vblank seuquence numbers.
-	 */
-	if (!IS_GEN(dev_priv, 2))
-		dev->vblank_disable_immediate = true;
+	dev->vblank_disable_immediate = true;
 
 	/* Most platforms treat the display irq block as an always-on
 	 * power domain. vlv/chv can disable it at runtime and need
-- 
2.19.2

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
  2019-03-22 18:08 ` [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2 Ville Syrjala
@ 2019-03-22 19:53 ` Patchwork
  2019-03-22 20:15 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-03-22 19:53 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
URL   : https://patchwork.freedesktop.org/series/58427/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Disable C3 when enabling vblank interrupts on i945gm
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3575:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3583:16: warning: expression using sizeof(void)

Commit: drm/i915: Use vblank_disable_immediate on gen2
Okay!

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
  2019-03-22 18:08 ` [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2 Ville Syrjala
  2019-03-22 19:53 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Patchwork
@ 2019-03-22 20:15 ` Patchwork
  2019-03-22 21:08 ` [PATCH 1/2] " Chris Wilson
  2019-03-23 23:03 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-03-22 20:15 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
URL   : https://patchwork.freedesktop.org/series/58427/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5796 -> Patchwork_12579
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58427/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12579 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@gem_exec_basic@gtt-bsd:
    - fi-bwr-2160:        NOTRUN -> SKIP [fdo#109271] +103

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] +57

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_uncore:
    - fi-ivb-3770:        PASS -> DMESG-FAIL [fdo#110210]

  * igt@kms_busy@basic-flip-c:
    - fi-bwr-2160:        NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          PASS -> FAIL [fdo#103167]

  
#### Possible fixes ####

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      DMESG-FAIL -> PASS

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       DMESG-WARN [fdo#107709] -> PASS

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210


Participating hosts (40 -> 35)
------------------------------

  Additional (2): fi-bwr-2160 fi-snb-2520m 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5796 -> Patchwork_12579

  CI_DRM_5796: 2165d3631bdb91a7b80110b1f76633a19666d0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4899: ba96339c238180b38d05d7fa2dca772d49eee332 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12579: 096380bc0b9ba1e91b43b2b5ec713e0fb28cbd93 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

096380bc0b9b drm/i915: Use vblank_disable_immediate on gen2
f7715f80c822 drm/i915: Disable C3 when enabling vblank interrupts on i945gm

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12579/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
                   ` (2 preceding siblings ...)
  2019-03-22 20:15 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-03-22 21:08 ` Chris Wilson
  2019-03-22 23:55   ` Ville Syrjälä
  2019-03-23 23:03 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
  4 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2019-03-22 21:08 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

Quoting Ville Syrjala (2019-03-22 18:08:03)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The AGPBUSY thing doesn't work on i945gm anymore. This means
> the gmch is incapable of waking the CPU from C3 when an interrupt
> is generated. The interrupts just get postponed indefinitely until
> something wakes up the CPU. This is rather annoying for vblank
> interrupts as we are unable to maintain a steady framerate
> unless the machine is sufficiently loaded to stay out of C3.
> 
> To combat this let's use pm_qos to prevent C3 whenever vblank
> interrupts are enabled. To maintain reasonable amount of powersaving
> we will attempt to limit this to C3 only while leaving C1 and C2
> enabled.

Interesting compromise. Frankly, I had considered pm_qos in an
all-or-nothing approach, partly because finding the C transitions is a
bit opaque.
 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30364
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  8 +++
>  drivers/gpu/drm/i915/i915_irq.c | 88 +++++++++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f723b15527f8..0c736f8ca1b2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2042,6 +2042,14 @@ struct drm_i915_private {
>                 struct i915_vma *scratch;
>         } gt;
>  
> +       /* For i945gm vblank irq vs. C3 workaround */
> +       struct {
> +               struct work_struct work;
> +               struct pm_qos_request pm_qos;
> +               u8 c3_disable_latency;

Ok, looks a bit tight, but checks out.

> +               u8 enabled;
> +       } i945gm_vblank;
> +
>         /* perform PHY state sanity checks? */
>         bool chv_phy_assert[2];
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2f788291cfe0..33386f0acab3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -30,6 +30,7 @@
>  
>  #include <linux/sysrq.h>
>  #include <linux/slab.h>
> +#include <linux/cpuidle.h>
>  #include <linux/circ_buf.h>
>  #include <drm/drm_irq.h>
>  #include <drm/drm_drv.h>
> @@ -3131,6 +3132,16 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
>         return 0;
>  }
>  
> +static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +       if (dev_priv->i945gm_vblank.enabled++ == 0)
> +               schedule_work(&dev_priv->i945gm_vblank.work);

I was thinking u8, isn't that a bit dangerous. But the max counter here
should be num_pipes. Hmm, is the vblank spinlock local to a pipe? Nope,
dev->vbl_lock.

> +
> +       return i8xx_enable_vblank(dev, pipe);
> +}
> +
>  static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3195,6 +3206,16 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
>         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> +static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +       i8xx_disable_vblank(dev, pipe);
> +
> +       if (--dev_priv->i945gm_vblank.enabled == 0)
> +               schedule_work(&dev_priv->i945gm_vblank.work);
> +}
> +
>  static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>         struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3228,6 +3249,60 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
>         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> +static void i945gm_vblank_work_func(struct work_struct *work)
> +{
> +       struct drm_i915_private *dev_priv =
> +               container_of(work, struct drm_i915_private, i945gm_vblank.work);
> +
> +       /*
> +        * Vblank interrupts fail to wake up the device from C3,
> +        * hence we want to prevent C3 usage while vblank interrupts
> +        * are enabled.
> +        */
> +       pm_qos_update_request(&dev_priv->i945gm_vblank.pm_qos,
> +                             dev_priv->i945gm_vblank.enabled ?
> +                             dev_priv->i945gm_vblank.c3_disable_latency :
> +                             PM_QOS_DEFAULT_VALUE);

Worker is required as the update may block.

I'd prefer that as READ_ONCE(dev_priv->i945gm_vblank.enabled)

> +}
> +
> +static int cstate_disable_latency(const char *name)
> +{
> +       const struct cpuidle_driver *drv;
> +       int i;
> +
> +       drv = cpuidle_get_driver();
> +       if (!drv)
> +               return 0;
> +
> +       for (i = 0; i < drv->state_count; i++) {
> +               const struct cpuidle_state *state = &drv->states[i];
> +
> +               if (!strcmp(state->name, name))
> +                       return state->exit_latency ?
> +                               state->exit_latency - 1 : 0;
> +       }

Mind if I say yuck?

Will only work with the intel_idle driver. And if not present, we force
the system to not sleep while vblanks are ticking over.

And it is exit_latency that is compared against the qos request.

Ok. It does what it says on the tin.

One of the reasons I've hesitated in the past was that I considered
vblanks as a background heartbeat while the system is active and pretty
much constantly on while the screen is. However, I suppose that is true
(and is evidenced by recent systems that do not sleep while the screens
are on, at least not with an active link).

The worst that can happen is someone complains about a hot laptop, and
the remedy is simple if we don't succeed in killing it first,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2
  2019-03-22 18:08 ` [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2 Ville Syrjala
@ 2019-03-22 21:09   ` Chris Wilson
  0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2019-03-22 21:09 UTC (permalink / raw)
  To: Ville Syrjala, intel-gfx

Quoting Ville Syrjala (2019-03-22 18:08:04)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The vblank timestamp->counter guesstimator seems to be
> working sufficiently well, so there's no reason not to
> disable vblank interrupts ASAP even on gen2.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 21:08 ` [PATCH 1/2] " Chris Wilson
@ 2019-03-22 23:55   ` Ville Syrjälä
  2019-03-25  7:03     ` Ville Syrjälä
  0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2019-03-22 23:55 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Mar 22, 2019 at 09:08:35PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2019-03-22 18:08:03)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The AGPBUSY thing doesn't work on i945gm anymore. This means
> > the gmch is incapable of waking the CPU from C3 when an interrupt
> > is generated. The interrupts just get postponed indefinitely until
> > something wakes up the CPU. This is rather annoying for vblank
> > interrupts as we are unable to maintain a steady framerate
> > unless the machine is sufficiently loaded to stay out of C3.
> > 
> > To combat this let's use pm_qos to prevent C3 whenever vblank
> > interrupts are enabled. To maintain reasonable amount of powersaving
> > we will attempt to limit this to C3 only while leaving C1 and C2
> > enabled.
> 
> Interesting compromise. Frankly, I had considered pm_qos in an
> all-or-nothing approach, partly because finding the C transitions is a
> bit opaque.
>  
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30364
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  8 +++
> >  drivers/gpu/drm/i915/i915_irq.c | 88 +++++++++++++++++++++++++++++++++
> >  2 files changed, 96 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f723b15527f8..0c736f8ca1b2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2042,6 +2042,14 @@ struct drm_i915_private {
> >                 struct i915_vma *scratch;
> >         } gt;
> >  
> > +       /* For i945gm vblank irq vs. C3 workaround */
> > +       struct {
> > +               struct work_struct work;
> > +               struct pm_qos_request pm_qos;
> > +               u8 c3_disable_latency;
> 
> Ok, looks a bit tight, but checks out.
> 
> > +               u8 enabled;
> > +       } i945gm_vblank;
> > +
> >         /* perform PHY state sanity checks? */
> >         bool chv_phy_assert[2];
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2f788291cfe0..33386f0acab3 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -30,6 +30,7 @@
> >  
> >  #include <linux/sysrq.h>
> >  #include <linux/slab.h>
> > +#include <linux/cpuidle.h>
> >  #include <linux/circ_buf.h>
> >  #include <drm/drm_irq.h>
> >  #include <drm/drm_drv.h>
> > @@ -3131,6 +3132,16 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >         return 0;
> >  }
> >  
> > +static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +       if (dev_priv->i945gm_vblank.enabled++ == 0)
> > +               schedule_work(&dev_priv->i945gm_vblank.work);
> 
> I was thinking u8, isn't that a bit dangerous. But the max counter here
> should be num_pipes. Hmm, is the vblank spinlock local to a pipe? Nope,
> dev->vbl_lock.
> 
> > +
> > +       return i8xx_enable_vblank(dev, pipe);
> > +}
> > +
> >  static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -3195,6 +3206,16 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
> >         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  }
> >  
> > +static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > +{
> > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +       i8xx_disable_vblank(dev, pipe);
> > +
> > +       if (--dev_priv->i945gm_vblank.enabled == 0)
> > +               schedule_work(&dev_priv->i945gm_vblank.work);
> > +}
> > +
> >  static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -3228,6 +3249,60 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
> >         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  }
> >  
> > +static void i945gm_vblank_work_func(struct work_struct *work)
> > +{
> > +       struct drm_i915_private *dev_priv =
> > +               container_of(work, struct drm_i915_private, i945gm_vblank.work);
> > +
> > +       /*
> > +        * Vblank interrupts fail to wake up the device from C3,
> > +        * hence we want to prevent C3 usage while vblank interrupts
> > +        * are enabled.
> > +        */
> > +       pm_qos_update_request(&dev_priv->i945gm_vblank.pm_qos,
> > +                             dev_priv->i945gm_vblank.enabled ?
> > +                             dev_priv->i945gm_vblank.c3_disable_latency :
> > +                             PM_QOS_DEFAULT_VALUE);
> 
> Worker is required as the update may block.
> 
> I'd prefer that as READ_ONCE(dev_priv->i945gm_vblank.enabled)

Yeah, that does seem appropriate.

> 
> > +}
> > +
> > +static int cstate_disable_latency(const char *name)
> > +{
> > +       const struct cpuidle_driver *drv;
> > +       int i;
> > +
> > +       drv = cpuidle_get_driver();
> > +       if (!drv)
> > +               return 0;
> > +
> > +       for (i = 0; i < drv->state_count; i++) {
> > +               const struct cpuidle_state *state = &drv->states[i];
> > +
> > +               if (!strcmp(state->name, name))
> > +                       return state->exit_latency ?
> > +                               state->exit_latency - 1 : 0;
> > +       }
> 
> Mind if I say yuck?

Go right ahead.

> 
> Will only work with the intel_idle driver.

acpi_idle here. I doubt there are 945gm systems with a CPU fancy enough
for intel_idle to pick up. I'd like that for my i965gm actually where
the silly acpi tables don't let me have C4 unless it's running on
battery.

> And if not present, we force
> the system to not sleep while vblanks are ticking over.

I guess we could just skip it all if the idle driver is funky,
but I wouldn't expect much power savings in that case anyway.

> 
> And it is exit_latency that is compared against the qos request.
> 
> Ok. It does what it says on the tin.
> 
> One of the reasons I've hesitated in the past was that I considered
> vblanks as a background heartbeat while the system is active and pretty
> much constantly on while the screen is. However, I suppose that is true
> (and is evidenced by recent systems that do not sleep while the screens
> are on, at least not with an active link).
> 
> The worst that can happen is someone complains about a hot laptop, and
> the remedy is simple if we don't succeed in killing it first,

Now you made me doubt how costly disabling C3 actually is.
So I had to measure it. The cpu is a core duo something or
another.

Idle system with the display off shows a difference of ~2W, which
is quite a bit more than I was expecting tbh. That's in the ballpark
of +25%. Fortunately we wouldn't hit this since no vblanks with display
off.

With the display on the difference drops to ~0.7W, or perhaps around
+5% with minimum backlight level. Not great but maybe not a total
disaster.

Actually having vblank interrupts ticking didn't make a significant
difference to the C2 or C3 numbers, but of course we can't entirely
trust the C3 numbers or else we wouldn't be here.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
                   ` (3 preceding siblings ...)
  2019-03-22 21:08 ` [PATCH 1/2] " Chris Wilson
@ 2019-03-23 23:03 ` Patchwork
  4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-03-23 23:03 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
URL   : https://patchwork.freedesktop.org/series/58427/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5796_full -> Patchwork_12579_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_12579_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12579_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12579_full:

### IGT changes ###

#### Warnings ####

  * igt@prime_nv_test@nv_write_i915_cpu_mmap_read:
    - shard-iclb:         SKIP [fdo#109291] -> INCOMPLETE

  
Known issues
------------

  Here are the changes found in Patchwork_12579_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_store@pages-bsd1:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +10

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#109100]

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         PASS -> FAIL [fdo#108686]

  * igt@i915_pm_rpm@cursor-dpms:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@kms_atomic_transition@5x-modeset-transitions-nonblocking:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4

  * igt@kms_busy@extended-modeset-hang-newfb-render-c:
    - shard-skl:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#110222]

  * igt@kms_busy@extended-pageflip-hang-newfb-render-e:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +79

  * igt@kms_fbcon_fbt@fbc:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109593]

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          PASS -> FAIL [fdo#105363]

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#109507]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +8

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-blt:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +48

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-mmap-wc:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +16

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-cpu:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#109247]

  * igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
    - shard-skl:          NOTRUN -> FAIL [fdo#105683]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          PASS -> FAIL [fdo#108145]

  * igt@kms_plane_scaling@pipe-a-scaler-with-rotation:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_psr@sprite_render:
    - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +4

  * igt@kms_sysfs_edid_timing:
    - shard-skl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_vblank@pipe-c-ts-continuation-idle:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +6

  * igt@kms_vblank@pipe-c-ts-continuation-modeset-rpm:
    - shard-apl:          PASS -> FAIL [fdo#104894] +1

  * igt@runner@aborted:
    - shard-iclb:         NOTRUN -> FAIL [fdo#109593]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@gem-mmap-gtt:
    - shard-skl:          INCOMPLETE [fdo#107807] -> PASS

  * igt@kms_busy@extended-pageflip-hang-newfb-render-b:
    - shard-apl:          DMESG-WARN [fdo#110222] -> PASS +1

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          INCOMPLETE [fdo#107773] / [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +8

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +20

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          FAIL [fdo#108145] -> PASS +1

  * igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
    - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> PASS +2

  * igt@kms_psr@cursor_mmap_cpu:
    - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +1

  * igt@kms_psr@no_drrs:
    - shard-iclb:         FAIL [fdo#108341] -> PASS

  * igt@kms_vblank@pipe-b-ts-continuation-modeset:
    - shard-apl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]

  * igt@i915_selftest@live_contexts:
    - shard-iclb:         DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-hsw 


Build changes
-------------

    * Linux: CI_DRM_5796 -> Patchwork_12579

  CI_DRM_5796: 2165d3631bdb91a7b80110b1f76633a19666d0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4899: ba96339c238180b38d05d7fa2dca772d49eee332 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12579: 096380bc0b9ba1e91b43b2b5ec713e0fb28cbd93 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12579/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm
  2019-03-22 23:55   ` Ville Syrjälä
@ 2019-03-25  7:03     ` Ville Syrjälä
  0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2019-03-25  7:03 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Sat, Mar 23, 2019 at 01:55:34AM +0200, Ville Syrjälä wrote:
> On Fri, Mar 22, 2019 at 09:08:35PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2019-03-22 18:08:03)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The AGPBUSY thing doesn't work on i945gm anymore. This means
> > > the gmch is incapable of waking the CPU from C3 when an interrupt
> > > is generated. The interrupts just get postponed indefinitely until
> > > something wakes up the CPU. This is rather annoying for vblank
> > > interrupts as we are unable to maintain a steady framerate
> > > unless the machine is sufficiently loaded to stay out of C3.
> > > 
> > > To combat this let's use pm_qos to prevent C3 whenever vblank
> > > interrupts are enabled. To maintain reasonable amount of powersaving
> > > we will attempt to limit this to C3 only while leaving C1 and C2
> > > enabled.
> > 
> > Interesting compromise. Frankly, I had considered pm_qos in an
> > all-or-nothing approach, partly because finding the C transitions is a
> > bit opaque.
> >  
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=30364
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |  8 +++
> > >  drivers/gpu/drm/i915/i915_irq.c | 88 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 96 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index f723b15527f8..0c736f8ca1b2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2042,6 +2042,14 @@ struct drm_i915_private {
> > >                 struct i915_vma *scratch;
> > >         } gt;
> > >  
> > > +       /* For i945gm vblank irq vs. C3 workaround */
> > > +       struct {
> > > +               struct work_struct work;
> > > +               struct pm_qos_request pm_qos;
> > > +               u8 c3_disable_latency;
> > 
> > Ok, looks a bit tight, but checks out.
> > 
> > > +               u8 enabled;
> > > +       } i945gm_vblank;
> > > +
> > >         /* perform PHY state sanity checks? */
> > >         bool chv_phy_assert[2];
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 2f788291cfe0..33386f0acab3 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -30,6 +30,7 @@
> > >  
> > >  #include <linux/sysrq.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/cpuidle.h>
> > >  #include <linux/circ_buf.h>
> > >  #include <drm/drm_irq.h>
> > >  #include <drm/drm_drv.h>
> > > @@ -3131,6 +3132,16 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > >         return 0;
> > >  }
> > >  
> > > +static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +       if (dev_priv->i945gm_vblank.enabled++ == 0)
> > > +               schedule_work(&dev_priv->i945gm_vblank.work);
> > 
> > I was thinking u8, isn't that a bit dangerous. But the max counter here
> > should be num_pipes. Hmm, is the vblank spinlock local to a pipe? Nope,
> > dev->vbl_lock.
> > 
> > > +
> > > +       return i8xx_enable_vblank(dev, pipe);
> > > +}
> > > +
> > >  static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > >  {
> > >         struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -3195,6 +3206,16 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > >         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > >  }
> > >  
> > > +static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +       i8xx_disable_vblank(dev, pipe);
> > > +
> > > +       if (--dev_priv->i945gm_vblank.enabled == 0)
> > > +               schedule_work(&dev_priv->i945gm_vblank.work);
> > > +}
> > > +
> > >  static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > >  {
> > >         struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -3228,6 +3249,60 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
> > >         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > >  }
> > >  
> > > +static void i945gm_vblank_work_func(struct work_struct *work)
> > > +{
> > > +       struct drm_i915_private *dev_priv =
> > > +               container_of(work, struct drm_i915_private, i945gm_vblank.work);
> > > +
> > > +       /*
> > > +        * Vblank interrupts fail to wake up the device from C3,
> > > +        * hence we want to prevent C3 usage while vblank interrupts
> > > +        * are enabled.
> > > +        */
> > > +       pm_qos_update_request(&dev_priv->i945gm_vblank.pm_qos,
> > > +                             dev_priv->i945gm_vblank.enabled ?
> > > +                             dev_priv->i945gm_vblank.c3_disable_latency :
> > > +                             PM_QOS_DEFAULT_VALUE);
> > 
> > Worker is required as the update may block.
> > 
> > I'd prefer that as READ_ONCE(dev_priv->i945gm_vblank.enabled)
> 
> Yeah, that does seem appropriate.

Pushed with that before I have chance to regret the 0.7W.
Thanks for the review.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-03-25  7:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-22 18:08 [PATCH 1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Ville Syrjala
2019-03-22 18:08 ` [PATCH 2/2] drm/i915: Use vblank_disable_immediate on gen2 Ville Syrjala
2019-03-22 21:09   ` Chris Wilson
2019-03-22 19:53 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Disable C3 when enabling vblank interrupts on i945gm Patchwork
2019-03-22 20:15 ` ✓ Fi.CI.BAT: success " Patchwork
2019-03-22 21:08 ` [PATCH 1/2] " Chris Wilson
2019-03-22 23:55   ` Ville Syrjälä
2019-03-25  7:03     ` Ville Syrjälä
2019-03-23 23:03 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.