linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo
@ 2020-04-23 14:02 Colin King
  2020-04-23 15:18 ` Koo, Anthony
  0 siblings, 1 reply; 2+ messages in thread
From: Colin King @ 2020-04-23 14:02 UTC (permalink / raw)
  To: Harry Wentland, Leo Li, Alex Deucher, Christian König,
	David Zhou, David Airlie, Daniel Vetter, Anthony Koo, amd-gfx,
	dri-devel
  Cc: kernel-janitors, linux-kernel

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


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* RE: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo
  2020-04-23 14:02 [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo Colin King
@ 2020-04-23 15:18 ` Koo, Anthony
  0 siblings, 0 replies; 2+ messages in thread
From: Koo, Anthony @ 2020-04-23 15:18 UTC (permalink / raw)
  To: Colin King, Wentland, Harry, Li, Sun peng (Leo),
	Deucher, Alexander, Koenig, Christian, Zhou, David(ChunMing),
	David Airlie, Daniel Vetter, amd-gfx, dri-devel
  Cc: kernel-janitors, linux-kernel

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


^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-04-23 15:19 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 14:02 [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo Colin King
2020-04-23 15:18 ` Koo, Anthony

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).