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

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

On Mon, 07 Aug 2023 14:16:59 -0700, Dixit, Ashutosh wrote:
>

Hi Aravind,

> On Tue, 25 Jul 2023 04:38:45 -0700, Iddamsetty, Aravind wrote:
> > On 24-07-2023 22:01, Dixit, Ashutosh wrote:
> > > On Mon, 24 Jul 2023 09:05:53 -0700, Iddamsetty, Aravind wrote:
> > >>
> > >>>> 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.
> >
> > correct if we just store the enabled events in suspend, any other event
> > will have 0 initial value and when we read the register later it will
> > have all the accumulation and since past value we have is 0 we would end
> > up reporting the entire value which is wrong.
>
> Ok, agreed, so we need to do "something".
>
> >
> > >
> > >>
> > >>>
> > >>> 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.
> >
> > Afaik since PMU doesn't take DRM reference we shall not wake up the
> > device.
>
> Not sure what you mean because PMU does do this:
>
>	drm_dev_get(&xe->drm);
>
> Anyway I don't think it has anything to do with waking up the device since
> that is done via xe_device_mem_access_get.
>
> > if we were allowed to wake up the device why do we even need to
> > store during suspend. when ever PMU event is opened we could wake up the
> > device and read the register directly.
>
> No. That is why we are saving the counters during suspend so we don't have
> to wake up the device just to read the counters. So the issue is only how
> to *initialize* the counters.
>
> You are saying we initialize by saving all counters during suspend, whether
> or not they are enabled, which I don't agree with. I am saying we should
> only read and store the counters which are enabled during normal
> operation. And to initialize we wake the device up during
> xe_pmu_event_start and store the counter value. Alternatively, we can zero
> out the enabled counters during xe_pmu_event_start (the counters are RW)
> but in any case that will also need waking up the device.
>
> So this way we only wake up the device for initialization but not
> afterwards.
>
> Since this is the "base" patch we should try to set up a good
> infrastructure in this patch so that other stuff which is exposed via PMU
> can be easily added later.

After thinking a bit more about this, though I think this needs to be done,
I won't insist that we do this in this patch, we can review and do this in
a subsequent patch (if no one else objects).

So let's skip this for now. So if you can generate a new version of the
patch after addressing all of the other review comments, we can review that
again and try to get it merged.

Thanks.
--
Ashutosh

> > >>>
> > >>> 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?