[Intel-gfx] [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection

[Jesse Barnes]

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. 


--
Jesse Barnes, Intel Open Source Technology Center


-------- Original message --------
From: Kenneth Graunke <kenneth at whitecape.org> 
Date: 05/01/2013  6:38 PM  (GMT-08:00) 
To: Ben Widawsky <ben at bwidawsk.net> 
Cc: Jesse Barnes <jbarnes at virtuousgeek.org>,intel-gfx at 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 at 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 at bwidawsk.net>

For either the switch or Ben's table:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Incidentally, my Punit returns 3 here, but I believe the memory is 
actually 1066.  Not sure what the deal is.

--Ken

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20130501/6f48cf52/attachment.html>