| Message ID | 20230609121143.1232420-7-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fdinfo memory stats | expand |
On 09-06-2023 17:41, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which > will return a reference to a newly created GEM objects (if created), in > order to enable tracking of imported i915 GEM objects in the following > patch. instead of this what if we implement the open call back in i915 struct drm_gem_object_funcs { /** * @open: * * Called upon GEM handle creation. * * This callback is optional. */ int (*open)(struct drm_gem_object *obj, struct drm_file *file); which gets called whenever a handle(drm_gem_handle_create_tail) is created and in the open we can check if to_intel_bo(obj)->base.dma_buf then it is imported if not it is owned or created by it. Thanks, Aravind. > > Minor code reshuffule and only trivial additions on top of > drm_gem_prime_fd_to_handle. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/drm_prime.c | 41 ++++++++++++++++++++++++++++++++----- > include/drm/drm_prime.h | 4 ++++ > 2 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index d29dafce9bb0..ef75f67e3057 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) > EXPORT_SYMBOL(drm_gem_dmabuf_release); > > /** > - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers > + * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers > * @dev: drm_device to import into > * @file_priv: drm file-private structure > * @prime_fd: fd id of the dma-buf which should be imported > * @handle: pointer to storage for the handle of the imported buffer object > + * @objp: optional pointer in which reference to created GEM object can be returned > * > * This is the PRIME import function which must be used mandatorily by GEM > * drivers to ensure correct lifetime management of the underlying GEM object. > @@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); > * > * Returns 0 on success or a negative error code on failure. > */ > -int drm_gem_prime_fd_to_handle(struct drm_device *dev, > - struct drm_file *file_priv, int prime_fd, > - uint32_t *handle) > +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, > + struct drm_file *file_priv, int prime_fd, > + uint32_t *handle, > + struct drm_gem_object **objp) > { > struct dma_buf *dma_buf; > struct drm_gem_object *obj; > @@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > > /* _handle_create_tail unconditionally unlocks dev->object_name_lock. */ > ret = drm_gem_handle_create_tail(file_priv, obj, handle); > - drm_gem_object_put(obj); > + if (!objp) > + drm_gem_object_put(obj); > if (ret) > goto out_put; > > @@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > > dma_buf_put(dma_buf); > > + if (objp) > + *objp = obj; > + > return 0; > > fail: > @@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > */ > drm_gem_handle_delete(file_priv, *handle); > dma_buf_put(dma_buf); > + if (objp) > + drm_gem_object_put(obj); > return ret; > > out_unlock: > @@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, > dma_buf_put(dma_buf); > return ret; > } > +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj); > + > +/** > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers > + * @dev: drm_device to import into > + * @file_priv: drm file-private structure > + * @prime_fd: fd id of the dma-buf which should be imported > + * @handle: pointer to storage for the handle of the imported buffer object > + * > + * This is the PRIME import function which must be used mandatorily by GEM > + * drivers to ensure correct lifetime management of the underlying GEM object. > + * The actual importing of GEM object from the dma-buf is done through the > + * &drm_driver.gem_prime_import driver callback. > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int drm_gem_prime_fd_to_handle(struct drm_device *dev, > + struct drm_file *file_priv, int prime_fd, > + uint32_t *handle) > +{ > + return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle, > + NULL); > +} > EXPORT_SYMBOL(drm_gem_prime_fd_to_handle); > > int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index 2a1d01e5b56b..10d145ce6586 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -69,6 +69,10 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); > > int drm_gem_prime_fd_to_handle(struct drm_device *dev, > struct drm_file *file_priv, int prime_fd, uint32_t *handle); > +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, > + struct drm_file *file_priv, int prime_fd, > + uint32_t *handle, > + struct drm_gem_object **obj); > int drm_gem_prime_handle_to_fd(struct drm_device *dev, > struct drm_file *file_priv, uint32_t handle, uint32_t flags, > int *prime_fd);
On 09/06/2023 13:44, Iddamsetty, Aravind wrote: > On 09-06-2023 17:41, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which >> will return a reference to a newly created GEM objects (if created), in >> order to enable tracking of imported i915 GEM objects in the following >> patch. > > instead of this what if we implement the open call back in i915 > > struct drm_gem_object_funcs { > > /** > * @open: > * > * Called upon GEM handle creation. > * > * This callback is optional. > */ > int (*open)(struct drm_gem_object *obj, struct drm_file *file); > > which gets called whenever a handle(drm_gem_handle_create_tail) is > created and in the open we can check if to_intel_bo(obj)->base.dma_buf > then it is imported if not it is owned or created by it. I wanted to track as much memory usage as we have which is buffer object backed, including page tables and contexts. And since those are not user visible (they don't have handles), they wouldn't be covered by the obj->funcs->open() callback. If we wanted to limit to objects with handles we could simply do what Rob proposed and that is to walk the handles idr. But that does not feel like the right direction to me. Open for discussion I guess. Regards, Tvrtko
On Fri, Jun 9, 2023 at 7:12 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 09/06/2023 13:44, Iddamsetty, Aravind wrote: > > On 09-06-2023 17:41, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which > >> will return a reference to a newly created GEM objects (if created), in > >> order to enable tracking of imported i915 GEM objects in the following > >> patch. > > > > instead of this what if we implement the open call back in i915 > > > > struct drm_gem_object_funcs { > > > > /** > > * @open: > > * > > * Called upon GEM handle creation. > > * > > * This callback is optional. > > */ > > int (*open)(struct drm_gem_object *obj, struct drm_file *file); > > > > which gets called whenever a handle(drm_gem_handle_create_tail) is > > created and in the open we can check if to_intel_bo(obj)->base.dma_buf > > then it is imported if not it is owned or created by it. > > I wanted to track as much memory usage as we have which is buffer object > backed, including page tables and contexts. And since those are not user > visible (they don't have handles), they wouldn't be covered by the > obj->funcs->open() callback. > > If we wanted to limit to objects with handles we could simply do what > Rob proposed and that is to walk the handles idr. But that does not feel > like the right direction to me. Open for discussion I guess. I guess you just have a few special case objects per context? Wouldn't it be easier to just track _those_ specially and append them to the results after doing the normal idr table walk? (Also, doing something special for dma-buf smells a bit odd.. considering that we also have legacy flink name based sharing as well.) BR, -R
On 09/06/2023 18:09, Rob Clark wrote: > On Fri, Jun 9, 2023 at 7:12 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 09/06/2023 13:44, Iddamsetty, Aravind wrote: >>> On 09-06-2023 17:41, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which >>>> will return a reference to a newly created GEM objects (if created), in >>>> order to enable tracking of imported i915 GEM objects in the following >>>> patch. >>> >>> instead of this what if we implement the open call back in i915 >>> >>> struct drm_gem_object_funcs { >>> >>> /** >>> * @open: >>> * >>> * Called upon GEM handle creation. >>> * >>> * This callback is optional. >>> */ >>> int (*open)(struct drm_gem_object *obj, struct drm_file *file); >>> >>> which gets called whenever a handle(drm_gem_handle_create_tail) is >>> created and in the open we can check if to_intel_bo(obj)->base.dma_buf >>> then it is imported if not it is owned or created by it. >> >> I wanted to track as much memory usage as we have which is buffer object >> backed, including page tables and contexts. And since those are not user >> visible (they don't have handles), they wouldn't be covered by the >> obj->funcs->open() callback. >> >> If we wanted to limit to objects with handles we could simply do what >> Rob proposed and that is to walk the handles idr. But that does not feel >> like the right direction to me. Open for discussion I guess. > > I guess you just have a few special case objects per context? Per context we have context image (register state etc) and ring buffer (command emission), per engine. Then we have all the page table backing store per each VM/ppgtt/context allocated as GEM objects. > Wouldn't it be easier to just track _those_ specially and append them > to the results after doing the normal idr table walk? In a way yes and in a way it is not as elegant. IMHO at least. > (Also, doing something special for dma-buf smells a bit odd.. > considering that we also have legacy flink name based sharing as > well.) It's not really special, just needed to return a tuple of (object, handle) from the prime import helper. So it can plug into the very same tracking I use from other paths. I was going for some kind of elegance with one loop - single tracking - as long as I had to add new list head to our buffer object. Anyway, I can re-spin a simplified version (fewer patches) with two loops. Only downside is that the new list head will only be used for internal objects. Regards, Tvrtko P.S. Flink I indeed missed. Is that used nowadays btw?
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index d29dafce9bb0..ef75f67e3057 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf) EXPORT_SYMBOL(drm_gem_dmabuf_release); /** - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers + * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers * @dev: drm_device to import into * @file_priv: drm file-private structure * @prime_fd: fd id of the dma-buf which should be imported * @handle: pointer to storage for the handle of the imported buffer object + * @objp: optional pointer in which reference to created GEM object can be returned * * This is the PRIME import function which must be used mandatorily by GEM * drivers to ensure correct lifetime management of the underlying GEM object. @@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); * * Returns 0 on success or a negative error code on failure. */ -int drm_gem_prime_fd_to_handle(struct drm_device *dev, - struct drm_file *file_priv, int prime_fd, - uint32_t *handle) +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle, + struct drm_gem_object **objp) { struct dma_buf *dma_buf; struct drm_gem_object *obj; @@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, /* _handle_create_tail unconditionally unlocks dev->object_name_lock. */ ret = drm_gem_handle_create_tail(file_priv, obj, handle); - drm_gem_object_put(obj); + if (!objp) + drm_gem_object_put(obj); if (ret) goto out_put; @@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, dma_buf_put(dma_buf); + if (objp) + *objp = obj; + return 0; fail: @@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, */ drm_gem_handle_delete(file_priv, *handle); dma_buf_put(dma_buf); + if (objp) + drm_gem_object_put(obj); return ret; out_unlock: @@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, dma_buf_put(dma_buf); return ret; } +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj); + +/** + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers + * @dev: drm_device to import into + * @file_priv: drm file-private structure + * @prime_fd: fd id of the dma-buf which should be imported + * @handle: pointer to storage for the handle of the imported buffer object + * + * This is the PRIME import function which must be used mandatorily by GEM + * drivers to ensure correct lifetime management of the underlying GEM object. + * The actual importing of GEM object from the dma-buf is done through the + * &drm_driver.gem_prime_import driver callback. + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_gem_prime_fd_to_handle(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle) +{ + return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle, + NULL); +} EXPORT_SYMBOL(drm_gem_prime_fd_to_handle); int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 2a1d01e5b56b..10d145ce6586 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -69,6 +69,10 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle, + struct drm_gem_object **obj); int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd);