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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

[-- 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 --]