| Message ID | 1433143921-31114-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 1, 2015 at 12:32 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. This matches both the spec and > the accuracy test by pm_rc6_residency. > > v2: > - simplify logic checking for the CHV 320MHz special case (Rodrigo) > > Testcase: igt/pm_rc6_residency > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_sysfs.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index 2476268..55bd04c 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -64,24 +64,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg) > goto out; > } > > - units = 0; > - div = 1000000ULL; > - > - if (IS_CHERRYVIEW(dev)) { > + if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) { > /* Special case for 320Mhz */ > - if (czcount_30ns == 1) { > - div = 10000000ULL; > - units = 3125ULL; > - } else { > - /* chv counts are one less */ > - czcount_30ns += 1; > - } > + div = 10000000ULL; > + units = 3125ULL; > + } else { > + czcount_30ns += 1; > + div = 1000000ULL; > + units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns); Is (u64) cast unnecessary? But reading like this now I wonder if we couldn't just pass czcount_30ns+1 instead of the increment... But if we don't need the cast let's please just ignore this bikeshed and let's move fwd! ;) More organized than I had suggested, thanks. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > } > > - if (units == 0) > - units = DIV_ROUND_UP_ULL(30ULL * bias, > - (u64)czcount_30ns); > - > if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH) > units <<= 8; > > -- > 2.1.4 >
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6512
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 303/303 303/303
SNB -1 315/315 314/315
IVB 343/343 343/343
BYT 287/287 287/287
BDW -1 321/321 320/321
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*SNB igt@pm_rpm@dpms-mode-unset-non-lpsp 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
*BDW igt@gem_flink@bad-flink PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
On ma, 2015-06-01 at 12:01 -0700, Rodrigo Vivi wrote: > On Mon, Jun 1, 2015 at 12:32 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. This matches both the spec and > > the accuracy test by pm_rc6_residency. > > > > v2: > > - simplify logic checking for the CHV 320MHz special case (Rodrigo) > > > > Testcase: igt/pm_rc6_residency > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_sysfs.c | 22 +++++++--------------- > > 1 file changed, 7 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > > index 2476268..55bd04c 100644 > > --- a/drivers/gpu/drm/i915/i915_sysfs.c > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > > @@ -64,24 +64,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg) > > goto out; > > } > > > > - units = 0; > > - div = 1000000ULL; > > - > > - if (IS_CHERRYVIEW(dev)) { > > + if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) { > > /* Special case for 320Mhz */ > > - if (czcount_30ns == 1) { > > - div = 10000000ULL; > > - units = 3125ULL; > > - } else { > > - /* chv counts are one less */ > > - czcount_30ns += 1; > > - } > > + div = 10000000ULL; > > + units = 3125ULL; > > + } else { > > + czcount_30ns += 1; > > + div = 1000000ULL; > > + units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns); > Is (u64) cast unnecessary? Yes. Here the only reason for it would be overflow, but that's not possible. > But reading like this now I wonder if we couldn't just pass > czcount_30ns+1 instead of the increment... > But if we don't need the cast let's please just ignore this bikeshed > and let's move fwd! ;) Yes, I think we could do more cleanup in this function as a follow-up. For example we may still loose precision in the current way, would be better to calculate the result directly from the reference clock rate (CZ clock in case of VLV/CHV). > More organized than I had suggested, thanks. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thanks. Forgot to add: Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76877 > > } > > > > - if (units == 0) > > - units = DIV_ROUND_UP_ULL(30ULL * bias, > > - (u64)czcount_30ns); > > - > > if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH) > > units <<= 8; > > > > -- > > 2.1.4 > > > > >
On Tue, Jun 02, 2015 at 12:37:13PM +0300, Imre Deak wrote: > On ma, 2015-06-01 at 12:01 -0700, Rodrigo Vivi wrote: > > On Mon, Jun 1, 2015 at 12:32 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. This matches both the spec and > > > the accuracy test by pm_rc6_residency. > > > > > > v2: > > > - simplify logic checking for the CHV 320MHz special case (Rodrigo) > > > > > > Testcase: igt/pm_rc6_residency > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_sysfs.c | 22 +++++++--------------- > > > 1 file changed, 7 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > > > index 2476268..55bd04c 100644 > > > --- a/drivers/gpu/drm/i915/i915_sysfs.c > > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > > > @@ -64,24 +64,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg) > > > goto out; > > > } > > > > > > - units = 0; > > > - div = 1000000ULL; > > > - > > > - if (IS_CHERRYVIEW(dev)) { > > > + if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) { > > > /* Special case for 320Mhz */ > > > - if (czcount_30ns == 1) { > > > - div = 10000000ULL; > > > - units = 3125ULL; > > > - } else { > > > - /* chv counts are one less */ > > > - czcount_30ns += 1; > > > - } > > > + div = 10000000ULL; > > > + units = 3125ULL; > > > + } else { > > > + czcount_30ns += 1; > > > + div = 1000000ULL; > > > + units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns); > > Is (u64) cast unnecessary? > > Yes. Here the only reason for it would be overflow, but that's not > possible. > > > But reading like this now I wonder if we couldn't just pass > > czcount_30ns+1 instead of the increment... > > But if we don't need the cast let's please just ignore this bikeshed > > and let's move fwd! ;) > > Yes, I think we could do more cleanup in this function as a follow-up. > For example we may still loose precision in the current way, would be > better to calculate the result directly from the reference clock rate > (CZ clock in case of VLV/CHV). > > > More organized than I had suggested, thanks. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Thanks. > > Forgot to add: > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76877 Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 2476268..55bd04c 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -64,24 +64,16 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg) goto out; } - units = 0; - div = 1000000ULL; - - if (IS_CHERRYVIEW(dev)) { + if (IS_CHERRYVIEW(dev) && czcount_30ns == 1) { /* Special case for 320Mhz */ - if (czcount_30ns == 1) { - div = 10000000ULL; - units = 3125ULL; - } else { - /* chv counts are one less */ - czcount_30ns += 1; - } + div = 10000000ULL; + units = 3125ULL; + } else { + czcount_30ns += 1; + div = 1000000ULL; + units = DIV_ROUND_UP_ULL(30ULL * bias, czcount_30ns); } - if (units == 0) - units = DIV_ROUND_UP_ULL(30ULL * bias, - (u64)czcount_30ns); - if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH) units <<= 8;
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. v2: - simplify logic checking for the CHV 320MHz special case (Rodrigo) Testcase: igt/pm_rc6_residency Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_sysfs.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)