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

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx