[Intel-gfx] [Intel-gfx v2 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Teres Alexis, Alan Previn]

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