[PATCH 1/3] drm/i915: don't use stolen space on VLV

[PATCH 1/3] drm/i915: don't use stolen space on VLV

[Jesse Barnes]

BIOS stuffs things here we can't clobber.

Signed-off-by: Jesse Barnes <jbarnes@virtuosugeek.org>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 67d3510..85b3ea9 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -184,6 +184,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_VALLEYVIEW(dev))
+		return 0;
+
 	dev_priv->mm.stolen_base = i915_stolen_to_physical(dev);
 	if (dev_priv->mm.stolen_base == 0)
 		return 0;
-- 
1.7.10.4

[PATCH 2/3] drm/i915: read current freq from Punit on VLV

[Jesse Barnes]

Instead of returning the cached value, which is just what the kernel
requested.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_sysfs.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index ca00df2..c0d7875 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -212,10 +212,13 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	int ret;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-	if (IS_VALLEYVIEW(dev_priv->dev))
-		ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.cur_delay);
-	else
+	if (IS_VALLEYVIEW(dev_priv->dev)) {
+		u32 freq;
+		valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &freq);
+		ret = vlv_gpu_freq(dev_priv->mem_freq, (freq >> 8) & 0xff);
+	} else {
 		ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER;
+	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
-- 
1.7.10.4

Re: [PATCH 2/3] drm/i915: read current freq from Punit on VLV

[Kenneth Graunke]

On 05/01/2013 04:09 PM, Jesse Barnes wrote:
> Instead of returning the cached value, which is just what the kernel
> requested.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>   drivers/gpu/drm/i915/i915_sysfs.c |    9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index ca00df2..c0d7875 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -212,10 +212,13 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>   	int ret;
>
>   	mutex_lock(&dev_priv->rps.hw_lock);
> -	if (IS_VALLEYVIEW(dev_priv->dev))
> -		ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.cur_delay);
> -	else
> +	if (IS_VALLEYVIEW(dev_priv->dev)) {
> +		u32 freq;
> +		valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &freq);
> +		ret = vlv_gpu_freq(dev_priv->mem_freq, (freq >> 8) & 0xff);
> +	} else {
>   		ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER;
> +	}
>   	mutex_unlock(&dev_priv->rps.hw_lock);
>
>   	return snprintf(buf, PAGE_SIZE, "%d\n", ret);

I'd really like to see this for the rest of the platforms as well. 
Knowing what the kernel requested isn't nearly as interesting as knowing 
the real value.

But this VLV patch looks great, so:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

I'm not planning on reviewing patch 1; I have no idea about any of that 
stuff.

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

[Jesse Barnes]

We need to catch the invalid case and override it.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 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");
-- 
1.7.10.4

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

[Ben Widawsky]

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>
> ---
>  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

or

u32 freqs[] = {800,1066,1333,800};
dev_priv->mem_freq = freqs[(val >> 6) & 3];

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>

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Kenneth Graunke]

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

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

[Ben Widawsky]

On Wed, May 01, 2013 at 06:38:17PM -0700, Kenneth Graunke wrote:
> 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.

You're right about the missing shift, but otherwise the formula is
correct. Val is 1 in the case of 1066MHz...

> 
> >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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Kenneth Graunke]

On 05/01/2013 06:45 PM, Ben Widawsky wrote:
> On Wed, May 01, 2013 at 06:38:17PM -0700, Kenneth Graunke wrote:
>> 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.
>
> You're right about the missing shift, but otherwise the formula is
> correct. Val is 1 in the case of 1066MHz...
>
>>
>>> 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.

You're right - my mistake, sorry.  I had been doing this in a python 
interpreter and accidentally put 0b01 for the first val and 0b10 for the 
second val.

--Ken

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

[Ben Widawsky]

On Wed, May 01, 2013 at 06:26:28PM -0700, 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>
> > ---
> >  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
> 

conveniently, I've given you the reason not to use this one... error
prone. It's missing a:
val = (val >> 6) & 3
before the math.

> or
> 
> u32 freqs[] = {800,1066,1333,800};
> dev_priv->mem_freq = freqs[(val >> 6) & 3];
> 
> 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>
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: don't use stolen space on VLV

[Ben Widawsky]

On Wed, May 01, 2013 at 04:09:20PM -0700, Jesse Barnes wrote:
> BIOS stuffs things here we can't clobber.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuosugeek.org>
As I said in private email, I think the quirk is a bit better suited in
i915_stolen_to_physical(), and the commit message *could* explain things
a bit clearer, oh, and you're welcome.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 3264 bytes --]

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@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


[-- Attachment #1.2: Type: text/html, Size: 4160 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx