On Mon, May 26, 2014 at 03:21:41PM +0200, Daniel Vetter wrote:

On Mon, May 26, 2014 at 02:46:24PM +0300, ville.syrjala@linux.intel.com wrote:

...

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

Make sure drm_vblank_get() never succeeds when called between drm_vblank_off() and drm_vblank_on(). Borrow a trick from the old drm_vblank_{pre,post}_modeset() functions and just bump the refcount in drm_vblank_off() and drop it in drm_vblank_on().

Hopefully the use of inmodeset won't conflict badly with drm_vblank_{pre,post}_modeset().

For i915 there's a window between drm_vblank_off() and marking the crtc as inactive where the current code still allows drm_vblank_get().

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Afaics we never check anywhere for inmodeset in drm_vblank_get, so how does this work?

If refcount is already >0 and vblank irq is disabled, drm_vblank_get() will return -EINVAL.

-Daniel

...

---

drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index b565372..7b1e08c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc) } spin_unlock(&dev->event_lock);

• /*

• * Prevent subsequent drm_vblank_get() from re-enabling
  

• * the vblank interrupt by bumping the refcount.
  

• */
  

• if (!dev->vblank[crtc].inmodeset) {

• atomic_inc(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 1;
  

• }
• spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

} EXPORT_SYMBOL(drm_vblank_off); @@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc) unsigned long irqflags;

spin_lock_irqsave(&dev->vbl_lock, irqflags);

• /* Drop our private "prevent drm_vblank_get" refcount */
• if (dev->vblank[crtc].inmodeset) {

• atomic_dec(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 0;
  

• } /* re-enable interrupts if there's are users left */ if (atomic_read(&dev->vblank[crtc].refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc));

-- 1.8.5.5

---

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

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Mon, May 26, 2014 at 03:22:45PM +0200, Daniel Vetter wrote:

On Mon, May 26, 2014 at 02:46:25PM +0300, ville.syrjala@linux.intel.com wrote:

...

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

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3da73ef..da318a7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1305,6 +1305,17 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, } }

+static void assert_vblank_disabled(struct intel_crtc *crtc) +{

• struct drm_device *dev = crtc->base.dev;
• enum pipe pipe = crtc->pipe;
• if (WARN_ON(drm_vblank_get(dev, pipe) == 0)) {

• drm_vblank_put(dev, pipe);
  

• drm_vblank_off(dev, pipe);
  

Imo the _off is too much, since with that it's not just an assert but a "... and please make it so if not". Imo better to drop that.

The idea was that if drm_vblank_get() managed to re-enable vblank interrupts when it wasn't supposed to, we should turn them off again. But the whole thing is a bug, and another drm_vblank_get() might happen just after the drm_vblank_off() anyway, so I guess it's a bit pointless.

-Daniel

...

• }

+}

static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) { u32 val; @@ -3885,6 +3896,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;

• assert_vblank_disabled(intel_crtc);
• intel_enable_primary_hw_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); /* The fixup needs to happen before cursor is enabled */

@@ -3921,6 +3934,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc) intel_crtc_update_cursor(crtc, false); intel_disable_planes(crtc); intel_disable_primary_hw_plane(dev_priv, plane, pipe);

• assert_vblank_disabled(intel_crtc);

}

static void ironlake_crtc_enable(struct drm_crtc *crtc)

1.8.5.5

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

v2: Drop the drm_vblank_off() (Daniel) Use drm_crtc_vblank_{get,put}()

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3da73ef..94ac51f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1305,6 +1305,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv, } }

+static void assert_vblank_disabled(struct drm_crtc *crtc) +{ + if (WARN_ON(drm_crtc_vblank_get(crtc) == 0)) + drm_crtc_vblank_put(crtc); +} + static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv) { u32 val; @@ -3885,6 +3891,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; int plane = intel_crtc->plane;

+ assert_vblank_disabled(crtc); + intel_enable_primary_hw_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); /* The fixup needs to happen before cursor is enabled */ @@ -3921,6 +3929,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc) intel_crtc_update_cursor(crtc, false); intel_disable_planes(crtc); intel_disable_primary_hw_plane(dev_priv, plane, pipe); + + assert_vblank_disabled(crtc); }

static void ironlake_crtc_enable(struct drm_crtc *crtc)

On Mon, May 26, 2014 at 02:46:28PM +0300, ville.syrjala@linux.intel.com wrote:

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

If the vblank irq has already been disabled (via the disable timer) when we call drm_vblank_off() sample the counter and timestamp one last time. This will make the sure that the user space visible counter will account for time between vblank irq disable and drm_vblank_off().

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_irq.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index e12cf69..bb64f0f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -140,6 +140,19 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) */ spin_lock_irqsave(&dev->vblank_time_lock, irqflags);

• /*

• * If the vblank interrupt was already disbled update the count
  

• * and timestamp to maintain the appearance that the counter
  

• * has been ticking all along until this time. This makes the
  

• * count account for the entire time between drm_vblank_on() and
  

• * drm_vblank_off().
  

• */
  

• if (!dev->vblank[crtc].enabled) {

• drm_update_vblank_count(dev, crtc);
  

• spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
  

• return;
  

• }
• dev->driver->disable_vblank(dev, crtc); dev->vblank[crtc].enabled = false;

-- 1.8.5.5

---

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

On Mon, May 26, 2014 at 02:46:29PM +0300, ville.syrjala@linux.intel.com wrote:

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

When drm_vblank_on() is called the hardware vblank counter may have been reset, so we can't trust that the old values sampled prior to drm_vblank_off() have anything to do with the new values.

So update the .last count in drm_vblank_on() to make the first drm_vblank_enable() consider that as the reference point. This will correct the user space visible counter to account for the time between drm_vblank_on() and the first drm_vblank_enable() calls.

For extra safety subtract one from the .last count in drm_vblank_on() to make sure that user space will never see the same counter value before and after modeset.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_irq.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index bb64f0f..54cb85d 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1095,6 +1095,18 @@ void drm_vblank_on(struct drm_device *dev, int crtc) atomic_dec(&dev->vblank[crtc].refcount); dev->vblank[crtc].inmodeset = 0; }

• /*

• * sample the current counter to avoid random jumps
  

• * when drm_vblank_enable() applies the diff
  

• *
  

• * -1 to make sure user will never see the same
  

• * vblank counter value before and after a modeset
  

• */
  

• dev->vblank[crtc].last =

• (dev->driver->get_vblank_counter(dev, crtc) - 1) &
  

• dev->max_vblank_count;
  

• /* re-enable interrupts if there's are users left */ if (atomic_read(&dev->vblank[crtc].refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc));

-- 1.8.5.5

---

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

On Mon, May 26, 2014 at 02:46:30PM +0300, ville.syrjala@linux.intel.com wrote:

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

Make drm_vblank_put() disable the vblank interrupt immediately when the refcount drops to zero and drm_vblank_offdelay==0.

Currently drm_vblank_put() would just leave vblank interrupts enabled all the time if drm_vblank_offdelay==0. In case someone might still want that behaviour drm_vblank_offdelay is now signed and a negative value will allow the user to keep vblank interrupts on all the time. Well, since the first drm_vblank_get() anyway.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

I think what we actually want is a new dev->vblank_is_race_free or something which is a boolean. Then we can keep the drm_vblank_offdelay as-is and seletively kill the entire logic for drivers where this works correctly. -Daniel

---

Documentation/DocBook/drm.tmpl | 1 + drivers/gpu/drm/drm_irq.c | 11 +++++++---- drivers/gpu/drm/drm_stub.c | 4 ++-- drivers/gpu/drm/exynos/exynos_drm_drv.h | 2 +- include/drm/drmP.h | 2 +- 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 9574bf2..25632b0 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2508,6 +2508,7 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis> by scheduling a timer. The delay is accessible through the vblankoffdelay module parameter or the <varname>drm_vblank_offdelay</varname> global variable and expressed in milliseconds. Its default value is 5000 ms.

•  Zero means disable immediately, and a negative value means never disable.
  

  </para> <para> When a vertical blanking interrupt occurs drivers only need to call the

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 54cb85d..54a56b2 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -978,10 +978,13 @@ void drm_vblank_put(struct drm_device *dev, int crtc) BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);

/* Last user schedules interrupt disable */

• if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&

•    (drm_vblank_offdelay > 0))
  

• mod_timer(&dev->vblank[crtc].disable_timer,
  

• 	  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

• if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {

• if (drm_vblank_offdelay > 0)
  

• 	mod_timer(&dev->vblank[crtc].disable_timer,
  

• 		  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

• else if (drm_vblank_offdelay == 0)
  

• 	vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
  

• }

} EXPORT_SYMBOL(drm_vblank_put);

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 3727ac8..8758f81 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -49,7 +49,7 @@ EXPORT_SYMBOL(drm_rnodes); unsigned int drm_universal_planes = 0; EXPORT_SYMBOL(drm_universal_planes);

-unsigned int drm_vblank_offdelay = 5000; /* Default to 5000 msecs. */ +int drm_vblank_offdelay = 5000; /* Default to 5000 msecs. */ EXPORT_SYMBOL(drm_vblank_offdelay);

unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */ @@ -66,7 +66,7 @@ MODULE_DESCRIPTION(CORE_DESC); MODULE_LICENSE("GPL and additional rights"); MODULE_PARM_DESC(debug, "Enable debug output"); MODULE_PARM_DESC(rnodes, "Enable experimental render nodes API"); -MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs]"); +MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (< 0 means never disable)"); MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]"); MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index ce3e6a3..9734bec 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -40,7 +40,7 @@ struct drm_device; struct exynos_drm_overlay; struct drm_connector;

-extern unsigned int drm_vblank_offdelay; +extern int drm_vblank_offdelay;

/* this enumerates display type. */ enum exynos_drm_output_type { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 76ccaab..979a498 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1404,7 +1404,7 @@ extern unsigned int drm_debug; extern unsigned int drm_rnodes; extern unsigned int drm_universal_planes;

-extern unsigned int drm_vblank_offdelay; +extern int drm_vblank_offdelay; extern unsigned int drm_timestamp_precision; extern unsigned int drm_timestamp_monotonic;

-- 1.8.5.5

---

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

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

Now that the vblank races are plugged, we can opt out of using the vblank disable timer and just let vblank interrupts get disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 28bae6e..4b2e7af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */ }

+ /* + * 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_GEN2(dev)) + dev->vblank_disable_immediate = true; + if (drm_core_check_feature(dev, DRIVER_MODESET)) { dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;

On Mon, May 26, 2014 at 05:26:48PM +0300, ville.syrjala@linux.intel.com wrote:

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

Now that the vblank races are plugged, we can opt out of using the vblank disable timer and just let vblank interrupts get disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

I've forgotten to mention (I think so at least) that I'd like to have a new kms_flip subtest which alternates vblank events with longer hrtimer sleeps and still enables all the precise vblank counter/ts checks we have. That should give us tons of flip-flopping of the vblank counter.

After all videos run at 25fps, so are about the worst case for this (since the enable vblank for the 1 frame vblank wait and the for the pageflip, disabling it each time in between) and we very much don't want to fail this.

Bonus points if you add a 2nd thread which races against the first one for added fun (in a 2nd subtest). -Daniel

---

drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 28bae6e..4b2e7af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */ }

• /*

• * 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_GEN2(dev))

• dev->vblank_disable_immediate = true;
  

• if (drm_core_check_feature(dev, DRIVER_MODESET)) { dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;

-- 1.8.5.5

On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:

2014-05-26 11:26 GMT-03:00 ville.syrjala@linux.intel.com:

...

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

Now that the vblank races are plugged, we can opt out of using the vblank disable timer and just let vblank interrupts get disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

Hi

Remember last week's FBC vblank optimization patch that had an erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? After I fixed the bug in the patch I realized that it was the unbalanced vblank_get() call that moved PC state residency up.

I did some experiments, and on my specific BDW machine, after running "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 residency goes from 60-70% to 85-90% when I revert this patch. I'm running just an idle Cinnamon with an open terminal.

So, since the commit message lacks more details, what are the downsides of reverting this patch? What are the advantages of opting out of the vblank timer? I see my desktop does tons and tons of vblank get/put calls per second, so the disable timer makes a lot of sense.

"Idle" desktop :(

Really the immediate disable should save power. Where are these tons of vblank get/puts coming from actually? I would assume you'd get a handful per frame at most, and that when you're actually doing something. On an idle system I would expect nothing at all happens during most frames.

Not sure, but I guess it's possible the extra register accesses in the get/puts actually cause the display to exit low power states all the time, or something.

There's also this note in Bspec (for HSW at least): "Workaround : Do not enable and unmask this interrupt if the associated pipe is disabled. Do not leave this interrupt enabled and unmasked after the associated pipe is disabled." which we took to mean that having the interrupt masked but enabled is fine. But maybe we'd actually have to frob IER too to avoid wasting power somehow?

I also wish there was some easy way to check how this patch (or its revert) affect a bunch of different workloads...

(Also CCing Chris for insightful comments on performance)

IIRC Chris had a patch to not disable the interrupt immediately when the refcount drops to 0, but instead delay the disable until the next interrupt actually happens. But I guess it didn't go in? Probably I should have reviewed it but didn't. It sounds like a decent idea to me in any case for the active use case.

Thanks, Paulo

...

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 28bae6e..4b2e7af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */ }

