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

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

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?

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?

Doing this IMO would be better than always doing these PCIe reads on
runtime suspend even when PMU is not being used.

Thanks.
--
Ashutosh