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

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

[Michel Dänzer]

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

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

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Imre Deak]

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


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

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: Don't pass negative delta to ktime_sub_ns()

[Michel Dänzer]

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


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

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