Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Keith Packard]

Dhinakaran Pandiyan  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.

-- 
-keith


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

On Thu, 2018-01-18 at 23:36 -0800, Rodrigo Vivi wrote:
> On Fri, Jan 12, 2018 at 09:57:03PM +, 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 
> 
> > 
> > Cc: Keith Packard 
> > Cc: Michel Dänzer 
> > Cc: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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 = >vblank[pipe];
> >  
> > -- 
> > 2.11.0
> > 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Rodrigo Vivi]

On Fri, Jan 19, 2018 at 04:53:34AM +, 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 
> > Cc: Michel Dänzer 
> > Cc: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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 = >vblank[pipe];
> >  
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Rodrigo Vivi]

On Fri, Jan 12, 2018 at 09:57:03PM +, 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 

> 
> Cc: Keith Packard 
> Cc: Michel Dänzer 
> Cc: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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 = >vblank[pipe];
>  
> -- 
> 2.11.0
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

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 
> Cc: Michel Dänzer 
> Cc: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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 = >vblank[pipe];
>  
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Pandiyan, Dhinakaran]

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 
> > Cc: Michel Dänzer 
> > Cc: Daniel Vetter 
> > Signed-off-by: Dhinakaran Pandiyan 
> 
> 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 = >vblank[pipe];
> >  
> > -- 
> > 2.11.0
> > 
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Daniel Vetter]

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 
> Cc: Michel Dänzer 
> Cc: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 

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 = >vblank[pipe];
>  
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()

[Dhinakaran Pandiyan]

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 
Cc: Michel Dänzer 
Cc: Daniel Vetter 
Signed-off-by: Dhinakaran Pandiyan 
---
 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 = >vblank[pipe];
 
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel