* re: drm/i915: Workaround to bump rc6 voltage to 450
@ 2013-02-01 14:07 Dan Carpenter
2013-02-02 0:41 ` [PATCH] drm/i915: Fix RC6VIDS encode/devoce Ben Widawsky
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2013-02-01 14:07 UTC (permalink / raw)
To: ben; +Cc: dri-devel
Hi Ben,
The patch 31643d54a739: "drm/i915: Workaround to bump rc6 voltage to
450" from Sep 26, 2012, leads to the following warning:
"drivers/gpu/drm/i915/intel_pm.c:2662 gen6_enable_rps()
warn: boolean comparison inside select"
drivers/gpu/drm/i915/i915_reg.h
4260 #define GEN6_ENCODE_RC6_VID(mv) (((mv) / 5) - 245) < 0 ?: 0
^^^^^^^^^^^^^^^^^^^^^^
It returns either 1 or 0 depending on if mv is >= 1230.
4261 #define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) > 0 ? ((vids) * 5) + 245 : 0)
I think this is trying to reverse the other macro but the order of
operations are wrong. It should be "(vids + 245) * 5".
drivers/gpu/drm/i915/intel_pm.c
2654 rc6vids = 0;
2655 ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
2656 if (IS_GEN6(dev) && ret) {
2657 DRM_DEBUG_DRIVER("Couldn't check for BIOS workaround\n");
2658 } else if (IS_GEN6(dev) && (GEN6_DECODE_RC6_VID(rc6vids & 0xff) < 450)) {
2659 DRM_DEBUG_DRIVER("You should update your BIOS. Correcting minimum rc6 voltage (%dmV->%dmV)\n",
2660 GEN6_DECODE_RC6_VID(rc6vids & 0xff), 450);
2661 rc6vids &= 0xffff00;
2662 rc6vids |= GEN6_ENCODE_RC6_VID(450);
^^^^^^^^^^^^^^^^^^^^^^^
This is the only caller and 450 is less than 1230 we store zero.
2663 ret = sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_RC6VIDS, rc6vids);
2664 if (ret)
2665 DRM_ERROR("Couldn't fix incorrect rc6 voltage\n");
2666 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] drm/i915: Fix RC6VIDS encode/devoce
2013-02-01 14:07 drm/i915: Workaround to bump rc6 voltage to 450 Dan Carpenter
@ 2013-02-02 0:41 ` Ben Widawsky
2013-02-04 17:55 ` Daniel Vetter
0 siblings, 1 reply; 3+ messages in thread
From: Ben Widawsky @ 2013-02-02 0:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky, stable, dri-devel
The RC6 VIDS has a linear ramp starting at 250mv, which means any values
below 250 are invalid. The old buggy macros tried to adjust for this to
be more flexible, but there is no need. As Dan pointed out the ENCODE
only ever has one value. The only invalid value for decode is an input
of 0 which means something is really wonky, and the cases where DECODE
are used either don't matter (debug values), or would be implicitly
correct (the check for less than 450).
This patch makes simpler, easier to read macros which are actually
correct. Maybe this patch can actually fix some bugs now.
Thanks to Dan for catching this. /me hides
Cc: stable@kernel.org
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_reg.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 59afb7e..2a754e2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4280,8 +4280,8 @@
#define GEN6_PCODE_READ_MIN_FREQ_TABLE 0x9
#define GEN6_PCODE_WRITE_RC6VIDS 0x4
#define GEN6_PCODE_READ_RC6VIDS 0x5
-#define GEN6_ENCODE_RC6_VID(mv) (((mv) / 5) - 245) < 0 ?: 0
-#define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) > 0 ? ((vids) * 5) + 245 : 0)
+#define GEN6_ENCODE_RC6_VID(mv) (((mv) - 245) / 5)
+#define GEN6_DECODE_RC6_VID(vids) (((vids) * 5) + 245)
#define GEN6_PCODE_DATA 0x138128
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
--
1.8.1.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Fix RC6VIDS encode/devoce
2013-02-02 0:41 ` [PATCH] drm/i915: Fix RC6VIDS encode/devoce Ben Widawsky
@ 2013-02-04 17:55 ` Daniel Vetter
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2013-02-04 17:55 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, stable, dri-devel
On Fri, Feb 01, 2013 at 04:41:14PM -0800, Ben Widawsky wrote:
> The RC6 VIDS has a linear ramp starting at 250mv, which means any values
> below 250 are invalid. The old buggy macros tried to adjust for this to
> be more flexible, but there is no need. As Dan pointed out the ENCODE
> only ever has one value. The only invalid value for decode is an input
> of 0 which means something is really wonky, and the cases where DECODE
> are used either don't matter (debug values), or would be implicitly
> correct (the check for less than 450).
>
> This patch makes simpler, easier to read macros which are actually
> correct. Maybe this patch can actually fix some bugs now.
>
> Thanks to Dan for catching this. /me hides
>
> Cc: stable@kernel.org
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-04 17:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-01 14:07 drm/i915: Workaround to bump rc6 voltage to 450 Dan Carpenter
2013-02-02 0:41 ` [PATCH] drm/i915: Fix RC6VIDS encode/devoce Ben Widawsky
2013-02-04 17:55 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox