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

[Iddamsetty, Aravind]

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


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