[01/10] drm/vblank: Data type fixes for 64-bit vblank sequences.

Commit Message

Dhinakaran Pandiyan Feb. 3, 2018, 5:12 a.m. UTC

From: "Pandiyan, Dhinakaran" <[e-mail]>

drm_vblank_count() has an 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.

With drm_vblank_count() returning all bits of the vblank count, update
drm_crtc_accurate_vblank_count() so that drm_crtc_arm_vblank_event() queues
the correct sequence. Otherwise, this leads to prolonged waits for a vblank
sequence when the current count is >=2^32.

Finally, fix drm_wait_one_vblank() too.

v2: Commit message fix (Keith)
    Squash commits (Rodrigo)

Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
Cc: Keith Packard <[e-mail]>
Cc: Michel Dänzer <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Signed-off-by: Dhinakaran Pandiyan <[e-mail]>
Acked-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/drm_vblank.c | 8 ++++----
 include/drm/drm_vblank.h     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Keith Packard Feb. 3, 2018, 8:14 a.m. UTC | #1

Dhinakaran Pandiyan <[e-mail]> writes:

> From: "Pandiyan, Dhinakaran" <[e-mail]>
>
> drm_vblank_count() has an 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.

For patches 1-7:

Reviewed-by: Keith Packard <[e-mail]>

Rodrigo Vivi Feb. 5, 2018, 8:32 p.m. UTC | #2

On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> Dhinakaran Pandiyan <[e-mail]> writes:
> 
> > From: "Pandiyan, Dhinakaran" <[e-mail]>
> >
> > drm_vblank_count() has an 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.
> 
> For patches 1-7:
> 
> Reviewed-by: Keith Packard <[e-mail]>

Dave, ack to merge them through drm-intel-next-queued ?

Patches 8 to 10 are ready as well.

Thanks,
Rodrigo.

> 
> -- 
> -keith

Dave Airlie Feb. 5, 2018, 8:37 p.m. UTC | #3

On 6 February 2018 at 06:32, Rodrigo Vivi <[e-mail]> wrote:
> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
>> Dhinakaran Pandiyan <[e-mail]> writes:
>>
>> > From: "Pandiyan, Dhinakaran" <[e-mail]>
>> >
>> > drm_vblank_count() has an 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.
>>
>> For patches 1-7:
>>
>> Reviewed-by: Keith Packard <[e-mail]>
>
> Dave, ack to merge them through drm-intel-next-queued ?

Ack. do we know if any of those need to be in -fixes?

or too early to tell?

Dave.

Rodrigo Vivi Feb. 5, 2018, 8:49 p.m. UTC | #4

On Mon, Feb 05, 2018 at 08:37:13PM +0000, Dave Airlie wrote:
> On 6 February 2018 at 06:32, Rodrigo Vivi <[e-mail]> wrote:
> > On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
> >> Dhinakaran Pandiyan <[e-mail]> writes:
> >>
> >> > From: "Pandiyan, Dhinakaran" <[e-mail]>
> >> >
> >> > drm_vblank_count() has an 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.
> >>
> >> For patches 1-7:
> >>
> >> Reviewed-by: Keith Packard <[e-mail]>
> >
> > Dave, ack to merge them through drm-intel-next-queued ?
> 
> Ack. do we know if any of those need to be in -fixes?
> 
> or too early to tell?

I didn't checked the drm one close enough yet to decide for that.
DK or Keith? do you guys see anyone suitable for fixes?

For the other work on top I believe we don't need to move to fixes
since psr is still disabled.

> 
> Dave.

Dhinakaran Pandiyan Feb. 5, 2018, 9:11 p.m. UTC | #5

On Mon, 2018-02-05 at 12:49 -0800, Rodrigo Vivi wrote:
> On Mon, Feb 05, 2018 at 08:37:13PM +0000, Dave Airlie wrote:

> > On 6 February 2018 at 06:32, Rodrigo Vivi <[e-mail]> wrote:

> > > On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:

> > >> Dhinakaran Pandiyan <[e-mail]> writes:

> > >>

> > >> > From: "Pandiyan, Dhinakaran" <[e-mail]>

> > >> >

> > >> > drm_vblank_count() has an 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.

> > >>

> > >> For patches 1-7:

