[libcamera-devel] [PATCH v4 01/32] libcamera: request: remove prepare()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Jan 12, 2020 at 02:01:41AM +0100, Niklas Söderlund wrote:
> The association of buffers to a request can be done directly in
> addBuffer() instead of when the request is queued to the camera. Keep
> the check that a request contains buffers by moving it to
> Camera::queueRequest() where prepare() was previously called.
> 
> As a bonus we can remove a friend statement in Request.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

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

> ---
>  include/libcamera/request.h |  2 --
>  src/libcamera/buffer.cpp    |  4 ++--
>  src/libcamera/camera.cpp    | 11 +++++------
>  src/libcamera/request.cpp   | 26 ++------------------------
>  4 files changed, 9 insertions(+), 34 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 2d5a5964e99eb75f..728f380de4f010c3 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -48,10 +48,8 @@ public:
>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>  
>  private:
> -	friend class Camera;
>  	friend class PipelineHandler;
>  
> -	int prepare();
>  	void complete();
>  
>  	bool completeBuffer(Buffer *buffer);
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 960eeb8f7d193ddd..5305e3c391da6a69 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -360,7 +360,7 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
>   * The intended callers of this method are buffer completion handlers that
>   * need to associate a buffer to the request it belongs to.
>   *
> - * A Buffer is associated to a request by Request::prepare() and the
> + * A Buffer is associated to a request by Request::addBuffer() and the
>   * association is valid until the buffer completes. The returned request
>   * pointer is valid only during that interval.
>   *
> @@ -397,7 +397,7 @@ void Buffer::cancel()
>   * \fn Buffer::setRequest()
>   * \brief Set the request this buffer belongs to
>   *
> - * The intended callers are Request::prepare() and Request::completeBuffer().
> + * The intended callers are Request::addBuffer() and Request::completeBuffer().
>   */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index e810fb725d81350d..5d294b10647db8da 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -810,6 +810,11 @@ int Camera::queueRequest(Request *request)
>  	if (!stateIs(CameraRunning))
>  		return -EACCES;
>  
> +	if (request->buffers().empty()) {
> +		LOG(Camera, Error) << "Request contains no buffers";
> +		return -EINVAL;
> +	}
> +
>  	for (auto const &it : request->buffers()) {
>  		Stream *stream = it.first;
>  		Buffer *buffer = it.second;
> @@ -832,12 +837,6 @@ int Camera::queueRequest(Request *request)
>  		buffer->mem_ = &stream->buffers()[buffer->index_];
>  	}
>  
> -	int ret = request->prepare();
> -	if (ret) {
> -		LOG(Camera, Error) << "Failed to prepare request";
> -		return ret;
> -	}
> -
>  	return pipe_->queueRequest(this, request);
>  }
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index c14ed1a4d3ce55d0..84a5f55879ccf5b5 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -139,6 +139,8 @@ int Request::addBuffer(std::unique_ptr<Buffer> buffer)
>  		return -EEXIST;
>  	}
>  
> +	buffer->setRequest(this);
> +	pending_.insert(buffer.get());
>  	bufferMap_[stream] = buffer.release();
>  
>  	return 0;
> @@ -203,30 +205,6 @@ Buffer *Request::findBuffer(Stream *stream) const
>   * otherwise
>   */
>  
> -/**
> - * \brief Validate the request and prepare it for the completion handler
> - *
> - * Requests that contain no buffers are invalid and are rejected.
> - *
> - * \return 0 on success or a negative error code otherwise
> - * \retval -EINVAL The request is invalid
> - */
> -int Request::prepare()
> -{
> -	if (bufferMap_.empty()) {
> -		LOG(Request, Error) << "Invalid request due to missing buffers";
> -		return -EINVAL;
> -	}
> -
> -	for (auto const &pair : bufferMap_) {
> -		Buffer *buffer = pair.second;
> -		buffer->setRequest(this);
> -		pending_.insert(buffer);
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * \brief Complete a queued request
>   *

-- 
Regards,

Laurent Pinchart