[libcamera-devel] [PATCH v2 23/25] qcam: Cache buffer memory mapping

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 30, 2019 at 01:05:08PM +0100, Niklas Söderlund wrote:
> With the buffer allocator in use it's possible to cache the dmabuf
> memory mappings when starting the camera instead of mapping and
> unmapping them each time.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/qcam/main_window.cpp | 28 ++++++++++++++++++++++------
>  src/qcam/main_window.h   |  1 +
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index e53ed2801291f2c5..2008931075b96260 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -201,6 +201,13 @@ int MainWindow::startCapture()
>  		}
>  
>  		requests.push_back(request);
> +
> +		/* Cache buffer memory mapping. */

"Map memory buffers and cache the mappings."

> +		const FrameBuffer::Plane &plane = buffer->planes().front();
> +		void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> +				    plane.fd.fd(), 0);
> +		mappedBuffers_[plane.fd.fd()] =
> +			std::make_pair(memory, plane.length);

I think we should create a helper for this, but it can be done on top.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	}
>  
>  	titleTimer_.start(2000);
> @@ -230,6 +237,13 @@ error:
>  	for (Request *request : requests)
>  		delete request;
>  
> +	for (auto &itr : mappedBuffers_) {

s/itr/iter/ or s/itr/it/ (that's what we use elsewhere in the code)

> +		void *memory = itr.second.first;
> +		unsigned int length = itr.second.second;
> +		munmap(memory, length);
> +	}
> +	mappedBuffers_.clear();
> +
>  	camera_->freeBuffers();
>  	return ret;
>  }
> @@ -243,6 +257,13 @@ void MainWindow::stopCapture()
>  	if (ret)
>  		std::cout << "Failed to stop capture" << std::endl;
>  
> +	for (auto &itr : mappedBuffers_) {
> +		void *memory = itr.second.first;
> +		unsigned int length = itr.second.second;
> +		munmap(memory, length);
> +	}
> +	mappedBuffers_.clear();
> +
>  	camera_->freeBuffers();
>  	isCapturing_ = false;
>  
> @@ -297,15 +318,10 @@ int MainWindow::display(FrameBuffer *buffer)
>  	if (buffer->planes().size() != 1)
>  		return -EINVAL;
>  
> -	/* \todo: Once the FrameBuffer is done cache mapped memory. */
>  	const FrameBuffer::Plane &plane = buffer->planes().front();
> -	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> -			    plane.fd.fd(), 0);
> -
> +	void *memory = mappedBuffers_[plane.fd.fd()].first;
>  	unsigned char *raw = static_cast<unsigned char *>(memory);
>  	viewfinder_->display(raw, buffer->metadata().planes[0].bytesused);
>  
> -	munmap(memory, plane.length);
> -
>  	return 0;
>  }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 05cde4ceab5f7ea1..04fb9e3ea869c3fb 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -71,6 +71,7 @@ private:
>  	uint32_t framesCaptured_;
>  
>  	ViewFinder *viewfinder_;
> +	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>  };
>  
>  #endif /* __QCAM_MAIN_WINDOW__ */

-- 
Regards,

Laurent Pinchart