[libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close Plane dmabuf fd

[Kieran Bingham]

Hi Laurent,

On 05/03/2019 12:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, Mar 04, 2019 at 11:25:29PM +0000, Kieran Bingham wrote:
>> When constructing a Plane, the exported buffer provides a dmabuf handle which
>> is set to the Plane object.
>>
>> This action duplicates the handle for internal storage, and the original fd is
>> not used and needs to be closed.
>>
>> Close the handle, ensuring that the resources can be correctly managed.
> 
> Good catch !
> 
> The fact that this problem went unnoticed makes me wonder if we have a
> problem with the API.

Well, it didn't exactly go unnoticed - It just hadn't been dug out.

This was part of the reason for the call to
V4L2Device::requestBuffers(0) failing.


> Would there be a way to make it more robust,
> preventing these errors from happening ? We could take ownership of the
> fd passed to the setDmabuf() function, requiring the caller to call
> dup() if needed, but that would create a different issue, equally bad in
> my opinion. We could however change the setDmabuf() prototype in that case


I did think about that too, and considered removing the dup. Especially
as it feels a bit weird to take the expbuf.fd and close it (almost)
immediately.

But, I think the duplication does keep the 'layering' and ownership in
better form. And in this instance it's just a matter of making sure the
internal ownership is tracked.

When we start using externally provided buffers - I believe the
separation of the internal FD will be beneficial.



> int Plane::setDmabuf(int &&fd, unsigned int length);
> 
> which would require the caller to use std::move(). Pros, the caller
> would be forced to think about it, cons, the caller may just add an
> std::move() to fix the compilation error without actually thinking about
> it (the default move constructor for int doesn't modify the value of the
> other instance). I thus wonder if it's worth it, or if we'll have to
> live with this. Or if there's another way I'm not thinking about right
> now.

I think that might just add more complication without any real benefit
right now. If it becomes an issue we can reconsider this when we add
support for externally provided buffers later.

> 
> In any case,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks, I'll merge this to master now as it fixes a bug.

--
Kieran


> 
>> Fixes: 771befc6dc0e ("libcamera: v4l2_device: Request buffers from the device")
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/v4l2_device.cpp | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 0cd9f4b8e178..a88a5f5ff036 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -676,6 +676,7 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,
>>  	buffer->planes().emplace_back();
>>  	Plane &plane = buffer->planes().back();
>>  	plane.setDmabuf(expbuf.fd, length);
>> +	::close(expbuf.fd);
>>  
>>  	return 0;
>>  }
> 

-- 
Regards
--
Kieran