Re: [RFC 7/7] drm/i915: Expose client engine utilisation via fdinfo

["Christian König" <christian.koenig@amd.com>]

Am 21.05.21 um 14:26 schrieb Tvrtko Ursulin:
>
> On 20/05/2021 18:47, Daniel Vetter wrote:
>> On Thu, May 20, 2021 at 6:31 PM Christian König
>> <christian.koenig@amd.com> wrote:
>>>
>>> Yeah, having the timestamp is a good idea as well.
>>>
>>>    drm-driver: i915
>>>
>>> I think we should rather add something like printing 
>>> file_operations->owner->name to the common fdinfo code.
>>>
>>> This way we would have something common for all drivers in the 
>>> system. I'm just not sure if that also works if they are compiled 
>>> into the kernel.
>>
>> Yeah common code could print driver name, busid and all that stuff. I
>> think the common code should also provide some helpers for the key:
>> value pair formatting (and maybe check for all lower-case and stuff
>> like that) because if we don't then this is going to be a complete
>> mess that's not parseable.
>
> I see we could have a few options here, non exhaustive list 
> (especially omitting some sub-options):
>
> 1)
> DRM core implements fdinfo, which emits the common parts, calling into 
> the driver to do the rest.
>
> 2)
> DRM adds helpers for driver to emit common parts of fdinfo.
>
> 3)
> DRM core establishes a "spec" defining the common fields, the optional 
> ones, and formats.
>
> I was trending towards 3) because it is most lightweight and feeling 
> is there isn't that much value in extracting a tiny bit of commonality 
> in code. Proof in the pudding is how short the fdinfo vfunc is in this 
> patch.
>

I would say that we should add printing the module name to the common 
fdinfo function for the whole kernel.

And for the DRM specific stuff either 2 or 3 is the way to go I think. 
Number 1 sounds to much like mid-layering to me.

Regards,
Christian.

>> And value should be real semantic stuff, not "here's a string". So
>> accumulated time as a struct ktime as the example.
>
> Ideally yes, but I have a feeling the ways how amdgpu and i915 track 
> things are so different so first lets learn more about that.
>
>>> Am 20.05.21 um 18:26 schrieb Nieto, David M:
>>>
>>> [AMD Official Use Only]
>>>
>>>
>>> i would like to add a unit marker for the stats that we monitor in 
>>> the fd, as we discussed currently we are displaying the usage 
>>> percentage, because we wanted to to provide single query 
>>> percentages, but this may evolve with time.
>>>
>>> May I suggest to add two new fields
>>>
>>> drm-stat-interval: <64 bit> ns
>>> drm-stat-timestamp: <64 bit> ns
>>>
>>> If interval is set, engine utilization is calculated by doing <perc 
>>> render> = 100*<drm_engine_render>/<drm_stat_interval>
>>> if interval is not set, two reads are needed : <perc render> = 
>>> 100*<drm_engine_render1 - drm_engine_render0> / <drm-stat-timestamp1 
>>> - drm-stat-timestamp0>
>
> I would like to understand how admgpu tracks GPU time since I am not 
> getting these fields yet.
>
> 1)
> You suggest to have a timestamp because of different clock domains?
>
> 2)
> With the interval option - you actually have a restarting counter? Do 
> you keep that in the driver or get it from hw itself?
>
> Regards,
>
> Tvrtko