| Message ID | 20180112215707.3084-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote: > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > space requested vblank sequence using this clipped 32-bit count(when the > value is >= 2^32) as reference, the requested sequence remains a 32-bit > value and gets queued like that. However, the code that checks if the > requested sequence has passed compares this against the 64-bit vblank > count. > > Cc: Keith Packard <keithp@keithp.com> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Sounds like the 64bit widening wasn't all that well tested ... do we have an igt for this? Iirc the base igt was merged already. -Daniel > --- > drivers/gpu/drm/drm_vblank.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 32d9bcf5be7f..768a8e44d99b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > } > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > > -- > 2.11.0 >
On Mon, 2018-01-15 at 10:38 +0100, Daniel Vetter wrote: > On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote: > > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > > space requested vblank sequence using this clipped 32-bit count(when the > > value is >= 2^32) as reference, the requested sequence remains a 32-bit > > value and gets queued like that. However, the code that checks if the > > requested sequence has passed compares this against the 64-bit vblank > > count. > > > > Cc: Keith Packard <keithp@keithp.com> > > Cc: Michel Dänzer <michel@daenzer.net> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Sounds like the 64bit widening wasn't all that well tested ... do we have > an igt for this? Iirc the base igt was merged already. I don't see anything that would particularly trigger this condition i.e., vblank->count > 2^32 in the IGTs. We'll need to implement something to force set a very large vblank->count and then request a vblank sequence. > -Daniel > > > --- > > drivers/gpu/drm/drm_vblank.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > index 32d9bcf5be7f..768a8e44d99b 100644 > > --- a/drivers/gpu/drm/drm_vblank.c > > +++ b/drivers/gpu/drm/drm_vblank.c > > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > > } > > > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > { > > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > > > > -- > > 2.11.0 > > >
ping for review. Let me know if there's anything that needs to be done, thanks! On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote: > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > space requested vblank sequence using this clipped 32-bit count(when the > value is >= 2^32) as reference, the requested sequence remains a 32-bit > value and gets queued like that. However, the code that checks if the > requested sequence has passed compares this against the 64-bit vblank > count. > > Cc: Keith Packard <keithp@keithp.com> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/drm_vblank.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 32d9bcf5be7f..768a8e44d99b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > } > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >
On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote: > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > space requested vblank sequence using this clipped 32-bit count(when the > value is >= 2^32) as reference, the requested sequence remains a 32-bit > value and gets queued like that. However, the code that checks if the > requested sequence has passed compares this against the 64-bit vblank > count. Worth to mention and probably Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") btw, I spotted at least one more place even with the series applied. 32 current_vblank; at drm_mode_page_flip_ioctl... so, probably worth to do a deeper check down to all paths... anayway, for this patch: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Keith Packard <keithp@keithp.com> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/drm_vblank.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 32d9bcf5be7f..768a8e44d99b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > } > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > > -- > 2.11.0 >
On Fri, Jan 19, 2018 at 04:53:34AM +0000, Pandiyan, Dhinakaran wrote: > ping for review. sorry for not getting back sooner here. But yey \o/ I finally have dmc and psr working well on my own laptop! so far so good! :) > > Let me know if there's anything that needs to be done, thanks! > > > On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote: > > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > > space requested vblank sequence using this clipped 32-bit count(when the > > value is >= 2^32) as reference, the requested sequence remains a 32-bit > > value and gets queued like that. However, the code that checks if the > > requested sequence has passed compares this against the 64-bit vblank > > count. > > > > Cc: Keith Packard <keithp@keithp.com> > > Cc: Michel Dänzer <michel@daenzer.net> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/drm_vblank.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > index 32d9bcf5be7f..768a8e44d99b 100644 > > --- a/drivers/gpu/drm/drm_vblank.c > > +++ b/drivers/gpu/drm/drm_vblank.c > > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > > } > > > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > { > > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > >
On Thu, 2018-01-18 at 23:36 -0800, Rodrigo Vivi wrote: > On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote: > > drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. > > The effect of this is when drm_wait_vblank_ioctl() tries to widen the user > > space requested vblank sequence using this clipped 32-bit count(when the > > value is >= 2^32) as reference, the requested sequence remains a 32-bit > > value and gets queued like that. However, the code that checks if the > > requested sequence has passed compares this against the 64-bit vblank > > count. > > Worth to mention and probably > Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") > > btw, I spotted at least one more place even with the series applied. > 32 current_vblank; at drm_mode_page_flip_ioctl... > There seem to be several such callers for drm_crtc_vblank_count(). I can fix it up as a follow-up series. > so, probably worth to do a deeper check down to all paths... > > anayway, for this patch: > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Keith Packard <keithp@keithp.com> > > Cc: Michel Dänzer <michel@daenzer.net> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/drm_vblank.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > index 32d9bcf5be7f..768a8e44d99b 100644 > > --- a/drivers/gpu/drm/drm_vblank.c > > +++ b/drivers/gpu/drm/drm_vblank.c > > @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, > > store_vblank(dev, pipe, diff, t_vblank, cur_vblank); > > } > > > > -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) > > { > > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > > > > -- > > 2.11.0 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> writes: > drm_vblank_count() has a u32 type returning what is a 64-bit vblank > count. It looks like a general review of the 64-bit widening patch is needed. * drm_crtc_accurate_vblank_count has a 32-bit return, and uses a 32-bit temporary * drm_wait_one_vblank uses a 32-bit temporary. I looked at every 'u32' in drm_vblank.c; it would be good to have more eyes check this. Thanks for finding the first one.
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); } -static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count. Cc: Keith Packard <keithp@keithp.com> Cc: Michel Dänzer <michel@daenzer.net> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)