[libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset in mapping IPABuffer

[Laurent Pinchart]

Hi Hiro,

On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:
> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has
> now an offset. This uses the offset variable to map the IPABuffer.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  .../libcamera/internal/mapped_framebuffer.h   |  1 +
>  src/ipa/rkisp1/rkisp1.cpp                     | 31 +++++++------------
>  src/libcamera/mapped_framebuffer.cpp          |  7 +++++
>  3 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index 42479541..ee0583d0 100644
> --- a/include/libcamera/internal/mapped_framebuffer.h
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -55,6 +55,7 @@ public:
>  
>  	using MapFlags = Flags<MapFlag>;
>  
> +	MappedFrameBuffer();
>  	MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>  };
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 06fb9640..54cf2885 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -10,7 +10,6 @@
>  #include <queue>
>  #include <stdint.h>
>  #include <string.h>
> -#include <sys/mman.h>
>  
>  #include <linux/rkisp1-config.h>
>  #include <linux/v4l2-controls.h>
> @@ -24,6 +23,8 @@
>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>  #include <libcamera/request.h>
>  
> +#include <libcamera/internal/mapped_framebuffer.h>
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPARkISP1)
> @@ -54,7 +55,7 @@ private:
>  	void metadataReady(unsigned int frame, unsigned int aeState);
>  
>  	std::map<unsigned int, FrameBuffer> buffers_;
> -	std::map<unsigned int, void *> buffersMemory_;
> +	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>  
>  	ControlInfoMap ctrls_;
>  
> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>  					     std::forward_as_tuple(buffer.planes));
>  		const FrameBuffer &fb = elem.first->second;
>  
> -		/*
> -		 * \todo Provide a helper to mmap() buffers (possibly exposed
> -		 * to applications).
> -		 */
> -		buffersMemory_[buffer.id] = mmap(NULL,
> -						 fb.planes()[0].length,
> -						 PROT_READ | PROT_WRITE,
> -						 MAP_SHARED,
> -						 fb.planes()[0].fd.fd(),
> -						 0);
> -
> -		if (buffersMemory_[buffer.id] == MAP_FAILED) {
> -			int ret = -errno;
> +		mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);
> +		if (!mappedBuffers_[buffer.id].isValid()) {
>  			LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: "
> -					      << strerror(-ret);
> +					      << strerror(mappedBuffers_[buffer.id].error());
>  		}

Another point I wanted to mention is that we could also fix the default
constructor issue by avoiding operator[](). Here we could have

		MappedFrameBuffer mappedBuffer{ &fb, MappedFrameBuffer::MapFlag::ReadWrite };
		if (!mappedBuffer.isValid()) {
			LOG(IPARkISP1, Fatal)
				<< "Failed to mmap buffer: "
				<< strerror(mappedBuffer.error());
		}

		mappedBuffers_.emplace(buffer.id, std::move(mappedBuffer));

>  	}
>  }
> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>  		if (fb == buffers_.end())
>  			continue;
>  
> -		munmap(buffersMemory_[id], fb->second.planes()[0].length);
> -		buffersMemory_.erase(id);
> +		mappedBuffers_.erase(id);
>  		buffers_.erase(id);
>  	}
>  }
> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>  		unsigned int bufferId = event.bufferId;
>  
>  		const rkisp1_stat_buffer *stats =
> -			static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);
> +			reinterpret_cast<rkisp1_stat_buffer *>(
> +				mappedBuffers_[bufferId].maps()[0].data());

And here and below, you could use

				mappedBuffers_.at(bufferId).maps()[0].data());

>  
>  		updateStatistics(frame, stats);
>  		break;
> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>  		unsigned int bufferId = event.bufferId;
>  
>  		rkisp1_params_cfg *params =
> -			static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]);
> +			reinterpret_cast<rkisp1_params_cfg *>(
> +				mappedBuffers_[bufferId].maps()[0].data());
>  
>  		queueRequest(frame, params, event.controls);
>  		break;
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index 03425dea..34d9564d 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()
>   * \brief A bitwise combination of MappedFrameBuffer::MapFlag values
>   */
>  
> +/**
> + * \brief Construct an empty MappedFrameBuffer
> + */
> +MappedFrameBuffer::MappedFrameBuffer()
> +	: MappedBuffer()
> +{
> +}
>  /**
>   * \brief Map all planes of a FrameBuffer
>   * \param[in] buffer FrameBuffer to be mapped

-- 
Regards,

Laurent Pinchart