[PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[Oleksandr Andrushchenko]

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.
  *
  * 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
  *
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[Daniel Vetter]

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()."
>   *
>   * 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."

Thanks, Daniel

> + *
>   * Thus the chain of references always flows in one direction
>   * (avoiding loops): importing_gem -> dmabuf -> exporting_gem
>   *
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[Chris Wilson]

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().

Just the emphasize the symmetry (because of the internal magics)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[Oleksandr Andrushchenko]

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
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Clarify DMA-BUF/GEM Object lifetime

[Oleksandr Andrushchenko]

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
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel