Re: [Intel-xe] [PATCH v2 2/2] drm/xe/pmu: Enable PMU interface

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 24 Jul 2023 09:05:53 -0700, Iddamsetty, Aravind wrote:
>
> On 24-07-2023 21:22, Dixit, Ashutosh wrote:
> Hi Ashutosh,
>
> > On Mon, 24 Jul 2023 01:03:23 -0700, Iddamsetty, Aravind wrote:
> >
> > Hi Aravind,
> >
> >> On 22-07-2023 11:34, Dixit, Ashutosh wrote:
> >>>> On Fri, 21 Jul 2023 16:36:02 -0700, Dixit, Ashutosh wrote:
> >>>> On Fri, 21 Jul 2023 04:51:09 -0700, Iddamsetty, Aravind wrote:
> >>>>>>> +void engine_group_busyness_store(struct xe_gt *gt)
> >>>>>>> +{
> >>>>>>> +	struct xe_pmu *pmu = &gt->tile->xe->pmu;
> >>>>>>> +	unsigned int gt_id = gt->info.id;
> >>>>>>> +	unsigned long flags;
> >>>>>>> +
> >>>>>>> +	spin_lock_irqsave(&pmu->lock, flags);
> >>>>>>> +
> >>>>>>> +	store_sample(pmu, gt_id, __XE_SAMPLE_RENDER_GROUP_BUSY,
> >>>>>>> +		     __engine_group_busyness_read(gt, XE_PMU_RENDER_GROUP_BUSY(0)));
> >>>>>>> +	store_sample(pmu, gt_id, __XE_SAMPLE_COPY_GROUP_BUSY,
> >>>>>>> +		     __engine_group_busyness_read(gt, XE_PMU_COPY_GROUP_BUSY(0)));
> >>>>>>> +	store_sample(pmu, gt_id, __XE_SAMPLE_MEDIA_GROUP_BUSY,
> >>>>>>> +		     __engine_group_busyness_read(gt, XE_PMU_MEDIA_GROUP_BUSY(0)));
> >>>>>>> +	store_sample(pmu, gt_id, __XE_SAMPLE_ANY_ENGINE_GROUP_BUSY,
> >>>>>>> +		     __engine_group_busyness_read(gt, XE_PMU_ANY_ENGINE_GROUP_BUSY(0)));
> >>>
> >>> Here why should we store everything, we should store only those events
> >>> which are enabled?
> >>
> >> The events are enabled only when they are opened which can happen after
> >> the device is suspended hence we need to store all. As in the present
> >> case device is put to suspend immediately after probe and event is
> >> opened post driver load is done.
> >
> > I don't think we can justify doing expensive PCIe reads and increasing the
> > time to go into runtime suspend, when PMU might not being used at all.
> >
> > If we store only enabled samples and start storing them only after they are
> > enabled, what would be the consequence of this? The first non-zero sample
> > seen by the perf tool would be wrong and later samples will be fine?
>
> Why do you say it is wrong perf reports relative from the time an event
> is opened.

I am asking you what is the consequence. Initial values will all be zero
and then there is some activity and we get a non zero value but this will
include all the previous activity so the first difference we send to perf
will be large/wrong I think.

>
> >
> > If there is a consequence, we might have to go back to what I was saying
> > earlier about waking the device up and reading the enabled counter when
> > xe_pmu_event_start happens, to initialize the counter values. I am assuming
> > this will work?
>
> xe_pmu_event_start can be called when device is in suspend so we shall
> not wake up the device i.e event being enabled when in suspend, so if we
> do not store while going to suspend we will not have any value to
> consider when event is enabled after suspend as we need to present
> relative value.

That is why I am saying wake up the device and initialize the counters in
xe_pmu_event_start.

> >
> > Doing this IMO would be better than always doing these PCIe reads on
> > runtime suspend even when PMU is not being used
>
> we have been doing these in i915 not sure if it affected any timing
> requirements for runtime suspend.

Hmm i915 indeed seems to be reading the RC6 residency in __gt_park even
when RC6 event is not enabled or PMU might not be used.

@Tvrtko, any comments here?

Thanks.
--
Ashutosh