drm: Don't pass negative delta to ktime_sub_ns()

Commit Message

Michel Dänzer June 12, 2013, 9:58 a.m. UTC

From: Michel Dänzer <[e-mail]>

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: [e-mail]
Signed-off-by: Michel Dänzer <[e-mail]>
---
 drivers/gpu/drm/drm_irq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Chris Wilson June 12, 2013, 10:48 a.m. UTC | #1

On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <[e-mail]>
> 
> 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: [e-mail]
> Signed-off-by: Michel Dänzer <[e-mail]>

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

Imre Deak June 12, 2013, 10:50 a.m. UTC | #2

On Wed, 2013-06-12 at 11:58 +0200, Michel Dänzer wrote:
> From: Michel Dänzer <[e-mail]>
> 
> 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: [e-mail]
> Signed-off-by: Michel Dänzer <[e-mail]>

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 <[e-mail]>

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

Chris Wilson June 12, 2013, 11:06 a.m. UTC | #3

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 <[e-mail]>
> > 
> > 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: [e-mail]
> > Signed-off-by: Michel Dänzer <[e-mail]>
> 
> 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

Daniel Vetter June 13, 2013, 7:40 a.m. UTC | #4

On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <[e-mail]>
> 
> 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: [e-mail]
> Signed-off-by: Michel Dänzer <[e-mail]>

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 <[e-mail]>

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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Chris Wilson July 17, 2013, 11:58 a.m. UTC | #5

On Wed, Jun 12, 2013 at 11:58:44AM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <[e-mail]>
> 
> 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: [e-mail]
> Signed-off-by: Michel Dänzer <[e-mail]>

Dave, it looks like you missed this patch.
-Chris

Michel Dänzer Aug. 7, 2013, 8:42 a.m. UTC | #6

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 <[e-mail]>
> > 
> > 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: [e-mail]
> > Signed-off-by: Michel Dänzer <[e-mail]>
> 
> 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...

Patch

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