Message ID | 1432630419-23490-1-git-send-email-michel@daenzer.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 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
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
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);