| Message ID | 20210325134231.1400051-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. 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@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. > + */ > + > /** > * \fn Request::cookie() > * \brief Retrieve the cookie set when the request was created
Hi Laurent, On 26/03/2021 02:05, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > 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@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? \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 > >> + */ >> + >> /** >> * \fn Request::cookie() >> * \brief Retrieve the cookie set when the request was created >
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@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@ideasonboard.com> > >> + */ > >> + > >> /** > >> * \fn Request::cookie() > >> * \brief Retrieve the cookie set when the request was created
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 + */ + /** * \fn Request::cookie() * \brief Retrieve the cookie set when the request was created
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@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(-)