Re: [PATCH v2] drm/i915/vlv: fix RC6 residency time calculation

[Imre Deak <imre.deak@intel.com>]

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


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