[PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

drm_vblank_count() has an 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.

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

Finally, fix drm_wait_one_vblank() too.

v2: Commit message fix (Keith)
    Squash commits (Rodrigo)

Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
Cc: Keith Packard <keithp@keithp.com>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_vblank.c | 8 ++++----
 include/drm/drm_vblank.h     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..f0d3ed5f2528 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];
 
@@ -292,11 +292,11 @@ static u32 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,
@@ -1055,7 +1055,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	int ret;
-	u32 last;
+	u64 last;
 
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return;
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.14.1

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

[PATCH 02/10] drm/i915/vblank: Make the vblank counter u64 -> u32 typecast explicit

[Dhinakaran Pandiyan]

Core returns a u64 vblank count and intel_crtc_get_vblank_counter()
expects a 32-bit value. Make the typecast explicit to add clarity.

Cc: Keith Packard <keithp@keithp.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ad8d9c6c40e4..f6b450de653c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12075,7 +12075,7 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 
 	if (!dev->max_vblank_count)
-		return drm_crtc_accurate_vblank_count(&crtc->base);
+		return (u32)drm_crtc_accurate_vblank_count(&crtc->base);
 
 	return dev->driver->get_vblank_counter(dev, crtc->pipe);
 }
-- 
2.14.1

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

[PATCH 03/10] drm/i915: Handle 64-bit return from drm_crtc_vblank_count()

[Dhinakaran Pandiyan]

570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64, store all the bits
without truncating. There is no need to type cast this value down to
32-bits.

Cc: Keith Packard <keithp@keithp.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 drivers/gpu/drm/i915/i915_drv.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3849ded354e3..4b9da04c1d4a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1599,7 +1599,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
 
 	if (fbc->work.scheduled)
-		seq_printf(m, "FBC worker scheduled on vblank %u, now %llu\n",
+		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
 			   fbc->work.scheduled_vblank,
 			   drm_crtc_vblank_count(&fbc->crtc->base));
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c676269ed843..d22677494499 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -717,7 +717,7 @@ struct intel_fbc {
 
 	struct intel_fbc_work {
 		bool scheduled;
-		u32 scheduled_vblank;
+		u64 scheduled_vblank;
 		struct work_struct work;
 	} work;
 
-- 
2.14.1

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

[PATCH 04/10] drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()

[Dhinakaran Pandiyan]

570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64. This could cause
potential problems if the return value is used in arithmetic operations
with a 32-bit reference HW vblank count. Explicitly typecasting this down
to u32 either fixes a potential problem or serves to add clarity in case
the typecasting was implicitly done.

Cc: Keith Packard <keithp@keithp.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 38d47559f098..c2fa5d55f04e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -207,7 +207,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
 	amdgpu_bo_unreserve(new_abo);
 
 	work->base = base;
-	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
+	work->target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
 		amdgpu_get_vblank_counter_kms(dev, work->crtc_id);
 
 	/* we borrow the event spin lock for protecting flip_wrok */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1ce4c98385e3..b7254a29b34a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3836,7 +3836,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
 
 
 	/* Prepare wait for target vblank early - before the fence-waits */
-	target_vblank = target - drm_crtc_vblank_count(crtc) +
+	target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
 			amdgpu_get_vblank_counter_kms(crtc->dev, acrtc->crtc_id);
 
 	/* TODO This might fail and hence better not used, wait
@@ -3982,7 +3982,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			amdgpu_dm_do_flip(
 				crtc,
 				fb,
-				drm_crtc_vblank_count(crtc) + *wait_for_vblank,
+				(uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
 				dm_state->context);
 		}
 
-- 
2.14.1

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

[PATCH 05/10] drm/radeon: Handle 64-bit return from drm_crtc_vblank_count()

[Dhinakaran Pandiyan]

570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64. This could cause
potential problems if the return value is used in arithmetic operations
with a 32-bit reference HW vblank count. Explicitly typecasting this down
to u32 either fixes a potential problem or serves to add clarity in case
the implicit typecasting was already correct.

Cc: Keith Packard <keithp@keithp.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/radeon/radeon_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index dfda5e0ed166..26129b2b082d 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -570,7 +570,7 @@ static int radeon_crtc_page_flip_target(struct drm_crtc *crtc,
 		base &= ~7;
 	}
 	work->base = base;
-	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
+	work->target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
 		dev->driver->get_vblank_counter(dev, work->crtc_id);
 
 	/* We borrow the event spin lock for protecting flip_work */
-- 
2.14.1

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

[PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Dhinakaran Pandiyan]

570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64. This could cause
potential problems if the return value is used in arithmetic operations
with a 32-bit reference HW vblank count. Explicitly typecasting this
down to u32 either fixes a potential problem or serves to add clarity in
case the implicit typecasting was already correct.

Cc: Keith Packard <keithp@keithp.com>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/tegra/dc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b8403ed48285..49df2db2ad46 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
 		return host1x_syncpt_read(dc->syncpt);
 
 	/* fallback to software emulated VBLANK counter */
-	return drm_crtc_vblank_count(&dc->base);
+	return (u32)drm_crtc_vblank_count(&dc->base);
 }
 
 static int tegra_dc_enable_vblank(struct drm_crtc *crtc)
-- 
2.14.1

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

[PATCH 07/10] drm/atomic: Handle 64-bit return from drm_crtc_vblank_count()

[Dhinakaran Pandiyan]

570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64.

The flip ioctl receives a 32-bit target sequence from user space and is
compared against the current sequence from drm_crtc_vblank_count(). So,
typecast return from drm_crtc_vblank_count() explicitly to add clarity.

__drm_crtcs_state.last_vblank_count however only ever stores the value from
drm_crtc_vblank_count() and can be upgraded to u64.

Cc: Keith Packard <keithp@keithp.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 include/drm/drm_atomic.h    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 22b54663b6e7..09de6ecb3968 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -948,7 +948,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		if (r)
 			return r;
 
-		current_vblank = drm_crtc_vblank_count(crtc);
+		current_vblank = (u32)drm_crtc_vblank_count(crtc);
 
 		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
 		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index cf13842a6dbd..2c711a24c80c 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -154,7 +154,7 @@ struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state, *old_state, *new_state;
 	s32 __user *out_fence_ptr;
-	unsigned last_vblank_count;
+	u64 last_vblank_count;
 };
 
 struct __drm_connnectors_state {
-- 
2.14.1

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

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

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

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>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 f0d3ed5f2528..913954765d9e 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.14.1

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

[PATCH 09/10] drm/vblank: Restoring vblank counts after device PM events.

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

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>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 913954765d9e..c781cb426bf1 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.14.1

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

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

[Dhinakaran Pandiyan]

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

The frame counter may have got reset between disabling and enabling vblank
interrupts due to DMC putting the hardware to DC5/6 states if PSR was
active. The frame counter could also have stalled if PSR was active in case
there was no DMC. The frame counter resetting has a user visible impact
of screen freezes.

Make use of drm_vblank_restore() to compute missed vblanks for the duration
in which vblank interrupts were disabled and update the vblank counter with
this value as diff. There's no need to check if PSR was actually active in
the interrupt disabled duration, so simplify the check to a feature check.

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.

This change is not applicable to CHV, as enabling interrupts does not
prevent the hardware from activating PSR.

v2: Added comments(Rodrigo) and rewrote commit message.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 252feff2892d..e86c645b6b07 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2968,6 +2968,12 @@ 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);
 
+	/* Even though there is no DMC, frame counter can get stuck when
+	 * PSR is active as no frames are generated.
+	 */
+	if (HAS_PSR(dev_priv))
+		drm_vblank_restore(dev, pipe);
+
 	return 0;
 }
 
@@ -2980,6 +2986,12 @@ 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);
 
+	/* Even if there is no DMC, frame counter can get stuck when
+	 * PSR is active as no frames are generated, so check only for PSR.
+	 */
+	if (HAS_PSR(dev_priv))
+		drm_vblank_restore(dev, pipe);
+
 	return 0;
 }
 
-- 
2.14.1

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

Re: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Keith Packard]

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

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

> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>
> drm_vblank_count() has an 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.

For patches 1-7:

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
-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

Re: [PATCH 04/10] drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()

[Harry Wentland]

