[libcamera-devel] [PATCH 7/9] libcamera: request: Support buffer mapping

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch, I really like it!

On 2019-07-05 00:53:32 +0200, Jacopo Mondi wrote:
> Use the Stream class buffer mapping operation to save the association in
> the request at Request::findBuffer() time and reverse it with a new
> operation Request::unmapBuffer() used by the pipeline handler base class
> at buffer completion time, to return to the applications the Buffer they
> originally provided with the Request without involving the pipeline
> handlers in the process.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/request.h        |  4 ++-
>  src/libcamera/pipeline_handler.cpp |  6 ++--
>  src/libcamera/request.cpp          | 45 ++++++++++++++++++++++++++++--
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 70f6d7fa7eeb..3353f037945e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -36,7 +36,8 @@ public:
>  	ControlList &controls() { return controls_; }
>  	const std::map<Stream *, Buffer *> &buffers() const { return buffers_; }
>  	int setBuffers(const std::map<Stream *, Buffer *> &streamMap);
> -	Buffer *findBuffer(Stream *stream) const;
> +	Buffer *findBuffer(Stream *stream);
> +	Buffer *unmapBuffer(Buffer *streamBuffer);
>  
>  	Status status() const { return status_; }
>  
> @@ -55,6 +56,7 @@ private:
>  	ControlList controls_;
>  	std::map<Stream *, Buffer *> buffers_;
>  	std::unordered_set<Buffer *> pending_;
> +	std::map<Buffer *, Buffer *> bufferMap_;
>  
>  	Status status_;
>  };
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 67b215483847..a47411ecf345 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -402,8 +402,10 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)
>  bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
>  				     Buffer *buffer)
>  {
> -	camera->bufferCompleted.emit(request, buffer);
> -	return request->completeBuffer(buffer);
> +	Buffer *requestBuffer = request->unmapBuffer(buffer);
> +
> +	camera->bufferCompleted.emit(request, requestBuffer);
> +	return request->completeBuffer(requestBuffer);
>  }
>  
>  /**
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 9ff0abbf119c..0e07d39f8941 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -105,17 +105,58 @@ int Request::setBuffers(const std::map<Stream *, Buffer *> &streamMap)
>   */
>  
>  /**
> - * \brief Return the buffer associated with a stream
> + * \brief Retrieve the stream buffer associated with a stream
>   * \param[in] stream The stream the buffer is associated to
> + *
> + * Depending on the configured memory type, the buffers originally provided
> + * by the application might get mapped to streams internal buffers.
> + *
> + * \sa Stream::mapBuffer()
> + *
>   * \return The buffer associated with the stream, or nullptr if the stream is
>   * not part of this request
>   */
> -Buffer *Request::findBuffer(Stream *stream) const
> +Buffer *Request::findBuffer(Stream *stream)
>  {
>  	auto it = buffers_.find(stream);
>  	if (it == buffers_.end())
>  		return nullptr;
>  
> +	/*
> +	 * Streams with internal memory mode do not need to perform any mapping
> +	 * between the application provided buffers (part of the request)
> +	 * and the one actually used by the Stream.
> +	 *
> +	 * Streams using externally allocated buffers need to create a mapping
> +	 * between the application provided buffers and the one used by pipeline
> +	 * handlers.
> +	 */
> +	Buffer *requestBuffer = it->second;
> +	Buffer *mappedBuffer = stream->memoryType() == InternalMemory ?
> +			       it->second : stream->mapBuffer(it->second);
> +	bufferMap_[mappedBuffer] = requestBuffer;

nit-pick: This is hard to read.


    Buffer *requestBuffer, *mappedBuffer;

    requestBuffer = it->second;

    if (stream->memoryType() == InternalMemory)
        mappedBuffer = requestBuffer;
    else
        mappedBuffer = stream->mapBuffer(requestBuffer);

    bufferMap_[mappedBuffer] = requestBuffer;

With or without this fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> +
> +	return mappedBuffer;
> +}
> +
> +/**
> + * \brief Retrieve the application buffer associated with \a streamBuffer
> + * \param streamBuffer The stream buffer returned from Request::findBuffer()
> + *
> + * This operation is used by the PipelineHandler base class to retrieve the
> + * buffer the application originally associated with the stream in the request.
> + * Depending on the configured memory type, application provided buffers might
> + * be mapped to streams internal buffers at Request::findBuffer() time.
> + *
> + * \sa Stream::mapBuffer()
> + *
> + * \return The application buffer provided to Request::findBuffer()
> + */
> +Buffer *Request::unmapBuffer(Buffer *streamBuffer)
> +{
> +	auto it = bufferMap_.find(streamBuffer);
> +	ASSERT(it != bufferMap_.end());
> +
>  	return it->second;
>  }
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund