From: Colin Ian King <[email protected]>
The assignment to infopacket->sb[7] looks incorrect, the comment states it
is the minimum refresh rate yet it is being assigned a value from the
maximum refresh rate max_refresh_in_uhz. Fix this by using min_refresh_in_uhz
instead.
Addresses-Coverity: ("Copy-paste error")
Fixes: d2bacc38f6ca ("drm/amd/display: Change infopacket type programming")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 2 +-
1 file changed, 1 insertion(+), 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 eb7421e83b86..fe11436536e8 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -587,7 +587,7 @@ static void build_vrr_infopacket_data_v3(const struct mod_vrr_params *vrr,
} else {
// Non-fs case, program nominal range
/* PB7 = FreeSync Minimum refresh rate (Hz) */
- infopacket->sb[7] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
+ infopacket->sb[7] = (unsigned char)((vrr->min_refresh_in_uhz + 500000) / 1000000);
/* PB8 = FreeSync Maximum refresh rate (Hz) */
infopacket->sb[8] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
}
--
2.25.1
Hi Colin,
Sorry for any confusion of this code.
I think in this case, it seems like the comment is wrong (but original implementation is somewhat wrong as well). Probably the original code implementation makes it unclear.
There are three scenarios:
1. Variable refresh active, targeting a fixed rate
In this case, the min = max = fixed rate
2. Variable refresh active, with a variable range
In this case, the min = minimum refresh rate of the range.
And the max = maximum refresh rate of the range.
3. Variable refresh rate is disabled (The case you are modifying)
In the disabled case, we want to indicate to the display that the refresh rate is fixed, so we want to program min = max = the base refresh rate.
Today there seems to be an implication that max refresh = base refresh, which is not necessarily true.
I guess to make the code more clear and correct, the min and max should both be programmed equal to the base refresh rate (nominal field rate from mod_freesync_calc_nominal_field_rate)
Does that make sense?
Thanks,
Anthony
-----Original Message-----
From: Colin King <[email protected]>
Sent: Thursday, April 23, 2020 10:03 AM
To: Wentland, Harry <[email protected]>; Li, Sun peng (Leo) <[email protected]>; Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Zhou, David(ChunMing) <[email protected]>; David Airlie <[email protected]>; Daniel Vetter <[email protected]>; Koo, Anthony <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]
Subject: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo
From: Colin Ian King <[email protected]>
The assignment to infopacket->sb[7] looks incorrect, the comment states it is the minimum refresh rate yet it is being assigned a value from the maximum refresh rate max_refresh_in_uhz. Fix this by using min_refresh_in_uhz instead.
Addresses-Coverity: ("Copy-paste error")
Fixes: d2bacc38f6ca ("drm/amd/display: Change infopacket type programming")
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 2 +-
1 file changed, 1 insertion(+), 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 eb7421e83b86..fe11436536e8 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -587,7 +587,7 @@ static void build_vrr_infopacket_data_v3(const struct mod_vrr_params *vrr,
} else {
// Non-fs case, program nominal range
/* PB7 = FreeSync Minimum refresh rate (Hz) */
- infopacket->sb[7] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
+ infopacket->sb[7] = (unsigned char)((vrr->min_refresh_in_uhz +
+500000) / 1000000);
/* PB8 = FreeSync Maximum refresh rate (Hz) */
infopacket->sb[8] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
}
--
2.25.1