| Message ID | 20210715091820.1613726-7-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Per client GPU stats | expand |
Hi Tvrtko, Thanks for typing this up! On Thu, 15 Jul 2021 at 10:18, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > +Mandatory fully standardised keys > +--------------------------------- > + > +- drm-driver: <str> > + > +String shall contain a fixed string uniquely identified the driver handling > +the device in question. For example name of the respective kernel module. I think let's be more prescriptive and just say that it is the module name. > +Optional fully standardised keys > +-------------------------------- > + > +- drm-pdev: <aaaa:bb.cc.d> > + > +For PCI devices this should contain the PCI slot address of the device in > +question. How about just major:minor of the DRM render node device it's attached to? > +- drm-client-id: <uint> > + > +Unique value relating to the open DRM file descriptor used to distinguish > +duplicated and shared file descriptors. Conceptually the value should map 1:1 > +to the in kernel representation of `struct drm_file` instances. > + > +Uniqueness of the value shall be either globally unique, or unique within the > +scope of each device, in which case `drm-pdev` shall be present as well. > + > +Userspace should make sure to not double account any usage statistics by using > +the above described criteria in order to associate data to individual clients. > + > +- drm-engine-<str>: <uint> ns > + > +GPUs usually contain multiple execution engines. Each shall be given a stable > +and unique name (str), with possible values documented in the driver specific > +documentation. > + > +Value shall be in specified time units which the respective GPU engine spent > +busy executing workloads belonging to this client. > + > +Values are not required to be constantly monotonic if it makes the driver > +implementation easier, but are required to catch up with the previously reported > +larger value within a reasonable period. Upon observing a value lower than what > +was previously read, userspace is expected to stay with that larger previous > +value until a monotonic update is seen. Yeah, that would work well for Mali/Panfrost. We can queue multiple jobs in the hardware, which can either be striped across multiple cores with an affinity mask (e.g. 3 cores for your client and 1 for your compositor), or picked according to priority, or ... The fine-grained performance counters (e.g. time spent waiting for sampler) are only GPU-global. So if you have two jobs running simultaneously, you have no idea who's responsible for what. But it does give us coarse-grained counters which are accounted per-job-slot, including exactly this metric: amount of 'GPU time' (whatever that means) occupied by that job slot during the sampling period. So we could support that nicely if we fenced job-slot updates with register reads/writes. Something I'm missing though is how we enable this information. Seems like it would be best to either only do it whilst fdinfo is open (and re-read it whenever you need an update), or on a per-driver sysfs toggle, or ... ? > +- drm-memory-<str>: <uint> [KiB|MiB] > + > +Each possible memory type which can be used to store buffer objects by the > +GPU in question shall be given a stable and unique name to be returned as the > +string here. > + > +Value shall reflect the amount of storage currently consumed by the buffer > +object belong to this client, in the respective memory region. > + > +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' > +indicating kibi- or mebi-bytes. I'm a bit wary of the accounting here. Is it buffer allocations originating from the client, in which case it conceptually clashes with gralloc? Is it the client which last wrote to the buffer? The client with the oldest open handle to the buffer? Other? Cheers, Daniel
On Fri, Jul 23, 2021 at 05:43:01PM +0100, Daniel Stone wrote: > Hi Tvrtko, > Thanks for typing this up! > > On Thu, 15 Jul 2021 at 10:18, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > > +Mandatory fully standardised keys > > +--------------------------------- > > + > > +- drm-driver: <str> > > + > > +String shall contain a fixed string uniquely identified the driver handling > > +the device in question. For example name of the respective kernel module. > > I think let's be more prescriptive and just say that it is the module name. Just a quick comment on this one. drm_driver.name is already uapi, so let's please not invent a new one. The shared code should probably make sure drivers don't get this wrong. Maybe good if we document the getverion ioctl, which also exposes this, and then link between the two. -Daniel > > > +Optional fully standardised keys > > +-------------------------------- > > + > > +- drm-pdev: <aaaa:bb.cc.d> > > + > > +For PCI devices this should contain the PCI slot address of the device in > > +question. > > How about just major:minor of the DRM render node device it's attached to? > > > +- drm-client-id: <uint> > > + > > +Unique value relating to the open DRM file descriptor used to distinguish > > +duplicated and shared file descriptors. Conceptually the value should map 1:1 > > +to the in kernel representation of `struct drm_file` instances. > > + > > +Uniqueness of the value shall be either globally unique, or unique within the > > +scope of each device, in which case `drm-pdev` shall be present as well. > > + > > +Userspace should make sure to not double account any usage statistics by using > > +the above described criteria in order to associate data to individual clients. > > + > > +- drm-engine-<str>: <uint> ns > > + > > +GPUs usually contain multiple execution engines. Each shall be given a stable > > +and unique name (str), with possible values documented in the driver specific > > +documentation. > > + > > +Value shall be in specified time units which the respective GPU engine spent > > +busy executing workloads belonging to this client. > > + > > +Values are not required to be constantly monotonic if it makes the driver > > +implementation easier, but are required to catch up with the previously reported > > +larger value within a reasonable period. Upon observing a value lower than what > > +was previously read, userspace is expected to stay with that larger previous > > +value until a monotonic update is seen. > > Yeah, that would work well for Mali/Panfrost. We can queue multiple > jobs in the hardware, which can either be striped across multiple > cores with an affinity mask (e.g. 3 cores for your client and 1 for > your compositor), or picked according to priority, or ... > > The fine-grained performance counters (e.g. time spent waiting for > sampler) are only GPU-global. So if you have two jobs running > simultaneously, you have no idea who's responsible for what. > > But it does give us coarse-grained counters which are accounted > per-job-slot, including exactly this metric: amount of 'GPU time' > (whatever that means) occupied by that job slot during the sampling > period. So we could support that nicely if we fenced job-slot updates > with register reads/writes. > > Something I'm missing though is how we enable this information. Seems > like it would be best to either only do it whilst fdinfo is open (and > re-read it whenever you need an update), or on a per-driver sysfs > toggle, or ... ? > > > +- drm-memory-<str>: <uint> [KiB|MiB] > > + > > +Each possible memory type which can be used to store buffer objects by the > > +GPU in question shall be given a stable and unique name to be returned as the > > +string here. > > + > > +Value shall reflect the amount of storage currently consumed by the buffer > > +object belong to this client, in the respective memory region. > > + > > +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' > > +indicating kibi- or mebi-bytes. > > I'm a bit wary of the accounting here. Is it buffer allocations > originating from the client, in which case it conceptually clashes > with gralloc? Is it the client which last wrote to the buffer? The > client with the oldest open handle to the buffer? Other? > > Cheers, > Daniel
[AMD Official Use Only] I just want to make a comment that with this approach (the ns) calculating the percentage will take at least two reads of the fdinfo per pid over some time. Some engines may be able to provide a single shot percentage usage over an internal integration period. That is, for example, what we currently have implemented for that exact reason. I'd like to propose that we add an optional set of fields for this. Also, I may have missed a message, but why did we remove the timstamp? It is needed for accurate measurements of engine usage. David
On 23/07/2021 17:43, Daniel Stone wrote: > Hi Tvrtko, > Thanks for typing this up! > > On Thu, 15 Jul 2021 at 10:18, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> +Mandatory fully standardised keys >> +--------------------------------- >> + >> +- drm-driver: <str> >> + >> +String shall contain a fixed string uniquely identified the driver handling >> +the device in question. For example name of the respective kernel module. > > I think let's be more prescriptive and just say that it is the module name. I liked the drm_driver.name the other Daniel made so I'll go with that. >> +Optional fully standardised keys >> +-------------------------------- >> + >> +- drm-pdev: <aaaa:bb.cc.d> >> + >> +For PCI devices this should contain the PCI slot address of the device in >> +question. > > How about just major:minor of the DRM render node device it's attached to? I don't have a strong opinion on this one. I can add it, but might keep the drm-dev tag under the optional list because it is handy for intel_gpu_top multi-device support. Or maybe the lookup to pci device is easier than I think now so okay, on my todo list to check. >> +- drm-client-id: <uint> >> + >> +Unique value relating to the open DRM file descriptor used to distinguish >> +duplicated and shared file descriptors. Conceptually the value should map 1:1 >> +to the in kernel representation of `struct drm_file` instances. >> + >> +Uniqueness of the value shall be either globally unique, or unique within the >> +scope of each device, in which case `drm-pdev` shall be present as well. >> + >> +Userspace should make sure to not double account any usage statistics by using >> +the above described criteria in order to associate data to individual clients. >> + >> +- drm-engine-<str>: <uint> ns >> + >> +GPUs usually contain multiple execution engines. Each shall be given a stable >> +and unique name (str), with possible values documented in the driver specific >> +documentation. >> + >> +Value shall be in specified time units which the respective GPU engine spent >> +busy executing workloads belonging to this client. >> + >> +Values are not required to be constantly monotonic if it makes the driver >> +implementation easier, but are required to catch up with the previously reported >> +larger value within a reasonable period. Upon observing a value lower than what >> +was previously read, userspace is expected to stay with that larger previous >> +value until a monotonic update is seen. > > Yeah, that would work well for Mali/Panfrost. We can queue multiple > jobs in the hardware, which can either be striped across multiple > cores with an affinity mask (e.g. 3 cores for your client and 1 for > your compositor), or picked according to priority, or ... > > The fine-grained performance counters (e.g. time spent waiting for > sampler) are only GPU-global. So if you have two jobs running > simultaneously, you have no idea who's responsible for what. > > But it does give us coarse-grained counters which are accounted > per-job-slot, including exactly this metric: amount of 'GPU time' > (whatever that means) occupied by that job slot during the sampling > period. So we could support that nicely if we fenced job-slot updates > with register reads/writes. > > Something I'm missing though is how we enable this information. Seems > like it would be best to either only do it whilst fdinfo is open (and > re-read it whenever you need an update), or on a per-driver sysfs > toggle, or ... ? Presumably there is non-trivial cost for querying this data on your driver? Would it be workable to enable tracking on first use and stop some time after last? Just a thought which may have significant downsides from driver to driver. >> +- drm-memory-<str>: <uint> [KiB|MiB] >> + >> +Each possible memory type which can be used to store buffer objects by the >> +GPU in question shall be given a stable and unique name to be returned as the >> +string here. >> + >> +Value shall reflect the amount of storage currently consumed by the buffer >> +object belong to this client, in the respective memory region. >> + >> +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' >> +indicating kibi- or mebi-bytes. > > I'm a bit wary of the accounting here. Is it buffer allocations > originating from the client, in which case it conceptually clashes > with gralloc? Is it the client which last wrote to the buffer? The > client with the oldest open handle to the buffer? Other? Haven't looked into AMD code here so know what they export. Gralloc allocates buffer from it's own drm client and shares them or it is just a library which runs from a client context? Regards, Tvrtko
On 23/07/2021 18:45, Nieto, David M wrote: > [AMD Official Use Only] > > > I just want to make a comment that with this approach (the ns) > calculating the percentage will take at least two reads of the fdinfo > per pid over some time. Some engines may be able to provide a single > shot percentage usage over an internal integration period. That is, for > example, what we currently have implemented for that exact reason. > > I'd like to propose that we add an optional set of fields for this. Yes it is already like that in the text I've sent out. Because I was unclear how the amdgpu accounting works I called out for you guys to fill in the blanks in the last patch: """ Opens: * Does it work for AMD? * What are the semantics of AMD engine utilisation reported in percents? Can it align with what i915 does or needs to document the alternative in the specification document? """ """ -- drm-engine-<str>: <uint> ns +- drm-engine-<str>: <uint> [ns|%] ... +Where time unit is given as a percentage...[AMD folks to fill the semantics +and interpretation of that]... """ So if cumulative nanoseconds definitely do not work for you, could you please fill in those blanks? > Also, I may have missed a message, but why did we remove the timstamp? > It is needed for accurate measurements of engine usage. Hm I did not remove anything - I only renamed some of the fields output from amdgpu fdinfo. Regards, Tvrtko > David > ------------------------------------------------------------------------ > *From:* Daniel Vetter <daniel@ffwll.ch> > *Sent:* Friday, July 23, 2021 9:47 AM > *To:* Daniel Stone <daniel@fooishbar.org> > *Cc:* Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; intel-gfx > <Intel-gfx@lists.freedesktop.org>; Tvrtko Ursulin > <tvrtko.ursulin@intel.com>; Koenig, Christian > <Christian.Koenig@amd.com>; dri-devel <dri-devel@lists.freedesktop.org>; > Nieto, David M <David.Nieto@amd.com> > *Subject:* Re: [RFC 6/8] drm: Document fdinfo format specification > On Fri, Jul 23, 2021 at 05:43:01PM +0100, Daniel Stone wrote: >> Hi Tvrtko, >> Thanks for typing this up! >> >> On Thu, 15 Jul 2021 at 10:18, Tvrtko Ursulin >> <tvrtko.ursulin@linux.intel.com> wrote: >> > +Mandatory fully standardised keys >> > +--------------------------------- >> > + >> > +- drm-driver: <str> >> > + >> > +String shall contain a fixed string uniquely identified the driver handling >> > +the device in question. For example name of the respective kernel module. >> >> I think let's be more prescriptive and just say that it is the module name. > > Just a quick comment on this one. > > drm_driver.name is already uapi, so let's please not invent a new one. The > shared code should probably make sure drivers don't get this wrong. Maybe > good if we document the getverion ioctl, which also exposes this, and then > link between the two. > -Daniel > >> >> > +Optional fully standardised keys >> > +-------------------------------- >> > + >> > +- drm-pdev: <aaaa:bb.cc.d> >> > + >> > +For PCI devices this should contain the PCI slot address of the device in >> > +question. >> >> How about just major:minor of the DRM render node device it's attached to? >> >> > +- drm-client-id: <uint> >> > + >> > +Unique value relating to the open DRM file descriptor used to distinguish >> > +duplicated and shared file descriptors. Conceptually the value should map 1:1 >> > +to the in kernel representation of `struct drm_file` instances. >> > + >> > +Uniqueness of the value shall be either globally unique, or unique within the >> > +scope of each device, in which case `drm-pdev` shall be present as well. >> > + >> > +Userspace should make sure to not double account any usage statistics by using >> > +the above described criteria in order to associate data to individual clients. >> > + >> > +- drm-engine-<str>: <uint> ns >> > + >> > +GPUs usually contain multiple execution engines. Each shall be given a stable >> > +and unique name (str), with possible values documented in the driver specific >> > +documentation. >> > + >> > +Value shall be in specified time units which the respective GPU engine spent >> > +busy executing workloads belonging to this client. >> > + >> > +Values are not required to be constantly monotonic if it makes the driver >> > +implementation easier, but are required to catch up with the previously reported >> > +larger value within a reasonable period. Upon observing a value lower than what >> > +was previously read, userspace is expected to stay with that larger previous >> > +value until a monotonic update is seen. >> >> Yeah, that would work well for Mali/Panfrost. We can queue multiple >> jobs in the hardware, which can either be striped across multiple >> cores with an affinity mask (e.g. 3 cores for your client and 1 for >> your compositor), or picked according to priority, or ... >> >> The fine-grained performance counters (e.g. time spent waiting for >> sampler) are only GPU-global. So if you have two jobs running >> simultaneously, you have no idea who's responsible for what. >> >> But it does give us coarse-grained counters which are accounted >> per-job-slot, including exactly this metric: amount of 'GPU time' >> (whatever that means) occupied by that job slot during the sampling >> period. So we could support that nicely if we fenced job-slot updates >> with register reads/writes. >> >> Something I'm missing though is how we enable this information. Seems >> like it would be best to either only do it whilst fdinfo is open (and >> re-read it whenever you need an update), or on a per-driver sysfs >> toggle, or ... ? >> >> > +- drm-memory-<str>: <uint> [KiB|MiB] >> > + >> > +Each possible memory type which can be used to store buffer objects by the >> > +GPU in question shall be given a stable and unique name to be returned as the >> > +string here. >> > + >> > +Value shall reflect the amount of storage currently consumed by the buffer >> > +object belong to this client, in the respective memory region. >> > + >> > +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' >> > +indicating kibi- or mebi-bytes. >> >> I'm a bit wary of the accounting here. Is it buffer allocations >> originating from the client, in which case it conceptually clashes >> with gralloc? Is it the client which last wrote to the buffer? The >> client with the oldest open handle to the buffer? Other? >> >> Cheers, >> Daniel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cda2d9f95ced44d09f66c08d94df991da%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626556571460650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GrjAYg9tG2QX7z4BDaqa4wMPj2nFcvGo4xCmD8OzwNE%3D&reserved=0 > <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cda2d9f95ced44d09f66c08d94df991da%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626556571460650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GrjAYg9tG2QX7z4BDaqa4wMPj2nFcvGo4xCmD8OzwNE%3D&reserved=0>
diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst new file mode 100644 index 000000000000..78dc01c30e22 --- /dev/null +++ b/Documentation/gpu/drm-usage-stats.rst @@ -0,0 +1,97 @@ +.. _drm-client-usage-stats: + +====================== +DRM client usage stats +====================== + +DRM drivers can choose to export partly standardised text output via the +`fops->show_fdinfo()` as part of the driver specific file operations registered +in the `struct drm_driver` object registered with the DRM core. + +One purpose of this output is to enable writing as generic as practicaly +feasible `top(1)` like userspace monitoring tools. + +Given the differences between various DRM drivers the specification of the +output is split between common and driver specific parts. Having said that, +wherever possible effort should still be made to standardise as much as +possible. + +File format specification +========================= + +- File shall contain one key value pair per one line of text. +- Colon character (`:`) must be used to delimit keys and values. +- All keys shall be prefixed with `drm-`. +- Whitespace between the delimiter and first non-whitespace character shall be + ignored when parsing. +- Neither keys or values are allowed to contain whitespace characters. +- Numerical key value pairs can end with optional unit string. +- Data type of the value is fixed as defined in the specification. + +Key types +--------- + +1. Mandatory, fully standardised. +2. Optional, fully standardised. +3. Driver specific. + +Data types +---------- + +- <uint> - Unsigned integer without defining the maximum value. +- <str> - String excluding any above defined reserved characters or whitespace. + +Mandatory fully standardised keys +--------------------------------- + +- drm-driver: <str> + +String shall contain a fixed string uniquely identified the driver handling +the device in question. For example name of the respective kernel module. + +Optional fully standardised keys +-------------------------------- + +- drm-pdev: <aaaa:bb.cc.d> + +For PCI devices this should contain the PCI slot address of the device in +question. + +- drm-client-id: <uint> + +Unique value relating to the open DRM file descriptor used to distinguish +duplicated and shared file descriptors. Conceptually the value should map 1:1 +to the in kernel representation of `struct drm_file` instances. + +Uniqueness of the value shall be either globally unique, or unique within the +scope of each device, in which case `drm-pdev` shall be present as well. + +Userspace should make sure to not double account any usage statistics by using +the above described criteria in order to associate data to individual clients. + +- drm-engine-<str>: <uint> ns + +GPUs usually contain multiple execution engines. Each shall be given a stable +and unique name (str), with possible values documented in the driver specific +documentation. + +Value shall be in specified time units which the respective GPU engine spent +busy executing workloads belonging to this client. + +Values are not required to be constantly monotonic if it makes the driver +implementation easier, but are required to catch up with the previously reported +larger value within a reasonable period. Upon observing a value lower than what +was previously read, userspace is expected to stay with that larger previous +value until a monotonic update is seen. + +- drm-memory-<str>: <uint> [KiB|MiB] + +Each possible memory type which can be used to store buffer objects by the +GPU in question shall be given a stable and unique name to be returned as the +string here. + +Value shall reflect the amount of storage currently consumed by the buffer +object belong to this client, in the respective memory region. + +Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB' +indicating kibi- or mebi-bytes. diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst index b9c1214d8f23..b99dede9a5b1 100644 --- a/Documentation/gpu/index.rst +++ b/Documentation/gpu/index.rst @@ -10,6 +10,7 @@ Linux GPU Driver Developer's Guide drm-kms drm-kms-helpers drm-uapi + drm-usage-stats driver-uapi drm-client drivers