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

13691 diff mbox series

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@ideasonboard.com>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/cam/drm.cpp | 29 +++++++++++++++++------------
 src/cam/drm.h   |  2 +-
 2 files changed, 18 insertions(+), 13 deletions(-)

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@ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@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
>