[libcamera-devel,v2,06/10] pipeline: raspberrypi: Reorder startup drop frame initialisation
diff mbox series

Message ID 20221129134534.2933-7-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Platform configuration and buffer allocation improvements
Related show

Commit Message

Naushir Patuck Nov. 29, 2022, 1:45 p.m. UTC 
Reorder the code such that the IPA requested startup drop frames count is
available before the pipeline handler allocates any stream buffers.

This will be used in a subsequent change to stop Unicam buffer allocations if
there are no startup drop frames required.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart Dec. 2, 2022, 1:52 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Tue, Nov 29, 2022 at 01:45:30PM +0000, Naushir Patuck via libcamera-devel wrote:
> Reorder the code such that the IPA requested startup drop frames count is
> available before the pipeline handler allocates any stream buffers.
> 
> This will be used in a subsequent change to stop Unicam buffer allocations if
> there are no startup drop frames required.

Do you mean "if there are no startup drop frames required and the
application has configured a raw stream and always provides buffers for
it" ?

Why is that related ? Can't you avoid allocating internal raw buffers
even without startup frames being dropped ? Can't you use the raw buffer
from the first request to capture all the dropped frames and the first
useful frame ?

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 30 +++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 86eb43a3a3c5..742521927780 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1044,21 +1044,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  	RPiCameraData *data = cameraData(camera);
>  	int ret;
>  
> -	for (auto const stream : data->streams_)
> -		stream->resetBuffers();
> -
> -	if (!data->buffersAllocated_) {
> -		/* Allocate buffers for internal pipeline usage. */
> -		ret = prepareBuffers(camera);
> -		if (ret) {
> -			LOG(RPI, Error) << "Failed to allocate buffers";
> -			data->freeBuffers();
> -			stop(camera);
> -			return ret;
> -		}
> -		data->buffersAllocated_ = true;
> -	}
> -
>  	/* Check if a ScalerCrop control was specified. */
>  	if (controls)
>  		data->applyScalerCrop(*controls);
> @@ -1075,6 +1060,21 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  	/* Configure the number of dropped frames required on startup. */
>  	data->dropFrameCount_ = startConfig.dropFrameCount;
>  
> +	for (auto const stream : data->streams_)
> +		stream->resetBuffers();
> +
> +	if (!data->buffersAllocated_) {
> +		/* Allocate buffers for internal pipeline usage. */
> +		ret = prepareBuffers(camera);
> +		if (ret) {
> +			LOG(RPI, Error) << "Failed to allocate buffers";
> +			data->freeBuffers();
> +			stop(camera);
> +			return ret;
> +		}
> +		data->buffersAllocated_ = true;
> +	}
> +
>  	/* We need to set the dropFrameCount_ before queueing buffers. */
>  	ret = queueAllBuffers(camera);
>  	if (ret) {
Naushir Patuck Dec. 2, 2022, 2:42 p.m. UTC | #2 
Hi Laurent,

On Fri, 2 Dec 2022 at 13:53, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Nov 29, 2022 at 01:45:30PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> > Reorder the code such that the IPA requested startup drop frames count is
> > available before the pipeline handler allocates any stream buffers.
> >
> > This will be used in a subsequent change to stop Unicam buffer
> allocations if
> > there are no startup drop frames required.
>
> Do you mean "if there are no startup drop frames required and the
> application has configured a raw stream and always provides buffers for
> it" ?
>

Yes!


>
> Why is that related ? Can't you avoid allocating internal raw buffers
> even without startup frames being dropped ? Can't you use the raw buffer
> from the first request to capture all the dropped frames and the first
> useful frame ?
>

There are a couple of reasons for this:

1) I'm working under the assumption that I cannot enqueue the same buffer
multiple times in v4l2  Because of this, I can only ever queue 1 buffer,
have it
dequeued, processed by the ISP, and requeue it back to Unicam.  This
effectively halves my overall framerate because of the added latency of
running
the ISP.

2) Even if V4L2 does allow enquing the same framebuffer multiple times,
there is
a possible (probable?) HW race condition where the ISP could be processing
the
buffer for frame N, while Unicam is writing out to the buffer for frame N +
1.

 It would be interesting to know if (1) is actually possible.  I don't
recall exactly, but
I think I did try it, and might have got complaints from the v4l2 framework.

Regards,
Naush


> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 30 +++++++++----------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 86eb43a3a3c5..742521927780 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1044,21 +1044,6 @@ int PipelineHandlerRPi::start(Camera *camera,
> const ControlList *controls)
> >       RPiCameraData *data = cameraData(camera);
> >       int ret;
> >
> > -     for (auto const stream : data->streams_)
> > -             stream->resetBuffers();
> > -
> > -     if (!data->buffersAllocated_) {
> > -             /* Allocate buffers for internal pipeline usage. */
> > -             ret = prepareBuffers(camera);
> > -             if (ret) {
> > -                     LOG(RPI, Error) << "Failed to allocate buffers";
> > -                     data->freeBuffers();
> > -                     stop(camera);
> > -                     return ret;
> > -             }
> > -             data->buffersAllocated_ = true;
> > -     }
> > -
> >       /* Check if a ScalerCrop control was specified. */
> >       if (controls)
> >               data->applyScalerCrop(*controls);
> > @@ -1075,6 +1060,21 @@ int PipelineHandlerRPi::start(Camera *camera,
> const ControlList *controls)
> >       /* Configure the number of dropped frames required on startup. */
> >       data->dropFrameCount_ = startConfig.dropFrameCount;
> >
> > +     for (auto const stream : data->streams_)
> > +             stream->resetBuffers();
> > +
> > +     if (!data->buffersAllocated_) {
> > +             /* Allocate buffers for internal pipeline usage. */
> > +             ret = prepareBuffers(camera);
> > +             if (ret) {
> > +                     LOG(RPI, Error) << "Failed to allocate buffers";
> > +                     data->freeBuffers();
> > +                     stop(camera);
> > +                     return ret;
> > +             }
> > +             data->buffersAllocated_ = true;
> > +     }
> > +
> >       /* We need to set the dropFrameCount_ before queueing buffers. */
> >       ret = queueAllBuffers(camera);
> >       if (ret) {
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Dec. 2, 2022, 8:18 p.m. UTC | #3 
Hi Naush,

On Fri, Dec 02, 2022 at 02:42:38PM +0000, Naushir Patuck wrote:
> On Fri, 2 Dec 2022 at 13:53, Laurent Pinchart wrote:
> > On Tue, Nov 29, 2022 at 01:45:30PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > Reorder the code such that the IPA requested startup drop frames count is
> > > available before the pipeline handler allocates any stream buffers.
> > >
> > > This will be used in a subsequent change to stop Unicam buffer allocations if
> > > there are no startup drop frames required.
> >
> > Do you mean "if there are no startup drop frames required and the
> > application has configured a raw stream and always provides buffers for
> > it" ?
> 
> Yes!

Could you update the commit message please ?

> > Why is that related ? Can't you avoid allocating internal raw buffers
> > even without startup frames being dropped ? Can't you use the raw buffer
> > from the first request to capture all the dropped frames and the first
> > useful frame ?
> 
> There are a couple of reasons for this:
> 
> 1) I'm working under the assumption that I cannot enqueue the same buffer
> multiple times in v4l2  Because of this, I can only ever queue 1 buffer, have it
> dequeued, processed by the ISP, and requeue it back to Unicam.  This
> effectively halves my overall framerate because of the added latency of running
> the ISP.
> 
> 2) Even if V4L2 does allow enquing the same framebuffer multiple times, there is
> a possible (probable?) HW race condition where the ISP could be processing the
> buffer for frame N, while Unicam is writing out to the buffer for frame N + 1.
> 
> It would be interesting to know if (1) is actually possible.  I don't recall exactly, but
> I think I did try it, and might have got complaints from the v4l2 framework.

(1) should be checked indeed. I had missed in my initial review the fact
that the buffer still needed to be processed by the ISP for algorithms
to converge, so (2) is a valid concern.

In any case, even if we handled frame drop and buffer allocation
differently, this patch wouldn't hurt, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 30 +++++++++----------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 86eb43a3a3c5..742521927780 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1044,21 +1044,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >       RPiCameraData *data = cameraData(camera);
> > >       int ret;
> > >
> > > -     for (auto const stream : data->streams_)
> > > -             stream->resetBuffers();
> > > -
> > > -     if (!data->buffersAllocated_) {
> > > -             /* Allocate buffers for internal pipeline usage. */
> > > -             ret = prepareBuffers(camera);
> > > -             if (ret) {
> > > -                     LOG(RPI, Error) << "Failed to allocate buffers";
> > > -                     data->freeBuffers();
> > > -                     stop(camera);
> > > -                     return ret;
> > > -             }
> > > -             data->buffersAllocated_ = true;
> > > -     }
> > > -
> > >       /* Check if a ScalerCrop control was specified. */
> > >       if (controls)
> > >               data->applyScalerCrop(*controls);
> > > @@ -1075,6 +1060,21 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >       /* Configure the number of dropped frames required on startup. */
> > >       data->dropFrameCount_ = startConfig.dropFrameCount;
> > >
> > > +     for (auto const stream : data->streams_)
> > > +             stream->resetBuffers();
> > > +
> > > +     if (!data->buffersAllocated_) {
> > > +             /* Allocate buffers for internal pipeline usage. */
> > > +             ret = prepareBuffers(camera);
> > > +             if (ret) {
> > > +                     LOG(RPI, Error) << "Failed to allocate buffers";
> > > +                     data->freeBuffers();
> > > +                     stop(camera);
> > > +                     return ret;
> > > +             }
> > > +             data->buffersAllocated_ = true;
> > > +     }
> > > +
> > >       /* We need to set the dropFrameCount_ before queueing buffers. */
> > >       ret = queueAllBuffers(camera);
> > >       if (ret) {

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 86eb43a3a3c5..742521927780 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1044,21 +1044,6 @@  int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
 	RPiCameraData *data = cameraData(camera);
 	int ret;
 
-	for (auto const stream : data->streams_)
-		stream->resetBuffers();
-
-	if (!data->buffersAllocated_) {
-		/* Allocate buffers for internal pipeline usage. */
-		ret = prepareBuffers(camera);
-		if (ret) {
-			LOG(RPI, Error) << "Failed to allocate buffers";
-			data->freeBuffers();
-			stop(camera);
-			return ret;
-		}
-		data->buffersAllocated_ = true;
-	}
-
 	/* Check if a ScalerCrop control was specified. */
 	if (controls)
 		data->applyScalerCrop(*controls);
@@ -1075,6 +1060,21 @@  int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
 	/* Configure the number of dropped frames required on startup. */
 	data->dropFrameCount_ = startConfig.dropFrameCount;
 
+	for (auto const stream : data->streams_)
+		stream->resetBuffers();
+
+	if (!data->buffersAllocated_) {
+		/* Allocate buffers for internal pipeline usage. */
+		ret = prepareBuffers(camera);
+		if (ret) {
+			LOG(RPI, Error) << "Failed to allocate buffers";
+			data->freeBuffers();
+			stop(camera);
+			return ret;
+		}
+		data->buffersAllocated_ = true;
+	}
+
 	/* We need to set the dropFrameCount_ before queueing buffers. */
 	ret = queueAllBuffers(camera);
 	if (ret) {