RE: [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.wallmen...@gmail.com]
Sent: Friday, April 13, 2018 1:05 AM
To: Wentland, Harry <harry.wentl...@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Koo, Anthony <anthony@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.wentl...@amd.com<mailto:harry.wentl...@amd.com>> skrev:
From: Anthony Koo <anthony@amd.com<mailto:anthony@amd.com>>

Signed-off-by: Anthony Koo <anthony@amd.com<mailto:anthony@amd.com>>
Reviewed-by: Aric Cyr <aric@amd.com<mailto:aric@amd.com>>
Acked-by: Harry Wentland <harry.wentl...@amd.com<mailto:harry.wentl...@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, 100);
+   min_refresh_cap_in_uhz /= 100;
+   max_refresh_cap_in_uhz /= 100;
+   min_refresh_request_in_uhz /= 100;
+   max_refresh_request_in_uhz /= 100;
+
// 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 < 
1000))
+   (max_refresh_request_in_uhz - min_refresh_request_in_uhz < 10))
return false;

return true;
--
2.15.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Nils Wallménius]

Den tis 10 apr. 2018 23:11Harry Wentland  skrev:

> From: Anthony Koo 
>
> Signed-off-by: Anthony Koo 
> Reviewed-by: Aric Cyr 
> Acked-by: Harry Wentland 
> ---
>  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,
> 100);
> +   min_refresh_cap_in_uhz /= 100;
> +   max_refresh_cap_in_uhz /= 100;
> +   min_refresh_request_in_uhz /= 100;
> +   max_refresh_request_in_uhz /= 100;
> +
> // 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 <
> 1000))
> +   (max_refresh_request_in_uhz - min_refresh_request_in_uhz <
> 10))
> return false;
>
> return true;
> --
> 2.15.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx