lkml.org 
[lkml]   [2020]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo
Date
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 <colin.king@canonical.com>
Sent: Thursday, April 23, 2020 10:03 AM
To: Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Koo, Anthony <Anthony.Koo@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo

From: Colin Ian King <colin.king@canonical.com>

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 <colin.king@canonical.com>
---
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
\
 
 \ /
  Last update: 2020-04-23 17:19    [W:0.028 / U:1.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site