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

[Kieran Bingham]

Hi Laurent,

On 14/03/2021 00:51, Laurent Pinchart wrote:
> 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" ?

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

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


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