[Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
return type for drm_crtc_vblank_count() to u64. This could cause
potential problems if the return value is used in arithmetic operations
with a 32-bit reference HW vblank count. Explicitly typecasting this
down to u32 either fixes a potential problem or serves to add clarity in
case the implicit typecasting was already correct.
Cc: Keith Packard
Cc: Thierry Reding
Signed-off-by: Dhinakaran Pandiyan
---
drivers/gpu/drm/tegra/dc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b8403ed48285..49df2db2ad46 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc
*crtc)
return host1x_syncpt_read(dc->syncpt);
/* fallback to software emulated VBLANK counter */
- return drm_crtc_vblank_count(&dc->base);
+ return (u32)drm_crtc_vblank_count(&dc->base);
}
static int tegra_dc_enable_vblank(struct drm_crtc *crtc)
--
2.14.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> return type for drm_crtc_vblank_count() to u64. This could cause
> potential problems if the return value is used in arithmetic operations
> with a 32-bit reference HW vblank count. Explicitly typecasting this
> down to u32 either fixes a potential problem or serves to add clarity in
> case the implicit typecasting was already correct.
>
> Cc: Keith Packard
> Cc: Thierry Reding
Thierry,
Can I get an Ack on this please?
> Signed-off-by: Dhinakaran Pandiyan
> ---
> drivers/gpu/drm/tegra/dc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index b8403ed48285..49df2db2ad46 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct drm_crtc
> *crtc)
> return host1x_syncpt_read(dc->syncpt);
>
> /* fallback to software emulated VBLANK counter */
> - return drm_crtc_vblank_count(&dc->base);
> + return (u32)drm_crtc_vblank_count(&dc->base);
> }
>
> static int tegra_dc_enable_vblank(struct drm_crtc *crtc)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
On Wed, Feb 07, 2018 at 01:41:18AM +, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > return type for drm_crtc_vblank_count() to u64. This could cause
> > potential problems if the return value is used in arithmetic operations
> > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > down to u32 either fixes a potential problem or serves to add clarity in
> > case the implicit typecasting was already correct.
> >
> > Cc: Keith Packard
> > Cc: Thierry Reding
>
>
> Thierry,
>
> Can I get an Ack on this please?
>
> > Signed-off-by: Dhinakaran Pandiyan
> > ---
> > drivers/gpu/drm/tegra/dc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index b8403ed48285..49df2db2ad46 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct
> > drm_crtc *crtc)
> > return host1x_syncpt_read(dc->syncpt);
> >
> > /* fallback to software emulated VBLANK counter */
> > - return drm_crtc_vblank_count(&dc->base);
> > + return (u32)drm_crtc_vblank_count(&dc->base);
Isn't this the wrong way around? Shouldn't we instead make the
->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
> On Wed, Feb 07, 2018 at 01:41:18AM +, Pandiyan, Dhinakaran wrote:
> > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > > return type for drm_crtc_vblank_count() to u64. This could cause
> > > potential problems if the return value is used in arithmetic operations
> > > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > > down to u32 either fixes a potential problem or serves to add clarity in
> > > case the implicit typecasting was already correct.
> > >
> > > Cc: Keith Packard
> > > Cc: Thierry Reding
> >
> >
> > Thierry,
> >
> > Can I get an Ack on this please?
> >
> > > Signed-off-by: Dhinakaran Pandiyan
> > > ---
> > > drivers/gpu/drm/tegra/dc.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > index b8403ed48285..49df2db2ad46 100644
> > > --- a/drivers/gpu/drm/tegra/dc.c
> > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct
> > > drm_crtc *crtc)
> > > return host1x_syncpt_read(dc->syncpt);
> > >
> > > /* fallback to software emulated VBLANK counter */
> > > - return drm_crtc_vblank_count(&dc->base);
> > > + return (u32)drm_crtc_vblank_count(&dc->base);
>
> Isn't this the wrong way around? Shouldn't we instead make the
> ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
Here's how I understand this -
To use the software emulated vblank counter, the driver should set
max_vblank_count = 0 and the core takes care of calculating elapsed
vblanks.
->get_vblank_counter() is meant to return the hardware counter if
available, which would be a 32-bit value. Hence the explicit typecast to
32-bit for the fallback case too.
Having said that, I believe drm_crtc_accurate_vblank_count() is the
appropriate function for fallback.
-DK
>
> Thierry
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
On Wed, Feb 07, 2018 at 09:32:35PM +, Pandiyan, Dhinakaran wrote:
>
>
>
> On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
> > On Wed, Feb 07, 2018 at 01:41:18AM +, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > > > return type for drm_crtc_vblank_count() to u64. This could cause
> > > > potential problems if the return value is used in arithmetic operations
> > > > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > > > down to u32 either fixes a potential problem or serves to add clarity in
> > > > case the implicit typecasting was already correct.
> > > >
> > > > Cc: Keith Packard
> > > > Cc: Thierry Reding
> > >
> > >
> > > Thierry,
> > >
> > > Can I get an Ack on this please?
> > >
> > > > Signed-off-by: Dhinakaran Pandiyan
> > > > ---
> > > > drivers/gpu/drm/tegra/dc.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > > index b8403ed48285..49df2db2ad46 100644
> > > > --- a/drivers/gpu/drm/tegra/dc.c
> > > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct
> > > > drm_crtc *crtc)
> > > > return host1x_syncpt_read(dc->syncpt);
> > > >
> > > > /* fallback to software emulated VBLANK counter */
> > > > - return drm_crtc_vblank_count(&dc->base);
> > > > + return (u32)drm_crtc_vblank_count(&dc->base);
> >
> > Isn't this the wrong way around? Shouldn't we instead make the
> > ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
>
> Here's how I understand this -
>
> To use the software emulated vblank counter, the driver should set
> max_vblank_count = 0 and the core takes care of calculating elapsed
> vblanks.
>
> ->get_vblank_counter() is meant to return the hardware counter if
> available, which would be a 32-bit value. Hence the explicit typecast to
> 32-bit for the fallback case too.
>
> Having said that, I believe drm_crtc_accurate_vblank_count() is the
> appropriate function for fallback.
Hi Thierry,
any further concerns or thoughts here?
I'd like to merge all together on drm-intel since the ones
around us is kind of blocking us.
Thanks,
Rodrigo.
>
> -DK
>
>
>
> >
> > Thierry
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
Rodrigo Vivi writes:
> On Wed, Feb 07, 2018 at 09:32:35PM +, Pandiyan, Dhinakaran wrote:
>>
>>
>>
>> On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote:
>> > On Wed, Feb 07, 2018 at 01:41:18AM +, Pandiyan, Dhinakaran wrote:
>> > > On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
>> > > > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
>> > > > return type for drm_crtc_vblank_count() to u64. This could cause
>> > > > potential problems if the return value is used in arithmetic operations
>> > > > with a 32-bit reference HW vblank count. Explicitly typecasting this
>> > > > down to u32 either fixes a potential problem or serves to add clarity
>> > > > in
>> > > > case the implicit typecasting was already correct.
>> > > >
>> > > > Cc: Keith Packard
>> > > > Cc: Thierry Reding
>> > >
>> > >
>> > > Thierry,
>> > >
>> > > Can I get an Ack on this please?
>> > >
>> > > > Signed-off-by: Dhinakaran Pandiyan
>> > > > ---
>> > > > drivers/gpu/drm/tegra/dc.c | 2 +-
>> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> > > > index b8403ed48285..49df2db2ad46 100644
>> > > > --- a/drivers/gpu/drm/tegra/dc.c
>> > > > +++ b/drivers/gpu/drm/tegra/dc.c
>> > > > @@ -1359,7 +1359,7 @@ static u32 tegra_dc_get_vblank_counter(struct
>> > > > drm_crtc *crtc)
>> > > >return host1x_syncpt_read(dc->syncpt);
>> > > >
>> > > >/* fallback to software emulated VBLANK counter */
>> > > > - return drm_crtc_vblank_count(&dc->base);
>> > > > + return (u32)drm_crtc_vblank_count(&dc->base);
>> >
>> > Isn't this the wrong way around? Shouldn't we instead make the
>> > ->get_vblank_counter() callback return u64 like drm_crtc_vblank_count()?
>>
>> Here's how I understand this -
>>
>> To use the software emulated vblank counter, the driver should set
>> max_vblank_count = 0 and the core takes care of calculating elapsed
>> vblanks.
>>
>> ->get_vblank_counter() is meant to return the hardware counter if
>> available, which would be a 32-bit value. Hence the explicit typecast to
>> 32-bit for the fallback case too.
>>
>> Having said that, I believe drm_crtc_accurate_vblank_count() is the
>> appropriate function for fallback.
>
> Hi Thierry,
>
> any further concerns or thoughts here?
>
> I'd like to merge all together on drm-intel since the ones
> around us is kind of blocking us.
>
Hi Thierry,
I was taking a deeper look to the code here and talking to DK.
This patch only aims to bring more clarity to where the crops are happening.
Furthermore making the whole function to return u64 would have
the same effect since it would get cropped one level above.
I believe you are the best one to make the choice for one
way over another, or none, but the result would be the same.
Since this has no functional impact, I'm planing to move with
other patches but leaving this one behind so you can decide later.
If you still have any concerns please let me know.
Thanks,
Rodrigo,
> Thanks,
> Rodrigo.
>
>>
>> -DK
>>
>>
>>
>> >
>> > Thierry
>> > ___
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] drm/tegra: Handle 64-bit return from drm_crtc_vblank_count()
On Wed, Feb 07, 2018 at 01:41:18AM +, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-02 at 21:12 -0800, Dhinakaran Pandiyan wrote:
> > 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]") changed the
> > return type for drm_crtc_vblank_count() to u64. This could cause
> > potential problems if the return value is used in arithmetic operations
> > with a 32-bit reference HW vblank count. Explicitly typecasting this
> > down to u32 either fixes a potential problem or serves to add clarity in
> > case the implicit typecasting was already correct.
> >
> > Cc: Keith Packard
> > Cc: Thierry Reding
>
>
> Thierry,
>
> Can I get an Ack on this please?
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
