| Message ID | 20231117214419.418556-1-Felix.Kuehling@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion" | expand |
Hi Am 17.11.23 um 22:44 schrieb Felix Kuehling: > This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. > > These helper functions are needed for KFD to export and import DMABufs > the right way without duplicating the tracking of DMABufs associated with > GEM objects while ensuring that move notifier callbacks are working as > intended. I'm unhappy to see these functions making a comeback. They are the boiler-plate logic that all drivers should use. Historically, drivers did a lot one things in their GEM code that was only semi-correct. Unifying most of that made the memory management more readable. Not giving back drivers to option of tinkering with this might be preferable. The rsp hooks in struct drm_driver, prime_fd_to_handle and prime_handle_to_fd, are only there for vmwgfx. If you want to hook into prime import and export, there are drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't it possible to move the additional code behind these pointers? Best regards Thomas > > CC: Christian König <christian.koenig@amd.com> > CC: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > --- > drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- > include/drm/drm_prime.h | 7 +++++++ > 2 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 63b709a67471..834a5e28abbe 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -278,7 +278,7 @@ 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 > * @dev: drm_device to import into > * @file_priv: drm file-private structure > @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); > * > * Returns 0 on success or a negative error code on failure. > */ > -static 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(struct drm_device *dev, > + struct drm_file *file_priv, int prime_fd, > + uint32_t *handle) > { > struct dma_buf *dma_buf; > struct drm_gem_object *obj; > @@ -360,6 +360,7 @@ static 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); > > int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > @@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > return dmabuf; > } > > -/* > +/** > * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > * @dev: dev to export the buffer from > * @file_priv: drm file-private structure > @@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > * The actual exporting from GEM object to a dma-buf is done through the > * &drm_gem_object_funcs.export callback. > */ > -static 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) > +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) > { > struct drm_gem_object *obj; > int ret = 0; > @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev, > > return ret; > } > +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); > * @obj: GEM object to export > * @flags: flags like DRM_CLOEXEC and DRM_RDWR > * > - * This is the implementation of the &drm_gem_object_funcs.export functions > - * for GEM drivers using the PRIME helpers. It is used as the default for > - * drivers that do not set their own. > + * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers > + * using the PRIME helpers. It is used as the default in > + * drm_gem_prime_handle_to_fd(). > */ > struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, > int flags) > @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); > * @dev: drm_device to import into > * @dma_buf: dma-buf object to import > * > - * This is the implementation of the gem_prime_import functions for GEM > - * drivers using the PRIME helpers. It is the default for drivers that do > - * not set their own &drm_driver.gem_prime_import. > + * This is the implementation of the gem_prime_import functions for GEM drivers > + * using the PRIME helpers. Drivers can use this as their > + * &drm_driver.gem_prime_import implementation. It is used as the default > + * implementation in drm_gem_prime_fd_to_handle(). > * > * Drivers must arrange to call drm_prime_gem_destroy() from their > * &drm_gem_object_funcs.free hook when using this function. > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index a7abf9f3e697..2a1d01e5b56b 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -60,12 +60,19 @@ enum dma_data_direction; > > struct drm_device; > struct drm_gem_object; > +struct drm_file; > > /* core prime functions */ > struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, > struct dma_buf_export_info *exp_info); > 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_handle_to_fd(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, uint32_t flags, > + int *prime_fd); > + > /* helper functions for exporting */ > int drm_gem_map_attach(struct dma_buf *dma_buf, > struct dma_buf_attachment *attach);
On 2023-11-20 6:54, Thomas Zimmermann wrote: > Hi > > Am 17.11.23 um 22:44 schrieb Felix Kuehling: >> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >> >> These helper functions are needed for KFD to export and import DMABufs >> the right way without duplicating the tracking of DMABufs associated >> with >> GEM objects while ensuring that move notifier callbacks are working as >> intended. > > I'm unhappy to see these functions making a comeback. They are the > boiler-plate logic that all drivers should use. Historically, drivers > did a lot one things in their GEM code that was only semi-correct. > Unifying most of that made the memory management more readable. Not > giving back drivers to option of tinkering with this might be > preferable. The rsp hooks in struct drm_driver, prime_fd_to_handle and > prime_handle_to_fd, are only there for vmwgfx. > > If you want to hook into prime import and export, there are > drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't it > possible to move the additional code behind these pointers? I'm not trying to hook into these functions, I'm just trying to call them. I'm not bringing back the .prime_handle_to_fd and .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to export and import DMAbuffers from a kernel mode context. I had incorrect or semi-correct ways of doing that by calling some driver-internal functions, but then my DMABufs aren't correctly linked with GEM handles in DRM and move notifiers in amdgpu aren't working correctly. Regards, Felix > > Best regards > Thomas > >> >> CC: Christian König <christian.koenig@amd.com> >> CC: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >> --- >> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >> include/drm/drm_prime.h | 7 +++++++ >> 2 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 63b709a67471..834a5e28abbe 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -278,7 +278,7 @@ 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 >> * @dev: drm_device to import into >> * @file_priv: drm file-private structure >> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >> * >> * Returns 0 on success or a negative error code on failure. >> */ >> -static 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(struct drm_device *dev, >> + struct drm_file *file_priv, int prime_fd, >> + uint32_t *handle) >> { >> struct dma_buf *dma_buf; >> struct drm_gem_object *obj; >> @@ -360,6 +360,7 @@ static 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); >> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -408,7 +409,7 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> return dmabuf; >> } >> -/* >> +/** >> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >> * @dev: dev to export the buffer from >> * @file_priv: drm file-private structure >> @@ -421,10 +422,10 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> * The actual exporting from GEM object to a dma-buf is done >> through the >> * &drm_gem_object_funcs.export callback. >> */ >> -static 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) >> +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) >> { >> struct drm_gem_object *obj; >> int ret = 0; >> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >> drm_device *dev, >> return ret; >> } >> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >> * @obj: GEM object to export >> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >> * >> - * This is the implementation of the &drm_gem_object_funcs.export >> functions >> - * for GEM drivers using the PRIME helpers. It is used as the >> default for >> - * drivers that do not set their own. >> + * This is the implementation of the &drm_gem_object_funcs.export >> functions for GEM drivers >> + * using the PRIME helpers. It is used as the default in >> + * drm_gem_prime_handle_to_fd(). >> */ >> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >> int flags) >> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >> * @dev: drm_device to import into >> * @dma_buf: dma-buf object to import >> * >> - * This is the implementation of the gem_prime_import functions for GEM >> - * drivers using the PRIME helpers. It is the default for drivers >> that do >> - * not set their own &drm_driver.gem_prime_import. >> + * This is the implementation of the gem_prime_import functions for >> GEM drivers >> + * using the PRIME helpers. Drivers can use this as their >> + * &drm_driver.gem_prime_import implementation. It is used as the >> default >> + * implementation in drm_gem_prime_fd_to_handle(). >> * >> * Drivers must arrange to call drm_prime_gem_destroy() from their >> * &drm_gem_object_funcs.free hook when using this function. >> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >> index a7abf9f3e697..2a1d01e5b56b 100644 >> --- a/include/drm/drm_prime.h >> +++ b/include/drm/drm_prime.h >> @@ -60,12 +60,19 @@ enum dma_data_direction; >> struct drm_device; >> struct drm_gem_object; >> +struct drm_file; >> /* core prime functions */ >> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >> struct dma_buf_export_info *exp_info); >> 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_handle_to_fd(struct drm_device *dev, >> + struct drm_file *file_priv, uint32_t handle, >> uint32_t flags, >> + int *prime_fd); >> + >> /* helper functions for exporting */ >> int drm_gem_map_attach(struct dma_buf *dma_buf, >> struct dma_buf_attachment *attach); >
Am 20.11.23 um 12:54 schrieb Thomas Zimmermann: > Hi > > Am 17.11.23 um 22:44 schrieb Felix Kuehling: >> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >> >> These helper functions are needed for KFD to export and import DMABufs >> the right way without duplicating the tracking of DMABufs associated >> with >> GEM objects while ensuring that move notifier callbacks are working as >> intended. > > I'm unhappy to see these functions making a comeback. They are the > boiler-plate logic that all drivers should use. Historically, drivers > did a lot one things in their GEM code that was only semi-correct. > Unifying most of that made the memory management more readable. Not > giving back drivers to option of tinkering with this might be > preferable. The rsp hooks in struct drm_driver, prime_fd_to_handle and > prime_handle_to_fd, are only there for vmwgfx. > > If you want to hook into prime import and export, there are > drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't it > possible to move the additional code behind these pointers? No, the problem here is that the KFD code wants to create DMA-buf exports for GEM buffers but from a different file descriptor than the DRM render or primary node. Essentially the KFD node is a separate file descriptor AMD GPUs came up with for supporting compute. Regards, Christian. > > Best regards > Thomas > >> >> CC: Christian König <christian.koenig@amd.com> >> CC: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >> --- >> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >> include/drm/drm_prime.h | 7 +++++++ >> 2 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 63b709a67471..834a5e28abbe 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -278,7 +278,7 @@ 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 >> * @dev: drm_device to import into >> * @file_priv: drm file-private structure >> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >> * >> * Returns 0 on success or a negative error code on failure. >> */ >> -static 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(struct drm_device *dev, >> + struct drm_file *file_priv, int prime_fd, >> + uint32_t *handle) >> { >> struct dma_buf *dma_buf; >> struct drm_gem_object *obj; >> @@ -360,6 +360,7 @@ static 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); >> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -408,7 +409,7 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> return dmabuf; >> } >> -/* >> +/** >> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >> * @dev: dev to export the buffer from >> * @file_priv: drm file-private structure >> @@ -421,10 +422,10 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> * The actual exporting from GEM object to a dma-buf is done >> through the >> * &drm_gem_object_funcs.export callback. >> */ >> -static 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) >> +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) >> { >> struct drm_gem_object *obj; >> int ret = 0; >> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >> drm_device *dev, >> return ret; >> } >> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >> * @obj: GEM object to export >> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >> * >> - * This is the implementation of the &drm_gem_object_funcs.export >> functions >> - * for GEM drivers using the PRIME helpers. It is used as the >> default for >> - * drivers that do not set their own. >> + * This is the implementation of the &drm_gem_object_funcs.export >> functions for GEM drivers >> + * using the PRIME helpers. It is used as the default in >> + * drm_gem_prime_handle_to_fd(). >> */ >> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >> int flags) >> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >> * @dev: drm_device to import into >> * @dma_buf: dma-buf object to import >> * >> - * This is the implementation of the gem_prime_import functions for GEM >> - * drivers using the PRIME helpers. It is the default for drivers >> that do >> - * not set their own &drm_driver.gem_prime_import. >> + * This is the implementation of the gem_prime_import functions for >> GEM drivers >> + * using the PRIME helpers. Drivers can use this as their >> + * &drm_driver.gem_prime_import implementation. It is used as the >> default >> + * implementation in drm_gem_prime_fd_to_handle(). >> * >> * Drivers must arrange to call drm_prime_gem_destroy() from their >> * &drm_gem_object_funcs.free hook when using this function. >> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >> index a7abf9f3e697..2a1d01e5b56b 100644 >> --- a/include/drm/drm_prime.h >> +++ b/include/drm/drm_prime.h >> @@ -60,12 +60,19 @@ enum dma_data_direction; >> struct drm_device; >> struct drm_gem_object; >> +struct drm_file; >> /* core prime functions */ >> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >> struct dma_buf_export_info *exp_info); >> 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_handle_to_fd(struct drm_device *dev, >> + struct drm_file *file_priv, uint32_t handle, >> uint32_t flags, >> + int *prime_fd); >> + >> /* helper functions for exporting */ >> int drm_gem_map_attach(struct dma_buf *dma_buf, >> struct dma_buf_attachment *attach); >
Hi Am 20.11.23 um 16:06 schrieb Felix Kuehling: > On 2023-11-20 6:54, Thomas Zimmermann wrote: >> Hi >> >> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>> >>> These helper functions are needed for KFD to export and import DMABufs >>> the right way without duplicating the tracking of DMABufs associated >>> with >>> GEM objects while ensuring that move notifier callbacks are working as >>> intended. >> >> I'm unhappy to see these functions making a comeback. They are the >> boiler-plate logic that all drivers should use. Historically, drivers >> did a lot one things in their GEM code that was only semi-correct. >> Unifying most of that made the memory management more readable. Not >> giving back drivers to option of tinkering with this might be >> preferable. The rsp hooks in struct drm_driver, prime_fd_to_handle and >> prime_handle_to_fd, are only there for vmwgfx. >> >> If you want to hook into prime import and export, there are >> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't it >> possible to move the additional code behind these pointers? > > I'm not trying to hook into these functions, I'm just trying to call > them. I'm not bringing back the .prime_handle_to_fd and > .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to > export and import DMAbuffers from a kernel mode context. I had incorrect > or semi-correct ways of doing that by calling some driver-internal > functions, but then my DMABufs aren't correctly linked with GEM handles > in DRM and move notifiers in amdgpu aren't working correctly. I understand that. But why don't you use drm_driver.gem_prime_import and drm_gem_object_funcs.export to add the amdkfd-specific code? These callbacks are being invoked from within drm_gem_prime_fd_to_handle() and drm_gem_prime_handle_to_fd() as part of the importing and exporting logic. With the intention of doing driver-specific things. Hence you should not have to re-export the internal drm_gem_prime_*_to_*() helpers. My question is if the existing hooks are not suitable for your needs. If so, how could we improve them? Best regards Thomas > > Regards, > Felix > > >> >> Best regards >> Thomas >> >>> >>> CC: Christian König <christian.koenig@amd.com> >>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>> --- >>> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >>> include/drm/drm_prime.h | 7 +++++++ >>> 2 files changed, 25 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 63b709a67471..834a5e28abbe 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -278,7 +278,7 @@ 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 >>> * @dev: drm_device to import into >>> * @file_priv: drm file-private structure >>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>> * >>> * Returns 0 on success or a negative error code on failure. >>> */ >>> -static 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(struct drm_device *dev, >>> + struct drm_file *file_priv, int prime_fd, >>> + uint32_t *handle) >>> { >>> struct dma_buf *dma_buf; >>> struct drm_gem_object *obj; >>> @@ -360,6 +360,7 @@ static 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); >>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >>> struct drm_file *file_priv) >>> @@ -408,7 +409,7 @@ static struct dma_buf >>> *export_and_register_object(struct drm_device *dev, >>> return dmabuf; >>> } >>> -/* >>> +/** >>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >>> * @dev: dev to export the buffer from >>> * @file_priv: drm file-private structure >>> @@ -421,10 +422,10 @@ static struct dma_buf >>> *export_and_register_object(struct drm_device *dev, >>> * The actual exporting from GEM object to a dma-buf is done >>> through the >>> * &drm_gem_object_funcs.export callback. >>> */ >>> -static 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) >>> +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) >>> { >>> struct drm_gem_object *obj; >>> int ret = 0; >>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>> drm_device *dev, >>> return ret; >>> } >>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >>> struct drm_file *file_priv) >>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>> * @obj: GEM object to export >>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>> * >>> - * This is the implementation of the &drm_gem_object_funcs.export >>> functions >>> - * for GEM drivers using the PRIME helpers. It is used as the >>> default for >>> - * drivers that do not set their own. >>> + * This is the implementation of the &drm_gem_object_funcs.export >>> functions for GEM drivers >>> + * using the PRIME helpers. It is used as the default in >>> + * drm_gem_prime_handle_to_fd(). >>> */ >>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>> int flags) >>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>> * @dev: drm_device to import into >>> * @dma_buf: dma-buf object to import >>> * >>> - * This is the implementation of the gem_prime_import functions for GEM >>> - * drivers using the PRIME helpers. It is the default for drivers >>> that do >>> - * not set their own &drm_driver.gem_prime_import. >>> + * This is the implementation of the gem_prime_import functions for >>> GEM drivers >>> + * using the PRIME helpers. Drivers can use this as their >>> + * &drm_driver.gem_prime_import implementation. It is used as the >>> default >>> + * implementation in drm_gem_prime_fd_to_handle(). >>> * >>> * Drivers must arrange to call drm_prime_gem_destroy() from their >>> * &drm_gem_object_funcs.free hook when using this function. >>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>> index a7abf9f3e697..2a1d01e5b56b 100644 >>> --- a/include/drm/drm_prime.h >>> +++ b/include/drm/drm_prime.h >>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>> struct drm_device; >>> struct drm_gem_object; >>> +struct drm_file; >>> /* core prime functions */ >>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>> struct dma_buf_export_info *exp_info); >>> 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_handle_to_fd(struct drm_device *dev, >>> + struct drm_file *file_priv, uint32_t handle, >>> uint32_t flags, >>> + int *prime_fd); >>> + >>> /* helper functions for exporting */ >>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>> struct dma_buf_attachment *attach); >>
Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: > Hi > > Am 20.11.23 um 16:06 schrieb Felix Kuehling: >> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>> >>>> These helper functions are needed for KFD to export and import DMABufs >>>> the right way without duplicating the tracking of DMABufs >>>> associated with >>>> GEM objects while ensuring that move notifier callbacks are working as >>>> intended. >>> >>> I'm unhappy to see these functions making a comeback. They are the >>> boiler-plate logic that all drivers should use. Historically, >>> drivers did a lot one things in their GEM code that was only >>> semi-correct. Unifying most of that made the memory management more >>> readable. Not giving back drivers to option of tinkering with this >>> might be preferable. The rsp hooks in struct drm_driver, >>> prime_fd_to_handle and prime_handle_to_fd, are only there for vmwgfx. >>> >>> If you want to hook into prime import and export, there are >>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>> it possible to move the additional code behind these pointers? >> >> I'm not trying to hook into these functions, I'm just trying to call >> them. I'm not bringing back the .prime_handle_to_fd and >> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >> export and import DMAbuffers from a kernel mode context. I had >> incorrect or semi-correct ways of doing that by calling some >> driver-internal functions, but then my DMABufs aren't correctly >> linked with GEM handles in DRM and move notifiers in amdgpu aren't >> working correctly. > > I understand that. But why don't you use drm_driver.gem_prime_import > and drm_gem_object_funcs.export to add the amdkfd-specific code? These > callbacks are being invoked from within drm_gem_prime_fd_to_handle() and > drm_gem_prime_handle_to_fd() as part of the importing and exporting > logic. With the intention of doing driver-specific things. Hence you > should not have to re-export the internal drm_gem_prime_*_to_*() helpers. > > My question is if the existing hooks are not suitable for your needs. > If so, how could we improve them? No no. You don't seem to understand the use case :) Felix doesn't try to implement any driver-specific things. What Felix tries to do is to export a DMA-buf handle from kernel space. Regards, Christian. > > Best regards > Thomas > > >> >> Regards, >> Felix >> >> >>> >>> Best regards >>> Thomas >>> >>>> >>>> CC: Christian König <christian.koenig@amd.com> >>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>> --- >>>> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >>>> include/drm/drm_prime.h | 7 +++++++ >>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>>> index 63b709a67471..834a5e28abbe 100644 >>>> --- a/drivers/gpu/drm/drm_prime.c >>>> +++ b/drivers/gpu/drm/drm_prime.c >>>> @@ -278,7 +278,7 @@ 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 >>>> * @dev: drm_device to import into >>>> * @file_priv: drm file-private structure >>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>> * >>>> * Returns 0 on success or a negative error code on failure. >>>> */ >>>> -static 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(struct drm_device *dev, >>>> + struct drm_file *file_priv, int prime_fd, >>>> + uint32_t *handle) >>>> { >>>> struct dma_buf *dma_buf; >>>> struct drm_gem_object *obj; >>>> @@ -360,6 +360,7 @@ static 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); >>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>> *data, >>>> struct drm_file *file_priv) >>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>> *export_and_register_object(struct drm_device *dev, >>>> return dmabuf; >>>> } >>>> -/* >>>> +/** >>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>> drivers >>>> * @dev: dev to export the buffer from >>>> * @file_priv: drm file-private structure >>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>> *export_and_register_object(struct drm_device *dev, >>>> * The actual exporting from GEM object to a dma-buf is done >>>> through the >>>> * &drm_gem_object_funcs.export callback. >>>> */ >>>> -static 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) >>>> +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) >>>> { >>>> struct drm_gem_object *obj; >>>> int ret = 0; >>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>> drm_device *dev, >>>> return ret; >>>> } >>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>> *data, >>>> struct drm_file *file_priv) >>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>> * @obj: GEM object to export >>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>> * >>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>> functions >>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>> default for >>>> - * drivers that do not set their own. >>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>> functions for GEM drivers >>>> + * using the PRIME helpers. It is used as the default in >>>> + * drm_gem_prime_handle_to_fd(). >>>> */ >>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>> int flags) >>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>> * @dev: drm_device to import into >>>> * @dma_buf: dma-buf object to import >>>> * >>>> - * This is the implementation of the gem_prime_import functions >>>> for GEM >>>> - * drivers using the PRIME helpers. It is the default for drivers >>>> that do >>>> - * not set their own &drm_driver.gem_prime_import. >>>> + * This is the implementation of the gem_prime_import functions >>>> for GEM drivers >>>> + * using the PRIME helpers. Drivers can use this as their >>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>> default >>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>> * >>>> * Drivers must arrange to call drm_prime_gem_destroy() from their >>>> * &drm_gem_object_funcs.free hook when using this function. >>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>> --- a/include/drm/drm_prime.h >>>> +++ b/include/drm/drm_prime.h >>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>> struct drm_device; >>>> struct drm_gem_object; >>>> +struct drm_file; >>>> /* core prime functions */ >>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>> struct dma_buf_export_info *exp_info); >>>> 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_handle_to_fd(struct drm_device *dev, >>>> + struct drm_file *file_priv, uint32_t handle, >>>> uint32_t flags, >>>> + int *prime_fd); >>>> + >>>> /* helper functions for exporting */ >>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>> struct dma_buf_attachment *attach); >>> >
Hi Christian Am 20.11.23 um 16:22 schrieb Christian König: > Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >> Hi >> >> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>> Hi >>>> >>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>> >>>>> These helper functions are needed for KFD to export and import DMABufs >>>>> the right way without duplicating the tracking of DMABufs >>>>> associated with >>>>> GEM objects while ensuring that move notifier callbacks are working as >>>>> intended. >>>> >>>> I'm unhappy to see these functions making a comeback. They are the >>>> boiler-plate logic that all drivers should use. Historically, >>>> drivers did a lot one things in their GEM code that was only >>>> semi-correct. Unifying most of that made the memory management more >>>> readable. Not giving back drivers to option of tinkering with this >>>> might be preferable. The rsp hooks in struct drm_driver, >>>> prime_fd_to_handle and prime_handle_to_fd, are only there for vmwgfx. >>>> >>>> If you want to hook into prime import and export, there are >>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>> it possible to move the additional code behind these pointers? >>> >>> I'm not trying to hook into these functions, I'm just trying to call >>> them. I'm not bringing back the .prime_handle_to_fd and >>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >>> export and import DMAbuffers from a kernel mode context. I had >>> incorrect or semi-correct ways of doing that by calling some >>> driver-internal functions, but then my DMABufs aren't correctly >>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>> working correctly. >> >> I understand that. But why don't you use drm_driver.gem_prime_import >> and drm_gem_object_funcs.export to add the amdkfd-specific code? These >> callbacks are being invoked from within drm_gem_prime_fd_to_handle() and >> drm_gem_prime_handle_to_fd() as part of the importing and exporting >> logic. With the intention of doing driver-specific things. Hence you >> should not have to re-export the internal drm_gem_prime_*_to_*() helpers. >> >> My question is if the existing hooks are not suitable for your needs. >> If so, how could we improve them? > > No no. You don't seem to understand the use case :) Felix doesn't try to > implement any driver-specific things. I meant that I understand that this patchset is not about setting drm_driver.prime_handle_to_fd, et al. > > What Felix tries to do is to export a DMA-buf handle from kernel space. For example, looking at patch 2, it converts a GEM handle to a file descriptor and then assigns the rsp dmabuf to mem, which is of type struct kgd_mem. From my impression, this could be done within the existing ->export hook. Then there's close_fd(), which cannot go into ->export. It looks like the fd isn't really required. Could the drm_prime_handle_to_fd() be reworked into a helper that converts the handle to the dmabuf without the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would then be exported? And I have the question wrt the 3rd patch; just that it's about importing. (Looking further through the code, it appears that the fd could be removed from the helpers, the callbacks and vmwgfx. It would then be used entirely in the ioctl entry points, such as drm_prime_fd_to_handle_ioctl().) Best regards Thomas > > Regards, > Christian. > >> >> Best regards >> Thomas >> >> >>> >>> Regards, >>> Felix >>> >>> >>>> >>>> Best regards >>>> Thomas >>>> >>>>> >>>>> CC: Christian König <christian.koenig@amd.com> >>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>> --- >>>>> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >>>>> include/drm/drm_prime.h | 7 +++++++ >>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>>>> index 63b709a67471..834a5e28abbe 100644 >>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>> @@ -278,7 +278,7 @@ 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 >>>>> * @dev: drm_device to import into >>>>> * @file_priv: drm file-private structure >>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>> * >>>>> * Returns 0 on success or a negative error code on failure. >>>>> */ >>>>> -static 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(struct drm_device *dev, >>>>> + struct drm_file *file_priv, int prime_fd, >>>>> + uint32_t *handle) >>>>> { >>>>> struct dma_buf *dma_buf; >>>>> struct drm_gem_object *obj; >>>>> @@ -360,6 +360,7 @@ static 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); >>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>>> *data, >>>>> struct drm_file *file_priv) >>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>> *export_and_register_object(struct drm_device *dev, >>>>> return dmabuf; >>>>> } >>>>> -/* >>>>> +/** >>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>> drivers >>>>> * @dev: dev to export the buffer from >>>>> * @file_priv: drm file-private structure >>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>> *export_and_register_object(struct drm_device *dev, >>>>> * The actual exporting from GEM object to a dma-buf is done >>>>> through the >>>>> * &drm_gem_object_funcs.export callback. >>>>> */ >>>>> -static 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) >>>>> +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) >>>>> { >>>>> struct drm_gem_object *obj; >>>>> int ret = 0; >>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>> drm_device *dev, >>>>> return ret; >>>>> } >>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>>> *data, >>>>> struct drm_file *file_priv) >>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>> * @obj: GEM object to export >>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>> * >>>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>>> functions >>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>> default for >>>>> - * drivers that do not set their own. >>>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>>> functions for GEM drivers >>>>> + * using the PRIME helpers. It is used as the default in >>>>> + * drm_gem_prime_handle_to_fd(). >>>>> */ >>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>>> int flags) >>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>> * @dev: drm_device to import into >>>>> * @dma_buf: dma-buf object to import >>>>> * >>>>> - * This is the implementation of the gem_prime_import functions >>>>> for GEM >>>>> - * drivers using the PRIME helpers. It is the default for drivers >>>>> that do >>>>> - * not set their own &drm_driver.gem_prime_import. >>>>> + * This is the implementation of the gem_prime_import functions >>>>> for GEM drivers >>>>> + * using the PRIME helpers. Drivers can use this as their >>>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>>> default >>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>> * >>>>> * Drivers must arrange to call drm_prime_gem_destroy() from their >>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>> --- a/include/drm/drm_prime.h >>>>> +++ b/include/drm/drm_prime.h >>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>> struct drm_device; >>>>> struct drm_gem_object; >>>>> +struct drm_file; >>>>> /* core prime functions */ >>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>> struct dma_buf_export_info *exp_info); >>>>> 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_handle_to_fd(struct drm_device *dev, >>>>> + struct drm_file *file_priv, uint32_t handle, >>>>> uint32_t flags, >>>>> + int *prime_fd); >>>>> + >>>>> /* helper functions for exporting */ >>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>> struct dma_buf_attachment *attach); >>>> >> >
On 2023-11-20 11:02, Thomas Zimmermann wrote: > Hi Christian > > Am 20.11.23 um 16:22 schrieb Christian König: >> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >>> Hi >>> >>> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>>> Hi >>>>> >>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>>> >>>>>> These helper functions are needed for KFD to export and import DMABufs >>>>>> the right way without duplicating the tracking of DMABufs >>>>>> associated with >>>>>> GEM objects while ensuring that move notifier callbacks are working as >>>>>> intended. >>>>> I'm unhappy to see these functions making a comeback. They are the >>>>> boiler-plate logic that all drivers should use. Historically, >>>>> drivers did a lot one things in their GEM code that was only >>>>> semi-correct. Unifying most of that made the memory management more >>>>> readable. Not giving back drivers to option of tinkering with this >>>>> might be preferable. The rsp hooks in struct drm_driver, >>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for vmwgfx. >>>>> >>>>> If you want to hook into prime import and export, there are >>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>>> it possible to move the additional code behind these pointers? >>>> I'm not trying to hook into these functions, I'm just trying to call >>>> them. I'm not bringing back the .prime_handle_to_fd and >>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >>>> export and import DMAbuffers from a kernel mode context. I had >>>> incorrect or semi-correct ways of doing that by calling some >>>> driver-internal functions, but then my DMABufs aren't correctly >>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>>> working correctly. >>> I understand that. But why don't you use drm_driver.gem_prime_import >>> and drm_gem_object_funcs.export to add the amdkfd-specific code? These >>> callbacks are being invoked from within drm_gem_prime_fd_to_handle() and >>> drm_gem_prime_handle_to_fd() as part of the importing and exporting >>> logic. With the intention of doing driver-specific things. Hence you >>> should not have to re-export the internal drm_gem_prime_*_to_*() helpers. >>> >>> My question is if the existing hooks are not suitable for your needs. >>> If so, how could we improve them? >> No no. You don't seem to understand the use case :) Felix doesn't try to >> implement any driver-specific things. > I meant that I understand that this patchset is not about setting > drm_driver.prime_handle_to_fd, et al. > >> What Felix tries to do is to export a DMA-buf handle from kernel space. > For example, looking at patch 2, it converts a GEM handle to a file > descriptor and then assigns the rsp dmabuf to mem, which is of type > struct kgd_mem. From my impression, this could be done within the > existing ->export hook. That would skip the call to export_and_register_object. I think that's what I'm currently missing to set up gem_obj->dmabuf. Regards, Felix > > Then there's close_fd(), which cannot go into ->export. It looks like > the fd isn't really required. Could the drm_prime_handle_to_fd() be > reworked into a helper that converts the handle to the dmabuf without > the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would > then be exported? > > And I have the question wrt the 3rd patch; just that it's about importing. > > (Looking further through the code, it appears that the fd could be > removed from the helpers, the callbacks and vmwgfx. It would then be > used entirely in the ioctl entry points, such as > drm_prime_fd_to_handle_ioctl().) > > Best regards > Thomas > > >> Regards, >> Christian. >> >>> Best regards >>> Thomas >>> >>> >>>> Regards, >>>> Felix >>>> >>>> >>>>> Best regards >>>>> Thomas >>>>> >>>>>> CC: Christian König <christian.koenig@amd.com> >>>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >>>>>> include/drm/drm_prime.h | 7 +++++++ >>>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>>>>> index 63b709a67471..834a5e28abbe 100644 >>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>> @@ -278,7 +278,7 @@ 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 >>>>>> * @dev: drm_device to import into >>>>>> * @file_priv: drm file-private structure >>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>>> * >>>>>> * Returns 0 on success or a negative error code on failure. >>>>>> */ >>>>>> -static 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(struct drm_device *dev, >>>>>> + struct drm_file *file_priv, int prime_fd, >>>>>> + uint32_t *handle) >>>>>> { >>>>>> struct dma_buf *dma_buf; >>>>>> struct drm_gem_object *obj; >>>>>> @@ -360,6 +360,7 @@ static 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); >>>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>>>> *data, >>>>>> struct drm_file *file_priv) >>>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>>> *export_and_register_object(struct drm_device *dev, >>>>>> return dmabuf; >>>>>> } >>>>>> -/* >>>>>> +/** >>>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>>> drivers >>>>>> * @dev: dev to export the buffer from >>>>>> * @file_priv: drm file-private structure >>>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>>> *export_and_register_object(struct drm_device *dev, >>>>>> * The actual exporting from GEM object to a dma-buf is done >>>>>> through the >>>>>> * &drm_gem_object_funcs.export callback. >>>>>> */ >>>>>> -static 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) >>>>>> +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) >>>>>> { >>>>>> struct drm_gem_object *obj; >>>>>> int ret = 0; >>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>>> drm_device *dev, >>>>>> return ret; >>>>>> } >>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>>>> *data, >>>>>> struct drm_file *file_priv) >>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>>> * @obj: GEM object to export >>>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>>> * >>>>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>>>> functions >>>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>>> default for >>>>>> - * drivers that do not set their own. >>>>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>>>> functions for GEM drivers >>>>>> + * using the PRIME helpers. It is used as the default in >>>>>> + * drm_gem_prime_handle_to_fd(). >>>>>> */ >>>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>>>> int flags) >>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>>> * @dev: drm_device to import into >>>>>> * @dma_buf: dma-buf object to import >>>>>> * >>>>>> - * This is the implementation of the gem_prime_import functions >>>>>> for GEM >>>>>> - * drivers using the PRIME helpers. It is the default for drivers >>>>>> that do >>>>>> - * not set their own &drm_driver.gem_prime_import. >>>>>> + * This is the implementation of the gem_prime_import functions >>>>>> for GEM drivers >>>>>> + * using the PRIME helpers. Drivers can use this as their >>>>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>>>> default >>>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>>> * >>>>>> * Drivers must arrange to call drm_prime_gem_destroy() from their >>>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>>> --- a/include/drm/drm_prime.h >>>>>> +++ b/include/drm/drm_prime.h >>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>>> struct drm_device; >>>>>> struct drm_gem_object; >>>>>> +struct drm_file; >>>>>> /* core prime functions */ >>>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>>> struct dma_buf_export_info *exp_info); >>>>>> 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_handle_to_fd(struct drm_device *dev, >>>>>> + struct drm_file *file_priv, uint32_t handle, >>>>>> uint32_t flags, >>>>>> + int *prime_fd); >>>>>> + >>>>>> /* helper functions for exporting */ >>>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>>> struct dma_buf_attachment *attach);
Am 20.11.23 um 17:15 schrieb Felix Kuehling: > > On 2023-11-20 11:02, Thomas Zimmermann wrote: >> Hi Christian >> >> Am 20.11.23 um 16:22 schrieb Christian König: >>> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >>>> Hi >>>> >>>> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>>>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>>>> Hi >>>>>> >>>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>>>> >>>>>>> These helper functions are needed for KFD to export and import >>>>>>> DMABufs >>>>>>> the right way without duplicating the tracking of DMABufs >>>>>>> associated with >>>>>>> GEM objects while ensuring that move notifier callbacks are >>>>>>> working as >>>>>>> intended. >>>>>> I'm unhappy to see these functions making a comeback. They are the >>>>>> boiler-plate logic that all drivers should use. Historically, >>>>>> drivers did a lot one things in their GEM code that was only >>>>>> semi-correct. Unifying most of that made the memory management more >>>>>> readable. Not giving back drivers to option of tinkering with this >>>>>> might be preferable. The rsp hooks in struct drm_driver, >>>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for >>>>>> vmwgfx. >>>>>> >>>>>> If you want to hook into prime import and export, there are >>>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>>>> it possible to move the additional code behind these pointers? >>>>> I'm not trying to hook into these functions, I'm just trying to call >>>>> them. I'm not bringing back the .prime_handle_to_fd and >>>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >>>>> export and import DMAbuffers from a kernel mode context. I had >>>>> incorrect or semi-correct ways of doing that by calling some >>>>> driver-internal functions, but then my DMABufs aren't correctly >>>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>>>> working correctly. >>>> I understand that. But why don't you use drm_driver.gem_prime_import >>>> and drm_gem_object_funcs.export to add the amdkfd-specific code? These >>>> callbacks are being invoked from within >>>> drm_gem_prime_fd_to_handle() and >>>> drm_gem_prime_handle_to_fd() as part of the importing and exporting >>>> logic. With the intention of doing driver-specific things. Hence you >>>> should not have to re-export the internal drm_gem_prime_*_to_*() >>>> helpers. >>>> >>>> My question is if the existing hooks are not suitable for your needs. >>>> If so, how could we improve them? >>> No no. You don't seem to understand the use case :) Felix doesn't >>> try to >>> implement any driver-specific things. >> I meant that I understand that this patchset is not about setting >> drm_driver.prime_handle_to_fd, et al. >> >>> What Felix tries to do is to export a DMA-buf handle from kernel space. >> For example, looking at patch 2, it converts a GEM handle to a file >> descriptor and then assigns the rsp dmabuf to mem, which is of type >> struct kgd_mem. From my impression, this could be done within the >> existing ->export hook. > > That would skip the call to export_and_register_object. I think that's > what I'm currently missing to set up gem_obj->dmabuf. I think we are talking past each other. kgd_mem is not part of the amdgpu driver, so it can't go into an ->export callback. What happens here is that a drm_client code uses the amdgpu driver to export some DMA-buf to file descriptors. In other words this is a high level user of drm_client and not something driver internal. If you don't want to export the drm_gem_prime_handle_to_fd() function directly we could add some drm_client_buffer_export() or similar. Regards, Christian. > > Regards, > Felix > > >> >> Then there's close_fd(), which cannot go into ->export. It looks like >> the fd isn't really required. Could the drm_prime_handle_to_fd() be >> reworked into a helper that converts the handle to the dmabuf without >> the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would >> then be exported? >> >> And I have the question wrt the 3rd patch; just that it's about >> importing. >> >> (Looking further through the code, it appears that the fd could be >> removed from the helpers, the callbacks and vmwgfx. It would then be >> used entirely in the ioctl entry points, such as >> drm_prime_fd_to_handle_ioctl().) >> >> Best regards >> Thomas >> >> >>> Regards, >>> Christian. >>> >>>> Best regards >>>> Thomas >>>> >>>> >>>>> Regards, >>>>> Felix >>>>> >>>>> >>>>>> Best regards >>>>>> Thomas >>>>>> >>>>>>> CC: Christian König <christian.koenig@amd.com> >>>>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/drm_prime.c | 33 >>>>>>> ++++++++++++++++++--------------- >>>>>>> include/drm/drm_prime.h | 7 +++++++ >>>>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>>>> b/drivers/gpu/drm/drm_prime.c >>>>>>> index 63b709a67471..834a5e28abbe 100644 >>>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>>> @@ -278,7 +278,7 @@ 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 >>>>>>> * @dev: drm_device to import into >>>>>>> * @file_priv: drm file-private structure >>>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>>>> * >>>>>>> * Returns 0 on success or a negative error code on failure. >>>>>>> */ >>>>>>> -static 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(struct drm_device *dev, >>>>>>> + struct drm_file *file_priv, int prime_fd, >>>>>>> + uint32_t *handle) >>>>>>> { >>>>>>> struct dma_buf *dma_buf; >>>>>>> struct drm_gem_object *obj; >>>>>>> @@ -360,6 +360,7 @@ static 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); >>>>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>>>>> *data, >>>>>>> struct drm_file *file_priv) >>>>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>> return dmabuf; >>>>>>> } >>>>>>> -/* >>>>>>> +/** >>>>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>>>> drivers >>>>>>> * @dev: dev to export the buffer from >>>>>>> * @file_priv: drm file-private structure >>>>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>> * The actual exporting from GEM object to a dma-buf is done >>>>>>> through the >>>>>>> * &drm_gem_object_funcs.export callback. >>>>>>> */ >>>>>>> -static 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) >>>>>>> +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) >>>>>>> { >>>>>>> struct drm_gem_object *obj; >>>>>>> int ret = 0; >>>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>>>> drm_device *dev, >>>>>>> return ret; >>>>>>> } >>>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>>>>> *data, >>>>>>> struct drm_file *file_priv) >>>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>>>> * @obj: GEM object to export >>>>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>>>> * >>>>>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>>>>> functions >>>>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>>>> default for >>>>>>> - * drivers that do not set their own. >>>>>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>>>>> functions for GEM drivers >>>>>>> + * using the PRIME helpers. It is used as the default in >>>>>>> + * drm_gem_prime_handle_to_fd(). >>>>>>> */ >>>>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>>>>> int flags) >>>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>>>> * @dev: drm_device to import into >>>>>>> * @dma_buf: dma-buf object to import >>>>>>> * >>>>>>> - * This is the implementation of the gem_prime_import functions >>>>>>> for GEM >>>>>>> - * drivers using the PRIME helpers. It is the default for drivers >>>>>>> that do >>>>>>> - * not set their own &drm_driver.gem_prime_import. >>>>>>> + * This is the implementation of the gem_prime_import functions >>>>>>> for GEM drivers >>>>>>> + * using the PRIME helpers. Drivers can use this as their >>>>>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>>>>> default >>>>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>>>> * >>>>>>> * Drivers must arrange to call drm_prime_gem_destroy() from >>>>>>> their >>>>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>>>> --- a/include/drm/drm_prime.h >>>>>>> +++ b/include/drm/drm_prime.h >>>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>>>> struct drm_device; >>>>>>> struct drm_gem_object; >>>>>>> +struct drm_file; >>>>>>> /* core prime functions */ >>>>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>>>> struct dma_buf_export_info *exp_info); >>>>>>> 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_handle_to_fd(struct drm_device *dev, >>>>>>> + struct drm_file *file_priv, uint32_t handle, >>>>>>> uint32_t flags, >>>>>>> + int *prime_fd); >>>>>>> + >>>>>>> /* helper functions for exporting */ >>>>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>>>> struct dma_buf_attachment *attach);
Hi Am 20.11.23 um 17:15 schrieb Felix Kuehling: > > On 2023-11-20 11:02, Thomas Zimmermann wrote: >> Hi Christian >> >> Am 20.11.23 um 16:22 schrieb Christian König: >>> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >>>> Hi >>>> >>>> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>>>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>>>> Hi >>>>>> >>>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>>>> >>>>>>> These helper functions are needed for KFD to export and import >>>>>>> DMABufs >>>>>>> the right way without duplicating the tracking of DMABufs >>>>>>> associated with >>>>>>> GEM objects while ensuring that move notifier callbacks are >>>>>>> working as >>>>>>> intended. >>>>>> I'm unhappy to see these functions making a comeback. They are the >>>>>> boiler-plate logic that all drivers should use. Historically, >>>>>> drivers did a lot one things in their GEM code that was only >>>>>> semi-correct. Unifying most of that made the memory management more >>>>>> readable. Not giving back drivers to option of tinkering with this >>>>>> might be preferable. The rsp hooks in struct drm_driver, >>>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for vmwgfx. >>>>>> >>>>>> If you want to hook into prime import and export, there are >>>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>>>> it possible to move the additional code behind these pointers? >>>>> I'm not trying to hook into these functions, I'm just trying to call >>>>> them. I'm not bringing back the .prime_handle_to_fd and >>>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >>>>> export and import DMAbuffers from a kernel mode context. I had >>>>> incorrect or semi-correct ways of doing that by calling some >>>>> driver-internal functions, but then my DMABufs aren't correctly >>>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>>>> working correctly. >>>> I understand that. But why don't you use drm_driver.gem_prime_import >>>> and drm_gem_object_funcs.export to add the amdkfd-specific code? These >>>> callbacks are being invoked from within drm_gem_prime_fd_to_handle() >>>> and >>>> drm_gem_prime_handle_to_fd() as part of the importing and exporting >>>> logic. With the intention of doing driver-specific things. Hence you >>>> should not have to re-export the internal drm_gem_prime_*_to_*() >>>> helpers. >>>> >>>> My question is if the existing hooks are not suitable for your needs. >>>> If so, how could we improve them? >>> No no. You don't seem to understand the use case :) Felix doesn't try to >>> implement any driver-specific things. >> I meant that I understand that this patchset is not about setting >> drm_driver.prime_handle_to_fd, et al. >> >>> What Felix tries to do is to export a DMA-buf handle from kernel space. >> For example, looking at patch 2, it converts a GEM handle to a file >> descriptor and then assigns the rsp dmabuf to mem, which is of type >> struct kgd_mem. From my impression, this could be done within the >> existing ->export hook. > > That would skip the call to export_and_register_object. I think that's > what I'm currently missing to set up gem_obj->dmabuf. Well, OK. Nevermind. Best regards Thomas > > Regards, > Felix > > >> >> Then there's close_fd(), which cannot go into ->export. It looks like >> the fd isn't really required. Could the drm_prime_handle_to_fd() be >> reworked into a helper that converts the handle to the dmabuf without >> the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would >> then be exported? >> >> And I have the question wrt the 3rd patch; just that it's about >> importing. >> >> (Looking further through the code, it appears that the fd could be >> removed from the helpers, the callbacks and vmwgfx. It would then be >> used entirely in the ioctl entry points, such as >> drm_prime_fd_to_handle_ioctl().) >> >> Best regards >> Thomas >> >> >>> Regards, >>> Christian. >>> >>>> Best regards >>>> Thomas >>>> >>>> >>>>> Regards, >>>>> Felix >>>>> >>>>> >>>>>> Best regards >>>>>> Thomas >>>>>> >>>>>>> CC: Christian König <christian.koenig@amd.com> >>>>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/drm_prime.c | 33 >>>>>>> ++++++++++++++++++--------------- >>>>>>> include/drm/drm_prime.h | 7 +++++++ >>>>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>>>> b/drivers/gpu/drm/drm_prime.c >>>>>>> index 63b709a67471..834a5e28abbe 100644 >>>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>>> @@ -278,7 +278,7 @@ 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 >>>>>>> * @dev: drm_device to import into >>>>>>> * @file_priv: drm file-private structure >>>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>>>> * >>>>>>> * Returns 0 on success or a negative error code on failure. >>>>>>> */ >>>>>>> -static 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(struct drm_device *dev, >>>>>>> + struct drm_file *file_priv, int prime_fd, >>>>>>> + uint32_t *handle) >>>>>>> { >>>>>>> struct dma_buf *dma_buf; >>>>>>> struct drm_gem_object *obj; >>>>>>> @@ -360,6 +360,7 @@ static 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); >>>>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>>>>> *data, >>>>>>> struct drm_file *file_priv) >>>>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>> return dmabuf; >>>>>>> } >>>>>>> -/* >>>>>>> +/** >>>>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>>>> drivers >>>>>>> * @dev: dev to export the buffer from >>>>>>> * @file_priv: drm file-private structure >>>>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>> * The actual exporting from GEM object to a dma-buf is done >>>>>>> through the >>>>>>> * &drm_gem_object_funcs.export callback. >>>>>>> */ >>>>>>> -static 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) >>>>>>> +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) >>>>>>> { >>>>>>> struct drm_gem_object *obj; >>>>>>> int ret = 0; >>>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>>>> drm_device *dev, >>>>>>> return ret; >>>>>>> } >>>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>>>>> *data, >>>>>>> struct drm_file *file_priv) >>>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>>>> * @obj: GEM object to export >>>>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>>>> * >>>>>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>>>>> functions >>>>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>>>> default for >>>>>>> - * drivers that do not set their own. >>>>>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>>>>> functions for GEM drivers >>>>>>> + * using the PRIME helpers. It is used as the default in >>>>>>> + * drm_gem_prime_handle_to_fd(). >>>>>>> */ >>>>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>>>>> int flags) >>>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>>>> * @dev: drm_device to import into >>>>>>> * @dma_buf: dma-buf object to import >>>>>>> * >>>>>>> - * This is the implementation of the gem_prime_import functions >>>>>>> for GEM >>>>>>> - * drivers using the PRIME helpers. It is the default for drivers >>>>>>> that do >>>>>>> - * not set their own &drm_driver.gem_prime_import. >>>>>>> + * This is the implementation of the gem_prime_import functions >>>>>>> for GEM drivers >>>>>>> + * using the PRIME helpers. Drivers can use this as their >>>>>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>>>>> default >>>>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>>>> * >>>>>>> * Drivers must arrange to call drm_prime_gem_destroy() from >>>>>>> their >>>>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>>>> --- a/include/drm/drm_prime.h >>>>>>> +++ b/include/drm/drm_prime.h >>>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>>>> struct drm_device; >>>>>>> struct drm_gem_object; >>>>>>> +struct drm_file; >>>>>>> /* core prime functions */ >>>>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>>>> struct dma_buf_export_info *exp_info); >>>>>>> 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_handle_to_fd(struct drm_device *dev, >>>>>>> + struct drm_file *file_priv, uint32_t handle, >>>>>>> uint32_t flags, >>>>>>> + int *prime_fd); >>>>>>> + >>>>>>> /* helper functions for exporting */ >>>>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>>>> struct dma_buf_attachment *attach);
Hi Am 20.11.23 um 17:22 schrieb Christian König: > Am 20.11.23 um 17:15 schrieb Felix Kuehling: >> >> On 2023-11-20 11:02, Thomas Zimmermann wrote: >>> Hi Christian >>> >>> Am 20.11.23 um 16:22 schrieb Christian König: >>>> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >>>>> Hi >>>>> >>>>> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>>>>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>>>>> Hi >>>>>>> >>>>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>>>>> >>>>>>>> These helper functions are needed for KFD to export and import >>>>>>>> DMABufs >>>>>>>> the right way without duplicating the tracking of DMABufs >>>>>>>> associated with >>>>>>>> GEM objects while ensuring that move notifier callbacks are >>>>>>>> working as >>>>>>>> intended. >>>>>>> I'm unhappy to see these functions making a comeback. They are the >>>>>>> boiler-plate logic that all drivers should use. Historically, >>>>>>> drivers did a lot one things in their GEM code that was only >>>>>>> semi-correct. Unifying most of that made the memory management more >>>>>>> readable. Not giving back drivers to option of tinkering with this >>>>>>> might be preferable. The rsp hooks in struct drm_driver, >>>>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for >>>>>>> vmwgfx. >>>>>>> >>>>>>> If you want to hook into prime import and export, there are >>>>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>>>>> it possible to move the additional code behind these pointers? >>>>>> I'm not trying to hook into these functions, I'm just trying to call >>>>>> them. I'm not bringing back the .prime_handle_to_fd and >>>>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to >>>>>> export and import DMAbuffers from a kernel mode context. I had >>>>>> incorrect or semi-correct ways of doing that by calling some >>>>>> driver-internal functions, but then my DMABufs aren't correctly >>>>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>>>>> working correctly. >>>>> I understand that. But why don't you use drm_driver.gem_prime_import >>>>> and drm_gem_object_funcs.export to add the amdkfd-specific code? These >>>>> callbacks are being invoked from within >>>>> drm_gem_prime_fd_to_handle() and >>>>> drm_gem_prime_handle_to_fd() as part of the importing and exporting >>>>> logic. With the intention of doing driver-specific things. Hence you >>>>> should not have to re-export the internal drm_gem_prime_*_to_*() >>>>> helpers. >>>>> >>>>> My question is if the existing hooks are not suitable for your needs. >>>>> If so, how could we improve them? >>>> No no. You don't seem to understand the use case :) Felix doesn't >>>> try to >>>> implement any driver-specific things. >>> I meant that I understand that this patchset is not about setting >>> drm_driver.prime_handle_to_fd, et al. >>> >>>> What Felix tries to do is to export a DMA-buf handle from kernel space. >>> For example, looking at patch 2, it converts a GEM handle to a file >>> descriptor and then assigns the rsp dmabuf to mem, which is of type >>> struct kgd_mem. From my impression, this could be done within the >>> existing ->export hook. >> >> That would skip the call to export_and_register_object. I think that's >> what I'm currently missing to set up gem_obj->dmabuf. > > I think we are talking past each other. kgd_mem is not part of the > amdgpu driver, so it can't go into an ->export callback. > > What happens here is that a drm_client code uses the amdgpu driver to > export some DMA-buf to file descriptors. > > In other words this is a high level user of drm_client and not something > driver internal. > > If you don't want to export the drm_gem_prime_handle_to_fd() function > directly we could add some drm_client_buffer_export() or similar. I think it's the fd that's bothering me. As I've mentioned in another email, fb appears to be not required. So the overall interface looks suboptimal. Something like drm_gem_prime_handle_to_dmabuf() would be better. Such a helper would then also invoke the existing hooks. But it's certainly not a blocker. Best regards Thomas > > Regards, > Christian. > >> >> Regards, >> Felix >> >> >>> >>> Then there's close_fd(), which cannot go into ->export. It looks like >>> the fd isn't really required. Could the drm_prime_handle_to_fd() be >>> reworked into a helper that converts the handle to the dmabuf without >>> the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would >>> then be exported? >>> >>> And I have the question wrt the 3rd patch; just that it's about >>> importing. >>> >>> (Looking further through the code, it appears that the fd could be >>> removed from the helpers, the callbacks and vmwgfx. It would then be >>> used entirely in the ioctl entry points, such as >>> drm_prime_fd_to_handle_ioctl().) >>> >>> Best regards >>> Thomas >>> >>> >>>> Regards, >>>> Christian. >>>> >>>>> Best regards >>>>> Thomas >>>>> >>>>> >>>>>> Regards, >>>>>> Felix >>>>>> >>>>>> >>>>>>> Best regards >>>>>>> Thomas >>>>>>> >>>>>>>> CC: Christian König <christian.koenig@amd.com> >>>>>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_prime.c | 33 >>>>>>>> ++++++++++++++++++--------------- >>>>>>>> include/drm/drm_prime.h | 7 +++++++ >>>>>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>>>>> b/drivers/gpu/drm/drm_prime.c >>>>>>>> index 63b709a67471..834a5e28abbe 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>>>> @@ -278,7 +278,7 @@ 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 >>>>>>>> * @dev: drm_device to import into >>>>>>>> * @file_priv: drm file-private structure >>>>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>>>>> * >>>>>>>> * Returns 0 on success or a negative error code on failure. >>>>>>>> */ >>>>>>>> -static 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(struct drm_device *dev, >>>>>>>> + struct drm_file *file_priv, int prime_fd, >>>>>>>> + uint32_t *handle) >>>>>>>> { >>>>>>>> struct dma_buf *dma_buf; >>>>>>>> struct drm_gem_object *obj; >>>>>>>> @@ -360,6 +360,7 @@ static 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); >>>>>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void >>>>>>>> *data, >>>>>>>> struct drm_file *file_priv) >>>>>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>>> return dmabuf; >>>>>>>> } >>>>>>>> -/* >>>>>>>> +/** >>>>>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>>>>> drivers >>>>>>>> * @dev: dev to export the buffer from >>>>>>>> * @file_priv: drm file-private structure >>>>>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>>> * The actual exporting from GEM object to a dma-buf is done >>>>>>>> through the >>>>>>>> * &drm_gem_object_funcs.export callback. >>>>>>>> */ >>>>>>>> -static 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) >>>>>>>> +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) >>>>>>>> { >>>>>>>> struct drm_gem_object *obj; >>>>>>>> int ret = 0; >>>>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>>>>> drm_device *dev, >>>>>>>> return ret; >>>>>>>> } >>>>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void >>>>>>>> *data, >>>>>>>> struct drm_file *file_priv) >>>>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>>>>> * @obj: GEM object to export >>>>>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>>>>> * >>>>>>>> - * This is the implementation of the &drm_gem_object_funcs.export >>>>>>>> functions >>>>>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>>>>> default for >>>>>>>> - * drivers that do not set their own. >>>>>>>> + * This is the implementation of the &drm_gem_object_funcs.export >>>>>>>> functions for GEM drivers >>>>>>>> + * using the PRIME helpers. It is used as the default in >>>>>>>> + * drm_gem_prime_handle_to_fd(). >>>>>>>> */ >>>>>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>>>>>>> int flags) >>>>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>>>>> * @dev: drm_device to import into >>>>>>>> * @dma_buf: dma-buf object to import >>>>>>>> * >>>>>>>> - * This is the implementation of the gem_prime_import functions >>>>>>>> for GEM >>>>>>>> - * drivers using the PRIME helpers. It is the default for drivers >>>>>>>> that do >>>>>>>> - * not set their own &drm_driver.gem_prime_import. >>>>>>>> + * This is the implementation of the gem_prime_import functions >>>>>>>> for GEM drivers >>>>>>>> + * using the PRIME helpers. Drivers can use this as their >>>>>>>> + * &drm_driver.gem_prime_import implementation. It is used as the >>>>>>>> default >>>>>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>>>>> * >>>>>>>> * Drivers must arrange to call drm_prime_gem_destroy() from >>>>>>>> their >>>>>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>>>>> --- a/include/drm/drm_prime.h >>>>>>>> +++ b/include/drm/drm_prime.h >>>>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>>>>> struct drm_device; >>>>>>>> struct drm_gem_object; >>>>>>>> +struct drm_file; >>>>>>>> /* core prime functions */ >>>>>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>>>>> struct dma_buf_export_info *exp_info); >>>>>>>> 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_handle_to_fd(struct drm_device *dev, >>>>>>>> + struct drm_file *file_priv, uint32_t handle, >>>>>>>> uint32_t flags, >>>>>>>> + int *prime_fd); >>>>>>>> + >>>>>>>> /* helper functions for exporting */ >>>>>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>>>>> struct dma_buf_attachment *attach); >
Am 20.11.23 um 17:28 schrieb Thomas Zimmermann: > Hi > > Am 20.11.23 um 17:22 schrieb Christian König: >> Am 20.11.23 um 17:15 schrieb Felix Kuehling: >>> >>> On 2023-11-20 11:02, Thomas Zimmermann wrote: >>>> Hi Christian >>>> >>>> Am 20.11.23 um 16:22 schrieb Christian König: >>>>> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann: >>>>>> Hi >>>>>> >>>>>> Am 20.11.23 um 16:06 schrieb Felix Kuehling: >>>>>>> On 2023-11-20 6:54, Thomas Zimmermann wrote: >>>>>>>> Hi >>>>>>>> >>>>>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling: >>>>>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>>>>>>>> >>>>>>>>> These helper functions are needed for KFD to export and import >>>>>>>>> DMABufs >>>>>>>>> the right way without duplicating the tracking of DMABufs >>>>>>>>> associated with >>>>>>>>> GEM objects while ensuring that move notifier callbacks are >>>>>>>>> working as >>>>>>>>> intended. >>>>>>>> I'm unhappy to see these functions making a comeback. They are the >>>>>>>> boiler-plate logic that all drivers should use. Historically, >>>>>>>> drivers did a lot one things in their GEM code that was only >>>>>>>> semi-correct. Unifying most of that made the memory management >>>>>>>> more >>>>>>>> readable. Not giving back drivers to option of tinkering with this >>>>>>>> might be preferable. The rsp hooks in struct drm_driver, >>>>>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for >>>>>>>> vmwgfx. >>>>>>>> >>>>>>>> If you want to hook into prime import and export, there are >>>>>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't >>>>>>>> it possible to move the additional code behind these pointers? >>>>>>> I'm not trying to hook into these functions, I'm just trying to >>>>>>> call >>>>>>> them. I'm not bringing back the .prime_handle_to_fd and >>>>>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean >>>>>>> way to >>>>>>> export and import DMAbuffers from a kernel mode context. I had >>>>>>> incorrect or semi-correct ways of doing that by calling some >>>>>>> driver-internal functions, but then my DMABufs aren't correctly >>>>>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't >>>>>>> working correctly. >>>>>> I understand that. But why don't you use drm_driver.gem_prime_import >>>>>> and drm_gem_object_funcs.export to add the amdkfd-specific code? >>>>>> These >>>>>> callbacks are being invoked from within >>>>>> drm_gem_prime_fd_to_handle() and >>>>>> drm_gem_prime_handle_to_fd() as part of the importing and exporting >>>>>> logic. With the intention of doing driver-specific things. Hence you >>>>>> should not have to re-export the internal drm_gem_prime_*_to_*() >>>>>> helpers. >>>>>> >>>>>> My question is if the existing hooks are not suitable for your >>>>>> needs. >>>>>> If so, how could we improve them? >>>>> No no. You don't seem to understand the use case :) Felix doesn't >>>>> try to >>>>> implement any driver-specific things. >>>> I meant that I understand that this patchset is not about setting >>>> drm_driver.prime_handle_to_fd, et al. >>>> >>>>> What Felix tries to do is to export a DMA-buf handle from kernel >>>>> space. >>>> For example, looking at patch 2, it converts a GEM handle to a file >>>> descriptor and then assigns the rsp dmabuf to mem, which is of type >>>> struct kgd_mem. From my impression, this could be done within the >>>> existing ->export hook. >>> >>> That would skip the call to export_and_register_object. I think >>> that's what I'm currently missing to set up gem_obj->dmabuf. >> >> I think we are talking past each other. kgd_mem is not part of the >> amdgpu driver, so it can't go into an ->export callback. >> >> What happens here is that a drm_client code uses the amdgpu driver to >> export some DMA-buf to file descriptors. >> >> In other words this is a high level user of drm_client and not >> something driver internal. >> >> If you don't want to export the drm_gem_prime_handle_to_fd() function >> directly we could add some drm_client_buffer_export() or similar. > > I think it's the fd that's bothering me. As I've mentioned in another > email, fb appears to be not required. So the overall interface looks > suboptimal. Something like drm_gem_prime_handle_to_dmabuf() would be > better. Such a helper would then also invoke the existing hooks. Really good point. I think that should work for Felix as well. Thanks, Christian. > > But it's certainly not a blocker. > > Best regards > Thomas > >> >> Regards, >> Christian. >> >>> >>> Regards, >>> Felix >>> >>> >>>> >>>> Then there's close_fd(), which cannot go into ->export. It looks like >>>> the fd isn't really required. Could the drm_prime_handle_to_fd() be >>>> reworked into a helper that converts the handle to the dmabuf without >>>> the fd? Something like drm_gem_prime_handle_to_dmabuf(), which would >>>> then be exported? >>>> >>>> And I have the question wrt the 3rd patch; just that it's about >>>> importing. >>>> >>>> (Looking further through the code, it appears that the fd could be >>>> removed from the helpers, the callbacks and vmwgfx. It would then be >>>> used entirely in the ioctl entry points, such as >>>> drm_prime_fd_to_handle_ioctl().) >>>> >>>> Best regards >>>> Thomas >>>> >>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> Best regards >>>>>> Thomas >>>>>> >>>>>> >>>>>>> Regards, >>>>>>> Felix >>>>>>> >>>>>>> >>>>>>>> Best regards >>>>>>>> Thomas >>>>>>>> >>>>>>>>> CC: Christian König <christian.koenig@amd.com> >>>>>>>>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/drm_prime.c | 33 >>>>>>>>> ++++++++++++++++++--------------- >>>>>>>>> include/drm/drm_prime.h | 7 +++++++ >>>>>>>>> 2 files changed, 25 insertions(+), 15 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>>>>>> b/drivers/gpu/drm/drm_prime.c >>>>>>>>> index 63b709a67471..834a5e28abbe 100644 >>>>>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>>>>> @@ -278,7 +278,7 @@ 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 >>>>>>>>> * @dev: drm_device to import into >>>>>>>>> * @file_priv: drm file-private structure >>>>>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>>>>>>>> * >>>>>>>>> * Returns 0 on success or a negative error code on failure. >>>>>>>>> */ >>>>>>>>> -static 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(struct drm_device *dev, >>>>>>>>> + struct drm_file *file_priv, int prime_fd, >>>>>>>>> + uint32_t *handle) >>>>>>>>> { >>>>>>>>> struct dma_buf *dma_buf; >>>>>>>>> struct drm_gem_object *obj; >>>>>>>>> @@ -360,6 +360,7 @@ static 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); >>>>>>>>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, >>>>>>>>> void >>>>>>>>> *data, >>>>>>>>> struct drm_file *file_priv) >>>>>>>>> @@ -408,7 +409,7 @@ static struct dma_buf >>>>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>>>> return dmabuf; >>>>>>>>> } >>>>>>>>> -/* >>>>>>>>> +/** >>>>>>>>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM >>>>>>>>> drivers >>>>>>>>> * @dev: dev to export the buffer from >>>>>>>>> * @file_priv: drm file-private structure >>>>>>>>> @@ -421,10 +422,10 @@ static struct dma_buf >>>>>>>>> *export_and_register_object(struct drm_device *dev, >>>>>>>>> * The actual exporting from GEM object to a dma-buf is done >>>>>>>>> through the >>>>>>>>> * &drm_gem_object_funcs.export callback. >>>>>>>>> */ >>>>>>>>> -static 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) >>>>>>>>> +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) >>>>>>>>> { >>>>>>>>> struct drm_gem_object *obj; >>>>>>>>> int ret = 0; >>>>>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >>>>>>>>> drm_device *dev, >>>>>>>>> return ret; >>>>>>>>> } >>>>>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>>>>>>>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, >>>>>>>>> void >>>>>>>>> *data, >>>>>>>>> struct drm_file *file_priv) >>>>>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>>>>>>>> * @obj: GEM object to export >>>>>>>>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>>>>>>>> * >>>>>>>>> - * This is the implementation of the >>>>>>>>> &drm_gem_object_funcs.export >>>>>>>>> functions >>>>>>>>> - * for GEM drivers using the PRIME helpers. It is used as the >>>>>>>>> default for >>>>>>>>> - * drivers that do not set their own. >>>>>>>>> + * This is the implementation of the >>>>>>>>> &drm_gem_object_funcs.export >>>>>>>>> functions for GEM drivers >>>>>>>>> + * using the PRIME helpers. It is used as the default in >>>>>>>>> + * drm_gem_prime_handle_to_fd(). >>>>>>>>> */ >>>>>>>>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object >>>>>>>>> *obj, >>>>>>>>> int flags) >>>>>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>>>>>>>> * @dev: drm_device to import into >>>>>>>>> * @dma_buf: dma-buf object to import >>>>>>>>> * >>>>>>>>> - * This is the implementation of the gem_prime_import functions >>>>>>>>> for GEM >>>>>>>>> - * drivers using the PRIME helpers. It is the default for >>>>>>>>> drivers >>>>>>>>> that do >>>>>>>>> - * not set their own &drm_driver.gem_prime_import. >>>>>>>>> + * This is the implementation of the gem_prime_import functions >>>>>>>>> for GEM drivers >>>>>>>>> + * using the PRIME helpers. Drivers can use this as their >>>>>>>>> + * &drm_driver.gem_prime_import implementation. It is used as >>>>>>>>> the >>>>>>>>> default >>>>>>>>> + * implementation in drm_gem_prime_fd_to_handle(). >>>>>>>>> * >>>>>>>>> * Drivers must arrange to call drm_prime_gem_destroy() >>>>>>>>> from their >>>>>>>>> * &drm_gem_object_funcs.free hook when using this function. >>>>>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>>>>>>>> index a7abf9f3e697..2a1d01e5b56b 100644 >>>>>>>>> --- a/include/drm/drm_prime.h >>>>>>>>> +++ b/include/drm/drm_prime.h >>>>>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>>>>>>>> struct drm_device; >>>>>>>>> struct drm_gem_object; >>>>>>>>> +struct drm_file; >>>>>>>>> /* core prime functions */ >>>>>>>>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>>>>>>>> struct dma_buf_export_info *exp_info); >>>>>>>>> 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_handle_to_fd(struct drm_device *dev, >>>>>>>>> + struct drm_file *file_priv, uint32_t handle, >>>>>>>>> uint32_t flags, >>>>>>>>> + int *prime_fd); >>>>>>>>> + >>>>>>>>> /* helper functions for exporting */ >>>>>>>>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>>>>>>>> struct dma_buf_attachment *attach); >> >
[+Alex] On 2023-11-17 16:44, Felix Kuehling wrote: > This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. > > These helper functions are needed for KFD to export and import DMABufs > the right way without duplicating the tracking of DMABufs associated with > GEM objects while ensuring that move notifier callbacks are working as > intended. > > CC: Christian König <christian.koenig@amd.com> > CC: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> Re: our discussion about v2 of this patch: If this version is acceptable, can I get an R-b or A-b? I would like to get this patch into drm-next as a prerequisite for patches 2 and 3. I cannot submit it to the current amd-staging-drm-next because the patch I'm reverting doesn't exist there yet. Patch 2 and 3 could go into drm-next as well, or go through Alex's amd-staging-drm-next branch once patch 1 is in drm-next. Alex, how do you prefer to coordinate this? Regards, Felix > --- > drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- > include/drm/drm_prime.h | 7 +++++++ > 2 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 63b709a67471..834a5e28abbe 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -278,7 +278,7 @@ 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 > * @dev: drm_device to import into > * @file_priv: drm file-private structure > @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); > * > * Returns 0 on success or a negative error code on failure. > */ > -static 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(struct drm_device *dev, > + struct drm_file *file_priv, int prime_fd, > + uint32_t *handle) > { > struct dma_buf *dma_buf; > struct drm_gem_object *obj; > @@ -360,6 +360,7 @@ static 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); > > int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > @@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > return dmabuf; > } > > -/* > +/** > * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > * @dev: dev to export the buffer from > * @file_priv: drm file-private structure > @@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > * The actual exporting from GEM object to a dma-buf is done through the > * &drm_gem_object_funcs.export callback. > */ > -static 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) > +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) > { > struct drm_gem_object *obj; > int ret = 0; > @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev, > > return ret; > } > +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); > * @obj: GEM object to export > * @flags: flags like DRM_CLOEXEC and DRM_RDWR > * > - * This is the implementation of the &drm_gem_object_funcs.export functions > - * for GEM drivers using the PRIME helpers. It is used as the default for > - * drivers that do not set their own. > + * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers > + * using the PRIME helpers. It is used as the default in > + * drm_gem_prime_handle_to_fd(). > */ > struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, > int flags) > @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); > * @dev: drm_device to import into > * @dma_buf: dma-buf object to import > * > - * This is the implementation of the gem_prime_import functions for GEM > - * drivers using the PRIME helpers. It is the default for drivers that do > - * not set their own &drm_driver.gem_prime_import. > + * This is the implementation of the gem_prime_import functions for GEM drivers > + * using the PRIME helpers. Drivers can use this as their > + * &drm_driver.gem_prime_import implementation. It is used as the default > + * implementation in drm_gem_prime_fd_to_handle(). > * > * Drivers must arrange to call drm_prime_gem_destroy() from their > * &drm_gem_object_funcs.free hook when using this function. > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index a7abf9f3e697..2a1d01e5b56b 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -60,12 +60,19 @@ enum dma_data_direction; > > struct drm_device; > struct drm_gem_object; > +struct drm_file; > > /* core prime functions */ > struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, > struct dma_buf_export_info *exp_info); > 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_handle_to_fd(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, uint32_t flags, > + int *prime_fd); > + > /* helper functions for exporting */ > int drm_gem_map_attach(struct dma_buf *dma_buf, > struct dma_buf_attachment *attach);
Am 23.11.23 um 20:36 schrieb Felix Kuehling: > [+Alex] > > On 2023-11-17 16:44, Felix Kuehling wrote: > >> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >> >> These helper functions are needed for KFD to export and import DMABufs >> the right way without duplicating the tracking of DMABufs associated >> with >> GEM objects while ensuring that move notifier callbacks are working as >> intended. >> >> CC: Christian König <christian.koenig@amd.com> >> CC: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > > Re: our discussion about v2 of this patch: If this version is > acceptable, can I get an R-b or A-b? From my side feel free to add my Acked-by, I don't see how else you want to cleanly export DMA-bufs. Regards, Christian. > > I would like to get this patch into drm-next as a prerequisite for > patches 2 and 3. I cannot submit it to the current > amd-staging-drm-next because the patch I'm reverting doesn't exist > there yet. > > Patch 2 and 3 could go into drm-next as well, or go through Alex's > amd-staging-drm-next branch once patch 1 is in drm-next. Alex, how do > you prefer to coordinate this? > > Regards, > Felix > > >> --- >> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >> include/drm/drm_prime.h | 7 +++++++ >> 2 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 63b709a67471..834a5e28abbe 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -278,7 +278,7 @@ 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 >> * @dev: drm_device to import into >> * @file_priv: drm file-private structure >> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >> * >> * Returns 0 on success or a negative error code on failure. >> */ >> -static 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(struct drm_device *dev, >> + struct drm_file *file_priv, int prime_fd, >> + uint32_t *handle) >> { >> struct dma_buf *dma_buf; >> struct drm_gem_object *obj; >> @@ -360,6 +360,7 @@ static 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); >> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -408,7 +409,7 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> return dmabuf; >> } >> -/* >> +/** >> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >> * @dev: dev to export the buffer from >> * @file_priv: drm file-private structure >> @@ -421,10 +422,10 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> * The actual exporting from GEM object to a dma-buf is done >> through the >> * &drm_gem_object_funcs.export callback. >> */ >> -static 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) >> +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) >> { >> struct drm_gem_object *obj; >> int ret = 0; >> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >> drm_device *dev, >> return ret; >> } >> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >> * @obj: GEM object to export >> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >> * >> - * This is the implementation of the &drm_gem_object_funcs.export >> functions >> - * for GEM drivers using the PRIME helpers. It is used as the >> default for >> - * drivers that do not set their own. >> + * This is the implementation of the &drm_gem_object_funcs.export >> functions for GEM drivers >> + * using the PRIME helpers. It is used as the default in >> + * drm_gem_prime_handle_to_fd(). >> */ >> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >> int flags) >> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >> * @dev: drm_device to import into >> * @dma_buf: dma-buf object to import >> * >> - * This is the implementation of the gem_prime_import functions for GEM >> - * drivers using the PRIME helpers. It is the default for drivers >> that do >> - * not set their own &drm_driver.gem_prime_import. >> + * This is the implementation of the gem_prime_import functions for >> GEM drivers >> + * using the PRIME helpers. Drivers can use this as their >> + * &drm_driver.gem_prime_import implementation. It is used as the >> default >> + * implementation in drm_gem_prime_fd_to_handle(). >> * >> * Drivers must arrange to call drm_prime_gem_destroy() from their >> * &drm_gem_object_funcs.free hook when using this function. >> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >> index a7abf9f3e697..2a1d01e5b56b 100644 >> --- a/include/drm/drm_prime.h >> +++ b/include/drm/drm_prime.h >> @@ -60,12 +60,19 @@ enum dma_data_direction; >> struct drm_device; >> struct drm_gem_object; >> +struct drm_file; >> /* core prime functions */ >> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >> struct dma_buf_export_info *exp_info); >> 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_handle_to_fd(struct drm_device *dev, >> + struct drm_file *file_priv, uint32_t handle, >> uint32_t flags, >> + int *prime_fd); >> + >> /* helper functions for exporting */ >> int drm_gem_map_attach(struct dma_buf *dma_buf, >> struct dma_buf_attachment *attach);
Hi Am 23.11.23 um 20:36 schrieb Felix Kuehling: > [+Alex] > > On 2023-11-17 16:44, Felix Kuehling wrote: > >> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >> >> These helper functions are needed for KFD to export and import DMABufs >> the right way without duplicating the tracking of DMABufs associated with >> GEM objects while ensuring that move notifier callbacks are working as >> intended. >> >> CC: Christian König <christian.koenig@amd.com> >> CC: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > > Re: our discussion about v2 of this patch: If this version is > acceptable, can I get an R-b or A-b? Acked-by: Thomas Zimmermann <tzimmermann@suse.de> for patch 1. > > I would like to get this patch into drm-next as a prerequisite for > patches 2 and 3. I cannot submit it to the current amd-staging-drm-next > because the patch I'm reverting doesn't exist there yet. > > Patch 2 and 3 could go into drm-next as well, or go through Alex's > amd-staging-drm-next branch once patch 1 is in drm-next. Alex, how do > you prefer to coordinate this? > > Regards, > Felix > > >> --- >> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >> include/drm/drm_prime.h | 7 +++++++ >> 2 files changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 63b709a67471..834a5e28abbe 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -278,7 +278,7 @@ 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 >> * @dev: drm_device to import into >> * @file_priv: drm file-private structure >> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >> * >> * Returns 0 on success or a negative error code on failure. >> */ >> -static 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(struct drm_device *dev, >> + struct drm_file *file_priv, int prime_fd, >> + uint32_t *handle) >> { >> struct dma_buf *dma_buf; >> struct drm_gem_object *obj; >> @@ -360,6 +360,7 @@ static 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); >> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -408,7 +409,7 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> return dmabuf; >> } >> -/* >> +/** >> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >> * @dev: dev to export the buffer from >> * @file_priv: drm file-private structure >> @@ -421,10 +422,10 @@ static struct dma_buf >> *export_and_register_object(struct drm_device *dev, >> * The actual exporting from GEM object to a dma-buf is done through >> the >> * &drm_gem_object_funcs.export callback. >> */ >> -static 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) >> +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) >> { >> struct drm_gem_object *obj; >> int ret = 0; >> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct >> drm_device *dev, >> return ret; >> } >> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >> * @obj: GEM object to export >> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >> * >> - * This is the implementation of the &drm_gem_object_funcs.export >> functions >> - * for GEM drivers using the PRIME helpers. It is used as the default >> for >> - * drivers that do not set their own. >> + * This is the implementation of the &drm_gem_object_funcs.export >> functions for GEM drivers >> + * using the PRIME helpers. It is used as the default in >> + * drm_gem_prime_handle_to_fd(). >> */ >> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >> int flags) >> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >> * @dev: drm_device to import into >> * @dma_buf: dma-buf object to import >> * >> - * This is the implementation of the gem_prime_import functions for GEM >> - * drivers using the PRIME helpers. It is the default for drivers >> that do >> - * not set their own &drm_driver.gem_prime_import. >> + * This is the implementation of the gem_prime_import functions for >> GEM drivers >> + * using the PRIME helpers. Drivers can use this as their >> + * &drm_driver.gem_prime_import implementation. It is used as the >> default >> + * implementation in drm_gem_prime_fd_to_handle(). >> * >> * Drivers must arrange to call drm_prime_gem_destroy() from their >> * &drm_gem_object_funcs.free hook when using this function. >> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >> index a7abf9f3e697..2a1d01e5b56b 100644 >> --- a/include/drm/drm_prime.h >> +++ b/include/drm/drm_prime.h >> @@ -60,12 +60,19 @@ enum dma_data_direction; >> struct drm_device; >> struct drm_gem_object; >> +struct drm_file; >> /* core prime functions */ >> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >> struct dma_buf_export_info *exp_info); >> 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_handle_to_fd(struct drm_device *dev, >> + struct drm_file *file_priv, uint32_t handle, >> uint32_t flags, >> + int *prime_fd); >> + >> /* helper functions for exporting */ >> int drm_gem_map_attach(struct dma_buf *dma_buf, >> struct dma_buf_attachment *attach);
On Thu, Nov 23, 2023 at 6:12 PM Felix Kuehling <felix.kuehling@amd.com> wrote: > > [+Alex] > > On 2023-11-17 16:44, Felix Kuehling wrote: > > > This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. > > > > These helper functions are needed for KFD to export and import DMABufs > > the right way without duplicating the tracking of DMABufs associated with > > GEM objects while ensuring that move notifier callbacks are working as > > intended. > > > > CC: Christian König <christian.koenig@amd.com> > > CC: Thomas Zimmermann <tzimmermann@suse.de> > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > > Re: our discussion about v2 of this patch: If this version is > acceptable, can I get an R-b or A-b? > > I would like to get this patch into drm-next as a prerequisite for > patches 2 and 3. I cannot submit it to the current amd-staging-drm-next > because the patch I'm reverting doesn't exist there yet. > > Patch 2 and 3 could go into drm-next as well, or go through Alex's > amd-staging-drm-next branch once patch 1 is in drm-next. Alex, how do > you prefer to coordinate this? I guess ideally this would go through my drm-next tree since your other patches depend on it unless others feel strongly that it should go through drm-misc. Alex > > Regards, > Felix > > > > --- > > drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- > > include/drm/drm_prime.h | 7 +++++++ > > 2 files changed, 25 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > index 63b709a67471..834a5e28abbe 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -278,7 +278,7 @@ 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 > > * @dev: drm_device to import into > > * @file_priv: drm file-private structure > > @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); > > * > > * Returns 0 on success or a negative error code on failure. > > */ > > -static 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(struct drm_device *dev, > > + struct drm_file *file_priv, int prime_fd, > > + uint32_t *handle) > > { > > struct dma_buf *dma_buf; > > struct drm_gem_object *obj; > > @@ -360,6 +360,7 @@ static 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); > > > > int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > @@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > > return dmabuf; > > } > > > > -/* > > +/** > > * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > > * @dev: dev to export the buffer from > > * @file_priv: drm file-private structure > > @@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > > * The actual exporting from GEM object to a dma-buf is done through the > > * &drm_gem_object_funcs.export callback. > > */ > > -static 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) > > +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) > > { > > struct drm_gem_object *obj; > > int ret = 0; > > @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev, > > > > return ret; > > } > > +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > > > int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); > > * @obj: GEM object to export > > * @flags: flags like DRM_CLOEXEC and DRM_RDWR > > * > > - * This is the implementation of the &drm_gem_object_funcs.export functions > > - * for GEM drivers using the PRIME helpers. It is used as the default for > > - * drivers that do not set their own. > > + * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers > > + * using the PRIME helpers. It is used as the default in > > + * drm_gem_prime_handle_to_fd(). > > */ > > struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, > > int flags) > > @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); > > * @dev: drm_device to import into > > * @dma_buf: dma-buf object to import > > * > > - * This is the implementation of the gem_prime_import functions for GEM > > - * drivers using the PRIME helpers. It is the default for drivers that do > > - * not set their own &drm_driver.gem_prime_import. > > + * This is the implementation of the gem_prime_import functions for GEM drivers > > + * using the PRIME helpers. Drivers can use this as their > > + * &drm_driver.gem_prime_import implementation. It is used as the default > > + * implementation in drm_gem_prime_fd_to_handle(). > > * > > * Drivers must arrange to call drm_prime_gem_destroy() from their > > * &drm_gem_object_funcs.free hook when using this function. > > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > > index a7abf9f3e697..2a1d01e5b56b 100644 > > --- a/include/drm/drm_prime.h > > +++ b/include/drm/drm_prime.h > > @@ -60,12 +60,19 @@ enum dma_data_direction; > > > > struct drm_device; > > struct drm_gem_object; > > +struct drm_file; > > > > /* core prime functions */ > > struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, > > struct dma_buf_export_info *exp_info); > > 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_handle_to_fd(struct drm_device *dev, > > + struct drm_file *file_priv, uint32_t handle, uint32_t flags, > > + int *prime_fd); > > + > > /* helper functions for exporting */ > > int drm_gem_map_attach(struct dma_buf *dma_buf, > > struct dma_buf_attachment *attach);
On 2023-11-28 12:22, Alex Deucher wrote: > On Thu, Nov 23, 2023 at 6:12 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >> [+Alex] >> >> On 2023-11-17 16:44, Felix Kuehling wrote: >> >>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. >>> >>> These helper functions are needed for KFD to export and import DMABufs >>> the right way without duplicating the tracking of DMABufs associated with >>> GEM objects while ensuring that move notifier callbacks are working as >>> intended. >>> >>> CC: Christian König <christian.koenig@amd.com> >>> CC: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >> Re: our discussion about v2 of this patch: If this version is >> acceptable, can I get an R-b or A-b? >> >> I would like to get this patch into drm-next as a prerequisite for >> patches 2 and 3. I cannot submit it to the current amd-staging-drm-next >> because the patch I'm reverting doesn't exist there yet. >> >> Patch 2 and 3 could go into drm-next as well, or go through Alex's >> amd-staging-drm-next branch once patch 1 is in drm-next. Alex, how do >> you prefer to coordinate this? > I guess ideally this would go through my drm-next tree since your > other patches depend on it unless others feel strongly that it should > go through drm-misc. Yes, drm-next would work best for applying this patch and the two patches that depend on it. I can send you the rebased patches from my drm-next based branch that I used for testing this. Regards, Felix > > Alex > > >> Regards, >> Felix >> >> >>> --- >>> drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- >>> include/drm/drm_prime.h | 7 +++++++ >>> 2 files changed, 25 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 63b709a67471..834a5e28abbe 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -278,7 +278,7 @@ 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 >>> * @dev: drm_device to import into >>> * @file_priv: drm file-private structure >>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); >>> * >>> * Returns 0 on success or a negative error code on failure. >>> */ >>> -static 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(struct drm_device *dev, >>> + struct drm_file *file_priv, int prime_fd, >>> + uint32_t *handle) >>> { >>> struct dma_buf *dma_buf; >>> struct drm_gem_object *obj; >>> @@ -360,6 +360,7 @@ static 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); >>> >>> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, >>> struct drm_file *file_priv) >>> @@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, >>> return dmabuf; >>> } >>> >>> -/* >>> +/** >>> * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers >>> * @dev: dev to export the buffer from >>> * @file_priv: drm file-private structure >>> @@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, >>> * The actual exporting from GEM object to a dma-buf is done through the >>> * &drm_gem_object_funcs.export callback. >>> */ >>> -static 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) >>> +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) >>> { >>> struct drm_gem_object *obj; >>> int ret = 0; >>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev, >>> >>> return ret; >>> } >>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); >>> >>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, >>> struct drm_file *file_priv) >>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); >>> * @obj: GEM object to export >>> * @flags: flags like DRM_CLOEXEC and DRM_RDWR >>> * >>> - * This is the implementation of the &drm_gem_object_funcs.export functions >>> - * for GEM drivers using the PRIME helpers. It is used as the default for >>> - * drivers that do not set their own. >>> + * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers >>> + * using the PRIME helpers. It is used as the default in >>> + * drm_gem_prime_handle_to_fd(). >>> */ >>> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, >>> int flags) >>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); >>> * @dev: drm_device to import into >>> * @dma_buf: dma-buf object to import >>> * >>> - * This is the implementation of the gem_prime_import functions for GEM >>> - * drivers using the PRIME helpers. It is the default for drivers that do >>> - * not set their own &drm_driver.gem_prime_import. >>> + * This is the implementation of the gem_prime_import functions for GEM drivers >>> + * using the PRIME helpers. Drivers can use this as their >>> + * &drm_driver.gem_prime_import implementation. It is used as the default >>> + * implementation in drm_gem_prime_fd_to_handle(). >>> * >>> * Drivers must arrange to call drm_prime_gem_destroy() from their >>> * &drm_gem_object_funcs.free hook when using this function. >>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h >>> index a7abf9f3e697..2a1d01e5b56b 100644 >>> --- a/include/drm/drm_prime.h >>> +++ b/include/drm/drm_prime.h >>> @@ -60,12 +60,19 @@ enum dma_data_direction; >>> >>> struct drm_device; >>> struct drm_gem_object; >>> +struct drm_file; >>> >>> /* core prime functions */ >>> struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, >>> struct dma_buf_export_info *exp_info); >>> 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_handle_to_fd(struct drm_device *dev, >>> + struct drm_file *file_priv, uint32_t handle, uint32_t flags, >>> + int *prime_fd); >>> + >>> /* helper functions for exporting */ >>> int drm_gem_map_attach(struct dma_buf *dma_buf, >>> struct dma_buf_attachment *attach);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 63b709a67471..834a5e28abbe 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -278,7 +278,7 @@ 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 * @dev: drm_device to import into * @file_priv: drm file-private structure @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release); * * Returns 0 on success or a negative error code on failure. */ -static 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(struct drm_device *dev, + struct drm_file *file_priv, int prime_fd, + uint32_t *handle) { struct dma_buf *dma_buf; struct drm_gem_object *obj; @@ -360,6 +360,7 @@ static 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); int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) @@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, return dmabuf; } -/* +/** * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers * @dev: dev to export the buffer from * @file_priv: drm file-private structure @@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, * The actual exporting from GEM object to a dma-buf is done through the * &drm_gem_object_funcs.export callback. */ -static 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) +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) { struct drm_gem_object *obj; int ret = 0; @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev, return ret; } +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size); * @obj: GEM object to export * @flags: flags like DRM_CLOEXEC and DRM_RDWR * - * This is the implementation of the &drm_gem_object_funcs.export functions - * for GEM drivers using the PRIME helpers. It is used as the default for - * drivers that do not set their own. + * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers + * using the PRIME helpers. It is used as the default in + * drm_gem_prime_handle_to_fd(). */ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, int flags) @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev); * @dev: drm_device to import into * @dma_buf: dma-buf object to import * - * This is the implementation of the gem_prime_import functions for GEM - * drivers using the PRIME helpers. It is the default for drivers that do - * not set their own &drm_driver.gem_prime_import. + * This is the implementation of the gem_prime_import functions for GEM drivers + * using the PRIME helpers. Drivers can use this as their + * &drm_driver.gem_prime_import implementation. It is used as the default + * implementation in drm_gem_prime_fd_to_handle(). * * Drivers must arrange to call drm_prime_gem_destroy() from their * &drm_gem_object_funcs.free hook when using this function. diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index a7abf9f3e697..2a1d01e5b56b 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -60,12 +60,19 @@ enum dma_data_direction; struct drm_device; struct drm_gem_object; +struct drm_file; /* core prime functions */ struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, struct dma_buf_export_info *exp_info); 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_handle_to_fd(struct drm_device *dev, + struct drm_file *file_priv, uint32_t handle, uint32_t flags, + int *prime_fd); + /* helper functions for exporting */ int drm_gem_map_attach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach);
This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3. These helper functions are needed for KFD to export and import DMABufs the right way without duplicating the tracking of DMABufs associated with GEM objects while ensuring that move notifier callbacks are working as intended. CC: Christian König <christian.koenig@amd.com> CC: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> --- drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++--------------- include/drm/drm_prime.h | 7 +++++++ 2 files changed, 25 insertions(+), 15 deletions(-)