[PATCH 10/21] drm/amd/display: Add back code to allow for rounding error

[Koo, Anthony]

Thanks for the comments.

The input parameters are intended as uHz, but units do get translated within the function. We did not want to create new unnecessary parameters on the stack, which led up to the slightly confusing units. Maybe a better comment here would also help.
And indeed, the comment about ceil needs to be fixed up.

Thanks,
Anthony

From: Nils Wallménius [mailto:nils.wallmenius at gmail.com]
Sent: Friday, April 13, 2018 1:05 AM
To: Wentland, Harry <Harry.Wentland at amd.com>
Cc: amd-gfx at lists.freedesktop.org; Koo, Anthony <Anthony.Koo at amd.com>
Subject: Re: [PATCH 10/21] drm/amd/display: Add back code to allow for rounding error


Den tis 10 apr. 2018 23:11Harry Wentland <harry.wentland at amd.com<mailto:harry.wentland at amd.com>> skrev:
From: Anthony Koo <Anthony.Koo at amd.com<mailto:Anthony.Koo at amd.com>>

Signed-off-by: Anthony Koo <Anthony.Koo at amd.com<mailto:Anthony.Koo at amd.com>>
Reviewed-by: Aric Cyr <Aric.Cyr at amd.com<mailto:Aric.Cyr at amd.com>>
Acked-by: Harry Wentland <harry.wentland at amd.com<mailto:harry.wentland at amd.com>>
---
 drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index 4887c888bbe7..abd5c9374eb3 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -896,6 +896,17 @@ bool mod_freesync_is_valid_range(struct mod_freesync *mod_freesync,
        unsigned long long nominal_field_rate_in_uhz =
                        mod_freesync_calc_nominal_field_rate(stream);

+       /* Allow for some rounding error of actual video timing by taking ceil.
+        * For example, 144 Hz mode timing may actually be 143.xxx Hz when
+        * calculated from pixel rate and vertical/horizontal totals, but
+        * this should be allowed instead of blocking FreeSync.
+        */

Hi, with this change the var names ending in *_in_uhz are a bit confusing, also the integer division will not round up (take ceil) as seems to be the intention from the above comment. Perhaps the comment needs to be improved?

BR
Nils


+       nominal_field_rate_in_uhz = div_u64(nominal_field_rate_in_uhz, 1000000);
+       min_refresh_cap_in_uhz /= 1000000;
+       max_refresh_cap_in_uhz /= 1000000;
+       min_refresh_request_in_uhz /= 1000000;
+       max_refresh_request_in_uhz /= 1000000;
+
        // Check nominal is within range
        if (nominal_field_rate_in_uhz > max_refresh_cap_in_uhz ||
                nominal_field_rate_in_uhz < min_refresh_cap_in_uhz)
@@ -921,7 +932,7 @@ bool mod_freesync_is_valid_range(struct mod_freesync *mod_freesync,

        // For variable range, check for at least 10 Hz range
        if ((max_refresh_request_in_uhz != min_refresh_request_in_uhz) &&
-               (max_refresh_request_in_uhz - min_refresh_request_in_uhz < 10000000))
+               (max_refresh_request_in_uhz - min_refresh_request_in_uhz < 10))
                return false;

        return true;
--
2.15.1

_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180413/58fd6fe2/attachment.html>