[libcamera-devel] [PATCH v3 03/11] libcamera: request: Provide a sequence number

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 25, 2021 at 01:42:23PM +0000, Kieran Bingham wrote:
> Provide a sequence number on Requests which are added by the pipeline
> handler.
> 
> Each pipeline handler keeps a requestSequence per CameraData and
> increments everytime a request is queued on that camera.
> 
> The sequence number is associated with the Request and can be utilised
> for assisting with debugging, and printing the queueing sequence of in
> flight requests.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 +++-
>  include/libcamera/request.h                   |  2 ++
>  src/libcamera/pipeline_handler.cpp            |  2 ++
>  src/libcamera/request.cpp                     | 11 +++++++++--
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 6aca0b46f43d..9bdda8f3b799 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -38,7 +38,7 @@ class CameraData
>  {
>  public:
>  	explicit CameraData(PipelineHandler *pipe)
> -		: pipe_(pipe)
> +		: pipe_(pipe), requestSequence_(0)
>  	{
>  	}
>  	virtual ~CameraData() = default;
> @@ -48,6 +48,8 @@ public:
>  	ControlInfoMap controlInfo_;
>  	ControlList properties_;
>  
> +	uint32_t requestSequence_;
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY(CameraData)
>  };
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 6e5aad5f6b75..cd5a24741f8a 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -50,6 +50,7 @@ public:
>  	int addBuffer(const Stream *stream, FrameBuffer *buffer);
>  	FrameBuffer *findBuffer(const Stream *stream) const;
>  
> +	uint32_t sequence() const { return sequence_; }
>  	uint64_t cookie() const { return cookie_; }
>  	Status status() const { return status_; }
>  
> @@ -71,6 +72,7 @@ private:
>  	BufferMap bufferMap_;
>  	std::unordered_set<FrameBuffer *> pending_;
>  
> +	uint32_t sequence_;
>  	const uint64_t cookie_;
>  	Status status_;
>  	bool cancelled_;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index d22991d318c9..e3d4975d9ffd 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -382,6 +382,8 @@ int PipelineHandler::queueRequest(Request *request)
>  	CameraData *data = cameraData(camera);
>  	data->queuedRequests_.push_back(request);
>  
> +	request->sequence_ = data->requestSequence_++;
> +
>  	int ret = queueRequestDevice(camera, request);
>  	if (ret)
>  		data->queuedRequests_.remove(request);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 3ad83f3b8418..fc16b148a599 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -72,8 +72,8 @@ LOG_DEFINE_CATEGORY(Request)
>   *
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -	: camera_(camera), cookie_(cookie), status_(RequestPending),
> -	  cancelled_(false)
> +	: camera_(camera), sequence_(0), cookie_(cookie),
> +	  status_(RequestPending), cancelled_(false)
>  {
>  	/**
>  	 * \todo Should the Camera expose a validator instance, to avoid
> @@ -126,6 +126,7 @@ void Request::reuse(ReuseFlag flags)
>  		bufferMap_.clear();
>  	}
>  
> +	sequence_ = 0;
>  	status_ = RequestPending;
>  	cancelled_ = false;
>  
> @@ -227,6 +228,12 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   * \return The metadata associated with the request
>   */
>  
> +/**
> + * \fn Request::sequence()
> + * \brief Retrieve the sequence number for the request
> + * \return The request sequence number

As the sequence number is exposed in the public API, could you elaborate
a bit more, to explain what it is and what it's used for ? Otherwise,
the patch looks good to me.

> + */
> +
>  /**
>   * \fn Request::cookie()
>   * \brief Retrieve the cookie set when the request was created

-- 
Regards,

Laurent Pinchart