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

[Kenneth Graunke <kenneth@whitecape.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 <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