[PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[ville.syrjala]

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

Add drm_crtc_vblank_get_accurate() which is the same as
drm_crtc_vblank_get() except that it will forcefully update the
the current vblank counter/timestamp value even if the interrupt
is already enabled.

And we'll switch drm_wait_one_vblank() over to using it to make sure it
will really wait at least one vblank even if it races with the irq
handler.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 37 ++++++++++++++++++++++++++++++++-----
 include/drm/drm_vblank.h     |  1 +
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 823c853de052..c57383b8753b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -955,7 +955,8 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 	return ret;
 }
 
-static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
+static int drm_vblank_get(struct drm_device *dev, unsigned int pipe,
+			  bool force_update)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	unsigned long irqflags;
@@ -975,6 +976,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 		if (!vblank->enabled) {
 			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
+		} else if (force_update) {
+			spin_lock(&dev->vblank_time_lock);
+			drm_update_vblank_count(dev, pipe, false);
+			spin_unlock(&dev->vblank_time_lock);
 		}
 	}
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -994,10 +999,32 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
  */
 int drm_crtc_vblank_get(struct drm_crtc *crtc)
 {
-	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
+	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), false);
 }
 EXPORT_SYMBOL(drm_crtc_vblank_get);
 
+/**
+ * drm_crtc_vblank_get_accurate - get a reference count on vblank events and
+ *                                make sure the counter is uptodate
+ * @crtc: which CRTC to own
+ *
+ * Acquire a reference count on vblank events to avoid having them disabled
+ * while in use.
+ *
+ * Also make sure the current vblank counter is value is fully up to date
+ * even if we're already past the start of vblank but the irq hasn't fired
+ * yet, which may be the case with some hardware that raises the interrupt
+ * only some time after the start of vblank.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
+ */
+int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc)
+{
+	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), true);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_get_accurate);
+
 static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -1053,7 +1080,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return;
 
-	ret = drm_vblank_get(dev, pipe);
+	ret = drm_vblank_get(dev, pipe, true);
 	if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
 		return;
 
@@ -1248,7 +1275,7 @@ static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
 	 */
 	if (!vblank->inmodeset) {
 		vblank->inmodeset = 0x1;
-		if (drm_vblank_get(dev, pipe) == 0)
+		if (drm_vblank_get(dev, pipe, false) == 0)
 			vblank->inmodeset |= 0x2;
 	}
 }
@@ -1448,7 +1475,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 		return 0;
 	}
 
-	ret = drm_vblank_get(dev, pipe);
+	ret = drm_vblank_get(dev, pipe, false);
 	if (ret) {
 		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
 		return ret;
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 7fba9efe4951..5629e7841318 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -162,6 +162,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
 bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
 int drm_crtc_vblank_get(struct drm_crtc *crtc);
+int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc);
 void drm_crtc_vblank_put(struct drm_crtc *crtc);
 void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
 void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
-- 
2.13.0

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

[PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq

[ville.syrjala]

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

Make sure that we have an up to date vblank counter value for sending
out the completion event. On gen2/3 the vblank irq fires at frame start
rather than at start of vblank, so if we perform an update between
vblank start and frame start we would potentially sample a stale vblank
counter value, and thus send out the event one frame too soon.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..61681a11086a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -197,7 +197,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 	 * event outside of the critical section - the spinlock might spin for a
 	 * while ... */
 	if (crtc->base.state->event) {
-		WARN_ON(drm_crtc_vblank_get(&crtc->base) != 0);
+		WARN_ON(drm_crtc_vblank_get_accurate(&crtc->base) != 0);
 
 		spin_lock(&crtc->base.dev->event_lock);
 		drm_crtc_arm_vblank_event(&crtc->base, crtc->base.state->event);
-- 
2.13.0

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

[PATCH 3/3] drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks()

[ville.syrjala]

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

To make sure intel_atomic_wait_for_vblanks() really waits for at least
one vblank let's switch to using drm_crtc_vblank_get_accurate().

Also toss in a FIXME that we should really be sampling the vblank
counter when we did the plane/wm update instead of resampling it
potentially much later when we call intel_atomic_wait_for_vblanks().

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e03ca6c946f..12fc4fcf78c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12826,12 +12826,16 @@ static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
 		if (!((1 << pipe) & crtc_mask))
 			continue;
 
-		ret = drm_crtc_vblank_get(&crtc->base);
+		ret = drm_crtc_vblank_get_accurate(&crtc->base);
 		if (WARN_ON(ret != 0)) {
 			crtc_mask &= ~(1 << pipe);
 			continue;
 		}
 
+		/*
+		 * FIXME we should have sampled this
+		 * when we did the actual update.
+		 */
 		last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
 	}
 
-- 
2.13.0

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
URL   : https://patchwork.freedesktop.org/series/26633/
State : success

== Summary ==

Series 26633v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/26633/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-r) fdo#100125
Test kms_busy:
        Subgroup basic-flip-default-b:
                dmesg-warn -> FAIL       (fi-skl-6700hq) fdo#101144
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597 +1
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-skl-6700k)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-j1900) fdo#101516 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101516 https://bugs.freedesktop.org/show_bug.cgi?id=101516

fi-bdw-5557u     total:279  pass:264  dwarn:0   dfail:0   fail:3   skip:11  time:440s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:434s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:352s
fi-bsw-n3050     total:279  pass:239  dwarn:0   dfail:0   fail:3   skip:36  time:515s
fi-bxt-j4205     total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:509s
fi-byt-j1900     total:279  pass:250  dwarn:1   dfail:0   fail:3   skip:24  time:477s
fi-byt-n2820     total:279  pass:247  dwarn:0   dfail:0   fail:3   skip:28  time:474s
fi-glk-2a        total:279  pass:256  dwarn:0   dfail:0   fail:3   skip:19  time:584s
fi-hsw-4770      total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:429s
fi-hsw-4770r     total:279  pass:259  dwarn:0   dfail:0   fail:3   skip:16  time:414s
fi-ilk-650       total:279  pass:225  dwarn:0   dfail:0   fail:3   skip:50  time:414s
fi-ivb-3520m     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:495s
fi-ivb-3770      total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:463s
fi-kbl-7500u     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:457s
fi-kbl-7560u     total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:561s
fi-kbl-r         total:279  pass:256  dwarn:1   dfail:0   fail:3   skip:18  time:571s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:555s
fi-skl-6260u     total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:452s
fi-skl-6700hq    total:279  pass:219  dwarn:1   dfail:0   fail:33  skip:24  time:333s
fi-skl-6700k     total:279  pass:257  dwarn:0   dfail:0   fail:3   skip:18  time:460s
fi-skl-6770hq    total:279  pass:265  dwarn:0   dfail:0   fail:3   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:279  pass:247  dwarn:0   dfail:0   fail:3   skip:28  time:539s
fi-snb-2600      total:279  pass:246  dwarn:0   dfail:0   fail:3   skip:29  time:405s

bcea90f867d52f57e0919ae8eaad5fba3c53735e drm-tip: 2017y-06m-30d-13h-38m-04s UTC integration manifest
58679573 drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks()
47a42e9 drm/i915: Fix race between pipe update and vblank irq
139e473 drm/vblank: Introduce drm_crtc_vblank_get_accurate()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5079/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Daniel Vetter]

On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add drm_crtc_vblank_get_accurate() which is the same as
> drm_crtc_vblank_get() except that it will forcefully update the
> the current vblank counter/timestamp value even if the interrupt
> is already enabled.
> 
> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> will really wait at least one vblank even if it races with the irq
> handler.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hm, I just thought of doing an
s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
drm_crtc_arm_vblank_event.

What does this buy us above&beyond the other patch? And why isn't
vblank_get accurate always?

