From: Ville Syrjälä ville.syrjala@linux.intel.com
The new >8k CEA modes have dotclocks reaching 5.94 GHz, which means our clock*1000 will now overflow the 32bit unsigned integer. Switch to 64bit maths to avoid it.
Cc: stable@vger.kernel.org Reported-by: Randy Dunlap rdunlap@infradead.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- An interesting question how many other place might suffer from similar overflows. I think i915 should be mostly OK. The one place I know we use Hz instead kHz is the hsw DPLL code, which I would prefer we also change to use kHz. The other concern is whether we have any potential overflows before we check this against the platform's max dotclock.
I do have this unreviewed igt series https://patchwork.freedesktop.org/series/69531/ which extends the current testing with some other forms of invalid modes. Could probably extend that with a mode.clock=INT_MAX test to see if anything else might trip up.
No idea about other drivers.
drivers/gpu/drm/drm_modes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 501b4fe55a3d..511cde5c7fa6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->htotal == 0 || mode->vtotal == 0) return 0;
- num = mode->clock * 1000; + num = mode->clock; den = mode->htotal * mode->vtotal;
if (mode->flags & DRM_MODE_FLAG_INTERLACE) @@ -772,7 +772,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->vscan > 1) den *= mode->vscan;
- return DIV_ROUND_CLOSEST(num, den); + return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(num, 1000), den); } EXPORT_SYMBOL(drm_mode_vrefresh);
On 10/22/20 12:42 PM, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The new >8k CEA modes have dotclocks reaching 5.94 GHz, which means our clock*1000 will now overflow the 32bit unsigned integer. Switch to 64bit maths to avoid it.
Cc: stable@vger.kernel.org Reported-by: Randy Dunlap rdunlap@infradead.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
This cures the problem that I reported. Thanks.
Tested-by: Randy Dunlap rdunlap@infradead.org
An interesting question how many other place might suffer from similar overflows. I think i915 should be mostly OK. The one place I know we use Hz instead kHz is the hsw DPLL code, which I would prefer we also change to use kHz. The other concern is whether we have any potential overflows before we check this against the platform's max dotclock.
I do have this unreviewed igt series https://patchwork.freedesktop.org/series/69531/ which extends the current testing with some other forms of invalid modes. Could probably extend that with a mode.clock=INT_MAX test to see if anything else might trip up.
No idea about other drivers.
drivers/gpu/drm/drm_modes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 501b4fe55a3d..511cde5c7fa6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->htotal == 0 || mode->vtotal == 0) return 0;
- num = mode->clock * 1000;
num = mode->clock; den = mode->htotal * mode->vtotal;
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
@@ -772,7 +772,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->vscan > 1) den *= mode->vscan;
- return DIV_ROUND_CLOSEST(num, den);
- return DIV_ROUND_CLOSEST_ULL(mul_u32_u32(num, 1000), den);
} EXPORT_SYMBOL(drm_mode_vrefresh);
Quoting Ville Syrjala (2020-10-22 20:42:56)
From: Ville Syrjälä ville.syrjala@linux.intel.com
The new >8k CEA modes have dotclocks reaching 5.94 GHz, which means our clock*1000 will now overflow the 32bit unsigned integer. Switch to 64bit maths to avoid it.
Cc: stable@vger.kernel.org Reported-by: Randy Dunlap rdunlap@infradead.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
An interesting question how many other place might suffer from similar overflows. I think i915 should be mostly OK. The one place I know we use Hz instead kHz is the hsw DPLL code, which I would prefer we also change to use kHz. The other concern is whether we have any potential overflows before we check this against the platform's max dotclock.
I do have this unreviewed igt series https://patchwork.freedesktop.org/series/69531/ which extends the current testing with some other forms of invalid modes. Could probably extend that with a mode.clock=INT_MAX test to see if anything else might trip up.
No idea about other drivers.
drivers/gpu/drm/drm_modes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 501b4fe55a3d..511cde5c7fa6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->htotal == 0 || mode->vtotal == 0) return 0;
num = mode->clock * 1000;
num = mode->clock; den = mode->htotal * mode->vtotal;
You don't want to promote den to u64 while you are here? We are at 8kx4k, throw in dblscan and some vscan, and we could soon have wacky refresh rates. -Chris
On Fri, Oct 30, 2020 at 02:19:45PM +0000, Chris Wilson wrote:
Quoting Ville Syrjala (2020-10-22 20:42:56)
From: Ville Syrjälä ville.syrjala@linux.intel.com
The new >8k CEA modes have dotclocks reaching 5.94 GHz, which means our clock*1000 will now overflow the 32bit unsigned integer. Switch to 64bit maths to avoid it.
Cc: stable@vger.kernel.org Reported-by: Randy Dunlap rdunlap@infradead.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
An interesting question how many other place might suffer from similar overflows. I think i915 should be mostly OK. The one place I know we use Hz instead kHz is the hsw DPLL code, which I would prefer we also change to use kHz. The other concern is whether we have any potential overflows before we check this against the platform's max dotclock.
I do have this unreviewed igt series https://patchwork.freedesktop.org/series/69531/ which extends the current testing with some other forms of invalid modes. Could probably extend that with a mode.clock=INT_MAX test to see if anything else might trip up.
No idea about other drivers.
drivers/gpu/drm/drm_modes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 501b4fe55a3d..511cde5c7fa6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->htotal == 0 || mode->vtotal == 0) return 0;
num = mode->clock * 1000;
num = mode->clock; den = mode->htotal * mode->vtotal;You don't want to promote den to u64 while you are here? We are at 8kx4k, throw in dblscan and some vscan, and we could soon have wacky refresh rates.
i915 has 16kx8k hard limit currently, and we reject vscan>1 (wish we could also reject DBLSCAN). So we should not hit that, at least not yet. Other drivers might not be so strict I guess.
I have a nagging feeling that other places are in danger of overflows if we try to push the current limits significantly. But I guess no real harm in going full 64bit here, except maybe making it a bit slower.
Quoting Ville Syrjälä (2020-10-30 14:43:46)
On Fri, Oct 30, 2020 at 02:19:45PM +0000, Chris Wilson wrote:
Quoting Ville Syrjala (2020-10-22 20:42:56)
From: Ville Syrjälä ville.syrjala@linux.intel.com
The new >8k CEA modes have dotclocks reaching 5.94 GHz, which means our clock*1000 will now overflow the 32bit unsigned integer. Switch to 64bit maths to avoid it.
Cc: stable@vger.kernel.org Reported-by: Randy Dunlap rdunlap@infradead.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
An interesting question how many other place might suffer from similar overflows. I think i915 should be mostly OK. The one place I know we use Hz instead kHz is the hsw DPLL code, which I would prefer we also change to use kHz. The other concern is whether we have any potential overflows before we check this against the platform's max dotclock.
I do have this unreviewed igt series https://patchwork.freedesktop.org/series/69531/ which extends the current testing with some other forms of invalid modes. Could probably extend that with a mode.clock=INT_MAX test to see if anything else might trip up.
No idea about other drivers.
drivers/gpu/drm/drm_modes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 501b4fe55a3d..511cde5c7fa6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode) if (mode->htotal == 0 || mode->vtotal == 0) return 0;
num = mode->clock * 1000;
num = mode->clock; den = mode->htotal * mode->vtotal;You don't want to promote den to u64 while you are here? We are at 8kx4k, throw in dblscan and some vscan, and we could soon have wacky refresh rates.
i915 has 16kx8k hard limit currently, and we reject vscan>1 (wish we could also reject DBLSCAN). So we should not hit that, at least not yet. Other drivers might not be so strict I guess.
I have a nagging feeling that other places are in danger of overflows if we try to push the current limits significantly. But I guess no real harm in going full 64bit here, except maybe making it a bit slower.
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Randy Dunlap -
Ville Syrjala -
Ville Syrjälä