drm/i915/vlv: fix RC6 residency time calculation
diff mbox

Message ID 1431963961-24349-1-git-send-email-imre.deak@intel.com
State New
Headers show

Commit Message

Imre Deak May 18, 2015, 3:46 p.m. UTC
The divider value to convert from CZ clock rate to ms needs a +1
adjustment on VLV just like on CHV. This matches both the spec and
the accuracy test by pm_rc6_residency.

Testcase: igt/pm_rc6_residency
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Rodrigo Vivi May 18, 2015, 4:13 p.m. UTC | #1
On Mon, May 18, 2015 at 8:46 AM, Imre Deak <imre.deak@intel.com> wrote:
> The divider value to convert from CZ clock rate to ms needs a +1
> adjustment on VLV just like on CHV.

On CHV this was an special case for 320MHz, on VLV we have only one
freq possible or it is global?
The spec I have here doesn't show different freqs here on VLV as we have on CHV.

> This matches both the spec and

Anyway, even on CHV I couldn't find where spec mentions this. Could
you please point me or share your spec in pvt so I can give  a rv-b
here.

> the accuracy test by pm_rc6_residency.
>
> Testcase: igt/pm_rc6_residency
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2476268..aa99efc 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -76,6 +76,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>                                 /* chv counts are one less */
>                                 czcount_30ns += 1;
>                         }
> +               } else {
> +                       czcount_30ns += 1;
>                 }
>
>                 if (units == 0)
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak May 18, 2015, 4:24 p.m. UTC | #2
On ma, 2015-05-18 at 09:13 -0700, Rodrigo Vivi wrote:
> On Mon, May 18, 2015 at 8:46 AM, Imre Deak <imre.deak@intel.com> wrote:
> > The divider value to convert from CZ clock rate to ms needs a +1
> > adjustment on VLV just like on CHV.
> 
> On CHV this was an special case for 320MHz, on VLV we have only one
> freq possible or it is global?
> The spec I have here doesn't show different freqs here on VLV as we have on CHV.

The CZ clock rate on VLV can be any of 200, 266 and 300 MHz. In all
these cases we need to have the +1 adjustment.

> > This matches both the spec and
> 
> Anyway, even on CHV I couldn't find where spec mentions this. Could
> you please point me or share your spec in pvt so I can give  a rv-b
> here.

Ok, sent it to you.

> > the accuracy test by pm_rc6_residency.
> >
> > Testcase: igt/pm_rc6_residency
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 2476268..aa99efc 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -76,6 +76,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
> >                                 /* chv counts are one less */
> >                                 czcount_30ns += 1;
> >                         }
> > +               } else {
> > +                       czcount_30ns += 1;
> >                 }
> >
> >                 if (units == 0)
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
>
Shuang He May 19, 2015, 7:41 p.m. UTC | #3
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6429
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  234/234              234/234
ILK                                  262/262              262/262
SNB                 -1              282/282              281/282
IVB                                  300/300              300/300
BYT                                  254/254              254/254
BDW                                  275/275              275/275
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(7)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
Imre Deak May 29, 2015, 12:56 p.m. UTC | #4
On ma, 2015-05-18 at 18:46 +0300, Imre Deak wrote:
> The divider value to convert from CZ clock rate to ms needs a +1
> adjustment on VLV just like on CHV. This matches both the spec and
> the accuracy test by pm_rc6_residency.
> 
> Testcase: igt/pm_rc6_residency
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76877

Rodrigo, could you review this?

Thanks,
Imre

> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2476268..aa99efc 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -76,6 +76,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>  				/* chv counts are one less */
>  				czcount_30ns += 1;
>  			}
> +		} else {
> +			czcount_30ns += 1;
>  		}
>  
>  		if (units == 0)
Rodrigo Vivi May 29, 2015, 8:22 p.m. UTC | #5
On Fri, May 29, 2015 at 5:56 AM, Imre Deak <imre.deak@intel.com> wrote:
> On ma, 2015-05-18 at 18:46 +0300, Imre Deak wrote:
>> The divider value to convert from CZ clock rate to ms needs a +1
>> adjustment on VLV just like on CHV. This matches both the spec and
>> the accuracy test by pm_rc6_residency.
>>
>> Testcase: igt/pm_rc6_residency
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76877
>
> Rodrigo, could you review this?
>
> Thanks,
> Imre
>
>> ---
>>  drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> index 2476268..aa99efc 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -76,6 +76,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>>                               /* chv counts are one less */
>>                               czcount_30ns += 1;
>>                       }
>> +             } else {
>> +                     czcount_30ns += 1;

Thanks for spec and explanation...
Reading code again I believe czcount_30ns += 1; shouldn't be
duplicated in middle of elses here.

It would be better to do something like:
units = 0;
div = 1000000ULL;

if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
    /* Special case for 320Mhz on CHV */
     div = 10000000ULL;
     units = 3125ULL;
}

/* counts are one less */
czcount_30ns += 1;

>>               if (units == 0)
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


But with my bikeshed or not let's move fwd because I hold this for to
long already:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Imre Deak June 1, 2015, 6:29 a.m. UTC | #6
On pe, 2015-05-29 at 13:22 -0700, Rodrigo Vivi wrote:
> On Fri, May 29, 2015 at 5:56 AM, Imre Deak <imre.deak@intel.com> wrote:
> > On ma, 2015-05-18 at 18:46 +0300, Imre Deak wrote:
> >> The divider value to convert from CZ clock rate to ms needs a +1
> >> adjustment on VLV just like on CHV. This matches both the spec and
> >> the accuracy test by pm_rc6_residency.
> >>
> >> Testcase: igt/pm_rc6_residency
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76877
> >
> > Rodrigo, could you review this?
> >
> > Thanks,
> > Imre
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_sysfs.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> >> index 2476268..aa99efc 100644
> >> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> >> @@ -76,6 +76,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
> >>                               /* chv counts are one less */
> >>                               czcount_30ns += 1;
> >>                       }
> >> +             } else {
> >> +                     czcount_30ns += 1;
> 
> Thanks for spec and explanation...
> Reading code again I believe czcount_30ns += 1; shouldn't be
> duplicated in middle of elses here.
> 
> It would be better to do something like:
> units = 0;
> div = 1000000ULL;
> 
> if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) {
>     /* Special case for 320Mhz on CHV */
>      div = 10000000ULL;
>      units = 3125ULL;
> }
> 
> /* counts are one less */
> czcount_30ns += 1;

This would increment also for the special case, but I agree we should
simplify this. I'll follow up with v2.

> 
> >>               if (units == 0)
> >
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> But with my bikeshed or not let's move fwd because I hold this for to
> long already:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 2476268..aa99efc 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -76,6 +76,8 @@  static u32 calc_residency(struct drm_device *dev, const u32 reg)
 				/* chv counts are one less */
 				czcount_30ns += 1;
 			}
+		} else {
+			czcount_30ns += 1;
 		}
 
 		if (units == 0)