/me confused

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_vblank.c | 37 ++++++++++++++++++++++++++++++++-----
>  include/drm/drm_vblank.h     |  1 +
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 823c853de052..c57383b8753b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -955,7 +955,8 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  	return ret;
>  }
>  
> -static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
> +static int drm_vblank_get(struct drm_device *dev, unsigned int pipe,
> +			  bool force_update)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	unsigned long irqflags;
> @@ -975,6 +976,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>  		if (!vblank->enabled) {
>  			atomic_dec(&vblank->refcount);
>  			ret = -EINVAL;
> +		} else if (force_update) {
> +			spin_lock(&dev->vblank_time_lock);
> +			drm_update_vblank_count(dev, pipe, false);
> +			spin_unlock(&dev->vblank_time_lock);
>  		}
>  	}
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -994,10 +999,32 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>   */
>  int drm_crtc_vblank_get(struct drm_crtc *crtc)
>  {
> -	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
> +	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), false);
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_get);
>  
> +/**
> + * drm_crtc_vblank_get_accurate - get a reference count on vblank events and
> + *                                make sure the counter is uptodate
> + * @crtc: which CRTC to own
> + *
> + * Acquire a reference count on vblank events to avoid having them disabled
> + * while in use.
> + *
> + * Also make sure the current vblank counter is value is fully up to date
> + * even if we're already past the start of vblank but the irq hasn't fired
> + * yet, which may be the case with some hardware that raises the interrupt
> + * only some time after the start of vblank.
> + *
> + * Returns:
> + * Zero on success or a negative error code on failure.
> + */
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc)
> +{
> +	return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), true);
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_get_accurate);
> +
>  static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> @@ -1053,7 +1080,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>  	if (WARN_ON(pipe >= dev->num_crtcs))
>  		return;
>  
> -	ret = drm_vblank_get(dev, pipe);
> +	ret = drm_vblank_get(dev, pipe, true);
>  	if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
>  		return;
>  
> @@ -1248,7 +1275,7 @@ static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
>  	 */
>  	if (!vblank->inmodeset) {
>  		vblank->inmodeset = 0x1;
> -		if (drm_vblank_get(dev, pipe) == 0)
> +		if (drm_vblank_get(dev, pipe, false) == 0)
>  			vblank->inmodeset |= 0x2;
>  	}
>  }
> @@ -1448,7 +1475,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  		return 0;
>  	}
>  
> -	ret = drm_vblank_get(dev, pipe);
> +	ret = drm_vblank_get(dev, pipe, false);
>  	if (ret) {
>  		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
>  		return ret;
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7fba9efe4951..5629e7841318 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -162,6 +162,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc);
>  void drm_crtc_vblank_put(struct drm_crtc *crtc);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
>  void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
> -- 
> 2.13.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Daniel Vetter]

On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Add drm_crtc_vblank_get_accurate() which is the same as
>> drm_crtc_vblank_get() except that it will forcefully update the
>> the current vblank counter/timestamp value even if the interrupt
>> is already enabled.
>>
>> And we'll switch drm_wait_one_vblank() over to using it to make sure it
>> will really wait at least one vblank even if it races with the irq
>> handler.
>>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Hm, I just thought of doing an
> s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> drm_crtc_arm_vblank_event.
>
> What does this buy us above&beyond the other patch? And why isn't
> vblank_get accurate always?

This also seems to have the risk of not fixing the arm_vblank_event
race if someone puts the vblank_get_accurate outside of the critical
section. Then we're back to the same old race. And since that means
you need to have a vblank_get_accurate right before the
arm_vblank_event in all cases, we might as well just put it into the
helper. You wrote in the other thread putting it into arm_vblank_event
might be wasteful, but I don't see any scenario where that could be
the case ...

Or do I miss something?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Ville Syrjälä]

On Fri, Jun 30, 2017 at 08:18:19PM +0200, Daniel Vetter wrote:
> On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Add drm_crtc_vblank_get_accurate() which is the same as
> >> drm_crtc_vblank_get() except that it will forcefully update the
> >> the current vblank counter/timestamp value even if the interrupt
> >> is already enabled.
> >>
> >> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> >> will really wait at least one vblank even if it races with the irq
> >> handler.
> >>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Hm, I just thought of doing an
> > s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> > drm_crtc_arm_vblank_event.
> >
> > What does this buy us above&beyond the other patch? And why isn't
> > vblank_get accurate always?
> 
> This also seems to have the risk of not fixing the arm_vblank_event
> race if someone puts the vblank_get_accurate outside of the critical
> section. Then we're back to the same old race. And since that means
> you need to have a vblank_get_accurate right before the
> arm_vblank_event in all cases, we might as well just put it into the
> helper. You wrote in the other thread putting it into arm_vblank_event
> might be wasteful, but I don't see any scenario where that could be
> the case ...
> 
> Or do I miss something?

