[PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Dhinakaran Pandiyan]

drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
space requested vblank sequence using this clipped 32-bit count(when the
value is >= 2^32) as reference, the requested sequence remains a 32-bit
value and gets queued like that. However, the code that checks if the
requested sequence has passed compares this against the 64-bit vblank
count.

Cc: Keith Packard <keithp@keithp.com>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..768a8e44d99b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
 }
 
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
+static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 
-- 
2.11.0

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

[PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()

[Dhinakaran Pandiyan]

Now that drm_vblank_count() returns all bits of the vblank count, update
drm_crtc_arm_vblank_event() so that it queues the correct sequence.
Otherwise, this leads to prolonged waits for a vblank sequence when the
current count is >=2^32.

Cc: Keith Packard <keithp@keithp.com>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 4 ++--
 include/drm/drm_vblank.h     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 768a8e44d99b..f2bf1f5dbaa5 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -292,11 +292,11 @@ static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
  * This is mostly useful for hardware that can obtain the scanout position, but
  * doesn't have a hardware frame counter.
  */
-u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
+u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	unsigned int pipe = drm_crtc_index(crtc);
-	u32 vblank;
+	u64 vblank;
 	unsigned long flags;
 
 	WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 848b463a0af5..a4c3b0a0a197 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -179,7 +179,7 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
 void drm_crtc_vblank_off(struct drm_crtc *crtc);
 void drm_crtc_vblank_reset(struct drm_crtc *crtc);
 void drm_crtc_vblank_on(struct drm_crtc *crtc);
-u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
+u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,
-- 
2.11.0

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

Re: [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:04PM +0000, Dhinakaran Pandiyan wrote:
> Now that drm_vblank_count() returns all bits of the vblank count, update
> drm_crtc_arm_vblank_event() so that it queues the correct sequence.
> Otherwise, this leads to prolonged waits for a vblank sequence when the
> current count is >=2^32.

This could be probably squashed to the previous patch...
Specially if you apply the Fixes tag.

Anyways, in case you decide to go with 2 patches:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 4 ++--
>  include/drm/drm_vblank.h     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 768a8e44d99b..f2bf1f5dbaa5 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -292,11 +292,11 @@ static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
>   * This is mostly useful for hardware that can obtain the scanout position, but
>   * doesn't have a hardware frame counter.
>   */
> -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
> +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = drm_crtc_index(crtc);
> -	u32 vblank;
> +	u64 vblank;
>  	unsigned long flags;
>  
>  	WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 848b463a0af5..a4c3b0a0a197 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -179,7 +179,7 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
>  void drm_crtc_vblank_off(struct drm_crtc *crtc);
>  void drm_crtc_vblank_reset(struct drm_crtc *crtc);
>  void drm_crtc_vblank_on(struct drm_crtc *crtc);
> -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>  
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe, int *max_error,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:

> Now that drm_vblank_count() returns all bits of the vblank count, update
> drm_crtc_arm_vblank_event() so that it queues the correct sequence.
> Otherwise, this leads to prolonged waits for a vblank sequence when the
> current count is >=2^32.

The summary for this patch is incorrect; the function being fixed is
drm_crtc_accurate_vblank_event.

And, I'm afraid you've uncovered a rabbit hole here -- there are a
couple of users of this function outside of the core, including i915 in
a couple of places and nouveau. We should at least review the whole call
graph starting here and make sure it does what we think it should.

Thanks for finding these bugs!

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled.

[Dhinakaran Pandiyan]

Updating vblank counts requires register reads and these reads may not
return meaningful values if the device was in a low power state after
vblank interrupts were last disabled. So, update the count only if vblank
interrupts are enabled. Secondly, this means the registers should be read
before disabling vblank interrupts.

v2: Don't check vblank->enabled outside it's lock (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Michel Dänzer <michel@daenzer.net>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f2bf1f5dbaa5..2559d2d7b907 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -347,23 +347,25 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
 
 	/*
-	 * Only disable vblank interrupts if they're enabled. This avoids
-	 * calling the ->disable_vblank() operation in atomic context with the
-	 * hardware potentially runtime suspended.
+	 * Update vblank count and disable vblank interrupts only if the
+	 * interrupts were enabled. This avoids calling the ->disable_vblank()
+	 * operation in atomic context with the hardware potentially runtime
+	 * suspended.
 	 */
-	if (vblank->enabled) {
-		__disable_vblank(dev, pipe);
-		vblank->enabled = false;
-	}
+	if (!vblank->enabled)
+		goto out;
 
 	/*
-	 * Always update the count and timestamp to maintain the
+	 * 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_crtc_vblank_on() and drm_crtc_vblank_off().
 	 */
 	drm_update_vblank_count(dev, pipe, false);
+	__disable_vblank(dev, pipe);
+	vblank->enabled = false;
 
+out:
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 }
 
-- 
2.11.0

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

Re: [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled.

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:05PM +0000, Dhinakaran Pandiyan wrote:
> Updating vblank counts requires register reads and these reads may not
> return meaningful values if the device was in a low power state after
> vblank interrupts were last disabled. So, update the count only if vblank
> interrupts are enabled. Secondly, this means the registers should be read
> before disabling vblank interrupts.
> 
> v2: Don't check vblank->enabled outside it's lock (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Michel Dänzer <michel@daenzer.net>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f2bf1f5dbaa5..2559d2d7b907 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -347,23 +347,25 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
>  	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
>  
>  	/*
> -	 * Only disable vblank interrupts if they're enabled. This avoids
> -	 * calling the ->disable_vblank() operation in atomic context with the
> -	 * hardware potentially runtime suspended.
> +	 * Update vblank count and disable vblank interrupts only if the
> +	 * interrupts were enabled. This avoids calling the ->disable_vblank()
> +	 * operation in atomic context with the hardware potentially runtime
> +	 * suspended.
>  	 */
> -	if (vblank->enabled) {
> -		__disable_vblank(dev, pipe);
> -		vblank->enabled = false;
> -	}
> +	if (!vblank->enabled)
> +		goto out;
>  
>  	/*
> -	 * Always update the count and timestamp to maintain the
> +	 * 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_crtc_vblank_on() and drm_crtc_vblank_off().
>  	 */

I feel that this entire comment can be simply removed now...
The approach looks good and right to me so you can use

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

but please ping Ville to take a look here since he introduced this approach with
4dfd64862ff8 ("drm: Use vblank timestamps to guesstimate how many vblanks were missed")

>  	drm_update_vblank_count(dev, pipe, false);
> +	__disable_vblank(dev, pipe);
> +	vblank->enabled = false;
>  
> +out:
>  	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  }
>  
> -- 
> 2.11.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.

[Dhinakaran Pandiyan]

The HW frame counter can get reset if device enters a low power state after
vblank interrupts were disabled. This messes up any following vblank count
update as a negative diff (huge unsigned diff) is calculated from the HW
frame counter change. We cannot ignore negative diffs altogther as there
could be legitimate wrap arounds. So, allow drivers to update vblank->count
with missed vblanks for the time interrupts were disabled. This is similar
to _crtc_vblank_on() except that vblanks interrupts are not enabled at the
end as this function is expected to be called from the driver
_enable_vblank() vfunc.

v2: drm_crtc_vblank_restore should take crtc as arg. (Chris)
    Add docs and sprinkle some asserts.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Dänzer <michel@daenzer.net>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_vblank.h     |  2 ++
 2 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 2559d2d7b907..2690966694f0 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_on);
 
+/**
+ * drm_vblank_restore - estimated vblanks using timestamps and update it.
+ *
+ * Power manamement features can cause frame counter resets between vblank
+ * disable and enable. Drivers can then use this function in their
+ * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
+ * the last &drm_crtc_funcs.disable_vblank.
+ *
+ * This function is the legacy version of drm_crtc_vblank_restore().
+ */
+void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
+{
+	ktime_t t_vblank;
+	struct drm_vblank_crtc *vblank;
+	int framedur_ns;
+	u64 diff_ns;
+	u32 cur_vblank, diff = 1;
+	int count = DRM_TIMESTAMP_MAXRETRIES;
+
+	if (WARN_ON(pipe >= dev->num_crtcs))
+		return;
+
+	assert_spin_locked(&dev->vbl_lock);
+	assert_spin_locked(&dev->vblank_time_lock);
+
+	vblank = &dev->vblank[pipe];
+	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
+		  "Cannot compute missed vblanks without frame duration\n");
+	framedur_ns = vblank->framedur_ns;
+
+	do {
+		cur_vblank = __get_vblank_counter(dev, pipe);
+		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
+	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+
+	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
+	if (framedur_ns)
+		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
+
+
+	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
+		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
+	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
+}
+EXPORT_SYMBOL(drm_vblank_restore);
+
+/**
+ * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
+ * Power manamement features can cause frame counter resets between vblank
+ * disable and enable. Drivers can then use this function in their
+ * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
+ * the last &drm_crtc_funcs.disable_vblank.
+ */
+void drm_crtc_vblank_restore(struct drm_crtc *crtc)
+{
+	drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_restore);
+
 static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
 					  unsigned int pipe)
 {
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index a4c3b0a0a197..16d46e2a6854 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
 void drm_crtc_vblank_reset(struct drm_crtc *crtc);
 void drm_crtc_vblank_on(struct drm_crtc *crtc);
 u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
+void drm_vblank_restore(struct drm_device *dev, unsigned int pipe);
+void drm_crtc_vblank_restore(struct drm_crtc *crtc);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,
-- 
2.11.0

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

Re: [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
> The HW frame counter can get reset if device enters a low power state after
> vblank interrupts were disabled. This messes up any following vblank count
> update as a negative diff (huge unsigned diff) is calculated from the HW
> frame counter change. We cannot ignore negative diffs altogther as there
> could be legitimate wrap arounds. So, allow drivers to update vblank->count
> with missed vblanks for the time interrupts were disabled. This is similar
> to _crtc_vblank_on() except that vblanks interrupts are not enabled at the
> end as this function is expected to be called from the driver
> _enable_vblank() vfunc.
> 
> v2: drm_crtc_vblank_restore should take crtc as arg. (Chris)
>     Add docs and sprinkle some asserts.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Dänzer <michel@daenzer.net>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_vblank.h     |  2 ++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 2559d2d7b907..2690966694f0 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_on);
>  
> +/**
> + * drm_vblank_restore - estimated vblanks using timestamps and update it.
> + *
> + * Power manamement features can cause frame counter resets between vblank
> + * disable and enable. Drivers can then use this function in their
> + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> + * the last &drm_crtc_funcs.disable_vblank.
> + *
> + * This function is the legacy version of drm_crtc_vblank_restore().
> + */
> +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
> +{
> +	ktime_t t_vblank;
> +	struct drm_vblank_crtc *vblank;
> +	int framedur_ns;
> +	u64 diff_ns;
> +	u32 cur_vblank, diff = 1;
> +	int count = DRM_TIMESTAMP_MAXRETRIES;
> +
> +	if (WARN_ON(pipe >= dev->num_crtcs))
> +		return;
> +
> +	assert_spin_locked(&dev->vbl_lock);
> +	assert_spin_locked(&dev->vblank_time_lock);
> +
> +	vblank = &dev->vblank[pipe];
> +	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,

do we really only need this warn on debug vlb?

> +		  "Cannot compute missed vblanks without frame duration\n");

The message seems hard... if we *cannot* why do we move fwd?

> +	framedur_ns = vblank->framedur_ns;
> +
> +	do {
> +		cur_vblank = __get_vblank_counter(dev, pipe);
> +		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);

Based on the commend of drm_update_vblank_count I don't feel that we have to
do the loop here... And if we have maybe we should re-org things to avoid
duplication?

> +
> +	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> +	if (framedur_ns)
> +		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
> +
> +
> +	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
> +		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
> +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);

hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe));
wouldn't work here...

> +}
> +EXPORT_SYMBOL(drm_vblank_restore);
> +
> +/**
> + * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
> + * Power manamement features can cause frame counter resets between vblank
> + * disable and enable. Drivers can then use this function in their
> + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> + * the last &drm_crtc_funcs.disable_vblank.
> + */
> +void drm_crtc_vblank_restore(struct drm_crtc *crtc)
> +{
> +	drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_restore);
> +
>  static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
>  					  unsigned int pipe)
>  {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index a4c3b0a0a197..16d46e2a6854 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
>  void drm_crtc_vblank_reset(struct drm_crtc *crtc);
>  void drm_crtc_vblank_on(struct drm_crtc *crtc);
>  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe);
> +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
>  
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe, int *max_error,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.

[Pandiyan, Dhinakaran]

On Fri, 2018-01-19 at 00:01 -0800, Rodrigo Vivi wrote:
> On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
> > The HW frame counter can get reset if device enters a low power state after
> > vblank interrupts were disabled. This messes up any following vblank count
> > update as a negative diff (huge unsigned diff) is calculated from the HW
> > frame counter change. We cannot ignore negative diffs altogther as there
> > could be legitimate wrap arounds. So, allow drivers to update vblank->count
> > with missed vblanks for the time interrupts were disabled. This is similar
> > to _crtc_vblank_on() except that vblanks interrupts are not enabled at the
> > end as this function is expected to be called from the driver
> > _enable_vblank() vfunc.
> > 
> > v2: drm_crtc_vblank_restore should take crtc as arg. (Chris)
> >     Add docs and sprinkle some asserts.
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_vblank.h     |  2 ++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 2559d2d7b907..2690966694f0 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
> >  }
> >  EXPORT_SYMBOL(drm_crtc_vblank_on);
> >  
> > +/**
> > + * drm_vblank_restore - estimated vblanks using timestamps and update it.
> > + *
> > + * Power manamement features can cause frame counter resets between vblank
> > + * disable and enable. Drivers can then use this function in their
> > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > + * the last &drm_crtc_funcs.disable_vblank.
> > + *
> > + * This function is the legacy version of drm_crtc_vblank_restore().
> > + */
> > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
> > +{
> > +	ktime_t t_vblank;
> > +	struct drm_vblank_crtc *vblank;
> > +	int framedur_ns;
> > +	u64 diff_ns;
> > +	u32 cur_vblank, diff = 1;
> > +	int count = DRM_TIMESTAMP_MAXRETRIES;
> > +
> > +	if (WARN_ON(pipe >= dev->num_crtcs))
> > +		return;
> > +
> > +	assert_spin_locked(&dev->vbl_lock);
> > +	assert_spin_locked(&dev->vblank_time_lock);
> > +
> > +	vblank = &dev->vblank[pipe];
> > +	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
> 
> do we really only need this warn on debug vlb?
> 
> > +		  "Cannot compute missed vblanks without frame duration\n");
> 
> The message seems hard... if we *cannot* why do we move fwd?

We assume the diff is 1 and make an update, some kind of a default
similar to what is implemented in drm_update_vblank_count()

> 
> > +	framedur_ns = vblank->framedur_ns;
> > +
> > +	do {
> > +		cur_vblank = __get_vblank_counter(dev, pipe);
> > +		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> > +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> 
> Based on the commend of drm_update_vblank_count I don't feel that we have to


This one? -
"* We repeat the hardware vblank counter & timestamp query until
 * we get consistent results. This to prevent races between gpu
 * updating its hardware counter while we are retrieving the
 * corresponding vblank timestamp." 


I added the loop based on that comment. If the register if partially
updated, we want to discard that and loop until it is stable.


> do the loop here... And if we have maybe we should re-org things to avoid
> duplication?
> 

I considered that, we need to pass at least four args for three lines of
code. Felt it was too small to warrant a separate function.


> > +
> > +	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > +	if (framedur_ns)
> > +		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
> > +
> > +
> > +	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
> > +		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
> > +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> 
> hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe));
> wouldn't work here...

We have to store a stable count.

> 
> > +}
> > +EXPORT_SYMBOL(drm_vblank_restore);
> > +
> > +/**
> > + * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
> > + * Power manamement features can cause frame counter resets between vblank
> > + * disable and enable. Drivers can then use this function in their
> > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > + * the last &drm_crtc_funcs.disable_vblank.
> > + */
> > +void drm_crtc_vblank_restore(struct drm_crtc *crtc)
> > +{
> > +	drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_restore);
> > +
> >  static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
> >  					  unsigned int pipe)
> >  {
> > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > index a4c3b0a0a197..16d46e2a6854 100644
> > --- a/include/drm/drm_vblank.h
> > +++ b/include/drm/drm_vblank.h
> > @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
> >  void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> >  void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe);
> > +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
> >  
> >  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> >  					   unsigned int pipe, int *max_error,
> > -- 
> > 2.11.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.

[Rodrigo Vivi]

On Fri, Jan 19, 2018 at 10:02:12PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Fri, 2018-01-19 at 00:01 -0800, Rodrigo Vivi wrote:
> > On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
> > > The HW frame counter can get reset if device enters a low power state after
> > > vblank interrupts were disabled. This messes up any following vblank count
> > > update as a negative diff (huge unsigned diff) is calculated from the HW
> > > frame counter change. We cannot ignore negative diffs altogther as there
> > > could be legitimate wrap arounds. So, allow drivers to update vblank->count
> > > with missed vblanks for the time interrupts were disabled. This is similar
> > > to _crtc_vblank_on() except that vblanks interrupts are not enabled at the
> > > end as this function is expected to be called from the driver
> > > _enable_vblank() vfunc.
> > > 
> > > v2: drm_crtc_vblank_restore should take crtc as arg. (Chris)
> > >     Add docs and sprinkle some asserts.
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michel Dänzer <michel@daenzer.net>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_vblank.h     |  2 ++
> > >  2 files changed, 61 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 2559d2d7b907..2690966694f0 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
> > >  }
> > >  EXPORT_SYMBOL(drm_crtc_vblank_on);
> > >  
> > > +/**
> > > + * drm_vblank_restore - estimated vblanks using timestamps and update it.
> > > + *
> > > + * Power manamement features can cause frame counter resets between vblank
> > > + * disable and enable. Drivers can then use this function in their
> > > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > > + * the last &drm_crtc_funcs.disable_vblank.
> > > + *
> > > + * This function is the legacy version of drm_crtc_vblank_restore().
> > > + */
> > > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
> > > +{
> > > +	ktime_t t_vblank;
> > > +	struct drm_vblank_crtc *vblank;
> > > +	int framedur_ns;
> > > +	u64 diff_ns;
> > > +	u32 cur_vblank, diff = 1;
> > > +	int count = DRM_TIMESTAMP_MAXRETRIES;
> > > +
> > > +	if (WARN_ON(pipe >= dev->num_crtcs))
> > > +		return;
> > > +
> > > +	assert_spin_locked(&dev->vbl_lock);
> > > +	assert_spin_locked(&dev->vblank_time_lock);
> > > +
> > > +	vblank = &dev->vblank[pipe];
> > > +	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
> > 
> > do we really only need this warn on debug vlb?
> > 
> > > +		  "Cannot compute missed vblanks without frame duration\n");
> > 
> > The message seems hard... if we *cannot* why do we move fwd?
> 
> We assume the diff is 1 and make an update, some kind of a default
> similar to what is implemented in drm_update_vblank_count()
> 
> > 
> > > +	framedur_ns = vblank->framedur_ns;
> > > +
> > > +	do {
> > > +		cur_vblank = __get_vblank_counter(dev, pipe);
> > > +		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> > > +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> > 
> > Based on the commend of drm_update_vblank_count I don't feel that we have to
> 
> 
> This one? -
> "* We repeat the hardware vblank counter & timestamp query until
>  * we get consistent results. This to prevent races between gpu
>  * updating its hardware counter while we are retrieving the
>  * corresponding vblank timestamp." 
> 
> 
> I added the loop based on that comment. If the register if partially
> updated, we want to discard that and loop until it is stable.
> 
> 
> > do the loop here... And if we have maybe we should re-org things to avoid
> > duplication?
> > 
> 
> I considered that, we need to pass at least four args for three lines of
> code. Felt it was too small to warrant a separate function.
> 
> 
> > > +
> > > +	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > > +	if (framedur_ns)
> > > +		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
> > > +
> > > +
> > > +	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
> > > +		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
> > > +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> > 
> > hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe));
> > wouldn't work here...
> 
> We have to store a stable count.

I don't see why our counter wouldn't be stable at that point?

Our register is zero or the proper counter. Our frm_counter reg
doesn't stop when vblank interrupts are disabled afaik...

But well, I see your point of making something more robust, using
the well known and tested methods and preparing for the case
where someone start using this call in other platforms as well....
So, whatever you decide..

If you go wit this:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > +}
> > > +EXPORT_SYMBOL(drm_vblank_restore);
> > > +
> > > +/**
> > > + * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
> > > + * Power manamement features can cause frame counter resets between vblank
> > > + * disable and enable. Drivers can then use this function in their
> > > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > > + * the last &drm_crtc_funcs.disable_vblank.
> > > + */
> > > +void drm_crtc_vblank_restore(struct drm_crtc *crtc)
> > > +{
> > > +	drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
> > > +}
> > > +EXPORT_SYMBOL(drm_crtc_vblank_restore);
> > > +
> > >  static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
> > >  					  unsigned int pipe)
> > >  {
> > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > index a4c3b0a0a197..16d46e2a6854 100644
> > > --- a/include/drm/drm_vblank.h
> > > +++ b/include/drm/drm_vblank.h
> > > @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
> > >  void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> > >  void drm_crtc_vblank_on(struct drm_crtc *crtc);
> > >  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> > > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe);
> > > +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
> > >  
> > >  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> > >  					   unsigned int pipe, int *max_error,
> > > -- 
> > > 2.11.0
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Dhinakaran Pandiyan]

The frame counter may have got reset between disabling and enabling vblank
interrupts due to DMC putting the hardware to DC5/6 state if PSR was
active. The frame counter also could have stalled if PSR is active in cases
where there is no DMC. The frame counter resetting as a user visible impact
of screen freezes. Use drm_vblank_restore() to compute missed vblanks
in the duration for which vblank interrupts are disabled. There's no need
particularly check if PSR was active in the interrupt disabled duration.

Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it
from going back again as long as the there are pending interrupts. So, we
don't have to explicity disallow DC5/6 after enabling vblank interrupts
to keep the counter running.

Let's not apply this to CHV for now, as enabling interrupts does not
prevent the hardware from activating PSR and thereby stalling the counter.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3517c6548e2c..db3466ec6faa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	ilk_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
+	if (HAS_PSR(dev_priv))
+		drm_vblank_restore(dev, pipe);
+
 	return 0;
 }
 
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
+	if (HAS_PSR(dev_priv))
+		drm_vblank_restore(dev, pipe);
+
 	return 0;
 }
 
-- 
2.11.0

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

Re: [PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Daniel Vetter]

On Fri, Jan 12, 2018 at 01:57:07PM -0800, Dhinakaran Pandiyan wrote:
> The frame counter may have got reset between disabling and enabling vblank
> interrupts due to DMC putting the hardware to DC5/6 state if PSR was
> active. The frame counter also could have stalled if PSR is active in cases
> where there is no DMC. The frame counter resetting as a user visible impact
> of screen freezes. Use drm_vblank_restore() to compute missed vblanks
> in the duration for which vblank interrupts are disabled. There's no need
> particularly check if PSR was active in the interrupt disabled duration.
> 
> Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it
> from going back again as long as the there are pending interrupts. So, we
> don't have to explicity disallow DC5/6 after enabling vblank interrupts
> to keep the counter running.
> 
> Let's not apply this to CHV for now, as enabling interrupts does not
> prevent the hardware from activating PSR and thereby stalling the counter.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Read this series, and I think compared to all the previous attempts to fix
this issue this one here seems the cleanest. Not an expert on the vblank
code (Michel or Ville are best for that), but on the series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3517c6548e2c..db3466ec6faa 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	ilk_enable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> +	if (HAS_PSR(dev_priv))
> +		drm_vblank_restore(dev, pipe);
> +
>  	return 0;
>  }
>  
> @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> +	if (HAS_PSR(dev_priv))
> +		drm_vblank_restore(dev, pipe);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
> The frame counter may have got reset between disabling and enabling vblank
> interrupts due to DMC putting the hardware to DC5/6 state if PSR was
> active. The frame counter also could have stalled if PSR is active in cases
> where there is no DMC. The frame counter resetting as a user visible impact
> of screen freezes. Use drm_vblank_restore() to compute missed vblanks
> in the duration for which vblank interrupts are disabled. There's no need
> particularly check if PSR was active in the interrupt disabled duration.
> 
> Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it
> from going back again as long as the there are pending interrupts. So, we
> don't have to explicity disallow DC5/6 after enabling vblank interrupts
> to keep the counter running.
> 
> Let's not apply this to CHV for now, as enabling interrupts does not
> prevent the hardware from activating PSR and thereby stalling the counter.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3517c6548e2c..db3466ec6faa 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	ilk_enable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> +	if (HAS_PSR(dev_priv))
> +		drm_vblank_restore(dev, pipe);
> +

I don't believe we need this one here.

pre-gen9 platform has psr but not dmc, so the case
where we need to restore the counter doesn't exist.

>  	return 0;
>  }
>  
> @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> +	if (HAS_PSR(dev_priv))

HAS_PSR(dev_priv) && HAS_CSR(dev_priv)
maybe?
So it gets clear that it is not because PSR that we need to restore
the counter, but also don't do it when not needed.

> +		drm_vblank_restore(dev, pipe);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Pandiyan, Dhinakaran]

On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote:
> On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
> > The frame counter may have got reset between disabling and enabling vblank
> > interrupts due to DMC putting the hardware to DC5/6 state if PSR was
> > active. The frame counter also could have stalled if PSR is active in cases
> > where there is no DMC. The frame counter resetting as a user visible impact
> > of screen freezes. Use drm_vblank_restore() to compute missed vblanks
> > in the duration for which vblank interrupts are disabled. There's no need
> > particularly check if PSR was active in the interrupt disabled duration.
> > 
> > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it
> > from going back again as long as the there are pending interrupts. So, we
> > don't have to explicity disallow DC5/6 after enabling vblank interrupts
> > to keep the counter running.
> > 
> > Let's not apply this to CHV for now, as enabling interrupts does not
> > prevent the hardware from activating PSR and thereby stalling the counter.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 3517c6548e2c..db3466ec6faa 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >  	ilk_enable_display_irq(dev_priv, bit);
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> > +	if (HAS_PSR(dev_priv))
> > +		drm_vblank_restore(dev, pipe);
> > +
> 
> I don't believe we need this one here.
> 
> pre-gen9 platform has psr but not dmc, so the case
> where we need to restore the counter doesn't exist.

Even without DMC, counter should be stuck when PSR is active as no
frames are generated by the pipe. I am using drm_vblank_restore_count()
to take care of that.

> 
> >  	return 0;
> >  }
> >  
> > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
> >  	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> > +	if (HAS_PSR(dev_priv))
> 
> HAS_PSR(dev_priv) && HAS_CSR(dev_priv)
> maybe?
> So it gets clear that it is not because PSR that we need to restore
> the counter, but also don't do it when not needed.

Same reason as above, let me test this again by disabling DMC.

> 
> > +		drm_vblank_restore(dev, pipe);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.11.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Rodrigo Vivi]

On Fri, Jan 19, 2018 at 09:42:14PM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote:
> > On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
> > > The frame counter may have got reset between disabling and enabling vblank
> > > interrupts due to DMC putting the hardware to DC5/6 state if PSR was
> > > active. The frame counter also could have stalled if PSR is active in cases
> > > where there is no DMC. The frame counter resetting as a user visible impact
> > > of screen freezes. Use drm_vblank_restore() to compute missed vblanks
> > > in the duration for which vblank interrupts are disabled. There's no need
> > > particularly check if PSR was active in the interrupt disabled duration.
> > > 
> > > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it
> > > from going back again as long as the there are pending interrupts. So, we
> > > don't have to explicity disallow DC5/6 after enabling vblank interrupts
> > > to keep the counter running.
> > > 
> > > Let's not apply this to CHV for now, as enabling interrupts does not
> > > prevent the hardware from activating PSR and thereby stalling the counter.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 3517c6548e2c..db3466ec6faa 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > >  	ilk_enable_display_irq(dev_priv, bit);
> > >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > >  
> > > +	if (HAS_PSR(dev_priv))
> > > +		drm_vblank_restore(dev, pipe);
> > > +
> > 
> > I don't believe we need this one here.
> > 
> > pre-gen9 platform has psr but not dmc, so the case
> > where we need to restore the counter doesn't exist.
> 
> Even without DMC, counter should be stuck when PSR is active as no
> frames are generated by the pipe. I am using drm_vblank_restore_count()
> to take care of that.

Oh oh! Indeed. Now I remember you had told me this here.
Can you please add a comment with this info somewhere so I don't ask
the same question again ;)

anyways:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



> 
> > 
> > >  	return 0;
> > >  }
> > >  
> > > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
> > >  	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
> > >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > >  
> > > +	if (HAS_PSR(dev_priv))
> > 
> > HAS_PSR(dev_priv) && HAS_CSR(dev_priv)
> > maybe?
> > So it gets clear that it is not because PSR that we need to restore
> > the counter, but also don't do it when not needed.
> 
> Same reason as above, let me test this again by disabling DMC.
> 
> > 
> > > +		drm_vblank_restore(dev, pipe);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.11.0
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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/5] drm/vblank: Fix return type for drm_vblank_count()

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
URL   : https://patchwork.freedesktop.org/series/36435/
State : success

== Summary ==

Series 36435v1 series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
https://patchwork.freedesktop.org/api/1.0/series/36435/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (fi-skl-6700k2) fdo#104108

fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:424s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:428s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:370s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:487s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:467s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:461s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:273s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:392s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:416s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:467s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:498s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:503s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:578s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:509s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:529s
fi-skl-6700k2    total:108  pass:96   dwarn:0   dfail:0   fail:0   skip:11 
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:538s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:395s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:573s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:472s

fcf7fdfd2db6cfe8e529c79c5822f555a8b38fd4 drm-tip: 2018y-01m-12d-17h-40m-49s UTC integration manifest
ef67bfb35c68 drm/i915: Estimate and update missed vblanks.
b1edcc47812a drm/vblank: Restoring vblank counts after device PM events.
365390c83462 drm/vblank: Do not update vblank count if interrupts are already disabled.
18e5209cfde5 drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()
60165b22a0be drm/vblank: Fix return type for drm_vblank_count()

== Logs ==

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

✗ Fi.CI.IGT: failure for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
URL   : https://patchwork.freedesktop.org/series/36435/
State : failure

== Summary ==

Test gem_caching:
        Subgroup writes:
                pass       -> INCOMPLETE (shard-snb)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-pri-indfb-multidraw:
                pass       -> FAIL       (shard-snb) fdo#103167
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623 +1
Test kms_flip:
        Subgroup vblank-vs-suspend-interruptible:
                incomplete -> PASS       (shard-hsw) fdo#100368 +1
        Subgroup vblank-vs-suspend:
                pass       -> SKIP       (shard-snb) fdo#102365

fdo#103167 
fdo#101623 
fdo#100368 
fdo#102365 

shard-hsw        total:2713 pass:1537 dwarn:1   dfail:0   fail:10  skip:1165 time:9012s
shard-snb        total:2683 pass:1294 dwarn:1   dfail:0   fail:12  skip:1375 time:7610s
Blacklisted hosts:
shard-apl        total:2713 pass:1687 dwarn:1   dfail:0   fail:24  skip:1001 time:13691s
shard-kbl        total:2708 pass:1804 dwarn:5   dfail:0   fail:24  skip:874 time:10328s

== Logs ==

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

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Daniel Vetter]

On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote:
> drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> space requested vblank sequence using this clipped 32-bit count(when the
> value is >= 2^32) as reference, the requested sequence remains a 32-bit
> value and gets queued like that. However, the code that checks if the
> requested sequence has passed compares this against the 64-bit vblank
> count.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Sounds like the 64bit widening wasn't all that well tested ... do we have
an igt for this? Iirc the base igt was merged already.
-Daniel

> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 32d9bcf5be7f..768a8e44d99b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
>  }
>  
> -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> -- 
> 2.11.0
> 

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

Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

On Mon, 2018-01-15 at 10:38 +0100, Daniel Vetter wrote:
> On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote:
> > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> > space requested vblank sequence using this clipped 32-bit count(when the
> > value is >= 2^32) as reference, the requested sequence remains a 32-bit
> > value and gets queued like that. However, the code that checks if the
> > requested sequence has passed compares this against the 64-bit vblank
> > count.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Sounds like the 64bit widening wasn't all that well tested ... do we have
> an igt for this? Iirc the base igt was merged already.

I don't see anything that would particularly trigger this condition
i.e., vblank->count > 2^32 in the IGTs. We'll need to implement
something to force set a very large vblank->count and then request a
vblank sequence.




> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 32d9bcf5be7f..768a8e44d99b 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> >  }
> >  
> > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> >  {
> >  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> >  
> > -- 
> > 2.11.0
> > 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

ping for review.

Let me know if there's anything that needs to be done, thanks!


On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote:
> drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> space requested vblank sequence using this clipped 32-bit count(when the
> value is >= 2^32) as reference, the requested sequence remains a 32-bit
> value and gets queued like that. However, the code that checks if the
> requested sequence has passed compares this against the 64-bit vblank
> count.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 32d9bcf5be7f..768a8e44d99b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
>  }
>  
> -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Rodrigo Vivi]

On Fri, Jan 19, 2018 at 04:53:34AM +0000, Pandiyan, Dhinakaran wrote:
> ping for review.

sorry for not getting back sooner here.

But yey \o/
I finally have dmc and psr working well on my own laptop!
so far so good! :)

> 
> Let me know if there's anything that needs to be done, thanks!
> 
> 
> On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote:
> > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> > space requested vblank sequence using this clipped 32-bit count(when the
> > value is >= 2^32) as reference, the requested sequence remains a 32-bit
> > value and gets queued like that. However, the code that checks if the
> > requested sequence has passed compares this against the 64-bit vblank
> > count.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 32d9bcf5be7f..768a8e44d99b 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> >  }
> >  
> > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> >  {
> >  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote:
> drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> space requested vblank sequence using this clipped 32-bit count(when the
> value is >= 2^32) as reference, the requested sequence remains a 32-bit
> value and gets queued like that. However, the code that checks if the
> requested sequence has passed compares this against the 64-bit vblank
> count.

Worth to mention and probably
Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")

btw, I spotted at least one more place even with the series applied.
32 current_vblank; at drm_mode_page_flip_ioctl...

so, probably worth to do a deeper check down to all paths...

anayway, for this patch:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 32d9bcf5be7f..768a8e44d99b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
>  }
>  
> -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

On Thu, 2018-01-18 at 23:36 -0800, Rodrigo Vivi wrote:
> On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote:
> > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
> > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user
> > space requested vblank sequence using this clipped 32-bit count(when the
> > value is >= 2^32) as reference, the requested sequence remains a 32-bit
> > value and gets queued like that. However, the code that checks if the
> > requested sequence has passed compares this against the 64-bit vblank
> > count.
> 
> Worth to mention and probably
> Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
> 
> btw, I spotted at least one more place even with the series applied.
> 32 current_vblank; at drm_mode_page_flip_ioctl...
> 

There seem to be several such callers for drm_crtc_vblank_count(). I can
fix it up as a follow-up series.

> so, probably worth to do a deeper check down to all paths...
> 
> anayway, for this patch:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 32d9bcf5be7f..768a8e44d99b 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >  	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> >  }
> >  
> > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> >  {
> >  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> >  
> > -- 
> > 2.11.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 497 bytes --]

Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:

> drm_vblank_count() has a u32 type returning what is a 64-bit vblank
> count.

It looks like a general review of the 64-bit widening patch is needed.

 * drm_crtc_accurate_vblank_count has a 32-bit return, and uses a 32-bit temporary
 * drm_wait_one_vblank uses a 32-bit temporary.

I looked at every 'u32' in drm_vblank.c; it would be good to have more
eyes check this.

Thanks for finding the first one.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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