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

["Iddamsetty, Aravind" <aravind.iddamsetty@intel.com>]

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:
>>
>> Hi Ashutosh,
>>
>>> On Tue, 08 Aug 2023 04:54:36 -0700, Aravind Iddamsetty wrote:
>>>>
>>>
>>> Hi Aravind,
>>>
>>> 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.

Thanks,
Aravind.
> 
> 
> Thanks,
> Aravind.
>>>
>>>> +	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));
>>>
>>
>> Thanks,
>> Aravind.
>>
>> <snip>
>>>
>>> Thanks.
>>> --
>>> Ashutosh