> > +           /**
> > +            * @last_jiffies: jiffies at last actual stats collection time
> > +            * We use this timestamp to ensure we don't oversample the
> > +            * stats because runtime power management events can trigger
> > +            * stats collection at much higher rates than required.
> > +            */
> > +           u64 last_stat_jiffs;
> 
> Why the new "jiffs" naming and not the usual jiffies?
> 
> Otherwise a good comment - just align the member name with the kerneldoc 
> name.
> 
my mistake - will align the names.

> >     unsigned long flags;
> >     ktime_t unused;
> >   
> > +   guc->timestamp.last_stat_jiffs = get_jiffies_64();
> 
> Why the 64 bit flavour? It's a first in i915 but it doesn't feel so special.
> 
sure - will use the regular jiffies

> > +
> >     spin_lock_irqsave(&guc->timestamp.lock, flags);
> >   
> >     guc_update_pm_timestamp(guc, &unused);
> > @@ -1386,6 +1388,16 @@ void intel_guc_busyness_park(struct intel_gt *gt)
> >             return;
> >   
> >     cancel_delayed_work(&guc->timestamp.work);
> > +
> > +   /*
> > +    * Before parking, we should sample engine busyness stats if we need to.
> > +    * We can skip it if we are less than half a ping from the last time we
> > +    * sampled the business stats.
> 
> busyness
yup.
> 
> > +    */
> > +   if (guc->timestamp.last_stat_jiffs && (get_jiffies_64() - 
> > guc->timestamp.last_stat_jiffs  <
> > +      (guc->timestamp.ping_delay >> 1)))
> > +           return;
> 
> 1)
> Recommend a division instead of a shift.
ok
> 
> 2)
> Is there a time_after() macro for this?
> 
yes there is - will do.

> 3)
> Should the logic be contained/consolidated in __update_guc_busyness_stats?
As Umesh mentioned, __update_guc_busyness_stats is called from the non 
__gt_park callers and in those cases we don't
want it to skip. I wanted avoid adding additional unnecessary params to signal 
if the caller would be okay with skipping
- so rather just make that decision at the caller's level. However, for the 
updating of the latest last_stat_jiffies, i
wanted to ensure that it got updated for all callers so we ensure the absolute 
minimal required busyness updates are
made when gt_park is called while other callers also got called in between.

> 
> There is cancel_delayed_work in there - is it okay for that to be 
> bypassed from here?
> 
I believe Umesh addressed this.

> Regards,
> 
> Tvrtko
> 
> > +
> >     __update_guc_busyness_stats(guc);
> >   }
> >   

Reply via email to