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

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

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

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.

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));