[libcamera-devel] [PATCH v3 25/30] cam: drm: Avoid importing the same dmabuf multiple times

[Kieran Bingham]

On 06/09/2021 23:56, Laurent Pinchart wrote:
> When creating a DRM frame buffer, the dmabufs for the planes are
> imported as GEM objects. For multi-planar formats, all planes may use
> the same dmabuf, which results in multiple imports. This doesn't cause
> any issue at import time, as DRM detects this situation and returns the
> same GEM object. However, when destroying the frame buffer, the same GEM
> object ends up being closed multiple times, which generates an error.
> 
> Fix this by avoiding multiple imports of the same dmabuf for the same
> frame buffer. While the issue may theoretically occur with identical
> dmabufs for different frame buffers, this is quite unlikely and is thus
> not addressed.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/cam/drm.cpp | 29 +++++++++++++++++------------
>  src/cam/drm.h   |  2 +-
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index d5a75d039fd8..f25300913a7f 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -283,9 +283,9 @@ FrameBuffer::FrameBuffer(Device *dev)
>  
>  FrameBuffer::~FrameBuffer()
>  {
> -	for (FrameBuffer::Plane &plane : planes_) {
> +	for (const auto &plane : planes_) {
>  		struct drm_gem_close gem_close = {
> -			.handle = plane.handle,
> +			.handle = plane.second.handle,
>  			.pad = 0,
>  		};
>  		int ret;
> @@ -605,22 +605,27 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
>  	int ret;
>  
>  	const std::vector<libcamera::FrameBuffer::Plane> &planes = buffer.planes();
> -	fb->planes_.reserve(planes.size());
>  
>  	unsigned int i = 0;
>  	for (const libcamera::FrameBuffer::Plane &plane : planes) {
> +		int fd = plane.fd.fd();
>  		uint32_t handle;
>  
> -		ret = drmPrimeFDToHandle(fd_, plane.fd.fd(), &handle);
> -		if (ret < 0) {
> -			ret = -errno;
> -			std::cerr
> -				<< "Unable to import framebuffer dmabuf: "
> -				<< strerror(-ret) << std::endl;
> -			return nullptr;
> -		}
> +		auto iter = fb->planes_.find(fd);
> +		if (iter == fb->planes_.end()) {
> +			ret = drmPrimeFDToHandle(fd_, plane.fd.fd(), &handle);
> +			if (ret < 0) {
> +				ret = -errno;
> +				std::cerr
> +					<< "Unable to import framebuffer dmabuf: "
> +					<< strerror(-ret) << std::endl;
> +				return nullptr;
> +			}
>  
> -		fb->planes_.push_back({ handle });
> +			fb->planes_[fd] = { handle };
> +		} else {
> +			handle = iter->second.handle;
> +		}
>  
>  		handles[i] = handle;
>  		offsets[i] = plane.offset;
> diff --git a/src/cam/drm.h b/src/cam/drm.h
> index 00f7e798b771..0b88f9a33912 100644
> --- a/src/cam/drm.h
> +++ b/src/cam/drm.h
> @@ -242,7 +242,7 @@ private:
>  
>  	FrameBuffer(Device *dev);
>  
> -	std::vector<Plane> planes_;
> +	std::map<int, Plane> planes_;
>  };
>  
>  class AtomicRequest
>