[libcamera-devel] [PATCH 4/8] libcamera: pipeline: ipu3: frames: Use the request sequence

[Niklas Söderlund]

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 at ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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>> &paramBuff
> > >  	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

-- 
Regards,
Niklas Söderlund