[PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

dev->max_vblank_count contains the largest value that can be represented
by the hardware counter. When the hardware counter wraps around, we have
to add that value + 1 to get the same value as if the hardware counter
didn't wrap around.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a3447..f9634da 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -100,7 +100,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 	/*
 	 * Interrupts were disabled prior to this call, so deal with counter
 	 * wrap if needed.
-	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
+	 * NOTE!  It's possible we lost a full dev->max_vblank_count + 1 events
 	 * here if the register is small or we had vblank interrupts off for
 	 * a long time.
 	 *
@@ -117,7 +117,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 	/* Deal with counter wrap */
 	diff = cur_vblank - vblank->last;
 	if (cur_vblank < vblank->last) {
-		diff += dev->max_vblank_count;
+		diff += dev->max_vblank_count + 1;
 
 		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
 			  crtc, vblank->last, cur_vblank, diff);
-- 
2.1.4

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

[PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The value was much too low, which could cause the userspace visible
vblank counter to move backwards when the hardware counter wrapped
around.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/radeon/radeon_irq_kms.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
index 7162c93..d5c392b 100644
--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
@@ -143,7 +143,13 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
  */
 int radeon_driver_irq_postinstall_kms(struct drm_device *dev)
 {
-	dev->max_vblank_count = 0x001fffff;
+	struct radeon_device *rdev = dev->dev_private;
+
+	if (ASIC_IS_AVIVO(rdev))
+		dev->max_vblank_count = 0x00ffffff;
+	else
+		dev->max_vblank_count = 0x001fffff;
+
 	return 0;
 }
 
-- 
2.1.4

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

Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Christian König]

Nice catch! Both patches in this series are Reviewed-by: Christian König 
<christian.koenig@amd.com>

On 26.05.2015 10:53, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> dev->max_vblank_count contains the largest value that can be represented
> by the hardware counter. When the hardware counter wraps around, we have
> to add that value + 1 to get the same value as if the hardware counter
> didn't wrap around.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>   drivers/gpu/drm/drm_irq.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c8a3447..f9634da 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -100,7 +100,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>   	/*
>   	 * Interrupts were disabled prior to this call, so deal with counter
>   	 * wrap if needed.
> -	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
> +	 * NOTE!  It's possible we lost a full dev->max_vblank_count + 1 events
>   	 * here if the register is small or we had vblank interrupts off for
>   	 * a long time.
>   	 *
> @@ -117,7 +117,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>   	/* Deal with counter wrap */
>   	diff = cur_vblank - vblank->last;
>   	if (cur_vblank < vblank->last) {
> -		diff += dev->max_vblank_count;
> +		diff += dev->max_vblank_count + 1;
>   
>   		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
>   			  crtc, vblank->last, cur_vblank, diff);

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

Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Daniel Vetter]

On Tue, May 26, 2015 at 05:53:38PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> dev->max_vblank_count contains the largest value that can be represented
> by the hardware counter. When the hardware counter wraps around, we have
> to add that value + 1 to get the same value as if the hardware counter
> didn't wrap around.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Well there's two users of this really, one wants the max (this one here)
and the other a mask. And all the drivers use it as a mask. Maybe rename
it to vblank_counter_mask or similar while at it to prevent further
confusion?
-Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c8a3447..f9634da 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -100,7 +100,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  	/*
>  	 * Interrupts were disabled prior to this call, so deal with counter
>  	 * wrap if needed.
> -	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
> +	 * NOTE!  It's possible we lost a full dev->max_vblank_count + 1 events
>  	 * here if the register is small or we had vblank interrupts off for
>  	 * a long time.
>  	 *
> @@ -117,7 +117,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  	/* Deal with counter wrap */
>  	diff = cur_vblank - vblank->last;
>  	if (cur_vblank < vblank->last) {
> -		diff += dev->max_vblank_count;
> +		diff += dev->max_vblank_count + 1;
>  
>  		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
>  			  crtc, vblank->last, cur_vblank, diff);
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Michel Dänzer]

On 26.05.2015 20:48, Daniel Vetter wrote:
> On Tue, May 26, 2015 at 05:53:38PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> dev->max_vblank_count contains the largest value that can be represented
>> by the hardware counter. When the hardware counter wraps around, we have
>> to add that value + 1 to get the same value as if the hardware counter
>> didn't wrap around.
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> 
> Well there's two users of this really, one wants the max (this one here)
> and the other a mask.