The interrupt most likely will be on already when pipe_update_end() gets
called since pipe_update_start() will enable it already, and Chris's
disable_immediate optimization will keep it on until the next interrupt.
Prior to that optimization the drm_vblank_get() in pipe_update_end()
would have had to turn on the interrupt and perform the vblank counter
update, and thus the second update from drm_crtc_accurate_vblank_count()
would have been wasteful.

I'm not sure I like relying on the fact that pipe_update_start() already
turned the interrupt on and it's going to be kept on magically. One
solution for that would be to hang on to the reference from
pipe_update_start() until we've armed the event. Then we know the
vblank_get() won't have to enable the interrupt.

However, if we start to sample the counter for some other purpose
(watermarks, fb unpinning etc.) during pipe_update_end() then we'll
again be faced with another potentially pointless update if we
decide to use drm_crtc_accurate_vblank_count() again.

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

Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()

[Daniel Vetter]

On Mon, Jul 03, 2017 at 05:26:24PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 30, 2017 at 08:18:19PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >> Add drm_crtc_vblank_get_accurate() which is the same as
> > >> drm_crtc_vblank_get() except that it will forcefully update the
> > >> the current vblank counter/timestamp value even if the interrupt
> > >> is already enabled.
> > >>
> > >> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> > >> will really wait at least one vblank even if it races with the irq
> > >> handler.
> > >>
> > >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Hm, I just thought of doing an
> > > s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> > > drm_crtc_arm_vblank_event.
> > >
> > > What does this buy us above&beyond the other patch? And why isn't
> > > vblank_get accurate always?
> > 
> > This also seems to have the risk of not fixing the arm_vblank_event
> > race if someone puts the vblank_get_accurate outside of the critical
> > section. Then we're back to the same old race. And since that means
> > you need to have a vblank_get_accurate right before the
> > arm_vblank_event in all cases, we might as well just put it into the
> > helper. You wrote in the other thread putting it into arm_vblank_event
> > might be wasteful, but I don't see any scenario where that could be
> > the case ...
> > 
> > Or do I miss something?
> 
> The interrupt most likely will be on already when pipe_update_end() gets
> called since pipe_update_start() will enable it already, and Chris's
> disable_immediate optimization will keep it on until the next interrupt.
> Prior to that optimization the drm_vblank_get() in pipe_update_end()
> would have had to turn on the interrupt and perform the vblank counter
> update, and thus the second update from drm_crtc_accurate_vblank_count()
> would have been wasteful.
> 
> I'm not sure I like relying on the fact that pipe_update_start() already
> turned the interrupt on and it's going to be kept on magically. One
> solution for that would be to hang on to the reference from
> pipe_update_start() until we've armed the event. Then we know the
> vblank_get() won't have to enable the interrupt.

Yeah, relying on some implict vblank_get happening in the right way, far
away from the arm_vblank_event, is the part I don't like. Makes review
harder, and I don't see the gain we'll get in return.

> However, if we start to sample the counter for some other purpose
> (watermarks, fb unpinning etc.) during pipe_update_end() then we'll
> again be faced with another potentially pointless update if we
> decide to use drm_crtc_accurate_vblank_count() again.

I think at that point we should write our own arm_vblank_event and cache
the accurate vblank ts within i915 code. My point is that for a generic
helper, we should aim for safe over fastest option. Having a special
vblank_get that you have to call before you call arm_vblank_event, and
within the critical section, just adds to the list of caveats of that
function. Simply making arm_vblank_event more robust seems like the much
better plan (and we can always fix things in i915 if the overhead is too
much).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel