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);
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; }
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;elsedev->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
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);
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
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. :)
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
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
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Daniel Vetter -
Michel Dänzer -
Ville Syrjälä