•   /*
  

•    * 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_GEN2(dev))
  

•           dev->vblank_disable_immediate = true;
  

•   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
            dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
            dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
  

-- 1.8.5.5

---

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

-- Paulo Zanoni

On Thu, Nov 19, 2015 at 06:35:04PM -0200, Paulo Zanoni wrote:

2015-11-19 18:06 GMT-02:00 Ville Syrjälä ville.syrjala@linux.intel.com:

...

On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:

...

2014-05-26 11:26 GMT-03:00 ville.syrjala@linux.intel.com:

...

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

Now that the vblank races are plugged, we can opt out of using the vblank disable timer and just let vblank interrupts get disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

Hi

Remember last week's FBC vblank optimization patch that had an erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? After I fixed the bug in the patch I realized that it was the unbalanced vblank_get() call that moved PC state residency up.

I did some experiments, and on my specific BDW machine, after running "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 residency goes from 60-70% to 85-90% when I revert this patch. I'm running just an idle Cinnamon with an open terminal.

So, since the commit message lacks more details, what are the downsides of reverting this patch? What are the advantages of opting out of the vblank timer? I see my desktop does tons and tons of vblank get/put calls per second, so the disable timer makes a lot of sense.

"Idle" desktop :(

My first realization of this little problem was when I was implementing runtime PM :)

...

Really the immediate disable should save power. Where are these tons of vblank get/puts coming from actually?

I'll take a finer look tomorrow, but I assume it's probably some application redrawing. I see it does calm down sometimes, but that's not enough to get better PC7 residency.

...

I would assume you'd get a handful per frame at most, and that when you're actually doing something. On an idle system I would expect nothing at all happens during most frames.

Not sure, but I guess it's possible the extra register accesses in the get/puts actually cause the display to exit low power states all the time, or something.

I tried replacing the register macros with the _FW version and that didn't help.

Well, that would just get rid of the unclaimed reg checks. Nothing more I think.

...

There's also this note in Bspec (for HSW at least):

I think this not is present on most (all?) gens.

Doesn't really prove anything.

...

"Workaround : Do not enable and unmask this interrupt if the associated pipe is disabled. Do not leave this interrupt enabled and unmasked after the associated pipe is disabled." which we took to mean that having the interrupt masked but enabled is fine.

I'm aware of this, but I think the problem is that the resources drained by the constant enable+disable+enable+disable outweigh the resources saved by turning off vblanks.

Well the CPU is awake anyway doing the get/put, so not sure why a a few extra register accesses there would have such a huge impact.

Not sure if there's an extra reason why BSpec asks us to immediately disable vblanks though...

So, to summarize, the main (only?) reason is the BSpec comment?

The point is not to wake up due to interrupts when we don't need them.

...

But maybe we'd actually have to frob IER too to avoid wasting power somehow?

With the interrupt masked on IMR, I don't think IER matters.

I'm not sure anyone actually verified that.

...

...

I also wish there was some easy way to check how this patch (or its revert) affect a bunch of different workloads...

(Also CCing Chris for insightful comments on performance)

IIRC Chris had a patch to not disable the interrupt immediately when the refcount drops to 0, but instead delay the disable until the next interrupt actually happens. But I guess it didn't go in? Probably I should have reviewed it but didn't. It sounds like a decent idea to me in any case for the active use case.

...

Thanks, Paulo

...

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 28bae6e..4b2e7af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */ }

•   /*
  

•    * 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_GEN2(dev))
  

•           dev->vblank_disable_immediate = true;
  

•   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
            dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
            dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
  

-- 1.8.5.5

---

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

-- Paulo Zanoni

-- Ville Syrjälä Intel OTC

-- Paulo Zanoni

On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote:

On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:

...

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

Add a flag to drm_device which will cause the vblank code to bypass the disable timer and always disable the vblank interrupt immediately when the last reference is dropped.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/drm_irq.c | 6 +++--- include/drm/drmP.h | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 20a4855..b008803 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)

/* Last user schedules interrupt disable */ if (atomic_dec_and_test(&vblank->refcount)) {

• if (drm_vblank_offdelay > 0)
  

• if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
  

• 	vblank_disable_fn((unsigned long)vblank);
  

• else if (drm_vblank_offdelay > 0)
  mod_timer(&vblank->disable_timer,
  	  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

• else if (drm_vblank_offdelay == 0)
  

• 	vblank_disable_fn((unsigned long)vblank);
  

  }

} EXPORT_SYMBOL(drm_vblank_put);

Would it be better if we just squashed this device flag logic back into patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never disable?" I can see there being people who might already use this when debugging new and potentially flaky hardware platforms who would be surprised by the change in behavior. So basically something like:

    if (drm_vblank_offdelay == 0)
            return
    else if (dev->vblank_disable_immediate)
            vblank_disable_fn((unsigned long)vblank);
    else
            mod_timer(...);

I'm not sure I want drm_vblank_offdelay affecting drivers that have vblank_disable_immediate set since it's a global variable. If there are two devices on the system where one has vblank_disable_immediate==true and the other doesn't, the user might want to keep vblank interrupts enabled on the crappy device all time by frobbing drm_vblank_offdelay.

I agree that changing the meaning of drm_vblank_offdelay=0 is a bit questionable, but reversing the meaning so that '==0' meas 'never disable' and '<0' means 'disable immediately' seemed a bit weird for my taste. But I suppose I could make that change if people think it's better. Or maybe just forget about the 'disable immediately' behaviour when vblank_disable_immediate==false?

I'd also suggest adding a comment in drm_stub.c to indicate that drm_vblank_offdelay gets ignored for drivers that set vblank_disable_immediate.

Good idea.

Other than that, patches 1-8, 10, and 11 are Reviewed-by: Matt Roper matthew.d.roper@intel.com

I'll finish up reviewing #9 and 12-14 tomorrow when I have some more time.

Matt

...

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 979a498..0944544 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1117,6 +1117,16 @@ struct drm_device { */ bool vblank_disable_allowed;

• /*

• * If true, vblank interrupt will be disabled immediately when the
  

• * refcount drops to zero, as opposed to via the vblank disable
  

• * timer.
  

• * This can be set to true it the hardware has a working vblank
  

• * counter and the driver uses drm_vblank_on() and drm_vblank_off()
  

• * appropriately.
  

• */
  

• bool vblank_disable_immediate;
• /* array of size num_crtcs */ struct drm_vblank_crtc *vblank;

-- 1.8.5.5

---

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

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

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

Make sure drm_vblank_get() never succeeds when called between drm_vblank_off() and drm_vblank_on(). Borrow a trick from the old drm_vblank_{pre,post}_modeset() functions and just bump the refcount in drm_vblank_off() and drop it in drm_vblank_on().

When drm_vblank_get() encounters a >0 refcount and the vblank interrupt is already disabled it will simply return -EINVAL.

Hopefully the use of inmodeset won't conflict badly with drm_vblank_{pre,post}_modeset().

For i915 there's a window between drm_vblank_off() and marking the crtc as inactive where the current code still allows drm_vblank_get().

v2: Describe what drm_vblank_get() does to explain how a simple refcount bump manages to fix things (Daniel)

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index b565372..7b1e08c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc) } spin_unlock(&dev->event_lock);

+ /* + * Prevent subsequent drm_vblank_get() from re-enabling + * the vblank interrupt by bumping the refcount. + */ + if (!dev->vblank[crtc].inmodeset) { + atomic_inc(&dev->vblank[crtc].refcount); + dev->vblank[crtc].inmodeset = 1; + } + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off); @@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc) unsigned long irqflags;

spin_lock_irqsave(&dev->vbl_lock, irqflags); + /* Drop our private "prevent drm_vblank_get" refcount */ + if (dev->vblank[crtc].inmodeset) { + atomic_dec(&dev->vblank[crtc].refcount); + dev->vblank[crtc].inmodeset = 0; + } /* re-enable interrupts if there's are users left */ if (atomic_read(&dev->vblank[crtc].refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc));

On Mon, May 26, 2014 at 02:46:31PM +0300, ville.syrjala@linux.intel.com wrote:

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

Declare a local struct drm_vblank_crtc * and use that instead of having to do dig it out via 'dev->vblank[crtc]' everywhere.

Performed with the following coccinelle incantation, and a few manual whitespace cleanups:

@@ identifier func,member; expression num_crtcs; struct drm_device *dev; unsigned int crtc; @@ func (...) {

• struct drm_vblank_crtc *vblank;

... if (crtc >= num_crtcs) return ...;

• vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

@@ struct drm_device *dev; int crtc; identifier member; expression num_crtcs; @@ for (crtc = 0; crtc < num_crtcs; crtc++) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

@@ identifier func,member; @@ func (struct drm_device *dev, int crtc, ...) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Reviewing coccinelle scripts is a bit easier than reviewing the actual patch ;-)

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Btw one cleanup on top I've onticed is that drm_update_vblank_count and vblank_disable_and_save duplicate the same do {} while (vblank_not_stable) loop. But not quite since one has a timeout. I think one more patch on top to extract that would be pretty. -Daniel

---

drivers/gpu/drm/drm_irq.c | 136 +++++++++++++++++++++++++++------------------- 1 file changed, 80 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 54a56b2..20a4855 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -73,6 +73,7 @@ */ static void drm_update_vblank_count(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 cur_vblank, diff, tslot, rc; struct timeval t_vblank;

@@ -94,12 +95,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) } while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));

/* Deal with counter wrap */

• diff = cur_vblank - dev->vblank[crtc].last;
• if (cur_vblank < dev->vblank[crtc].last) {

• diff = cur_vblank - vblank->last;

• if (cur_vblank < vblank->last) { diff += dev->max_vblank_count;

  DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",

• 	  crtc, dev->vblank[crtc].last, cur_vblank, diff);
  

• 	  crtc, vblank->last, cur_vblank, diff);
  

  }

  DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",

@@ -110,12 +111,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) * reinitialize delayed at next vblank interrupt in that case. */ if (rc) {

• tslot = atomic_read(&dev->vblank[crtc].count) + diff;
  

• tslot = atomic_read(&vblank->count) + diff;
  

  vblanktimestamp(dev, crtc, tslot) = t_vblank; }

  smp_mb__before_atomic_inc();

• atomic_add(diff, &dev->vblank[crtc].count);

• atomic_add(diff, &vblank->count); smp_mb__after_atomic_inc();

}

@@ -127,6 +128,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) */ static void vblank_disable_and_save(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags; u32 vblcount; s64 diff_ns;

@@ -147,14 +149,14 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * count account for the entire time between drm_vblank_on() and * drm_vblank_off(). */

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { drm_update_vblank_count(dev, crtc); spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return; }

  dev->driver->disable_vblank(dev, crtc);

• dev->vblank[crtc].enabled = false;

• vblank->enabled = false;

  /* No further vblank irq's will be processed after

  • this point. Get current hardware vblank count and

@@ -169,9 +171,9 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * delayed gpu counter increment. */ do {

• dev->vblank[crtc].last = dev->driver->get_vblank_counter(dev, crtc);
  

• vblank->last = dev->driver->get_vblank_counter(dev, crtc);
  

  vblrc = drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0);

• } while (dev->vblank[crtc].last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);

• } while (vblank->last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);

  if (!count) vblrc = 0;

@@ -179,7 +181,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) /* Compute time difference to stored timestamp of last vblank * as updated by last invocation of drm_handle_vblank() in vblank irq. */

• vblcount = atomic_read(&dev->vblank[crtc].count);

• vblcount = atomic_read(&vblank->count); diff_ns = timeval_to_ns(&tvblank) - timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));

@@ -196,7 +198,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * hope for the best. */ if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {

• atomic_inc(&dev->vblank[crtc].count);
  

• atomic_inc(&vblank->count);
  

  smp_mb__after_atomic_inc(); }

@@ -236,8 +238,10 @@ void drm_vblank_cleanup(struct drm_device *dev) return;

for (crtc = 0; crtc < dev->num_crtcs; crtc++) {

• del_timer_sync(&dev->vblank[crtc].disable_timer);
  

• vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
  

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
  

• del_timer_sync(&vblank->disable_timer);
  

• vblank_disable_fn((unsigned long)vblank);
  

  }

  kfree(dev->vblank);

@@ -270,11 +274,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) goto err;

for (i = 0; i < num_crtcs; i++) {

• dev->vblank[i].dev = dev;
  

• dev->vblank[i].crtc = i;
  

• init_waitqueue_head(&dev->vblank[i].queue);
  

• setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,
  

• 	    (unsigned long)&dev->vblank[i]);
  

• struct drm_vblank_crtc *vblank = &dev->vblank[i];
  

• vblank->dev = dev;
  

• vblank->crtc = i;
  

• init_waitqueue_head(&vblank->queue);
  

• setup_timer(&vblank->disable_timer, vblank_disable_fn,
  

• 	    (unsigned long)vblank);
  

  }

  DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");

@@ -426,9 +432,11 @@ int drm_irq_uninstall(struct drm_device *dev) if (dev->num_crtcs) { spin_lock_irqsave(&dev->vbl_lock, irqflags); for (i = 0; i < dev->num_crtcs; i++) {

• 	wake_up(&dev->vblank[i].queue);
  

• 	dev->vblank[i].enabled = false;
  

• 	dev->vblank[i].last =
  

• 	struct drm_vblank_crtc *vblank = &dev->vblank[i];
  

• 	wake_up(&vblank->queue);
  

• 	vblank->enabled = false;
  

• 	vblank->last =
  	dev->driver->get_vblank_counter(dev, i);
  

  } spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

@@ -796,7 +804,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp); */ u32 drm_vblank_count(struct drm_device *dev, int crtc) {

• return atomic_read(&dev->vblank[crtc].count);

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
• return atomic_read(&vblank->count);

} EXPORT_SYMBOL(drm_vblank_count);

@@ -816,6 +826,7 @@ EXPORT_SYMBOL(drm_vblank_count); u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 cur_vblank;

  /* Read timestamp from slot of _vblank_time ringbuffer

@@ -824,10 +835,10 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, * a seqlock. */ do {

• cur_vblank = atomic_read(&dev->vblank[crtc].count);
  

• cur_vblank = atomic_read(&vblank->count);
  

  *vblanktime = vblanktimestamp(dev, crtc, cur_vblank); smp_rmb();

• } while (cur_vblank != atomic_read(&dev->vblank[crtc].count));

• } while (cur_vblank != atomic_read(&vblank->count));

  return cur_vblank;

} @@ -882,13 +893,14 @@ EXPORT_SYMBOL(drm_send_vblank_event); */ static int drm_vblank_enable(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; int ret = 0;

  assert_spin_locked(&dev->vbl_lock);

  spin_lock(&dev->vblank_time_lock);

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { /*
  • Enable vblank irqs under vblank_time_lock protection.
  • All vblank count & timestamp updates are held off

@@ -899,9 +911,9 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) ret = dev->driver->enable_vblank(dev, crtc); DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); if (ret)

• 	atomic_dec(&dev->vblank[crtc].refcount);
  

• 	atomic_dec(&vblank->refcount);
  

  else {

• 	dev->vblank[crtc].enabled = true;
  

• 	vblank->enabled = true;
  drm_update_vblank_count(dev, crtc);
  

  } }

@@ -926,16 +938,17 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) */ int drm_vblank_get(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags; int ret = 0;

  spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Going from 0->1 means we have to enable interrupts again */

• if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {

• if (atomic_add_return(1, &vblank->refcount) == 1) { ret = drm_vblank_enable(dev, crtc); } else {

• if (!dev->vblank[crtc].enabled) {
  

• 	atomic_dec(&dev->vblank[crtc].refcount);
  

• if (!vblank->enabled) {
  

• 	atomic_dec(&vblank->refcount);
  ret = -EINVAL;
  

  } }

@@ -975,15 +988,17 @@ EXPORT_SYMBOL(drm_crtc_vblank_get); */ void drm_vblank_put(struct drm_device *dev, int crtc) {

• BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

• BUG_ON(atomic_read(&vblank->refcount) == 0);

  /* Last user schedules interrupt disable */

• if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {

• if (atomic_dec_and_test(&vblank->refcount)) { if (drm_vblank_offdelay > 0)

• 	mod_timer(&dev->vblank[crtc].disable_timer,
  

• 	mod_timer(&vblank->disable_timer,
  	  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

  else if (drm_vblank_offdelay == 0)

• 	vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
  

• 	vblank_disable_fn((unsigned long)vblank);
  

  }

} EXPORT_SYMBOL(drm_vblank_put); @@ -1019,6 +1034,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_put); */ void drm_vblank_off(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; struct drm_pending_vblank_event *e, *t; struct timeval now; unsigned long irqflags;

@@ -1026,7 +1042,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)

spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc);

• wake_up(&dev->vblank[crtc].queue);

• wake_up(&vblank->queue);

  /* Send any queued vblank events, lest the natives grow disquiet */ seq = drm_vblank_count_and_time(dev, crtc, &now);

@@ -1048,9 +1064,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc) * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount. */

• if (!dev->vblank[crtc].inmodeset) {

• atomic_inc(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 1;
  

• if (!vblank->inmodeset) {

• atomic_inc(&vblank->refcount);
  

• vblank->inmodeset = 1;
  

  }

  spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

@@ -1090,13 +1106,14 @@ EXPORT_SYMBOL(drm_crtc_vblank_off); */ void drm_vblank_on(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags;

  spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Drop our private "prevent drm_vblank_get" refcount */

• if (dev->vblank[crtc].inmodeset) {

• atomic_dec(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 0;
  

• if (vblank->inmodeset) {

• atomic_dec(&vblank->refcount);
  

• vblank->inmodeset = 0;
  

  }

  /*

@@ -1106,12 +1123,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc) * -1 to make sure user will never see the same * vblank counter value before and after a modeset */

• dev->vblank[crtc].last =

• vblank->last = (dev->driver->get_vblank_counter(dev, crtc) - 1) & dev->max_vblank_count;

  /* re-enable interrupts if there's are users left */

• if (atomic_read(&dev->vblank[crtc].refcount) != 0)

• if (atomic_read(&vblank->refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc)); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

} @@ -1159,6 +1176,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_on); */ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
• /* vblank is not initialized (IRQ not installed ?), or has been freed */ if (!dev->num_crtcs) return;

@@ -1169,10 +1188,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) * to avoid corrupting the count if multiple, mismatch calls occur), * so that interrupts remain enabled in the interim. */

• if (!dev->vblank[crtc].inmodeset) {

• dev->vblank[crtc].inmodeset = 0x1;
  

• if (!vblank->inmodeset) {

• vblank->inmodeset = 0x1;
  

  if (drm_vblank_get(dev, crtc) == 0)

• 	dev->vblank[crtc].inmodeset |= 0x2;
  

• 	vblank->inmodeset |= 0x2;
  

  }

} EXPORT_SYMBOL(drm_vblank_pre_modeset); @@ -1187,21 +1206,22 @@ EXPORT_SYMBOL(drm_vblank_pre_modeset); */ void drm_vblank_post_modeset(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags;

  /* vblank is not initialized (IRQ not installed ?), or has been freed */ if (!dev->num_crtcs) return;

• if (dev->vblank[crtc].inmodeset) {

• if (vblank->inmodeset) { spin_lock_irqsave(&dev->vbl_lock, irqflags); dev->vblank_disable_allowed = true; spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

• if (dev->vblank[crtc].inmodeset & 0x2)
  

• if (vblank->inmodeset & 0x2)
  drm_vblank_put(dev, crtc);
  
  

• dev->vblank[crtc].inmodeset = 0;
  

• vblank->inmodeset = 0;
  

  }

} EXPORT_SYMBOL(drm_vblank_post_modeset); @@ -1336,6 +1356,7 @@ err_put: int drm_wait_vblank(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_vblank_crtc *vblank; union drm_wait_vblank *vblwait = data; int ret; unsigned int flags, seq, crtc, high_crtc;

@@ -1365,6 +1386,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data, if (crtc >= dev->num_crtcs) return -EINVAL;

• vblank = &dev->vblank[crtc];
• ret = drm_vblank_get(dev, crtc); if (ret) { DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);

@@ -1397,11 +1420,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,

DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc);

• dev->vblank[crtc].last_wait = vblwait->request.sequence;
• DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,

• vblank->last_wait = vblwait->request.sequence;
• DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||

•      !dev->vblank[crtc].enabled ||
  

•      !vblank->enabled ||
  !dev->irq_enabled));
  
  

  if (ret != -EINTR) {

@@ -1462,6 +1485,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) */ bool drm_handle_vblank(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 vblcount; s64 diff_ns; struct timeval tvblank;

@@ -1477,7 +1501,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) spin_lock_irqsave(&dev->vblank_time_lock, irqflags);

/* Vblank irq handling disabled. Nothing to do. */

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return false; }

@@ -1487,7 +1511,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) */

/* Get current timestamp and count. */

• vblcount = atomic_read(&dev->vblank[crtc].count);

• vblcount = atomic_read(&vblank->count); drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);

  /* Compute time difference to timestamp of last vblank */

@@ -1511,14 +1535,14 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) * the timestamp computed above. */ smp_mb__before_atomic_inc();

• atomic_inc(&dev->vblank[crtc].count);
  

• atomic_inc(&vblank->count);
  

  smp_mb__after_atomic_inc(); } else { DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n", crtc, (int) diff_ns); }

• wake_up(&dev->vblank[crtc].queue);

• wake_up(&vblank->queue); drm_handle_vblank_events(dev, crtc);

  spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);

