* [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
@ 2015-05-26 8:53 Michel Dänzer
2015-05-26 8:53 ` [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines Michel Dänzer
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Michel Dänzer @ 2015-05-26 8:53 UTC (permalink / raw)
To: dri-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines
2015-05-26 8:53 [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Michel Dänzer
@ 2015-05-26 8:53 ` Michel Dänzer
2015-05-28 4:17 ` Alex Deucher
2015-05-26 8:57 ` [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Christian König
2015-05-26 11:48 ` Daniel Vetter
2 siblings, 1 reply; 8+ messages in thread
From: Michel Dänzer @ 2015-05-26 8:53 UTC (permalink / raw)
To: dri-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
2015-05-26 8:53 [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Michel Dänzer
2015-05-26 8:53 ` [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines Michel Dänzer
@ 2015-05-26 8:57 ` Christian König
2015-05-26 11:48 ` Daniel Vetter
2 siblings, 0 replies; 8+ messages in thread
From: Christian König @ 2015-05-26 8:57 UTC (permalink / raw)
To: Michel Dänzer, dri-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
2015-05-26 8:53 [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Michel Dänzer
2015-05-26 8:53 ` [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines Michel Dänzer
2015-05-26 8:57 ` [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Christian König
@ 2015-05-26 11:48 ` Daniel Vetter
2015-05-27 7:17 ` Michel Dänzer
2015-05-27 7:42 ` Ville Syrjälä
2 siblings, 2 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-05-26 11:48 UTC (permalink / raw)
To: Michel Dänzer; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
2015-05-26 11:48 ` Daniel Vetter
@ 2015-05-27 7:17 ` Michel Dänzer
2015-05-27 11:03 ` Daniel Vetter
2015-05-27 7:42 ` Ville Syrjälä
1 sibling, 1 reply; 8+ messages in thread
From: Michel Dänzer @ 2015-05-27 7:17 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
2015-05-26 11:48 ` Daniel Vetter
2015-05-27 7:17 ` Michel Dänzer
@ 2015-05-27 7:42 ` Ville Syrjälä
1 sibling, 0 replies; 8+ messages in thread
From: Ville Syrjälä @ 2015-05-27 7:42 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Michel Dänzer, dri-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling
2015-05-27 7:17 ` Michel Dänzer
@ 2015-05-27 11:03 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-05-27 11:03 UTC (permalink / raw)
To: Michel Dänzer; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines
2015-05-26 8:53 ` [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines Michel Dänzer
@ 2015-05-28 4:17 ` Alex Deucher
0 siblings, 0 replies; 8+ messages in thread
From: Alex Deucher @ 2015-05-28 4:17 UTC (permalink / raw)
To: Michel Dänzer; +Cc: Maling list - DRI developers
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-28 4:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-26 8:53 [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Michel Dänzer
2015-05-26 8:53 ` [PATCH 2/2] drm/radeon: Fix max_vblank_count value for current display engines Michel Dänzer
2015-05-28 4:17 ` Alex Deucher
2015-05-26 8:57 ` [PATCH 1/2] drm: Fix off-by-one in vblank hardware counter wraparound handling Christian König
2015-05-26 11:48 ` Daniel Vetter
2015-05-27 7:17 ` Michel Dänzer
2015-05-27 11:03 ` Daniel Vetter
2015-05-27 7:42 ` Ville Syrjälä
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.