From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com --- drivers/gpu/drm/drm_irq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a6a8643..39665f8 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -708,7 +708,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc, /* Subtract time delta from raw timestamp to get final * vblank_time timestamp for end of vblank. */ - etime = ktime_sub_ns(etime, delta_ns); + if (delta_ns < 0) + etime = ktime_add_ns(etime, -delta_ns); + else + etime = ktime_sub_ns(etime, delta_ns); *vblank_time = ktime_to_timeval(etime);
DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
iiuc, this occurs when compensating for the early vblank interrupt. However,
#define ktime_sub_ns(kt, nsval) \ ({ (ktime_t){ .tv64 = (kt).tv64 - (nsval) }; })
so both tv64 and delta_ns are s64. I am not seeing the unsigned promotion bug here. -Chris
On Wed, Jun 12, 2013 at 11:48:13AM +0100, Chris Wilson wrote:
On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
iiuc, this occurs when compensating for the early vblank interrupt. However,
#define ktime_sub_ns(kt, nsval) \ ({ (ktime_t){ .tv64 = (kt).tv64 - (nsval) }; })
so both tv64 and delta_ns are s64. I am not seeing the unsigned promotion bug here.
Ok, Imre pointed out to me that there is a separate definition for 32-bit machines that does seem limited to only handling unsigned ns values. And so currently blows up big time for the early vblank case.
If we knew the values were guaranteed to be less that a second we could just use ktime_add() by keeping delta_ns as a ktime_t instead. Since we are dealing with fractions of a frame... -Chris
On Wed, 2013-06-12 at 11:58 +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
Yes, I introduced this regression in:
drm: use monotonic time in drm_calc_vbltimestamp_from_scanoutpos
The fix seems to be ok, perhaps you should also mention the regressing commit in the commit message.
Reviewed-by: Imre Deak imre.deak@intel.com
drivers/gpu/drm/drm_irq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a6a8643..39665f8 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -708,7 +708,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc, /* Subtract time delta from raw timestamp to get final * vblank_time timestamp for end of vblank. */
- etime = ktime_sub_ns(etime, delta_ns);
if (delta_ns < 0)
etime = ktime_add_ns(etime, -delta_ns);else
etime = ktime_sub_ns(etime, delta_ns);*vblank_time = ktime_to_timeval(etime);
DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
Our paranoid flip tester apparently hit this:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59339 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59836 Tested-by: shui yangwei yangweix.shui@intel.com
Cheers, Daniel
drivers/gpu/drm/drm_irq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a6a8643..39665f8 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -708,7 +708,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc, /* Subtract time delta from raw timestamp to get final * vblank_time timestamp for end of vblank. */
- etime = ktime_sub_ns(etime, delta_ns);
if (delta_ns < 0)
etime = ktime_add_ns(etime, -delta_ns);else
etime = ktime_sub_ns(etime, delta_ns);*vblank_time = ktime_to_timeval(etime);
DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
-- 1.8.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
Dave, it looks like you missed this patch. -Chris
vimOn Mit, 2013-07-17 at 12:58 +0100, Chris Wilson wrote:
On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
From: Michel Dänzer michel.daenzer@amd.com
It takes an unsigned value. This happens not to blow up on 64-bit architectures, but it does on 32-bit, causing drm_calc_vbltimestamp_from_scanoutpos() to calculate totally bogus timestamps for vblank events. Which in turn causes e.g. gnome-shell to hang after a DPMS off cycle with current xf86-video-ati Git.
Cc: stable@vger.kernel.org Signed-off-by: Michel Dänzer michel.daenzer@amd.com
Dave, it looks like you missed this patch.
Dave, any particular reason why this fix still isn't merged? It's approaching its two-month anniversary...
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Imre Deak -
Michel Dänzer