[PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Ankit Nautiyal]

DIV_ROUND_CLOSEST_ULL uses do_div(), which expects a 32-bit divisor.
When passing a 64-bit constant like CURVE2_MULTIPLIER, the value is
silently truncated to u32, potentially leading to incorrect results
on large divisors.

Replace DIV_ROUND_CLOSEST_ULL with div64_u64(), which correctly
handles full 64-bit division. Since the result is clamped between
1 and 127, rounding is unnecessary and truncating division
is sufficient.

Fixes: 5947642004bf ("drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2")
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: <stable@vger.kernel.org> # v6.15+
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
index 74bb3bedf30f..ac609bdf6653 100644
--- a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
+++ b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
@@ -103,8 +103,8 @@ static void get_ana_cp_int_prop(u64 vco_clk,
 			    DIV_ROUND_DOWN_ULL(curve_1_interpolated, CURVE0_MULTIPLIER)));
 
 	ana_cp_int_temp =
-		DIV_ROUND_CLOSEST_ULL(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
-				      CURVE2_MULTIPLIER);
+		div64_u64(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
+			  CURVE2_MULTIPLIER);
 
 	*ana_cp_int = max(1, min(ana_cp_int_temp, 127));
 
-- 
2.45.2

RE: [PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Kandpal, Suraj]

> -----Original Message-----
> From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>
> Sent: Friday, June 13, 2025 11:43 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org; Kandpal, Suraj <suraj.kandpal@intel.com>;
> jani.nikula@linux.intel.com; stable@vger.kernel.org
> Subject: [PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by
> using div64_u64
> 
> DIV_ROUND_CLOSEST_ULL uses do_div(), which expects a 32-bit divisor.
> When passing a 64-bit constant like CURVE2_MULTIPLIER, the value is silently
> truncated to u32, potentially leading to incorrect results on large divisors.
> 
> Replace DIV_ROUND_CLOSEST_ULL with div64_u64(), which correctly handles
> full 64-bit division. Since the result is clamped between
> 1 and 127, rounding is unnecessary and truncating division is sufficient.
> 
> Fixes: 5947642004bf ("drm/i915/display: Add support for SNPS PHY HDMI PLL
> algorithm for DG2")
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: <stable@vger.kernel.org> # v6.15+
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> index 74bb3bedf30f..ac609bdf6653 100644
> --- a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> +++ b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> @@ -103,8 +103,8 @@ static void get_ana_cp_int_prop(u64 vco_clk,
>  			    DIV_ROUND_DOWN_ULL(curve_1_interpolated,
> CURVE0_MULTIPLIER)));
> 
>  	ana_cp_int_temp =
> -
> 	DIV_ROUND_CLOSEST_ULL(DIV_ROUND_DOWN_ULL(adjusted_vco_cl
> k1, curve_2_scaled1),
> -				      CURVE2_MULTIPLIER);
> +		div64_u64(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1,
> curve_2_scaled1),
> +			  CURVE2_MULTIPLIER);
> 
>  	*ana_cp_int = max(1, min(ana_cp_int_temp, 127));
> 
> --
> 2.45.2

Re: [PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Jani Nikula]

On Fri, 13 Jun 2025, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> DIV_ROUND_CLOSEST_ULL uses do_div(), which expects a 32-bit divisor.
> When passing a 64-bit constant like CURVE2_MULTIPLIER, the value is
> silently truncated to u32, potentially leading to incorrect results
> on large divisors.
>
> Replace DIV_ROUND_CLOSEST_ULL with div64_u64(), which correctly
> handles full 64-bit division. Since the result is clamped between
> 1 and 127, rounding is unnecessary and truncating division
> is sufficient.

I don't understand how you can make that conclusion. Please explain.

Would it be safer to just use DIV64_U64_ROUND_CLOSEST()?

> Fixes: 5947642004bf ("drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2")
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: <stable@vger.kernel.org> # v6.15+
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> index 74bb3bedf30f..ac609bdf6653 100644
> --- a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> +++ b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
> @@ -103,8 +103,8 @@ static void get_ana_cp_int_prop(u64 vco_clk,
>  			    DIV_ROUND_DOWN_ULL(curve_1_interpolated, CURVE0_MULTIPLIER)));
>  
>  	ana_cp_int_temp =
> -		DIV_ROUND_CLOSEST_ULL(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
> -				      CURVE2_MULTIPLIER);
> +		div64_u64(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
> +			  CURVE2_MULTIPLIER);
>  
>  	*ana_cp_int = max(1, min(ana_cp_int_temp, 127));

Unrelated to this patch, but this should be:

	*ana_cp_int = clamp(ana_cp_int_temp, 1, 127);

There's a similar issue with ana_cp_prop also in the file.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Nautiyal, Ankit K]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]


On 6/13/2025 3:06 PM, Jani Nikula wrote:
> On Fri, 13 Jun 2025, Ankit Nautiyal<ankit.k.nautiyal@intel.com> wrote:
>> DIV_ROUND_CLOSEST_ULL uses do_div(), which expects a 32-bit divisor.
>> When passing a 64-bit constant like CURVE2_MULTIPLIER, the value is
>> silently truncated to u32, potentially leading to incorrect results
>> on large divisors.
>>
>> Replace DIV_ROUND_CLOSEST_ULL with div64_u64(), which correctly
>> handles full 64-bit division. Since the result is clamped between
>> 1 and 127, rounding is unnecessary and truncating division
>> is sufficient.
> I don't understand how you can make that conclusion. Please explain.
>
> Would it be safer to just use DIV64_U64_ROUND_CLOSEST()?

I was thinking in terms of the boundary values that less than 1 will be 
rounded to 1 and anything > 127 will round to 127.

But my reasoning about rounding being unnecessary was flawed, as any 
thing in between will indeed matter.

Moreover on checking with spec the algorithm too uses ROUND function.

You are right, DIV64_U64_ROUND_CLOSEST is a better choice here.

>
>> Fixes: 5947642004bf ("drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2")
>> Cc: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>> Cc: Suraj Kandpal<suraj.kandpal@intel.com>
>> Cc: Jani Nikula<jani.nikula@intel.com>
>> Cc:<stable@vger.kernel.org> # v6.15+
>> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
>> index 74bb3bedf30f..ac609bdf6653 100644
>> --- a/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
>> +++ b/drivers/gpu/drm/i915/display/intel_snps_hdmi_pll.c
>> @@ -103,8 +103,8 @@ static void get_ana_cp_int_prop(u64 vco_clk,
>>   			    DIV_ROUND_DOWN_ULL(curve_1_interpolated, CURVE0_MULTIPLIER)));
>>   
>>   	ana_cp_int_temp =
>> -		DIV_ROUND_CLOSEST_ULL(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
>> -				      CURVE2_MULTIPLIER);
>> +		div64_u64(DIV_ROUND_DOWN_ULL(adjusted_vco_clk1, curve_2_scaled1),
>> +			  CURVE2_MULTIPLIER);
>>   
>>   	*ana_cp_int = max(1, min(ana_cp_int_temp, 127));
> Unrelated to this patch, but this should be:
>
> 	*ana_cp_int = clamp(ana_cp_int_temp, 1, 127);
>
> There's a similar issue with ana_cp_prop also in the file.
>
Agreed. Should there be a separate patch for this?

Regards,

Ankit

> BR,
> Jani.
>
>

[-- Attachment #2: Type: text/html, Size: 4020 bytes --]

Re: [PATCH] drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Jani Nikula]

On Sun, 15 Jun 2025, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> On 6/13/2025 3:06 PM, Jani Nikula wrote:
>> On Fri, 13 Jun 2025, Ankit Nautiyal<ankit.k.nautiyal@intel.com> wrote:
>>>   	*ana_cp_int = max(1, min(ana_cp_int_temp, 127));
>> Unrelated to this patch, but this should be:
>>
>> 	*ana_cp_int = clamp(ana_cp_int_temp, 1, 127);
>>
>> There's a similar issue with ana_cp_prop also in the file.
>>
> Agreed. Should there be a separate patch for this?

Yes. That's why I emphasized "unrelated to this patch". ;)

BR,
Jani.


-- 
Jani Nikula, Intel

✗ i915.CI.BAT: failure for drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3767 bytes --]

== Series Details ==

Series: drm/i915/snps_hdmi_pll: Fix 64-bit divisor truncation by using div64_u64
URL   : https://patchwork.freedesktop.org/series/150203/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16700 -> Patchwork_150203v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_150203v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_150203v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/index.html

Participating hosts (43 -> 42)
------------------------------

  Additional (1): bat-jsl-1 
  Missing    (2): bat-arlh-2 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_150203v1:

### IGT changes ###

#### Possible regressions ####

  * igt@prime_self_import@basic-with_one_bo:
    - bat-jsl-1:          NOTRUN -> [ABORT][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@prime_self_import@basic-with_one_bo.html

  
Known issues
------------

  Here are the changes found in Patchwork_150203v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - bat-jsl-1:          NOTRUN -> [SKIP][2] ([i915#2190])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@gem_huc_copy@huc-copy.html

  * igt@intel_hwmon@hwmon-read:
    - bat-jsl-1:          NOTRUN -> [SKIP][3] ([i915#7707]) +1 other test skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@intel_hwmon@hwmon-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-jsl-1:          NOTRUN -> [SKIP][4] ([i915#4103]) +1 other test skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-jsl-1:          NOTRUN -> [SKIP][5] ([i915#3555] / [i915#9886])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-jsl-1:          NOTRUN -> [SKIP][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-jsl-1:          NOTRUN -> [SKIP][7] ([i915#3555])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/bat-jsl-1/igt@kms_setmode@basic-clone-single-crtc.html

  
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#7707]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7707
  [i915#9886]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9886


Build changes
-------------

  * Linux: CI_DRM_16700 -> Patchwork_150203v1

  CI-20190529: 20190529
  CI_DRM_16700: cce8a9af1c6cf1776511aa69e5f4b5bef7bf5938 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8410: 5826cdbf1cb8f5ec8a42bae33deb6b2b63e59e6e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_150203v1: cce8a9af1c6cf1776511aa69e5f4b5bef7bf5938 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_150203v1/index.html

[-- Attachment #2: Type: text/html, Size: 4623 bytes --]