[libcamera-devel,11/13] libcamera: pipeline: rkisp1: Track buffers for self path

9301 diff mbox series

In preparation of supporting both the main and self path extend
RkISP1FrameInfo to track buffers from the self path stream.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Hi Niklas,

On Thu, Aug 13, 2020 at 02:52:44AM +0200, Niklas Söderlund wrote:
> In preparation of supporting both the main and self path extend
> RkISP1FrameInfo to track buffers from the self path stream.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 671098e11a2e2750..3761b7ef7a9386e3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -67,6 +67,7 @@ struct RkISP1FrameInfo {
>  	FrameBuffer *paramBuffer;
>  	FrameBuffer *statBuffer;
>  	FrameBuffer *mainPathBuffer;
> +	FrameBuffer *selfPathBuffer;
>
>  	bool paramFilled;
>  	bool paramDequeued;
> @@ -270,7 +271,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
>
>  	FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> -	if (!mainPathBuffer) {
> +	FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> +	if (!mainPathBuffer && !selfPathBuffer) {

I don't see in the rest of the function anything checking if either of
the two is valid, hence I assum you expect -both- of them to be valid.
If that's the case, shouldn't the condition be || instead of && ?

>  		LOG(RkISP1, Error)
>  			<< "Attempt to queue request with invalid stream";
>  		return nullptr;
> @@ -285,6 +287,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>  	info->request = request;
>  	info->paramBuffer = paramBuffer;
>  	info->mainPathBuffer = mainPathBuffer;
> +	info->selfPathBuffer = selfPathBuffer;
>  	info->statBuffer = statBuffer;
>  	info->paramFilled = false;
>  	info->paramDequeued = false;
> @@ -343,7 +346,8 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
>
>  		if (info->paramBuffer == buffer ||
>  		    info->statBuffer == buffer ||
> -		    info->mainPathBuffer == buffer)
> +		    info->mainPathBuffer == buffer ||
> +		    info->selfPathBuffer == buffer)
>  			return info;
>  	}
>
> @@ -415,7 +419,12 @@ protected:
>
>  		pipe_->param_->queueBuffer(info->paramBuffer);
>  		pipe_->stat_->queueBuffer(info->statBuffer);
> -		pipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);
> +
> +		if (info->mainPathBuffer)
> +			pipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);
> +
> +		if (info->selfPathBuffer)
> +			pipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);

Ah, maybe that's where you check them, so I guess a nullptr is fine in
the above create() function.

>  	}
>
>  private:
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 2020-08-20 11:52:46 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Aug 13, 2020 at 02:52:44AM +0200, Niklas Söderlund wrote:
> > In preparation of supporting both the main and self path extend
> > RkISP1FrameInfo to track buffers from the self path stream.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 671098e11a2e2750..3761b7ef7a9386e3 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -67,6 +67,7 @@ struct RkISP1FrameInfo {
> >  	FrameBuffer *paramBuffer;
> >  	FrameBuffer *statBuffer;
> >  	FrameBuffer *mainPathBuffer;
> > +	FrameBuffer *selfPathBuffer;
> >
> >  	bool paramFilled;
> >  	bool paramDequeued;
> > @@ -270,7 +271,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
> >
> >  	FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> > -	if (!mainPathBuffer) {
> > +	FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> > +	if (!mainPathBuffer && !selfPathBuffer) {
> 
> I don't see in the rest of the function anything checking if either of
> the two is valid, hence I assum you expect -both- of them to be valid.
> If that's the case, shouldn't the condition be || instead of && ?

I expect one of them to be !nullptr as that would imply a Request have 
been queued without any buffers in it, something we currently can't 
handle. But it brings up an interesting point, going forward is queueing 
of empty Requests something we like to support? I'm thinking it could be 
useful to keep the IPA running even if the application is not really 
interested in frames at that point. Anyhow I think this is something for 
future work and this change only builds on the current behavior but 
extends it to the self path.

> 
> >  		LOG(RkISP1, Error)
> >  			<< "Attempt to queue request with invalid stream";
> >  		return nullptr;
> > @@ -285,6 +287,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >  	info->request = request;
> >  	info->paramBuffer = paramBuffer;
> >  	info->mainPathBuffer = mainPathBuffer;
> > +	info->selfPathBuffer = selfPathBuffer;
> >  	info->statBuffer = statBuffer;
> >  	info->paramFilled = false;
> >  	info->paramDequeued = false;
> > @@ -343,7 +346,8 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
> >
> >  		if (info->paramBuffer == buffer ||
> >  		    info->statBuffer == buffer ||
> > -		    info->mainPathBuffer == buffer)
> > +		    info->mainPathBuffer == buffer ||
> > +		    info->selfPathBuffer == buffer)
> >  			return info;
> >  	}
> >
> > @@ -415,7 +419,12 @@ protected:
> >
> >  		pipe_->param_->queueBuffer(info->paramBuffer);
> >  		pipe_->stat_->queueBuffer(info->statBuffer);
> > -		pipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);
> > +
> > +		if (info->mainPathBuffer)
> > +			pipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);
> > +
> > +		if (info->selfPathBuffer)
> > +			pipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);
> 
> Ah, maybe that's where you check them, so I guess a nullptr is fine in
> the above create() function.
> 
> >  	}
> >
> >  private:
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel