[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: close dmabuf fd

[Poduval, Karthik]

Thanks Laurent,

You are right. I saw this issue on an earlier version of libcamera (it caused vb2 memory leaks and also hit max fd limit) and I ported it over without diving deep. Glad to know its not an issue on latest version.

--
Regards,
Karthik Poduval
________________________________________
From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Sent: Thursday, October 14, 2021 6:45 PM
To: Poduval, Karthik
Cc: libcamera-devel at lists.libcamera.org
Subject: RE: [EXTERNAL] [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: close dmabuf fd

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



Hi Karthik,

Thank you for the patch.

On Thu, Oct 14, 2021 at 11:38:33PM +0000, Karthik Poduval via libcamera-devel wrote:
>
> The dmabuf fd that comes from V4L2_EXPBUF is passed to FileDescriptor
> which makes a dup of the fd and stores it. However the original fd
> isn't closed and this causes the vb2's buf reference count to remain at
> 3 (one for kernel, one for V4L2_EXPBUF and one for the dup fd by File
> Descriptor). When requestbufs is called with zero count, it unable to
> free the buffer as the refcount is still 2. After FileDecriptor dup the
> fd the original fd must be closed so that the buf refcount is maintained
> properly by vb2.
>
> Signed-off-by: Karthik Poduval <kpoduval at lab126.com>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ba5f88cd..8c85da22 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1384,6 +1384,7 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index,
>  {
>       struct v4l2_exportbuffer expbuf = {};
>       int ret;
> +     FileDescriptor fd;
>
>       expbuf.type = bufferType_;
>       expbuf.index = index;
> @@ -1397,7 +1398,14 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index,
>               return FileDescriptor();
>       }
>
> -     return FileDescriptor(std::move(expbuf.fd));
> +     fd = FileDescriptor(std::move(expbuf.fd));

With the std::move(), the following FileDescriptor constructor is
called:

FileDescriptor::FileDescriptor(int &&fd)
{
        if (fd < 0)
                return;

        fd_ = std::make_shared<Descriptor>(fd, false);
        /*
         * The Descriptor constructor can't have failed here, as it took over
         * the fd without duplicating it. Just set the original fd to -1 to
         * implement move semantics.
         */
        fd = -1;
}

The inner Descriptor constructor thus doesn't call dup(). After this
line, expbuf.fd will be equal to -1, so the close() call below will be a
no-op. Unless I'm missing something :-)

> +     /*
> +      * since FileDesciptor makes a dup of the fd, original fd must be
> +      * closed or else driver considers it as an orphaned buffer (due to
> +      * additional buffer refcount) thus causing a frame buffer leak
> +      */
> +     ::close(expbuf.fd);
> +     return fd;
>  }
>
>  /**

--
Regards,

Laurent Pinchart