Message ID | 20221129134534.2933-7-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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) {
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 >
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) {
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) {