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

[Laurent Pinchart]

Hi Kieran,

On Fri, Mar 26, 2021 at 02:54:41PM +0000, Kieran Bingham wrote:
> On 26/03/2021 02:05, Laurent Pinchart wrote:
> > 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.
> 
> I can - but I also expect this is likely to move to the private
> structures when Request becomes Extensible. Unless you'd prefer it to be
> part of the public API?
> 
> Maybe it has some value to applications?

Good question. I think it may, but I'm not sure.

> \brief Retrieve the sequence number for the request
> 
> When requests are queued, they are given a sequential number to track
> the order in which requests are queued to a camera. This number counts
> all requests given to a camera through its lifetime, and is not reset to
> zero between camera stop/start sequences.
> 
> It can be used to support debugging and identifying the flow of requests
> through a pipeline, but does not guarantee to represent the sequence
> number of any images in the stream. The sequence number is stored as an
> unsigned integer and will wrap when overflowed.
> 
> \return The request sequence number

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

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

-- 
Regards,

Laurent Pinchart