<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 6/13/2025 3:06 PM, Jani Nikula
wrote:<br>
</div>
<blockquote type="cite" cite="mid:0d7742055fbbadf97cc3a361de6838a7d0203f51@intel.com">
<pre wrap="" class="moz-quote-pre">On Fri, 13 Jun 2025, Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
I don't understand how you can make that conclusion. Please explain.
Would it be safer to just use DIV64_U64_ROUND_CLOSEST()?</pre>
</blockquote>
<p>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.</p>
<p>But my reasoning about rounding being unnecessary was flawed, as
any thing in between will indeed matter.</p>
<p>Moreover on checking with spec the algorithm too uses ROUND
function.</p>
<p>You are right, <span style="white-space: pre-wrap">DIV64_U64_ROUND_CLOSEST is a better choice here.</span></p>
<blockquote type="cite" cite="mid:0d7742055fbbadf97cc3a361de6838a7d0203f51@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Fixes: 5947642004bf ("drm/i915/display: Add support for SNPS PHY HDMI PLL algorithm for DG2")
Cc: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a>
Cc: Suraj Kandpal <a class="moz-txt-link-rfc2396E" href="mailto:suraj.kandpal@intel.com"><suraj.kandpal@intel.com></a>
Cc: Jani Nikula <a class="moz-txt-link-rfc2396E" href="mailto:jani.nikula@intel.com"><jani.nikula@intel.com></a>
Cc: <a class="moz-txt-link-rfc2396E" href="mailto:stable@vger.kernel.org"><stable@vger.kernel.org></a> # v6.15+
Signed-off-by: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a>
---
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));
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p>Agreed. Should there be a separate patch for this?</p>
<p>Regards,</p>
<p>Ankit</p>
<blockquote type="cite" cite="mid:0d7742055fbbadf97cc3a361de6838a7d0203f51@intel.com">
<pre wrap="" class="moz-quote-pre">
BR,
Jani.
</pre>
</blockquote>
</body>
</html>