<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head><body ><div>Yeah, saw that too but was too ashamed... anyway it needs fixing. And apparently the docs are wrong on this anyway and 11 indicates 1333, while 0 or 1 is 800. Will post an updated one. </div><div> </div><div> </div>--<div>Jesse Barnes, Intel Open Source Technology Center</div> -------- Original message -------- From: Kenneth Graunke <kenneth@whitecape.org> Date: 05/01/2013 6:38 PM (GMT-08:00) To: Ben Widawsky <ben@bwidawsk.net> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>,intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection On 05/01/2013 06:26 PM, Ben Widawsky wrote: > On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: >> We need to catch the invalid case and override it. >> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Jesse, it's actually worse than that: Your parenthesis were off in the previous version, causing it to do the multiplication by 266 before the bitwise-and: 800 + ((266 * (val >> 6)) & 3) This meant that you always got 800 or 802 Mhz. Also, even when corrected, your previous formula would return 1332 Mhz instead of 1333 Mhz, which is slightly off and doesn't work since you use it in explicit switch statements. It might be worth mentioning in the commit message. >> --- >> drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 0f4b46e..0a3b0b3 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) >> GEN7_RC_CTL_TO_MODE); >> >> valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); >> - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); >> + switch ((val >> 6) & 3) { >> + case 0: >> + dev_priv->mem_freq = 800; >> + break; >> + case 1: >> + dev_priv->mem_freq = 1066; >> + break; >> + case 2: >> + dev_priv->mem_freq = 1333; >> + break; >> + case 3: >> + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); >> + dev_priv->mem_freq = 800; >> + break; >> + } >> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); >> >> DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); > > > I guess it doesn't handle the last case, but: > dev_priv->mem_freq = 800 + (266 * (val))) + val/2 Don't use this method. Presumably Ben meant (val >> 6) here, but even with that fix this formula returns 1067 rather than 1066, which is wrong. > or > > u32 freqs[] = {800,1066,1333,800}; > dev_priv->mem_freq = freqs[(val >> 6) & 3]; I like this table based approach best. It's much more succinct than the switch statement, and exact. Might be worth preserving the DRM_ERROR. > Are two ways I would have used before making a switch block :P > > Just a thought, but perhaps we don't want to enable RPS if we can't > reliably figure out the memory freq. (case 3) > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> For either the switch or Ben's table: Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> Incidentally, my Punit returns 3 here, but I believe the memory is actually 1066. Not sure what the deal is. --Ken </body>