| Message ID | 1485415366-29337-1-git-send-email-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thank you for comments, I will update the patch and send v1. On 01/26/2017 12:36 PM, Daniel Vetter wrote: > On Thu, Jan 26, 2017 at 09:22:46AM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> From the description of the "DMA-BUF/GEM Object references >> and lifetime overview" it is not clear when exactly >> dma_buf gets destroyed and memory freed: only driver >> .release function mentioned which makes confusion on the >> real buffer's lifetime. >> >> Add more description so all the paths are covered. >> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Dave Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> drivers/gpu/drm/drm_prime.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 8d77b2462594..c061a0b29819 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -41,7 +41,7 @@ >> * object. It takes this reference in handle_to_fd_ioctl, when it >> * first calls .prime_export and stores the exporting GEM object in >> * the dma_buf priv. This reference is released when the dma_buf >> - * object goes away in the driver .release function. >> + * object goes away. > This was meant to talke about the release function of the dma-buf file, > not the drm_driver. Note that not all drivers use the same _release > function, e.g. vmwgfx is ttm-based, not gem and hence can't use > drm_gem_dmabuf_release(). ah, I am still new to DRM, so it is good to know > Maybe let's rewrite the entire sentence: > > "This refernce needs to be released when the final reference to the > &dma_buf itself is dropped and its &dma_buf_ops.release function is > called. For GEM-based drivers this is done by using > drm_gem_dmabuf_release()." Will put that into the v1 >> * >> * On the import the importing GEM object holds a reference to the >> * dma_buf (which in turn holds a ref to the exporting GEM object). >> @@ -51,6 +51,13 @@ >> * when the imported object is destroyed, we remove the attachment >> * and drop the reference to the dma_buf. >> * >> + * When all the references to the dma_buf are dropped, e.g. when >> + * userspace closes both handles to the imported (fd_to_handle_ioctl) >> + * and exported (handle_to_fd_ioctl) dma_buf and closes the corresponding >> + * file descriptor (handle_to_fd), then dma_buf gets destroyed. >> + * This can also happen as a part of the clean up procedure in the >> + * driver .release function if userspace fails to properly clean up. > This isn't accurate, since the kernel can also internally hold references > to the dma_buf, not just userspace by keeping the fd open. I'd drop this, > it doesn't really explain things. > > Maybe add a note to the previous hunk like "Note that both the kernel and > userspace (by keeeping the PRIME file descriptors open) can hold > references onto a &dma_buf." Will put that into the v1 > Thanks, Daniel Thank you, Oleksandr >> + * >> * Thus the chain of references always flows in one direction >> * (avoiding loops): importing_gem -> dmabuf -> exporting_gem >> * >> -- >> 2.7.4 >>
On 01/26/2017 12:45 PM, Chris Wilson wrote: > On Thu, Jan 26, 2017 at 11:36:05AM +0100, Daniel Vetter wrote: >> On Thu, Jan 26, 2017 at 09:22:46AM +0200, Oleksandr Andrushchenko wrote: >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> From the description of the "DMA-BUF/GEM Object references >>> and lifetime overview" it is not clear when exactly >>> dma_buf gets destroyed and memory freed: only driver >>> .release function mentioned which makes confusion on the >>> real buffer's lifetime. >>> >>> Add more description so all the paths are covered. >>> >>> Cc: Rob Clark <robdclark@gmail.com> >>> Cc: Dave Airlie <airlied@linux.ie> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> --- >>> drivers/gpu/drm/drm_prime.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 8d77b2462594..c061a0b29819 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -41,7 +41,7 @@ >>> * object. It takes this reference in handle_to_fd_ioctl, when it >>> * first calls .prime_export and stores the exporting GEM object in >>> * the dma_buf priv. This reference is released when the dma_buf >>> - * object goes away in the driver .release function. >>> + * object goes away. >> This was meant to talke about the release function of the dma-buf file, >> not the drm_driver. Note that not all drivers use the same _release >> function, e.g. vmwgfx is ttm-based, not gem and hence can't use >> drm_gem_dmabuf_release(). >> >> Maybe let's rewrite the entire sentence: >> >> "This refernce needs to be released when the final reference to the >> &dma_buf itself is dropped and its &dma_buf_ops.release function is >> called. For GEM-based drivers this is done by using >> drm_gem_dmabuf_release()." > For GEM-based drivers, the dmabuf should be exported using > drm_gem_dmabuf_export() and then released by drm_gem_dmabuf_release(). Will use this in v1 > Just the emphasize the symmetry (because of the internal magics)? > -Chris >
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 8d77b2462594..c061a0b29819 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -41,7 +41,7 @@ * object. It takes this reference in handle_to_fd_ioctl, when it * first calls .prime_export and stores the exporting GEM object in * the dma_buf priv. This reference is released when the dma_buf - * object goes away in the driver .release function. + * object goes away. * * On the import the importing GEM object holds a reference to the * dma_buf (which in turn holds a ref to the exporting GEM object). @@ -51,6 +51,13 @@ * when the imported object is destroyed, we remove the attachment * and drop the reference to the dma_buf. * + * When all the references to the dma_buf are dropped, e.g. when + * userspace closes both handles to the imported (fd_to_handle_ioctl) + * and exported (handle_to_fd_ioctl) dma_buf and closes the corresponding + * file descriptor (handle_to_fd), then dma_buf gets destroyed. + * This can also happen as a part of the clean up procedure in the + * driver .release function if userspace fails to properly clean up. + * * Thus the chain of references always flows in one direction * (avoiding loops): importing_gem -> dmabuf -> exporting_gem *