On 2018-02-03 12:12 AM, Dhinakaran Pandiyan wrote:
> 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> return type for drm_crtc_vblank_count() to u64. This could cause
> potential problems if the return value is used in arithmetic operations
> with a 32-bit reference HW vblank count. Explicitly typecasting this down
> to u32 either fixes a potential problem or serves to add clarity in case
> the typecasting was implicitly done.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 38d47559f098..c2fa5d55f04e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -207,7 +207,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>  	amdgpu_bo_unreserve(new_abo);
>  
>  	work->base = base;
> -	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
> +	work->target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
>  		amdgpu_get_vblank_counter_kms(dev, work->crtc_id);
>  
>  	/* we borrow the event spin lock for protecting flip_wrok */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1ce4c98385e3..b7254a29b34a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3836,7 +3836,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>  
>  
>  	/* Prepare wait for target vblank early - before the fence-waits */
> -	target_vblank = target - drm_crtc_vblank_count(crtc) +
> +	target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
>  			amdgpu_get_vblank_counter_kms(crtc->dev, acrtc->crtc_id);
>  
>  	/* TODO This might fail and hence better not used, wait
> @@ -3982,7 +3982,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  			amdgpu_dm_do_flip(
>  				crtc,
>  				fb,
> -				drm_crtc_vblank_count(crtc) + *wait_for_vblank,
> +				(uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
>  				dm_state->context);
>  		}
>  
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/10] drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()

[Alex Deucher]

On Sat, Feb 3, 2018 at 12:12 AM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> return type for drm_crtc_vblank_count() to u64. This could cause
> potential problems if the return value is used in arithmetic operations
> with a 32-bit reference HW vblank count. Explicitly typecasting this down
> to u32 either fixes a potential problem or serves to add clarity in case
> the typecasting was implicitly done.
>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com> for both this patch
and the radeon one.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 38d47559f098..c2fa5d55f04e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -207,7 +207,7 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
>         amdgpu_bo_unreserve(new_abo);
>
>         work->base = base;
> -       work->target_vblank = target - drm_crtc_vblank_count(crtc) +
> +       work->target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
>                 amdgpu_get_vblank_counter_kms(dev, work->crtc_id);
>
>         /* we borrow the event spin lock for protecting flip_wrok */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1ce4c98385e3..b7254a29b34a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3836,7 +3836,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>
>
>         /* Prepare wait for target vblank early - before the fence-waits */
> -       target_vblank = target - drm_crtc_vblank_count(crtc) +
> +       target_vblank = target - (uint32_t)drm_crtc_vblank_count(crtc) +
>                         amdgpu_get_vblank_counter_kms(crtc->dev, acrtc->crtc_id);
>
>         /* TODO This might fail and hence better not used, wait
> @@ -3982,7 +3982,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>                         amdgpu_dm_do_flip(
>                                 crtc,
>                                 fb,
> -                               drm_crtc_vblank_count(crtc) + *wait_for_vblank,
> +                               (uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
>                                 dm_state->context);
>                 }
>
> --
> 2.14.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Rodrigo Vivi]

On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
> 
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >
> > drm_vblank_count() has an 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.
> 
> For patches 1-7:
> 
> Reviewed-by: Keith Packard <keithp@keithp.com>

Dave, ack to merge them through drm-intel-next-queued ?

Patches 8 to 10 are ready as well.

Thanks,
Rodrigo.

> 
> -- 
> -keith


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

Re: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Dave Airlie]

On 6 February 2018 at 06:32, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
>> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
>>
>> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>> >
>> > drm_vblank_count() has an 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.
>>
>> For patches 1-7:
>>
>> Reviewed-by: Keith Packard <keithp@keithp.com>
>
> Dave, ack to merge them through drm-intel-next-queued ?

Ack. do we know if any of those need to be in -fixes?

or too early to tell?

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

Re: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Rodrigo Vivi]

On Mon, Feb 05, 2018 at 08:37:13PM +0000, Dave Airlie wrote:
> On 6 February 2018 at 06:32, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> >> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
> >>
> >> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >> >
> >> > drm_vblank_count() has an 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.
> >>
> >> For patches 1-7:
> >>
> >> Reviewed-by: Keith Packard <keithp@keithp.com>
> >
> > Dave, ack to merge them through drm-intel-next-queued ?
> 
> Ack. do we know if any of those need to be in -fixes?
> 
> or too early to tell?

I didn't checked the drm one close enough yet to decide for that.
DK or Keith? do you guys see anyone suitable for fixes?

For the other work on top I believe we don't need to move to fixes
since psr is still disabled.

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

Re: [Intel-gfx] [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Pandiyan, Dhinakaran]

On Mon, 2018-02-05 at 12:49 -0800, Rodrigo Vivi wrote:
> On Mon, Feb 05, 2018 at 08:37:13PM +0000, Dave Airlie wrote:
> > On 6 February 2018 at 06:32, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> > >> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
> > >>
> > >> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> > >> >
> > >> > drm_vblank_count() has an 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.
> > >>
> > >> For patches 1-7:
> > >>
> > >> Reviewed-by: Keith Packard <keithp@keithp.com>
> > >
> > > Dave, ack to merge them through drm-intel-next-queued ?
> > 
> > Ack. do we know if any of those need to be in -fixes?
> > 
> > or too early to tell?
> 
> I didn't checked the drm one close enough yet to decide for that.
> DK or Keith? do you guys see anyone suitable for fixes?

I was thinking patch 1 would be a candidate. But, it'd need the crtc to
be active continuously for > 2.2 years at 60 frames/s to trigger this.
The problem is exacerbated with PSR and PSR is disabled. So, I think we
can skip -fixes.

> 
> For the other work on top I believe we don't need to move to fixes
> since psr is still disabled.
> 
> > 
> > Dave.
> _______________________________________________
> 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: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Keith Packard]

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

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> I didn't checked the drm one close enough yet to decide for that.
> DK or Keith? do you guys see anyone suitable for fixes?

Yeah, we should probably get 1, 3 and 7 into fixes. 2, 4, 5 and 6 add
explicit casts where the compiler would already introduce equivalent
implicit casts.

-- 
-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

Re: [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Pandiyan, Dhinakaran]

On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> return type for drm_crtc_vblank_count() to u64. This could cause
> potential problems if the return value is used in arithmetic operations
> with a 32-bit reference HW vblank count. Explicitly typecasting this
> down to u32 either fixes a potential problem or serves to add clarity in
> case the implicit typecasting was already correct.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Thierry Reding <treding@nvidia.com>


Thierry, 

Can I get an Ack on this please? 

> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/tegra/dc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index b8403ed48285..49df2db2ad46 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
>  		return host1x_syncpt_read(dc->syncpt);
>  
>  	/* fallback to software emulated VBLANK counter */
> -	return drm_crtc_vblank_count(&dc->base);
> +	return (u32)drm_crtc_vblank_count(&dc->base);
>  }
>  
>  static int tegra_dc_enable_vblank(struct drm_crtc *crtc)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Thierry Reding]

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

On Wed, Feb 07, 2018 at 01:41:18AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > return type for drm_crtc_vblank_count() to u64. This could cause
> > potential problems if the return value is used in arithmetic operations
> > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > down to u32 either fixes a potential problem or serves to add clarity in
> > case the implicit typecasting was already correct.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> 
> 
> Thierry, 
> 
> Can I get an Ack on this please? 
> 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/tegra/dc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index b8403ed48285..49df2db2ad46 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
> >  		return host1x_syncpt_read(dc->syncpt);
> >  
> >  	/* fallback to software emulated VBLANK counter */
> > -	return drm_crtc_vblank_count(&dc->base);
> > +	return (u32)drm_crtc_vblank_count(&dc->base);

Isn't this the wrong way around? Shouldn't we instead make the
->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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

Re: [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Pandiyan, Dhinakaran]

On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
> On Wed, Feb 07, 2018 at 01:41:18AM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > > return type for drm_crtc_vblank_count() to u64. This could cause
> > > potential problems if the return value is used in arithmetic operations
> > > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > > down to u32 either fixes a potential problem or serves to add clarity in
> > > case the implicit typecasting was already correct.
> > > 
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Cc: Thierry Reding <treding@nvidia.com>
> > 
> > 
> > Thierry, 
> > 
> > Can I get an Ack on this please? 
> > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/tegra/dc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > index b8403ed48285..49df2db2ad46 100644
> > > --- a/drivers/gpu/drm/tegra/dc.c
> > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
> > >  		return host1x_syncpt_read(dc->syncpt);
> > >  
> > >  	/* fallback to software emulated VBLANK counter */
> > > -	return drm_crtc_vblank_count(&dc->base);
> > > +	return (u32)drm_crtc_vblank_count(&dc->base);
> 
> Isn't this the wrong way around? Shouldn't we instead make the
> ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?

