[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

✓ Fi.CI.BAT: success for series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
URL   : https://patchwork.freedesktop.org/series/37598/
State : success

== Summary ==

Series 37598v1 series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
https://patchwork.freedesktop.org/api/1.0/series/37598/revisions/1/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:421s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:420s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:371s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:482s
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:482s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:463s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:451s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:573s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:275s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:510s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:401s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:455s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:409s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:456s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:495s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:576s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:529s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:480s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:482s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:468s

2e76a2952923eba64c4f9baf461613bc42ee997a drm-tip: 2018y-02m-02d-20h-33m-12s UTC integration manifest
9df046e9e2ed drm/i915: Estimate and update missed vblanks.
5aa5a89ed404 drm/vblank: Restoring vblank counts after device PM events.
6d4184e530db drm/vblank: Do not update vblank count if interrupts are already disabled.
b038ce8dd537 drm/atomic: Handle 64-bit return from drm_crtc_vblank_count()
5a06910d4f70 drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
7235dbb2bfa1 drm/radeon: Handle 64-bit return from drm_crtc_vblank_count()
f9941350553f drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()
aca673b55435 drm/i915: Handle 64-bit return from drm_crtc_vblank_count()
bf1669999c93 drm/i915/vblank: Make the vblank counter u64 -> u32 typecast explicit
92c3f30a31da drm/vblank: Data type fixes for 64-bit vblank sequences.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7875/issues.html
_______________________________________________
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.

[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

✗ Fi.CI.IGT: failure for series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
URL   : https://patchwork.freedesktop.org/series/37598/
State : failure

== Summary ==

Test perf:
        Subgroup blocking:
                pass       -> FAIL       (shard-hsw) fdo#102252
        Subgroup oa-exponents:
                pass       -> FAIL       (shard-apl) fdo#102254
Test kms_flip_tiling:
        Subgroup flip-changes-tiling-yf:
                pass       -> FAIL       (shard-apl) fdo#103822
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                fail       -> PASS       (shard-hsw)
        Subgroup 2x-flip-vs-expired-vblank-interruptible:
                fail       -> PASS       (shard-hsw) fdo#102887
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-toggle:
                fail       -> PASS       (shard-hsw) fdo#102670 +1
Test kms_plane_lowres:
        Subgroup pipe-b-tiling-none:
                pass       -> FAIL       (shard-apl) fdo#103166
Test kms_cursor_crc:
        Subgroup cursor-128x128-suspend:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540
Test perf_pmu:
        Subgroup busy-double-start-vcs0:
                pass       -> INCOMPLETE (shard-apl)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> INCOMPLETE (shard-hsw) fdo#104874

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#104874 https://bugs.freedesktop.org/show_bug.cgi?id=104874

shard-apl        total:2795 pass:1722 dwarn:1   dfail:0   fail:22  skip:1049 time:12044s
shard-hsw        total:2765 pass:1690 dwarn:1   dfail:0   fail:12  skip:1059 time:10623s
shard-snb        total:2836 pass:1327 dwarn:2   dfail:0   fail:10  skip:1497 time:6423s
Blacklisted hosts:
shard-kbl        total:2836 pass:1868 dwarn:5   dfail:0   fail:22  skip:941 time:9351s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7875/shards.html
_______________________________________________
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

✓ Fi.CI.BAT: success for series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
URL   : https://patchwork.freedesktop.org/series/37598/
State : success

== Summary ==

Series 37598v1 series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
https://patchwork.freedesktop.org/api/1.0/series/37598/revisions/1/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:427s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:376s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:492s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:487s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:486s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:471s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:459s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:412s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:283s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:515s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:405s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:449s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:456s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:500s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:598s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:429s
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:531s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:480s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:429s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:527s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:399s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:473s

078873da383505cf8d6940229007115b31f1d5e0 drm-tip: 2018y-02m-06d-11h-21m-36s UTC integration manifest
1fbe766c4e29 drm/i915: Estimate and update missed vblanks.
93f6ed7165be drm/vblank: Restoring vblank counts after device PM events.
9dcfd30f84de drm/vblank: Do not update vblank count if interrupts are already disabled.
f5009e90de2e drm/atomic: Handle 64-bit return from drm_crtc_vblank_count()
24fe6af8f870 drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
4f811089d445 drm/radeon: Handle 64-bit return from drm_crtc_vblank_count()
51f4675db99e drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()
abc0899cc109 drm/i915: Handle 64-bit return from drm_crtc_vblank_count()
42824daf49fe drm/i915/vblank: Make the vblank counter u64 -> u32 typecast explicit
5f830a5a47dc drm/vblank: Data type fixes for 64-bit vblank sequences.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7910/issues.html
_______________________________________________
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

✗ Fi.CI.BAT: warning for series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

[Patchwork]

== Series Details ==

Series: series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
URL   : https://patchwork.freedesktop.org/series/37598/
State : warning

== Summary ==

Series 37598v1 series starting with [01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.
https://patchwork.freedesktop.org/api/1.0/series/37598/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-all:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-each:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-many-each:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-store-all:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-store-each:
                pass       -> SKIP       (fi-blb-e6850)
Test gem_tiled_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-blb-e6850)
Test gem_tiled_fence_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-blb-e6850)
Test gem_wait:
        Subgroup basic-busy-all:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-wait-all:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-await-all:
                pass       -> SKIP       (fi-blb-e6850)
Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> SKIP       (fi-blb-e6850)
        Subgroup basic-flip-b:
                pass       -> SKIP       (fi-blb-e6850)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> SKIP       (fi-blb-e6850)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                fail       -> PASS       (fi-glk-1) fdo#103167

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:429s
fi-blb-e6850     total:288  pass:210  dwarn:1   dfail:0   fail:0   skip:77  time:346s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:488s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:285s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:487s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:454s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:580s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:280s
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:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:447s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:414s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:461s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:500s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:590s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:434s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:507s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:418s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:427s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:516s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:393s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:470s

57bed53a064818de1d946bf7d432ca6e756f553e drm-tip: 2018y-02m-07d-05h-16m-20s UTC integration manifest
ca17e45f580b drm/i915: Estimate and update missed vblanks.
f8492dc84a20 drm/vblank: Restoring vblank counts after device PM events.
6ac7f096240e drm/vblank: Do not update vblank count if interrupts are already disabled.
aadc1d0ec136 drm/atomic: Handle 64-bit return from drm_crtc_vblank_count()
ada4171d3234 drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
c65f5b4472e8 drm/radeon: Handle 64-bit return from drm_crtc_vblank_count()
bb3dd95efe69 drm/amdgpu: Handle 64-bit return from drm_crtc_vblank_count()
1bd8a8d60a12 drm/i915: Handle 64-bit return from drm_crtc_vblank_count()
ef563a441faf drm/i915/vblank: Make the vblank counter u64 -> u32 typecast explicit
a71f221b1406 drm/vblank: Data type fixes for 64-bit vblank sequences.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7915/issues.html
_______________________________________________
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