[Intel-gfx] [RFC 3/8] drm/i915: Expose list of clients in sysfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-12-20 07:56:25)
> 
> On 19/12/2019 20:48, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-12-19 18:00:14)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> Expose a list of clients with open file handles in sysfs.
> >>
> >> This will be a basis for a top-like utility showing per-client and per-
> >> engine GPU load.
> >>
> >> Currently we only expose each client's pid and name under opaque numbered
> >> directories in /sys/class/drm/card0/clients/.
> >>
> >> For instance:
> >>
> >> /sys/class/drm/card0/clients/3/name: Xorg
> >> /sys/class/drm/card0/clients/3/pid: 5664
> >>
> >> v2:
> >>   Chris Wilson:
> >>   * Enclose new members into dedicated structs.
> >>   * Protect against failed sysfs registration.
> >>
> >> v3:
> >>   * sysfs_attr_init.
> >>
> >> v4:
> >>   * Fix for internal clients.
> >>
> >> v5:
> >>   * Use cyclic ida for client id. (Chris)
> >>   * Do not leak pid reference. (Chris)
> >>   * Tidy code with some locals.
> >>
> >> v6:
> >>   * Use xa_alloc_cyclic to simplify locking. (Chris)
> >>   * No need to unregister individial sysfs files. (Chris)
> >>   * Rebase on top of fpriv kref.
> >>   * Track client closed status and reflect in sysfs.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.h   |  20 +++++
> >>   drivers/gpu/drm/i915/i915_gem.c   | 133 ++++++++++++++++++++++++++++--
> >>   drivers/gpu/drm/i915/i915_sysfs.c |   8 ++
> >>   3 files changed, 155 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 6f13f0c619e9..e1d8361aafd7 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -188,6 +188,7 @@ struct i915_hotplug {
> >>   struct drm_i915_private;
> >>   struct i915_mm_struct;
> >>   struct i915_mmu_object;
> >> +struct i915_drm_clients;
> >>   
> >>   struct drm_i915_file_private {
> >>          struct kref kref;
> >> @@ -226,6 +227,19 @@ struct drm_i915_file_private {
> >>          /** ban_score: Accumulated score of all ctx bans and fast hangs. */
> >>          atomic_t ban_score;
> >>          unsigned long hang_timestamp;
> >> +
> >> +       struct i915_drm_client {
> > 
> > I agree with the distinction here between drm_client and
> > gem_client. (This concept will be required beyond GEM.)
> 
> So you think I should keep the i915_drm_client naming for these bits 
> throughout?

Yes -- with a bit of handwaving for an abstract interface to iterate
over the contexts. I do like the concept of having a sysfs/client
interface that GEM context plugs into, that is not exclusive to GEM
contexts. i.e. the focus should be on the information we need to present
via sysfs, and get those concepts embedded into the sysfs ABI.

So I liked the direction of having a standalone i915_drm_client for the
sysfs interface. [Who knows the end of GEM may be nigh!]
-Chris