-- 1.8.5.5

---

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

On Mon, May 26, 2014 at 02:46:32PM +0300, ville.syrjala@linux.intel.com wrote:

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

The new watermaek update mechanism requires interrupts to work correctly. Because of this we need interrupts while disabling crtcs during suspend. So move the irq disable to happen a bit later.

This also avoid clobbering the vblank.last count in case the vblank interrupt was already disabled earlier by the timer. In that case drm_vblank_off() will need .last to be correct so that it can update the user visible vblank counter value approapriately.

v2: Note vblank counter in commit message

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

First round of this patch had a question from me whether we have sufficient amounts of WARN_ON(! or !!dev->irqs_enabled) sprinkled all over the place ... Do we have that?

I'm specifically thinking of places where we cancel some work/time lauchned by interrupts and must have the guarantee in place that it can't rearm.

Otoh we have places where we absolutely want interrupts to still work, e.g. in gpu_idle.

Sprinkling some modeset amounts of WARNS in a follow-up would make me really happy. -Daniel

---

drivers/gpu/drm/i915/i915_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4619c9e..21554a0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -524,7 +524,6 @@ static int i915_drm_freeze(struct drm_device *dev) return error; }

• drm_irq_uninstall(dev);
  

  dev_priv->enable_hotplug_processing = false;

  intel_disable_gt_powersave(dev);

@@ -541,6 +540,8 @@ static int i915_drm_freeze(struct drm_device *dev) } mutex_unlock(&dev->mode_config.mutex);

• drm_irq_uninstall(dev);
  

• intel_modeset_suspend_hw(dev); }

-- 1.8.5.5

---

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

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

If the user is interested in getting accurate vblank sequence numbers all the time they may disable the vblank disable timer entirely. In that case it seems appropriate to kick start the vblank interrupts already from drm_vblank_on().

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 82a039a..6376d96 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1126,9 +1126,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc) vblank->last = (dev->driver->get_vblank_counter(dev, crtc) - 1) & dev->max_vblank_count; - - /* re-enable interrupts if there's are users left */ - if (atomic_read(&vblank->refcount) != 0) + /* + * re-enable interrupts if there are users left, or the + * user wishes vblank interrupts to be enabled all the time. + */ + if (atomic_read(&vblank->refcount) != 0 || + (!dev->vblank_disable_immediate && drm_vblank_offdelay < 0)) WARN_ON(drm_vblank_enable(dev, crtc)); spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }