From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenneth Graunke Subject: Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection Date: Wed, 01 May 2013 18:38:17 -0700 Message-ID: <5181C389.9010202@whitecape.org> References: <1367449762-2214-1-git-send-email-jbarnes@virtuousgeek.org> <1367449762-2214-3-git-send-email-jbarnes@virtuousgeek.org> <20130502012628.GA9668@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from homiemail-a60.g.dreamhost.com (caiajhbdcbef.dreamhost.com [208.97.132.145]) by gabe.freedesktop.org (Postfix) with ESMTP id 79496E5C52 for ; Wed, 1 May 2013 18:37:00 -0700 (PDT) In-Reply-To: <20130502012628.GA9668@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 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 For either the switch or Ben's table: Reviewed-by: Kenneth Graunke Incidentally, my Punit returns 3 here, but I believe the memory is actually 1066. Not sure what the deal is. --Ken