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

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