| Message ID | 20180203051302.9974-6-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <keithp@keithp.com> > Cc: Thierry Reding <treding@nvidia.com> Thierry, Can I get an Ack on this please? > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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)
On Wed, Feb 07, 2018 at 01:41:18AM +0000, 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 <keithp@keithp.com> > > Cc: Thierry Reding <treding@nvidia.com> > > > Thierry, > > Can I get an Ack on this please? > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > 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
On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote: > On Wed, Feb 07, 2018 at 01:41:18AM +0000, 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 <keithp@keithp.com> > > > Cc: Thierry Reding <treding@nvidia.com> > > > > > > Thierry, > > > > Can I get an Ack on this please? > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > 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
On Wed, Feb 07, 2018 at 09:32:35PM +0000, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote: > > On Wed, Feb 07, 2018 at 01:41:18AM +0000, 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 <keithp@keithp.com> > > > > Cc: Thierry Reding <treding@nvidia.com> > > > > > > > > > Thierry, > > > > > > Can I get an Ack on this please? > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > --- > > > > 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
Rodrigo Vivi <rodrigo.vivi@intel.com> writes: > On Wed, Feb 07, 2018 at 09:32:35PM +0000, Pandiyan, Dhinakaran wrote: >> >> >> >> On Wed, 2018-02-07 at 10:41 +0100, Thierry Reding wrote: >> > On Wed, Feb 07, 2018 at 01:41:18AM +0000, 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 <keithp@keithp.com> >> > > > Cc: Thierry Reding <treding@nvidia.com> >> > > >> > > >> > > Thierry, >> > > >> > > Can I get an Ack on this please? >> > > >> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> > > > --- >> > > > 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
On Wed, Feb 07, 2018 at 01:41:18AM +0000, 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 <keithp@keithp.com> > > Cc: Thierry Reding <treding@nvidia.com> > > > Thierry, > > Can I get an Ack on this please? Acked-by: Thierry Reding <treding@nvidia.com>
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)
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 <keithp@keithp.com> Cc: Thierry Reding <treding@nvidia.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/tegra/dc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)