| Message ID | 20210312061131.854849-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > For all frame indexes, use the same sequence number as generated > by the Request object. > > This allows clear matching of what operations occured to which request. nit-pick: This will not be true if we need to queue buffers internally (without a request) to feed the 3A right? I think this change may be a step in the right direction just wanted to bring up why an internal id was used. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > src/libcamera/pipeline/ipu3/frames.h | 1 - > 2 files changed, 1 insertion(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index 151ebfe7ca5f..4198e2019f3f 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -18,7 +18,6 @@ namespace libcamera { > LOG_DECLARE_CATEGORY(IPU3) > > IPU3Frames::IPU3Frames() > - : nextId_(0) > { > } > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > availableStatBuffers_.push(buffer.get()); > > - nextId_ = 0; > frameInfo_.clear(); > } > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > IPU3Frames::Info *IPU3Frames::create(Request *request) > { > - unsigned int id = nextId_++; > + unsigned int id = request->sequence(); > > if (availableParamBuffers_.empty()) { > LOG(IPU3, Error) << "Parameters buffer underrun"; > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > index 106e5c15cc7a..4acdf48eca9d 100644 > --- a/src/libcamera/pipeline/ipu3/frames.h > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -53,7 +53,6 @@ private: > std::queue<FrameBuffer *> availableParamBuffers_; > std::queue<FrameBuffer *> availableStatBuffers_; > > - unsigned int nextId_; > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > }; > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran and Niklas, On Sat, Mar 13, 2021 at 12:31:25AM +0100, Niklas Söderlund wrote: > On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > > For all frame indexes, use the same sequence number as generated > > by the Request object. > > > > This allows clear matching of what operations occured to which request. > > nit-pick: This will not be true if we need to queue buffers internally > (without a request) to feed the 3A right? I think this change may be a > step in the right direction just wanted to bring up why an internal id > was used. Wouldn't we still have a request, even if it has only a raw buffer ? > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > > src/libcamera/pipeline/ipu3/frames.h | 1 - > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > index 151ebfe7ca5f..4198e2019f3f 100644 > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > @@ -18,7 +18,6 @@ namespace libcamera { > > LOG_DECLARE_CATEGORY(IPU3) > > > > IPU3Frames::IPU3Frames() > > - : nextId_(0) > > { > > } > > > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > availableStatBuffers_.push(buffer.get()); > > > > - nextId_ = 0; > > frameInfo_.clear(); > > } > > > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > > > IPU3Frames::Info *IPU3Frames::create(Request *request) > > { > > - unsigned int id = nextId_++; > > + unsigned int id = request->sequence(); > > > > if (availableParamBuffers_.empty()) { > > LOG(IPU3, Error) << "Parameters buffer underrun"; > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > index 106e5c15cc7a..4acdf48eca9d 100644 > > --- a/src/libcamera/pipeline/ipu3/frames.h > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > @@ -53,7 +53,6 @@ private: > > std::queue<FrameBuffer *> availableParamBuffers_; > > std::queue<FrameBuffer *> availableStatBuffers_; > > > > - unsigned int nextId_; > > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > > }; > >
Hi Laurent, On 2021-03-14 03:53:09 +0200, Laurent Pinchart wrote: > Hi Kieran and Niklas, > > On Sat, Mar 13, 2021 at 12:31:25AM +0100, Niklas Söderlund wrote: > > On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > > > For all frame indexes, use the same sequence number as generated > > > by the Request object. > > > > > > This allows clear matching of what operations occured to which request. > > > > nit-pick: This will not be true if we need to queue buffers internally > > (without a request) to feed the 3A right? I think this change may be a > > step in the right direction just wanted to bring up why an internal id > > was used. > > Wouldn't we still have a request, even if it has only a raw buffer ? Not if it's on the pipelines behalf we run buffers to meet 3A requirements. In such cases the application would not be involved at all and using Requests infernally would then be confusing as the seq numbers domain available to applications would then have holes in it. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > > > src/libcamera/pipeline/ipu3/frames.h | 1 - > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > > index 151ebfe7ca5f..4198e2019f3f 100644 > > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > > @@ -18,7 +18,6 @@ namespace libcamera { > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > IPU3Frames::IPU3Frames() > > > - : nextId_(0) > > > { > > > } > > > > > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > > > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > > availableStatBuffers_.push(buffer.get()); > > > > > > - nextId_ = 0; > > > frameInfo_.clear(); > > > } > > > > > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > > > > > IPU3Frames::Info *IPU3Frames::create(Request *request) > > > { > > > - unsigned int id = nextId_++; > > > + unsigned int id = request->sequence(); > > > > > > if (availableParamBuffers_.empty()) { > > > LOG(IPU3, Error) << "Parameters buffer underrun"; > > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > > index 106e5c15cc7a..4acdf48eca9d 100644 > > > --- a/src/libcamera/pipeline/ipu3/frames.h > > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > > @@ -53,7 +53,6 @@ private: > > > std::queue<FrameBuffer *> availableParamBuffers_; > > > std::queue<FrameBuffer *> availableStatBuffers_; > > > > > > - unsigned int nextId_; > > > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > > > }; > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Sun, Mar 14, 2021 at 10:32:28AM +0100, Niklas Söderlund wrote: > On 2021-03-14 03:53:09 +0200, Laurent Pinchart wrote: > > On Sat, Mar 13, 2021 at 12:31:25AM +0100, Niklas Söderlund wrote: > > > On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > > > > For all frame indexes, use the same sequence number as generated > > > > by the Request object. > > > > > > > > This allows clear matching of what operations occured to which request. > > > > > > nit-pick: This will not be true if we need to queue buffers internally > > > (without a request) to feed the 3A right? I think this change may be a > > > step in the right direction just wanted to bring up why an internal id > > > was used. > > > > Wouldn't we still have a request, even if it has only a raw buffer ? > > Not if it's on the pipelines behalf we run buffers to meet 3A > requirements. In such cases the application would not be involved at all > and using Requests infernally would then be confusing as the seq numbers > domain available to applications would then have holes in it. That would only happen if applications stop queuing requests completely. That's considered, at least for now, as an invalid use of the API, the request queue must be fed at all times for proper operation. What use case do you envision for this ? > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > > > > src/libcamera/pipeline/ipu3/frames.h | 1 - > > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > > > index 151ebfe7ca5f..4198e2019f3f 100644 > > > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > > > @@ -18,7 +18,6 @@ namespace libcamera { > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > IPU3Frames::IPU3Frames() > > > > - : nextId_(0) > > > > { > > > > } > > > > > > > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > > > > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > > > availableStatBuffers_.push(buffer.get()); > > > > > > > > - nextId_ = 0; > > > > frameInfo_.clear(); > > > > } > > > > > > > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > > > > > > > IPU3Frames::Info *IPU3Frames::create(Request *request) > > > > { > > > > - unsigned int id = nextId_++; > > > > + unsigned int id = request->sequence(); > > > > > > > > if (availableParamBuffers_.empty()) { > > > > LOG(IPU3, Error) << "Parameters buffer underrun"; > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > > > index 106e5c15cc7a..4acdf48eca9d 100644 > > > > --- a/src/libcamera/pipeline/ipu3/frames.h > > > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > > > @@ -53,7 +53,6 @@ private: > > > > std::queue<FrameBuffer *> availableParamBuffers_; > > > > std::queue<FrameBuffer *> availableStatBuffers_; > > > > > > > > - unsigned int nextId_; > > > > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > > > > }; > > > >
Hi Laurent, On 2021-03-14 16:50:17 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Sun, Mar 14, 2021 at 10:32:28AM +0100, Niklas Söderlund wrote: > > On 2021-03-14 03:53:09 +0200, Laurent Pinchart wrote: > > > On Sat, Mar 13, 2021 at 12:31:25AM +0100, Niklas Söderlund wrote: > > > > On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > > > > > For all frame indexes, use the same sequence number as generated > > > > > by the Request object. > > > > > > > > > > This allows clear matching of what operations occured to which request. > > > > > > > > nit-pick: This will not be true if we need to queue buffers internally > > > > (without a request) to feed the 3A right? I think this change may be a > > > > step in the right direction just wanted to bring up why an internal id > > > > was used. > > > > > > Wouldn't we still have a request, even if it has only a raw buffer ? > > > > Not if it's on the pipelines behalf we run buffers to meet 3A > > requirements. In such cases the application would not be involved at all > > and using Requests infernally would then be confusing as the seq numbers > > domain available to applications would then have holes in it. > > That would only happen if applications stop queuing requests completely. > That's considered, at least for now, as an invalid use of the API, the > request queue must be fed at all times for proper operation. What use > case do you envision for this ? Yes this only becomes an issue if applications stop queueing requests. We have in the past talked about this and that it was not unthinkable that pipelines going forward would need to gain the ability to run things internally to satisfy 3A constraints instead of pushing it all to applications. Maybe we have now figured out this is not needed or desired and then my concern is addressed. But if we still think we internally need to queue buffers at some point I think this debug aid change may be confusing going forward. If we don't know what direction we are going to take here I see no problem with merging this patch as it makes things easier now. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > --- > > > > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > > > > > src/libcamera/pipeline/ipu3/frames.h | 1 - > > > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > > > > index 151ebfe7ca5f..4198e2019f3f 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > > > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > > > > @@ -18,7 +18,6 @@ namespace libcamera { > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > > > IPU3Frames::IPU3Frames() > > > > > - : nextId_(0) > > > > > { > > > > > } > > > > > > > > > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > > > > > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > > > > availableStatBuffers_.push(buffer.get()); > > > > > > > > > > - nextId_ = 0; > > > > > frameInfo_.clear(); > > > > > } > > > > > > > > > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > > > > > > > > > IPU3Frames::Info *IPU3Frames::create(Request *request) > > > > > { > > > > > - unsigned int id = nextId_++; > > > > > + unsigned int id = request->sequence(); > > > > > > > > > > if (availableParamBuffers_.empty()) { > > > > > LOG(IPU3, Error) << "Parameters buffer underrun"; > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > > > > index 106e5c15cc7a..4acdf48eca9d 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/frames.h > > > > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > > > > @@ -53,7 +53,6 @@ private: > > > > > std::queue<FrameBuffer *> availableParamBuffers_; > > > > > std::queue<FrameBuffer *> availableStatBuffers_; > > > > > > > > > > - unsigned int nextId_; > > > > > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > > > > > }; > > > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Mon, Mar 15, 2021 at 09:58:48AM +0100, Niklas Söderlund wrote: > On 2021-03-14 16:50:17 +0200, Laurent Pinchart wrote: > > On Sun, Mar 14, 2021 at 10:32:28AM +0100, Niklas Söderlund wrote: > > > On 2021-03-14 03:53:09 +0200, Laurent Pinchart wrote: > > > > On Sat, Mar 13, 2021 at 12:31:25AM +0100, Niklas Söderlund wrote: > > > > > On 2021-03-12 06:11:27 +0000, Kieran Bingham wrote: > > > > > > For all frame indexes, use the same sequence number as generated > > > > > > by the Request object. > > > > > > > > > > > > This allows clear matching of what operations occured to which request. > > > > > > > > > > nit-pick: This will not be true if we need to queue buffers internally > > > > > (without a request) to feed the 3A right? I think this change may be a > > > > > step in the right direction just wanted to bring up why an internal id > > > > > was used. > > > > > > > > Wouldn't we still have a request, even if it has only a raw buffer ? > > > > > > Not if it's on the pipelines behalf we run buffers to meet 3A > > > requirements. In such cases the application would not be involved at all > > > and using Requests infernally would then be confusing as the seq numbers > > > domain available to applications would then have holes in it. > > > > That would only happen if applications stop queuing requests completely. > > That's considered, at least for now, as an invalid use of the API, the > > request queue must be fed at all times for proper operation. What use > > case do you envision for this ? > > Yes this only becomes an issue if applications stop queueing requests. > We have in the past talked about this and that it was not unthinkable > that pipelines going forward would need to gain the ability to run > things internally to satisfy 3A constraints instead of pushing it all to > applications. > > Maybe we have now figured out this is not needed or desired and then my > concern is addressed. But if we still think we internally need to queue > buffers at some point I think this debug aid change may be confusing > going forward. > > If we don't know what direction we are going to take here I see no > problem with merging this patch as it makes things easier now. The only use case I can foresee requiring operating the camera internally without requests for every frame would be still image capture (with flash sequence). Even there, it's not clear if that would be needed. Given that it's not a short term development, I wouldn't care about it now in the context of this patch. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > --- > > > > > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- > > > > > > src/libcamera/pipeline/ipu3/frames.h | 1 - > > > > > > 2 files changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > > > > > index 151ebfe7ca5f..4198e2019f3f 100644 > > > > > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > > > > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > > > > > @@ -18,7 +18,6 @@ namespace libcamera { > > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > > > > > IPU3Frames::IPU3Frames() > > > > > > - : nextId_(0) > > > > > > { > > > > > > } > > > > > > > > > > > > @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff > > > > > > for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > > > > > availableStatBuffers_.push(buffer.get()); > > > > > > > > > > > > - nextId_ = 0; > > > > > > frameInfo_.clear(); > > > > > > } > > > > > > > > > > > > @@ -43,7 +41,7 @@ void IPU3Frames::clear() > > > > > > > > > > > > IPU3Frames::Info *IPU3Frames::create(Request *request) > > > > > > { > > > > > > - unsigned int id = nextId_++; > > > > > > + unsigned int id = request->sequence(); > > > > > > > > > > > > if (availableParamBuffers_.empty()) { > > > > > > LOG(IPU3, Error) << "Parameters buffer underrun"; > > > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > > > > > index 106e5c15cc7a..4acdf48eca9d 100644 > > > > > > --- a/src/libcamera/pipeline/ipu3/frames.h > > > > > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > > > > > @@ -53,7 +53,6 @@ private: > > > > > > std::queue<FrameBuffer *> availableParamBuffers_; > > > > > > std::queue<FrameBuffer *> availableStatBuffers_; > > > > > > > > > > > > - unsigned int nextId_; > > > > > > std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > > > > > > }; > > > > > >
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp index 151ebfe7ca5f..4198e2019f3f 100644 --- a/src/libcamera/pipeline/ipu3/frames.cpp +++ b/src/libcamera/pipeline/ipu3/frames.cpp @@ -18,7 +18,6 @@ namespace libcamera { LOG_DECLARE_CATEGORY(IPU3) IPU3Frames::IPU3Frames() - : nextId_(0) { } @@ -31,7 +30,6 @@ void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuff for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) availableStatBuffers_.push(buffer.get()); - nextId_ = 0; frameInfo_.clear(); } @@ -43,7 +41,7 @@ void IPU3Frames::clear() IPU3Frames::Info *IPU3Frames::create(Request *request) { - unsigned int id = nextId_++; + unsigned int id = request->sequence(); if (availableParamBuffers_.empty()) { LOG(IPU3, Error) << "Parameters buffer underrun"; diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h index 106e5c15cc7a..4acdf48eca9d 100644 --- a/src/libcamera/pipeline/ipu3/frames.h +++ b/src/libcamera/pipeline/ipu3/frames.h @@ -53,7 +53,6 @@ private: std::queue<FrameBuffer *> availableParamBuffers_; std::queue<FrameBuffer *> availableStatBuffers_; - unsigned int nextId_; std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; };
For all frame indexes, use the same sequence number as generated by the Request object. This allows clear matching of what operations occured to which request. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/frames.cpp | 4 +--- src/libcamera/pipeline/ipu3/frames.h | 1 - 2 files changed, 1 insertion(+), 4 deletions(-)