> > + /** > > + * @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); > > } > >
