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

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 15, 2021 at 11:59:53AM +0000, Kieran Bingham wrote:
> On 14/03/2021 00:51, Laurent Pinchart wrote:
> > 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" ?
> 
> Yes
> 
> >> 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.
> 
> Indeed, this is certainly a per camera sequence number.
> 
> I'll fix 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
> 
> But ... at 120 frames per second ... that would only give us something
> like 414 days before wrap around! ... how are we going to ...
> 
> :-D
> 
> 
> > I'd argue that wrap-around would actually be a good to have feature, as
> 
> If you /desire/ wrap around we could go to uint16_t ... 65536 frames
> ought to be enough for anybody right...

I've actually considered that. I'm not sure if I like it :-)

> > a 20 characters number isn't much more readable than a pointer).
> 
> Well ... I disagree slightly there - a number is sequential ;-)
> A pointer is not.... but getting too long indeed probably defies the point.
> 
> And I agree with you on 3/8 actually - I like the idea that we don't
> reset the sequence to zero on stop to provide a continuous counter.
> That may help spot other bugs / flow issues.
> 
> >> +
> >>  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 :-)
> 
> Ack.
> 
> >>  	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() ?
> 
> Maybe ...
> 
> That makes me wonder if we should initialise the requestSequence_ to
> always start at 1 to leave 0 as invalid - but that wouldn't help with
> the wrap-around. Or maybe that's my only weak argument to keep it as
> uint64_t...

I've also thought about this. I don't think it's a big deal, but we
could address it indeed.

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