Re: [PATCH] drm/sun4i: hdmi: Fix u64 div on 32bit arch
On Mon, 04 Mar 2024 10:12:25 +0100, Maxime Ripard wrote:
> Commit 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
> mode_valid") changed the clock rate from an unsigned long to an unsigned
> long long resulting in a a 64-bit division that might not be supported
> on all platforms.
>
> The resulted in compilation being broken at least for m68k, xtensa and
> some arm configurations, at least.
>
> [...]
Applied to misc/kernel.git (drm-misc-next-fixes).
Thanks!
Maxime
Re: [PATCH] drm/sun4i: hdmi: Fix u64 div on 32bit arch
On Mon, Mar 04, 2024 at 11:05:14AM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> Thanks for your patch!
>
> On Mon, Mar 4, 2024 at 10:12 AM Maxime Ripard wrote:
> > Commit 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
> > mode_valid") changed the clock rate from an unsigned long to an unsigned
> > long long resulting in a a 64-bit division that might not be supported
> > on all platforms.
>
> Why was this changed to unsigned long long?
> Can a valid pixel clock really not fit in 32-bit?
Yes, HDMI 2.1 supports pixel rates up until 5.940GHz, so the framework
has to use that.
> > The resulted in compilation being broken at least for m68k, xtensa and
> > some arm configurations, at least.
> >
> > Fixes: 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
> > mode_valid")
> > Reported-by: Geert Uytterhoeven
> > Reported-by: Naresh Kamboju
> > Closes:
> > https://lore.kernel.org/r/CA+G9fYvG9KE15PGNoLu+SBVyShe+u5HBLQ81+kK9Zop6u=y...@mail.gmail.com/
> > Reported-by: kernel test robot
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202403011839.klixh4wc-...@intel.com/
> > Signed-off-by: Maxime Ripard
>
> > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> > @@ -163,11 +163,11 @@ static enum drm_mode_status
> > sun4i_hdmi_connector_clock_valid(const struct drm_connector *connector,
> > const struct drm_display_mode *mode,
> > unsigned long long clock)
> > {
> > const struct sun4i_hdmi *hdmi =
> > drm_connector_to_sun4i_hdmi(connector);
> > - unsigned long diff = clock / 200; /* +-0.5% allowed by HDMI spec */
> > + unsigned long diff = div_u64(clock, 200); /* +-0.5% allowed by HDMI
> > spec */
>
> I'd rather see clock changed back to unsigned long.
No, the tolerance we allow is an unsigned long. The tolerance is 0.5% of
the pixel clock, so even if that controller supported HDMI 2.1 at its
full capacity (it doesn't, at all), it would be 29.7 MHz, which fits
comfortably in an unsigned long.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm/sun4i: hdmi: Fix u64 div on 32bit arch
Hi Maxime,
Thanks for your patch!
On Mon, Mar 4, 2024 at 10:12 AM Maxime Ripard wrote:
> Commit 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
> mode_valid") changed the clock rate from an unsigned long to an unsigned
> long long resulting in a a 64-bit division that might not be supported
> on all platforms.
Why was this changed to unsigned long long?
Can a valid pixel clock really not fit in 32-bit?
> The resulted in compilation being broken at least for m68k, xtensa and
> some arm configurations, at least.
>
> Fixes: 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
> mode_valid")
> Reported-by: Geert Uytterhoeven
> Reported-by: Naresh Kamboju
> Closes:
> https://lore.kernel.org/r/CA+G9fYvG9KE15PGNoLu+SBVyShe+u5HBLQ81+kK9Zop6u=y...@mail.gmail.com/
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403011839.klixh4wc-...@intel.com/
> Signed-off-by: Maxime Ripard
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -163,11 +163,11 @@ static enum drm_mode_status
> sun4i_hdmi_connector_clock_valid(const struct drm_connector *connector,
> const struct drm_display_mode *mode,
> unsigned long long clock)
> {
> const struct sun4i_hdmi *hdmi =
> drm_connector_to_sun4i_hdmi(connector);
> - unsigned long diff = clock / 200; /* +-0.5% allowed by HDMI spec */
> + unsigned long diff = div_u64(clock, 200); /* +-0.5% allowed by HDMI
> spec */
I'd rather see clock changed back to unsigned long.
> long rounded_rate;
>
> if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> return MODE_BAD;
>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH] drm/sun4i: hdmi: Fix u64 div on 32bit arch
Commit 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and
mode_valid") changed the clock rate from an unsigned long to an unsigned
long long resulting in a a 64-bit division that might not be supported
on all platforms.
The resulted in compilation being broken at least for m68k, xtensa and
some arm configurations, at least.
Fixes: 358e76fd613a ("drm/sun4i: hdmi: Consolidate atomic_check and mode_valid")
Reported-by: Geert Uytterhoeven
Reported-by: Naresh Kamboju
Closes:
https://lore.kernel.org/r/CA+G9fYvG9KE15PGNoLu+SBVyShe+u5HBLQ81+kK9Zop6u=y...@mail.gmail.com/
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202403011839.klixh4wc-...@intel.com/
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index b7cf369b1906..987041850df2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -163,11 +163,11 @@ static enum drm_mode_status
sun4i_hdmi_connector_clock_valid(const struct drm_connector *connector,
const struct drm_display_mode *mode,
unsigned long long clock)
{
const struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
- unsigned long diff = clock / 200; /* +-0.5% allowed by HDMI spec */
+ unsigned long diff = div_u64(clock, 200); /* +-0.5% allowed by HDMI
spec */
long rounded_rate;
if (mode->flags & DRM_MODE_FLAG_DBLCLK)
return MODE_BAD;
--
2.43.2
