[libcamera-devel] [PATCH 1/8] libcamera: request: Provide a sequence number

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 12, 2021 at 06:11:24AM +0000, Kieran Bingham wrote:
> Provide a sequence number on Requests which are added by the pipeline

Did you mean "which are queued to the pipeline handler" ?

> handler.
> 
> Each pipeline handler keeps a requestSequence and increments everytime a
> request is queued.

The sequence number is actually stored in camera data, so it's a
per-camera sequence. That's better than having it at the pipeline
handler level I think, you can just update the commit message.

> 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 ++
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 6aca0b46f43d..0655a665a85f 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_;
>  
> +	uint64_t requestSequence_;

Likely not a big deal, but 32-bit would very likely be sufficient (and
I'd argue that wrap-around would actually be a good to have feature, as
a 20 characters number isn't much more readable than a pointer).

> +
>  private:
>  	LIBCAMERA_DISABLE_COPY(CameraData)
>  };
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 6e5aad5f6b75..6f2f881e840a 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;
>  
> +	uint64_t sequence() const { return sequence_; }

This is of course missing documentation :-)

>  	uint64_t cookie() const { return cookie_; }
>  	Status status() const { return status_; }
>  
> @@ -71,6 +72,7 @@ private:
>  	BufferMap bufferMap_;
>  	std::unordered_set<FrameBuffer *> pending_;
>  
> +	uint64_t sequence_;

Should this be initialized to 0, and reset to 0 in Request::reuse() ?

>  	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);

-- 
Regards,

Laurent Pinchart