Here's how I understand this - 

To use the software emulated vblank counter, the driver should set
max_vblank_count = 0 and the core takes care of calculating elapsed
vblanks.

->get_vblank_counter() is meant to return the hardware counter if
available, which would be a 32-bit value. Hence the explicit typecast to
32-bit for the fallback case too.

Having said that, I believe drm_crtc_accurate_vblank_count() is the
appropriate function for fallback.

-DK



> 
> Thierry
> _______________________________________________
> 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 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Rodrigo Vivi]

On Wed, Feb 07, 2018 at 09:32:35PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
> > On Wed, Feb 07, 2018 at 01:41:18AM +0000, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > > > return type for drm_crtc_vblank_count() to u64. This could cause
> > > > potential problems if the return value is used in arithmetic operations
> > > > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > > > down to u32 either fixes a potential problem or serves to add clarity in
> > > > case the implicit typecasting was already correct.
> > > > 
> > > > Cc: Keith Packard <keithp@keithp.com>
> > > > Cc: Thierry Reding <treding@nvidia.com>
> > > 
> > > 
> > > Thierry, 
> > > 
> > > Can I get an Ack on this please? 
> > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/tegra/dc.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > > index b8403ed48285..49df2db2ad46 100644
> > > > --- a/drivers/gpu/drm/tegra/dc.c
> > > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
> > > >  		return host1x_syncpt_read(dc->syncpt);
> > > >  
> > > >  	/* fallback to software emulated VBLANK counter */
> > > > -	return drm_crtc_vblank_count(&dc->base);
> > > > +	return (u32)drm_crtc_vblank_count(&dc->base);
> > 
> > Isn't this the wrong way around? Shouldn't we instead make the
> > ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
> 
> Here's how I understand this - 
> 
> To use the software emulated vblank counter, the driver should set
> max_vblank_count = 0 and the core takes care of calculating elapsed
> vblanks.
> 
> ->get_vblank_counter() is meant to return the hardware counter if
> available, which would be a 32-bit value. Hence the explicit typecast to
> 32-bit for the fallback case too.
> 
> Having said that, I believe drm_crtc_accurate_vblank_count() is the
> appropriate function for fallback.

Hi Thierry,

any further concerns or thoughts here?

I'd like to merge all together on drm-intel since the ones
around us is kind of blocking us.

Thanks,
Rodrigo.

> 
> -DK
> 
> 
> 
> > 
> > Thierry
> > _______________________________________________
> > 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 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Rodrigo Vivi]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> On Wed, Feb 07, 2018 at 09:32:35PM +0000, Pandiyan, Dhinakaran wrote:
>> 
>> 
>> 
>> On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
>> > On Wed, Feb 07, 2018 at 01:41:18AM +0000, Pandiyan, Dhinakaran wrote:
>> > > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
>> > > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
>> > > > return type for drm_crtc_vblank_count() to u64. This could cause
>> > > > potential problems if the return value is used in arithmetic operations
>> > > > with a 32-bit reference HW vblank count. Explicitly typecasting this
>> > > > down to u32 either fixes a potential problem or serves to add clarity in
>> > > > case the implicit typecasting was already correct.
>> > > > 
>> > > > Cc: Keith Packard <keithp@keithp.com>
>> > > > Cc: Thierry Reding <treding@nvidia.com>
>> > > 
>> > > 
>> > > Thierry, 
>> > > 
>> > > Can I get an Ack on this please? 
>> > > 
>> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/tegra/dc.c | 2 +-
>> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > 
>> > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> > > > index b8403ed48285..49df2db2ad46 100644
>> > > > --- a/drivers/gpu/drm/tegra/dc.c
>> > > > +++ b/drivers/gpu/drm/tegra/dc.c
>> > > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
>> > > >  		return host1x_syncpt_read(dc->syncpt);
>> > > >  
>> > > >  	/* fallback to software emulated VBLANK counter */
>> > > > -	return drm_crtc_vblank_count(&dc->base);
>> > > > +	return (u32)drm_crtc_vblank_count(&dc->base);
>> > 
>> > Isn't this the wrong way around? Shouldn't we instead make the
>> > ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
>> 
>> Here's how I understand this - 
>> 
>> To use the software emulated vblank counter, the driver should set
>> max_vblank_count = 0 and the core takes care of calculating elapsed
>> vblanks.
>> 
>> ->get_vblank_counter() is meant to return the hardware counter if
>> available, which would be a 32-bit value. Hence the explicit typecast to
>> 32-bit for the fallback case too.
>> 
>> Having said that, I believe drm_crtc_accurate_vblank_count() is the
>> appropriate function for fallback.
>
> Hi Thierry,
>
> any further concerns or thoughts here?
>
> I'd like to merge all together on drm-intel since the ones
> around us is kind of blocking us.
>

