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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 10, 2023 at 01:40:16PM +0530, Iddamsetty, Aravind wrote:
> 
> 
> On 10-08-2023 08:10, Dixit, Ashutosh wrote:
> 
> Hi Ashutosh,
> 
> > On Wed, 09 Aug 2023 06:11:48 -0700, Iddamsetty, Aravind wrote:
> > 
> > Hi Aravind,
> > 
> >> On 09-08-2023 17:27, Iddamsetty, Aravind wrote:
> >>> On 09-08-2023 15:25, Iddamsetty, Aravind wrote:
> >>>> On 09-08-2023 12:58, Dixit, Ashutosh wrote:
> >>>>> On Tue, 08 Aug 2023 04:54:36 -0700, Aravind Iddamsetty wrote:
> >>>>>
> >>>>> Spotted a few remaining things. See if it's possible to fix these up and
> >>>>> send another version.
> >>>>>
> >>>>>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..9637f8283641
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
> >>>>>> @@ -0,0 +1,673 @@
> >>>>
> >>>> <snip>
> >>>>>> +static u64 __engine_group_busyness_read(struct xe_gt *gt, int sample_type)
> >>>>>> +{
> >>>>>> +	u64 val = 0;
> >>>>>> +
> >>>>>
> >>>>> What is the forcewake domain for these registers? Don't we need to get
> >>>>> forcewake before reading these. Something like:
> >>>>>
> >>>>>         XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> >>>>
> >>>> based on  BSPEC:67609 these belong to GT power domain, so acquiring that
> >>>> should be sufficient.
> >>>
> >>> But if i understand correctly taking forcewake is not allowed here as it
> >>> is an atomic context and forcewake can sleep and that is what I'm seeing
> >>> as well, might also be the reason why i915 didn't do that as well.
> >>>
> >>> [  899.114316] BUG: sleeping function called from invalid context at
> >>> kernel/locking/mutex.c:580
> >>> [  899.115768] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid:
> >>> 290, name: kworker/27:1
> >>
> >> that is the reason why in i915 we were doing similar thing of storing
> >> the counter as we enter rc6, not sure how do we do that in xe.
> > 
> > Just to check, which code path(s) is/are aotmic context:
> > 
> > a. xe_pm_suspend
> > b. xe_pm_runtime_suspend
> > c. xe_pmu_event_read
> 
> pmu_event_read and runtime_suspend are atomic contexts.

what about doing this at xe_pci_runtime_idle() ?

This will run after the autosuspend time elapses,
but before calling any suspend. Also, there's no requirement of
that function to be in atomic context. So you could forcewake_get/put
and stash your registers before we go runtime_suspend.

> 
> > 
> > Now I am wondering if GuC should provide these counters too along with
> > other busyness values it provides, since GuC is what control RC6
> > entry/exit. But let's try to understand the issue some more first.
> 
> do you mean GuC reading these registers and presenting us in a way, will
> need to think over how does it fit in the PMU.
> 
> Thanks,
> Aravind.
> 
> > 
> > Thanks.
> > --
> > Ashutosh
> > 
> > 
> >>>>>
> >>>>>> +	switch (sample_type) {
> >>>>>> +	case __XE_SAMPLE_RENDER_GROUP_BUSY:
> >>>>>> +		val = xe_mmio_read32(gt, XE_OAG_RENDER_BUSY_FREE);
> >>>>>> +		break;
> >>>>>> +	case __XE_SAMPLE_COPY_GROUP_BUSY:
> >>>>>> +		val = xe_mmio_read32(gt, XE_OAG_BLT_BUSY_FREE);
> >>>>>> +		break;
> >>>>>> +	case __XE_SAMPLE_MEDIA_GROUP_BUSY:
> >>>>>> +		val = xe_mmio_read32(gt, XE_OAG_ANY_MEDIA_FF_BUSY_FREE);
> >>>>>> +		break;
> >>>>>> +	case __XE_SAMPLE_ANY_ENGINE_GROUP_BUSY:
> >>>>>> +		val = xe_mmio_read32(gt, XE_OAG_RC0_ANY_ENGINE_BUSY_FREE);
> >>>>>> +		break;
> >>>>>> +	default:
> >>>>>> +		drm_warn(&gt->tile->xe->drm, "unknown pmu event\n");
> >>>>>> +	}
> >>>>>
> >>>>> And similarly here:
> >>>>>
> >>>>>         XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));