| Message ID | 20210312061131.854849-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@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);
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@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); >
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@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);
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_; + 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_; } 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_; 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);
Provide a sequence number on Requests which are added by the pipeline handler. Each pipeline handler keeps a requestSequence and increments everytime a request is queued. 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@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(-)