[libcamera-devel,v3,05/22] libcamera: pipeline: rkisp1: Prepare buffer ready handlers for multiple streams
diff mbox series

Message ID 20200925014207.1455796-6-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Sept. 25, 2020, 1:41 a.m. UTC 
The buffer ready handlers are designed for a single application facing
stream from the main path. To prepare for multiple application facing
streams from main and/or self path the handlers need to be prepared.

The data keeping tasks of frame number and advancing the timeline can be
moved from the application facing buffer ready handler to the statistics
handler. For each request processed there will always be a statistic
buffer and as the ISP is inline and is the source of both main, self and
statistic paths there is no lose in precision from this.

The application facing handler no longer needs a special case for
cancelled frames and can be made simpler. With this change the handlers
are ready to deal with any combinations of application facing streams.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Sept. 25, 2020, 2:20 p.m. UTC | #1 
Hi Niklas,

On Fri, Sep 25, 2020 at 03:41:50AM +0200, Niklas Söderlund wrote:
> The buffer ready handlers are designed for a single application facing
> stream from the main path. To prepare for multiple application facing
> streams from main and/or self path the handlers need to be prepared.
>
> The data keeping tasks of frame number and advancing the timeline can be

Keepin 'track'

the frame number
or
frame numbers


> moved from the application facing buffer ready handler to the statistics
> handler. For each request processed there will always be a statistic

be one statistic

> buffer and as the ISP is inline and is the source of both main, self and
> statistic paths there is no lose in precision from this.

not sure but 'lose in precision' sounds weird

>
> The application facing handler no longer needs a special case for
> cancelled frames and can be made simpler. With this change the handlers
> are ready to deal with any combinations of application facing streams.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1070,20 +1070,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
> -	RkISP1CameraData *data = cameraData(activeCamera_);
>  	Request *request = buffer->request();
>
> -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> -		completeBuffer(activeCamera_, request, buffer);
> -		completeRequest(activeCamera_, request);
> -		return;
> -	}
> -
> -	data->timeline_.bufferReady(buffer);
> -
> -	if (data->frame_ <= buffer->metadata().sequence)
> -		data->frame_ = buffer->metadata().sequence + 1;
> -
>  	completeBuffer(activeCamera_, request, buffer);
>  	tryCompleteRequest(request);
>  }
> @@ -1114,6 +1102,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>
> +	data->timeline_.bufferReady(buffer);
> +
> +	if (data->frame_ <= buffer->metadata().sequence)

How can frame_ be larger ?

Please keep my tag

Thanks
  j

> +		data->frame_ = buffer->metadata().sequence + 1;
> +
>  	IPAOperationData op;
>  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
>  	op.data = { info->frame, info->statBuffer->cookie() };
> --
> 2.28.0
>
Niklas Söderlund Sept. 25, 2020, 4:18 p.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2020-09-25 16:20:18 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 03:41:50AM +0200, Niklas Söderlund wrote:
> > The buffer ready handlers are designed for a single application facing
> > stream from the main path. To prepare for multiple application facing
> > streams from main and/or self path the handlers need to be prepared.
> >
> > The data keeping tasks of frame number and advancing the timeline can be
> 
> Keepin 'track'
> 
> the frame number
> or
> frame numbers
> 
> 
> > moved from the application facing buffer ready handler to the statistics
> > handler. For each request processed there will always be a statistic
> 
> be one statistic
> 
> > buffer and as the ISP is inline and is the source of both main, self and
> > statistic paths there is no lose in precision from this.
> 
> not sure but 'lose in precision' sounds weird
> 
> >
> > The application facing handler no longer needs a special case for
> > cancelled frames and can be made simpler. With this change the handlers
> > are ready to deal with any combinations of application facing streams.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1070,20 +1070,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> >  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >  {
> >  	ASSERT(activeCamera_);
> > -	RkISP1CameraData *data = cameraData(activeCamera_);
> >  	Request *request = buffer->request();
> >
> > -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > -		completeBuffer(activeCamera_, request, buffer);
> > -		completeRequest(activeCamera_, request);
> > -		return;
> > -	}
> > -
> > -	data->timeline_.bufferReady(buffer);
> > -
> > -	if (data->frame_ <= buffer->metadata().sequence)
> > -		data->frame_ = buffer->metadata().sequence + 1;
> > -
> >  	completeBuffer(activeCamera_, request, buffer);
> >  	tryCompleteRequest(request);
> >  }
> > @@ -1114,6 +1102,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  	if (!info)
> >  		return;
> >
> > +	data->timeline_.bufferReady(buffer);
> > +
> > +	if (data->frame_ <= buffer->metadata().sequence)
> 
> How can frame_ be larger ?

It can't :-) But if it's smaller the hardware have dropped frames and we 
need to account for that.

> 
> Please keep my tag
> 
> Thanks
>   j
> 
> > +		data->frame_ = buffer->metadata().sequence + 1;
> > +
> >  	IPAOperationData op;
> >  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> >  	op.data = { info->frame, info->statBuffer->cookie() };
> > --
> > 2.28.0
> >
Jacopo Mondi Sept. 28, 2020, 8:16 a.m. UTC | #3 
Hi Niklas,

On Fri, Sep 25, 2020 at 06:18:13PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-09-25 16:20:18 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Sep 25, 2020 at 03:41:50AM +0200, Niklas Söderlund wrote:
> > > The buffer ready handlers are designed for a single application facing
> > > stream from the main path. To prepare for multiple application facing
> > > streams from main and/or self path the handlers need to be prepared.
> > >
> > > The data keeping tasks of frame number and advancing the timeline can be
> >
> > Keepin 'track'
> >
> > the frame number
> > or
> > frame numbers
> >
> >
> > > moved from the application facing buffer ready handler to the statistics
> > > handler. For each request processed there will always be a statistic
> >
> > be one statistic
> >
> > > buffer and as the ISP is inline and is the source of both main, self and
> > > statistic paths there is no lose in precision from this.
> >
> > not sure but 'lose in precision' sounds weird
> >
> > >
> > > The application facing handler no longer needs a special case for
> > > cancelled frames and can be made simpler. With this change the handlers
> > > are ready to deal with any combinations of application facing streams.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
> > >  1 file changed, 5 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -1070,20 +1070,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> > >  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > >  {
> > >  	ASSERT(activeCamera_);
> > > -	RkISP1CameraData *data = cameraData(activeCamera_);
> > >  	Request *request = buffer->request();
> > >
> > > -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > > -		completeBuffer(activeCamera_, request, buffer);
> > > -		completeRequest(activeCamera_, request);
> > > -		return;
> > > -	}
> > > -
> > > -	data->timeline_.bufferReady(buffer);
> > > -
> > > -	if (data->frame_ <= buffer->metadata().sequence)
> > > -		data->frame_ = buffer->metadata().sequence + 1;
> > > -
> > >  	completeBuffer(activeCamera_, request, buffer);
> > >  	tryCompleteRequest(request);
> > >  }
> > > @@ -1114,6 +1102,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > >  	if (!info)
> > >  		return;
> > >
> > > +	data->timeline_.bufferReady(buffer);
> > > +
> > > +	if (data->frame_ <= buffer->metadata().sequence)
> >
> > How can frame_ be larger ?
>
> It can't :-) But if it's smaller the hardware have dropped frames and we
> need to account for that.
>

If it can't be larger why the check then ?
And if it's smaller it seems to me it just gets advanced to
metadata().sequence + 1 unconditionally.

What have I missed ?


> >
> > Please keep my tag
> >
> > Thanks
> >   j
> >
> > > +		data->frame_ = buffer->metadata().sequence + 1;
> > > +
> > >  	IPAOperationData op;
> > >  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> > >  	op.data = { info->frame, info->statBuffer->cookie() };
> > > --
> > > 2.28.0
> > >
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund Sept. 28, 2020, 9:22 p.m. UTC | #4 
Hi Jacopo,

On 2020-09-28 10:16:55 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Sep 25, 2020 at 06:18:13PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your feedback.
> >
> > On 2020-09-25 16:20:18 +0200, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >
> > > On Fri, Sep 25, 2020 at 03:41:50AM +0200, Niklas Söderlund wrote:
> > > > The buffer ready handlers are designed for a single application facing
> > > > stream from the main path. To prepare for multiple application facing
> > > > streams from main and/or self path the handlers need to be prepared.
> > > >
> > > > The data keeping tasks of frame number and advancing the timeline can be
> > >
> > > Keepin 'track'
> > >
> > > the frame number
> > > or
> > > frame numbers
> > >
> > >
> > > > moved from the application facing buffer ready handler to the statistics
> > > > handler. For each request processed there will always be a statistic
> > >
> > > be one statistic
> > >
> > > > buffer and as the ISP is inline and is the source of both main, self and
> > > > statistic paths there is no lose in precision from this.
> > >
> > > not sure but 'lose in precision' sounds weird
> > >
> > > >
> > > > The application facing handler no longer needs a special case for
> > > > cancelled frames and can be made simpler. With this change the handlers
> > > > are ready to deal with any combinations of application facing streams.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------
> > > >  1 file changed, 5 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -1070,20 +1070,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> > > >  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> > > >  {
> > > >  	ASSERT(activeCamera_);
> > > > -	RkISP1CameraData *data = cameraData(activeCamera_);
> > > >  	Request *request = buffer->request();
> > > >
> > > > -	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > > > -		completeBuffer(activeCamera_, request, buffer);
> > > > -		completeRequest(activeCamera_, request);
> > > > -		return;
> > > > -	}
> > > > -
> > > > -	data->timeline_.bufferReady(buffer);
> > > > -
> > > > -	if (data->frame_ <= buffer->metadata().sequence)
> > > > -		data->frame_ = buffer->metadata().sequence + 1;
> > > > -
> > > >  	completeBuffer(activeCamera_, request, buffer);
> > > >  	tryCompleteRequest(request);
> > > >  }
> > > > @@ -1114,6 +1102,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > > >  	if (!info)
> > > >  		return;
> > > >
> > > > +	data->timeline_.bufferReady(buffer);
> > > > +
> > > > +	if (data->frame_ <= buffer->metadata().sequence)
> > >
> > > How can frame_ be larger ?
> >
> > It can't :-) But if it's smaller the hardware have dropped frames and we
> > need to account for that.
> >
> 
> If it can't be larger why the check then ?
> And if it's smaller it seems to me it just gets advanced to
> metadata().sequence + 1 unconditionally.
> 
> What have I missed ?

This is not the optimum location to increment the internal frame counter 
as it's only incremented once a buffer is complete and have been 
dequeued. This will lead to multiple requests queued in a short interval 
would have the same (pipeline internal) frame number.

Instead we increment the frame whenever a request is queued to the 
pipeline from the application. But we still need this check here to 
detect if the hardware have dropped any frames due to the pipeline being 
to slow (this have been observed before OpenGL rendering was added to 
qcam).

Going forward I think we need to rethink the Timeline used in this 
pipeline, possibly looking to how this is solved in the RPi pipeline.  
But that is a task for another series.

> 
> 
> > >
> > > Please keep my tag
> > >
> > > Thanks
> > >   j
> > >
> > > > +		data->frame_ = buffer->metadata().sequence + 1;
> > > > +
> > > >  	IPAOperationData op;
> > > >  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> > > >  	op.data = { info->frame, info->statBuffer->cookie() };
> > > > --
> > > > 2.28.0
> > > >
> >
> > --
> > Regards,
> > Niklas Söderlund

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 8bb4582eeb688a4c..d6b5073b2084a9f4 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1070,20 +1070,8 @@  void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
 void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
 {
 	ASSERT(activeCamera_);
-	RkISP1CameraData *data = cameraData(activeCamera_);
 	Request *request = buffer->request();
 
-	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
-		completeBuffer(activeCamera_, request, buffer);
-		completeRequest(activeCamera_, request);
-		return;
-	}
-
-	data->timeline_.bufferReady(buffer);
-
-	if (data->frame_ <= buffer->metadata().sequence)
-		data->frame_ = buffer->metadata().sequence + 1;
-
 	completeBuffer(activeCamera_, request, buffer);
 	tryCompleteRequest(request);
 }
@@ -1114,6 +1102,11 @@  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
 	if (!info)
 		return;
 
+	data->timeline_.bufferReady(buffer);
+
+	if (data->frame_ <= buffer->metadata().sequence)
+		data->frame_ = buffer->metadata().sequence + 1;
+
 	IPAOperationData op;
 	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
 	op.data = { info->frame, info->statBuffer->cookie() };