Well, this one has been around much longer. :) Looks like drm_vblank_on
added a (probably valid, see below) assumption without updating the
field name to reflect that.


> And all the drivers use it as a mask.

How so? They just assign the value, which happens to work for both
meanings above in all cases.


> Maybe rename it to vblank_counter_mask or similar while at it to
> prevent further confusion?

I'm just fixing an off-by-one bug here; I invite you or anyone else to
be my guest for anything more. :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Ville Syrjälä]

On Tue, May 26, 2015 at 01:48:11PM +0200, Daniel Vetter wrote:
> On Tue, May 26, 2015 at 05:53:38PM +0900, Michel Dänzer wrote:
> > From: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > dev->max_vblank_count contains the largest value that can be represented
> > by the hardware counter. When the hardware counter wraps around, we have
> > to add that value + 1 to get the same value as if the hardware counter
> > didn't wrap around.
> > 
> > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> 
> Well there's two users of this really, one wants the max (this one here)
> and the other a mask. And all the drivers use it as a mask. Maybe rename
> it to vblank_counter_mask or similar while at it to prevent further
> confusion?

Well, really it's just 'value % (max + 1)'. Just happens to work with &
since it's POT-1.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index c8a3447..f9634da 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -100,7 +100,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> >  	/*
> >  	 * Interrupts were disabled prior to this call, so deal with counter
> >  	 * wrap if needed.
> > -	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
> > +	 * NOTE!  It's possible we lost a full dev->max_vblank_count + 1 events
> >  	 * here if the register is small or we had vblank interrupts off for
> >  	 * a long time.
> >  	 *
> > @@ -117,7 +117,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
> >  	/* Deal with counter wrap */
> >  	diff = cur_vblank - vblank->last;
> >  	if (cur_vblank < vblank->last) {
> > -		diff += dev->max_vblank_count;
> > +		diff += dev->max_vblank_count + 1;
> >  
> >  		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
> >  			  crtc, vblank->last, cur_vblank, diff);
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling

[Daniel Vetter]

On Wed, May 27, 2015 at 04:17:05PM +0900, Michel Dänzer wrote:
> On 26.05.2015 20:48, Daniel Vetter wrote:
> > On Tue, May 26, 2015 at 05:53:38PM +0900, Michel Dänzer wrote:
> >> From: Michel Dänzer <michel.daenzer@amd.com>
> >>
> >> dev->max_vblank_count contains the largest value that can be represented
> >> by the hardware counter. When the hardware counter wraps around, we have
> >> to add that value + 1 to get the same value as if the hardware counter
> >> didn't wrap around.
> >>
> >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > Well there's two users of this really, one wants the max (this one here)
> > and the other a mask.
> 
> Well, this one has been around much longer. :) Looks like drm_vblank_on
> added a (probably valid, see below) assumption without updating the
> field name to reflect that.

Yeah, might be clearer to open-code the (slower) divide just for clarity.
Anyway applied this patch to drm-misc.

> > And all the drivers use it as a mask.
> 
> How so? They just assign the value, which happens to work for both
> meanings above in all cases.

Yeah I had the usual confusion about where exactly we start counting here
;-) I was thinking of num_vblank_counts_before_wrap vs. mask.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines

[Alex Deucher]

On Tue, May 26, 2015 at 4:53 AM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> The value was much too low, which could cause the userspace visible
> vblank counter to move backwards when the hardware counter wrapped
> around.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Applied to my -next tree.

Thanks!

> ---
>  drivers/gpu/drm/radeon/radeon_irq_kms.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index 7162c93..d5c392b 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -143,7 +143,13 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
>   */
>  int radeon_driver_irq_postinstall_kms(struct drm_device *dev)
>  {
> -       dev->max_vblank_count = 0x001fffff;
> +       struct radeon_device *rdev = dev->dev_private;
> +
> +       if (ASIC_IS_AVIVO(rdev))
> +               dev->max_vblank_count = 0x00ffffff;
> +       else
> +               dev->max_vblank_count = 0x001fffff;
> +
>         return 0;
>  }
>
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel