[PATCH 1/1] nv50: improve nv50_pm_clock_get()

[PATCH 1/1] nv50: improve nv50_pm_clock_get()

[Emil Velikov]

On some cards the memory and/or shader pll can be switched off/disabled
Check and return the linked/standart clock

Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/nouveau/nv50_pm.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
index 7dbb305..c01a64f 100644
--- a/drivers/gpu/drm/nouveau/nv50_pm.c
+++ b/drivers/gpu/drm/nouveau/nv50_pm.c
@@ -47,6 +47,19 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
 
 	reg0 = nv_rd32(dev, pll.reg + 0);
 	reg1 = nv_rd32(dev, pll.reg + 4);
+
+	if ((reg0 & 0x80000000) == 0) {
+		if (id == PLL_SHADER) {
+			NV_INFO(dev, "Shader PLL appears to be OFF\n");
+			ret = nv50_pm_clock_get(dev, PLL_CORE);
+			if (ret > 0)
+				return ret*2;
+		} else if (id == PLL_MEMORY) {
+			NV_INFO(dev, "Memory PLL appears to be OFF\n");
+			return 100*1000;
+		}
+	}
+
 	P = (reg0 & 0x00070000) >> 16;
 	N = (reg1 & 0x0000ff00) >> 8;
 	M = (reg1 & 0x000000ff);
-- 
1.7.1

Re: [PATCH 1/1] nv50: improve nv50_pm_clock_get()

[Ben Skeggs]

On Fri, 2011-03-18 at 01:15 +0000, Emil Velikov wrote:
> On some cards the memory and/or shader pll can be switched off/disabled
> Check and return the linked/standart clock
> 
> Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/nv50_pm.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
> index 7dbb305..c01a64f 100644
> --- a/drivers/gpu/drm/nouveau/nv50_pm.c
> +++ b/drivers/gpu/drm/nouveau/nv50_pm.c
> @@ -47,6 +47,19 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
>  
>  	reg0 = nv_rd32(dev, pll.reg + 0);
>  	reg1 = nv_rd32(dev, pll.reg + 4);
> +
> +	if ((reg0 & 0x80000000) == 0) {
> +		if (id == PLL_SHADER) {
> +			NV_INFO(dev, "Shader PLL appears to be OFF\n");
> +			ret = nv50_pm_clock_get(dev, PLL_CORE);
> +			if (ret > 0)
> +				return ret*2;
This seems somewhat OK, from what I recall seeing.  Have you seen any
definite evidence to suggest that the shaders actually run at double the
core clock if the PLL is "off"?

> +		} else if (id == PLL_MEMORY) {
> +			NV_INFO(dev, "Memory PLL appears to be OFF\n");
> +			return 100*1000;
This, is more dubious.  Are you trying to indicate that it's running at
the reference clock?  Or always at a fixed 100MHz?

Ben.
> +		}
> +	}
> +
>  	P = (reg0 & 0x00070000) >> 16;
>  	N = (reg1 & 0x0000ff00) >> 8;
>  	M = (reg1 & 0x000000ff);

Re: [PATCH 1/1] nv50: improve nv50_pm_clock_get ()

[Emil Velikov]

On Fri, 2011-03-18 at 22:04 +1000, Ben Skeggs wrote:
> On Fri, 2011-03-18 at 01:15 +0000, Emil Velikov wrote:
> > On some cards the memory and/or shader pll can be switched off/disabled
> > Check and return the linked/standart clock
> > 
> > Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/gpu/drm/nouveau/nv50_pm.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
> > index 7dbb305..c01a64f 100644
> > --- a/drivers/gpu/drm/nouveau/nv50_pm.c
> > +++ b/drivers/gpu/drm/nouveau/nv50_pm.c
> > @@ -47,6 +47,19 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
> >  
> >  	reg0 = nv_rd32(dev, pll.reg + 0);
> >  	reg1 = nv_rd32(dev, pll.reg + 4);
> > +
> > +	if ((reg0 & 0x80000000) == 0) {
> > +		if (id == PLL_SHADER) {
> > +			NV_INFO(dev, "Shader PLL appears to be OFF\n");
> > +			ret = nv50_pm_clock_get(dev, PLL_CORE);
> > +			if (ret > 0)
> > +				return ret*2;
> This seems somewhat OK, from what I recall seeing.  Have you seen any
> definite evidence to suggest that the shaders actually run at double the
> core clock if the PLL is "off"?

The definite evidence can be taken from the blob's behaviour as well as
programs such as MSI Afterburner and EGA Precision (both windows apps)

Example 1 (blob) - on my system if the shader is 2x Core (in the perf
table) the shader plls is always off (GNU/Linux and Windows). If I
change the entry/perf table to 275/555 (core/shader), the blob sets
turns the shader pll ON (plus some misc, bit's that are coming later on)

Example 2 (windows apps) - both apps have a "link" button, upon
activation of which the shader plls being switched off and the shader
clk is being linked to the core (by a factor of two). Upon deactivation
the opposite behaviour has been observed. In both cases (link/unlink)
the blob reports the correct clks.

> 
> > +		} else if (id == PLL_MEMORY) {
> > +			NV_INFO(dev, "Memory PLL appears to be OFF\n");
> > +			return 100*1000;
> This, is more dubious.  Are you trying to indicate that it's running at
> the reference clock?  Or always at a fixed 100MHz?

This is the one that causes some oddness to the whole "pll off". I
cannot recall what was the exact reference clock (but I believe it was
25kHz). Thus making the fun even better.

The above has been confirmed by modifying the perf table (to 105) and it
can be easily seen that the PLL is then "on". No idea where the fixed
100Mhz comes from though. 
If the current implementation is correct then the memory clk would be
1000Mhz vs 100Mhz (as reported by the blob). Simply observe the
registers and calculate the clk, when the blob sets the clocks - ex.
169/337/100 (core/shr/mem) seen on many nv50+ laptops

In all cases the blob has a tolerance of up to 5MHz for reading/writing
the clks.

Note that some systems may still set the shader/memory plls even if they
fall under those rules (shader == 2*core , memory == 100Mhz). Thus they
will be "pm_clock_get" by the current code.

Regards,
Emil.
 

> 
> Ben.
> > +		}
> > +	}
> > +
> >  	P = (reg0 & 0x00070000) >> 16;
> >  	N = (reg1 & 0x0000ff00) >> 8;
> >  	M = (reg1 & 0x000000ff);
> 
> 

Re: [PATCH 1/1] nv50: improve nv50_pm_clock_get ()

[Ben Skeggs]

On Fri, 2011-03-18 at 19:57 +0000, Emil Velikov wrote:
> On Fri, 2011-03-18 at 22:04 +1000, Ben Skeggs wrote:
> > On Fri, 2011-03-18 at 01:15 +0000, Emil Velikov wrote:
> > > On some cards the memory and/or shader pll can be switched off/disabled
> > > Check and return the linked/standart clock
> > > 
> > > Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/gpu/drm/nouveau/nv50_pm.c |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
> > > index 7dbb305..c01a64f 100644
> > > --- a/drivers/gpu/drm/nouveau/nv50_pm.c
> > > +++ b/drivers/gpu/drm/nouveau/nv50_pm.c
> > > @@ -47,6 +47,19 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
> > >  
> > >  	reg0 = nv_rd32(dev, pll.reg + 0);
> > >  	reg1 = nv_rd32(dev, pll.reg + 4);
> > > +
> > > +	if ((reg0 & 0x80000000) == 0) {
> > > +		if (id == PLL_SHADER) {
> > > +			NV_INFO(dev, "Shader PLL appears to be OFF\n");
> > > +			ret = nv50_pm_clock_get(dev, PLL_CORE);
> > > +			if (ret > 0)
> > > +				return ret*2;
> > This seems somewhat OK, from what I recall seeing.  Have you seen any
> > definite evidence to suggest that the shaders actually run at double the
> > core clock if the PLL is "off"?
> 
> The definite evidence can be taken from the blob's behaviour as well as
> programs such as MSI Afterburner and EGA Precision (both windows apps)
> 
> Example 1 (blob) - on my system if the shader is 2x Core (in the perf
> table) the shader plls is always off (GNU/Linux and Windows). If I
> change the entry/perf table to 275/555 (core/shader), the blob sets
> turns the shader pll ON (plus some misc, bit's that are coming later on)
> 
> Example 2 (windows apps) - both apps have a "link" button, upon
> activation of which the shader plls being switched off and the shader
> clk is being linked to the core (by a factor of two). Upon deactivation
> the opposite behaviour has been observed. In both cases (link/unlink)
> the blob reports the correct clks.
Ok, this seems to be evidence enough of this.  Cool.

> 
> > 
> > > +		} else if (id == PLL_MEMORY) {
> > > +			NV_INFO(dev, "Memory PLL appears to be OFF\n");
> > > +			return 100*1000;
> > This, is more dubious.  Are you trying to indicate that it's running at
> > the reference clock?  Or always at a fixed 100MHz?
> 
> This is the one that causes some oddness to the whole "pll off". I
> cannot recall what was the exact reference clock (but I believe it was
> 25kHz). Thus making the fun even better.
> 
> The above has been confirmed by modifying the perf table (to 105) and it
> can be easily seen that the PLL is then "on". No idea where the fixed
> 100Mhz comes from though. 
Well, the reason I asked is that the refclk for core/mem/shader plls on
most (all?) GF8+ boards I've seen *is* 100MHz according to the PLL
limits tables.  One of the PLL's may have been 108 actually, but there's
definitely a couple of them that are 100.  We'd need to find some board
that doesn't have a 100MHz refclk for the mem pll to confirm whether or
not "off" means 100MHz or refclk however, so I'm not too sure what to
assume here?

Aside from that last thing to resolve, I'm ok with the patch, except
maybe push the "appears to be OFF" messages to NV_DEBUG instead.

Ben.

> If the current implementation is correct then the memory clk would be
> 1000Mhz vs 100Mhz (as reported by the blob). Simply observe the
> registers and calculate the clk, when the blob sets the clocks - ex.
> 169/337/100 (core/shr/mem) seen on many nv50+ laptops
> 
> In all cases the blob has a tolerance of up to 5MHz for reading/writing
> the clks.
> 
> Note that some systems may still set the shader/memory plls even if they
> fall under those rules (shader == 2*core , memory == 100Mhz). Thus they
> will be "pm_clock_get" by the current code.
> 
> Regards,
> Emil.
>  
> 
> > 
> > Ben.
> > > +		}
> > > +	}
> > > +
> > >  	P = (reg0 & 0x00070000) >> 16;
> > >  	N = (reg1 & 0x0000ff00) >> 8;
> > >  	M = (reg1 & 0x000000ff);
> > 
> > 
> 
> 

Re: [PATCH 1/1] nv50: improve nv50_pm_clock_get ()

[Emil Velikov]

On Sat, 19 Mar 2011 01:38:56 -0000, Ben Skeggs <skeggsb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, 2011-03-18 at 19:57 +0000, Emil Velikov wrote:
>> On Fri, 2011-03-18 at 22:04 +1000, Ben Skeggs wrote:
>> > On Fri, 2011-03-18 at 01:15 +0000, Emil Velikov wrote:
>> > > On some cards the memory and/or shader pll can be switched off/disabled
>> > > Check and return the linked/standart clock
>> > >
>> > > Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > > ---
>> > >  drivers/gpu/drm/nouveau/nv50_pm.c |   13 +++++++++++++
>> > >  1 files changed, 13 insertions(+), 0 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
>> > > index 7dbb305..c01a64f 100644
>> > > --- a/drivers/gpu/drm/nouveau/nv50_pm.c
>> > > +++ b/drivers/gpu/drm/nouveau/nv50_pm.c
>> > > @@ -47,6 +47,19 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
>> > >
>> > >  	reg0 = nv_rd32(dev, pll.reg + 0);
>> > >  	reg1 = nv_rd32(dev, pll.reg + 4);
>> > > +
>> > > +	if ((reg0 & 0x80000000) == 0) {
>> > > +		if (id == PLL_SHADER) {
>> > > +			NV_INFO(dev, "Shader PLL appears to be OFF\n");
>> > > +			ret = nv50_pm_clock_get(dev, PLL_CORE);
>> > > +			if (ret > 0)
>> > > +				return ret*2;
>> > This seems somewhat OK, from what I recall seeing.  Have you seen any
>> > definite evidence to suggest that the shaders actually run at double the
>> > core clock if the PLL is "off"?
>>
>> The definite evidence can be taken from the blob's behaviour as well as
>> programs such as MSI Afterburner and EGA Precision (both windows apps)
>>
>> Example 1 (blob) - on my system if the shader is 2x Core (in the perf
>> table) the shader plls is always off (GNU/Linux and Windows). If I
>> change the entry/perf table to 275/555 (core/shader), the blob sets
>> turns the shader pll ON (plus some misc, bit's that are coming later on)
>>
>> Example 2 (windows apps) - both apps have a "link" button, upon
>> activation of which the shader plls being switched off and the shader
>> clk is being linked to the core (by a factor of two). Upon deactivation
>> the opposite behaviour has been observed. In both cases (link/unlink)
>> the blob reports the correct clks.
> Ok, this seems to be evidence enough of this.  Cool.
>
>>
>> >
>> > > +		} else if (id == PLL_MEMORY) {
>> > > +			NV_INFO(dev, "Memory PLL appears to be OFF\n");
>> > > +			return 100*1000;
>> > This, is more dubious.  Are you trying to indicate that it's running at
>> > the reference clock?  Or always at a fixed 100MHz?
>>
>> This is the one that causes some oddness to the whole "pll off". I
>> cannot recall what was the exact reference clock (but I believe it was
>> 25kHz). Thus making the fun even better.
>>
>> The above has been confirmed by modifying the perf table (to 105) and it
>> can be easily seen that the PLL is then "on". No idea where the fixed
>> 100Mhz comes from though.
> Well, the reason I asked is that the refclk for core/mem/shader plls on
> most (all?) GF8+ boards I've seen *is* 100MHz according to the PLL
> limits tables.  One of the PLL's may have been 108 actually, but there's
> definitely a couple of them that are 100.  We'd need to find some board
> that doesn't have a 100MHz refclk for the mem pll to confirm whether or
> not "off" means 100MHz or refclk however, so I'm not too sure what to
> assume here?
>
> Aside from that last thing to resolve, I'm ok with the patch, except
> maybe push the "appears to be OFF" messages to NV_DEBUG instead.
>
> Ben.

I've been looking at some of the vbioses I have in my possession and
noticed that almost all of them have ref_clks 100/100/108 MHz -
core/shader/mem
Even my own card nv96 (GT120M), has a mem refclk of 108Mhz, where the
perf entry states 100Mhz (for the lowest perf level)

Still that represents a small portion of videocards (20 vbioses), and
they are not enough for any decisive conclusion.

I will have to do some more testing, and notify you with my results.
Until then I will modify the patch so that only the shader pll is
taken into account

Thanks for the points mentioned,
Emil

>
>> If the current implementation is correct then the memory clk would be
>> 1000Mhz vs 100Mhz (as reported by the blob). Simply observe the
>> registers and calculate the clk, when the blob sets the clocks - ex.
>> 169/337/100 (core/shr/mem) seen on many nv50+ laptops
>>
>> In all cases the blob has a tolerance of up to 5MHz for reading/writing
>> the clks.
>>
>> Note that some systems may still set the shader/memory plls even if they
>> fall under those rules (shader == 2*core , memory == 100Mhz). Thus they
>> will be "pm_clock_get" by the current code.
>>
>> Regards,
>> Emil.
>>
>>
>> >
>> > Ben.
>> > > +		}
>> > > +	}
>> > > +
>> > >  	P = (reg0 & 0x00070000) >> 16;
>> > >  	N = (reg1 & 0x0000ff00) >> 8;
>> > >  	M = (reg1 & 0x000000ff);
>> >
>> >
>>
>>
>
>

[PATCH] nv50: improve nv50_pm_get_clock()

[Emil Velikov]

Many of the nv50 cards have their shader and/or memory pll
disabled at some stage.
This patch addresses those cases, so that the function
returns the correct frequency.

When the shader pll is disabled, the blob reports 2*core clock
Whereas for memory, the data stored in the vbios. This action
is incorrect as some vbioses store a clock value that is less
than the refference clock of the pll.

Thus we are reporting the reff_clk as it is the frequency the
pll actually operates

v2 - Convert NV_INFO() messages to NV_DEBUG()
Provide more information in the actuall message

Signed-off-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/nouveau/nv50_pm.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_pm.c b/drivers/gpu/drm/nouveau/nv50_pm.c
index 7dbb305..8a28100 100644
--- a/drivers/gpu/drm/nouveau/nv50_pm.c
+++ b/drivers/gpu/drm/nouveau/nv50_pm.c
@@ -47,6 +47,21 @@ nv50_pm_clock_get(struct drm_device *dev, u32 id)
 
 	reg0 = nv_rd32(dev, pll.reg + 0);
 	reg1 = nv_rd32(dev, pll.reg + 4);
+
+	if ((reg0 & 0x80000000) == 0) {
+		if (id == PLL_SHADER) {
+			NV_DEBUG(dev, "Shader PLL is disabled. "
+				"Shader clock is twice the core\n");
+			ret = nv50_pm_clock_get(dev, PLL_CORE);
+			if (ret > 0)
+				return ret << 1;
+		} else if (id == PLL_MEMORY) {
+			NV_DEBUG(dev, "Memory PLL is disabled. "
+				"Memory clock is equal to the ref_clk\n");
+			return pll.refclk;
+		}
+	}
+
 	P = (reg0 & 0x00070000) >> 16;
 	N = (reg1 & 0x0000ff00) >> 8;
 	M = (reg1 & 0x000000ff);
-- 
1.7.1