> > >>

> > >> Reviewed-by: Keith Packard <[e-mail]>

> > >

> > > Dave, ack to merge them through drm-intel-next-queued ?

> > 

> > Ack. do we know if any of those need to be in -fixes?

> > 

> > or too early to tell?

> 

> I didn't checked the drm one close enough yet to decide for that.

> DK or Keith? do you guys see anyone suitable for fixes?


I was thinking patch 1 would be a candidate. But, it'd need the crtc to
be active continuously for > 2.2 years at 60 frames/s to trigger this.
The problem is exacerbated with PSR and PSR is disabled. So, I think we
can skip -fixes.

> 

> For the other work on top I believe we don't need to move to fixes

> since psr is still disabled.

> 

> > 

> > Dave.

> _______________________________________________

> Intel-gfx mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Keith Packard Feb. 5, 2018, 11:41 p.m. UTC | #6

Rodrigo Vivi <[e-mail]> writes:

> I didn't checked the drm one close enough yet to decide for that.
> DK or Keith? do you guys see anyone suitable for fixes?

Yeah, we should probably get 1, 3 and 7 into fixes. 2, 4, 5 and 6 add
explicit casts where the compiler would already introduce equivalent
implicit casts.

Rodrigo Vivi Feb. 15, 2018, 7:55 p.m. UTC | #7

Dave Airlie <[e-mail]> writes:

> On 6 February 2018 at 06:32, Rodrigo Vivi <[e-mail]> wrote:
>> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:
>>> Dhinakaran Pandiyan <[e-mail]> writes:
>>>
>>> > From: "Pandiyan, Dhinakaran" <[e-mail]>
>>> >
>>> > drm_vblank_count() has an 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.
>>>
>>> For patches 1-7:
>>>
>>> Reviewed-by: Keith Packard <[e-mail]>
>>
>> Dave, ack to merge them through drm-intel-next-queued ?
>
> Ack. do we know if any of those need to be in -fixes?

All patches merged to drm-intel-next-queued.
Thanks for the patches, reviews and acks.

>
> or too early to tell?
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Dhinakaran Pandiyan Feb. 16, 2018, 6:49 p.m. UTC | #8

On Thu, 2018-02-15 at 11:55 -0800, Rodrigo Vivi wrote:
> Dave Airlie <[e-mail]> writes:

> 

> > On 6 February 2018 at 06:32, Rodrigo Vivi <[e-mail]> wrote:

> >> On Sat, Feb 03, 2018 at 08:14:48AM +0000, Keith Packard wrote:

> >>> Dhinakaran Pandiyan <[e-mail]> writes:

> >>>

> >>> > From: "Pandiyan, Dhinakaran" <[e-mail]>

> >>> >

> >>> > drm_vblank_count() has an 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.

> >>>

> >>> For patches 1-7:

> >>>

> >>> Reviewed-by: Keith Packard <[e-mail]>

> >>

> >> Dave, ack to merge them through drm-intel-next-queued ?

> >

> > Ack. do we know if any of those need to be in -fixes?

> 

> All patches merged to drm-intel-next-queued.

> Thanks for the patches, reviews and acks.

> 



Thanks everyone for the reviews and Acks!
-DK

> >

> > or too early to tell?

> >

> > Dave.

> > _______________________________________________

> > dri-devel mailing list

> > [e-mail]

> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

> _______________________________________________

> Intel-gfx mailing list

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..f0d3ed5f2528 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];
 
@@ -292,11 +292,11 @@  static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
  * This is mostly useful for hardware that can obtain the scanout position, but
  * doesn't have a hardware frame counter.
  */
-u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
+u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	unsigned int pipe = drm_crtc_index(crtc);
-	u32 vblank;
+	u64 vblank;
 	unsigned long flags;
 
 	WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
@@ -1055,7 +1055,7 @@  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	int ret;
-	u32 last;
+	u64 last;
 
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return;
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 848b463a0af5..a4c3b0a0a197 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -179,7 +179,7 @@  void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
 void drm_crtc_vblank_off(struct drm_crtc *crtc);
 void drm_crtc_vblank_reset(struct drm_crtc *crtc);
 void drm_crtc_vblank_on(struct drm_crtc *crtc);
-u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
+u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,