Hi Thierry,

I was taking a deeper look to the code here and talking to DK.

This patch only aims to bring more clarity to where the crops are happening.

Furthermore making the whole function to return u64 would have
the same effect since it would get cropped one level above.

I believe you are the best one to make the choice for one
way over another, or none, but the result would be the same.

Since this has no functional impact, I'm planing to move with
other patches but leaving this one behind so you can decide later.

If you still have any concerns please let me know.

Thanks,
Rodrigo,

> Thanks,
> Rodrigo.
>
>> 
>> -DK
>> 
>> 
>> 
>> > 
>> > Thierry
>> > _______________________________________________
>> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()

[Thierry Reding]

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

On Wed, Feb 07, 2018 at 01:41:18AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > return type for drm_crtc_vblank_count() to u64. This could cause
> > potential problems if the return value is used in arithmetic operations
> > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > down to u32 either fixes a potential problem or serves to add clarity in
> > case the implicit typecasting was already correct.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> 
> 
> Thierry, 
> 
> Can I get an Ack on this please? 

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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

Re: [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Rodrigo Vivi]

Dave Airlie <airlied@gmail.com> writes:

> On 6 February 2018 at 06:32, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
>>> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
>>>
>>> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>>> >
>>> > drm_vblank_count() has an 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.
>>>
>>> For patches 1-7:
>>>
>>> Reviewed-by: Keith Packard <keithp@keithp.com>
>>
>> Dave, ack to merge them through drm-intel-next-queued ?
>
> Ack. do we know if any of those need to be in -fixes?

All patches merged to drm-intel-next-queued.
Thanks for the patches, reviews and acks.

>
> or too early to tell?
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Pandiyan, Dhinakaran]

On Thu, 2018-02-15 at 11:55 -0800, Rodrigo Vivi wrote:
> Dave Airlie <airlied@gmail.com> writes:
> 
> > On 6 February 2018 at 06:32, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> >> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> >>> Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes:
> >>>
> >>> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> >>> >
> >>> > drm_vblank_count() has an 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.
> >>>
> >>> For patches 1-7:
> >>>
> >>> Reviewed-by: Keith Packard <keithp@keithp.com>
> >>
> >> Dave, ack to merge them through drm-intel-next-queued ?
> >
> > Ack. do we know if any of those need to be in -fixes?
> 
> All patches merged to drm-intel-next-queued.
> Thanks for the patches, reviews and acks.
> 


Thanks everyone for the reviews and Acks!
-DK

> >
> > or too early to tell?
> >
> > Dave.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> 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: [PATCH 09/10] drm/vblank: Restoring vblank counts after device PM events.

[Daniel Vetter]

On Fri, Feb 02, 2018 at 09:13:01PM -0800, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> 
> 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>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

This now creates new warnings when building the docs. When changing any
kind of kernel doc please always run

$ make htmldocs

and make sure the parts you're touching are warning free, the results look
good (and consistent with existing docs) and that all the hyperlinks work
and point to the right places.

Please spin a follow up patch to fix this (since this one here is merged
already).

Thanks, Daniel

> ---
>  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 913954765d9e..c781cb426bf1 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.14.1
> 

-- 
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