| Message ID | 20221129134534.2933-6-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:29PM +0000, Naushir Patuck via libcamera-devel wrote: > If the pipeline handler is required to drop startup frames by the IPA, do not > call StreamOn on the ISP Output0 and Output1 device nodes from > PipelineHandlerRPi::start. This stops the ISP from generating output on those > channels giving improving latency, and more crucially does not require internal > buffers to be allocated to deal with this condition. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/data/default.json | 2 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++--- > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json > index d709e31850af..a7ea735c87f4 100644 > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > @@ -14,7 +14,7 @@ > # min_total_unicam_buffers - external buffer count) > "min_total_unicam_buffers": 4, > > - # The number of internal buffers used for ISP Output0. This must be set to 1. > + # The number of internal buffers used for ISP Output0. Can you explain this change in the commit message ? > "num_output0_buffers": 1 > } > } > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index ce411453db0a..86eb43a3a3c5 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > data->state_ = RPiCameraData::State::Idle; > > - /* Start all streams. */ > + /* > + * Start all streams with the exception of ISP Output0 and Output1 if > + * we are dropping some start-up frames. This saves a tiny bit of latency > + * and avoids the need for allocating an Output0 buffer only to handle > + * startup drop frame conditions. > + */ I'm not sure to follow you here. Wouldn't it be sufficient to drop the buffers at unicam's output without pushing them to the ISP ? Why does delaying streamOn() help ? > for (auto const stream : data->streams_) { > + if (data->dropFrameCount_ && > + (stream == &data->isp_[Isp::Output0] || > + stream == &data->isp_[Isp::Output1])) > + continue; > + > ret = stream->dev()->streamOn(); > if (ret) { > stop(camera); > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > if (ret < 0) > return ret; > } else { > + /* > + * We don't enable streaming for Output0 and Output1 for > + * startup frame drops, so don't queue any buffers. > + */ > + if (stream == &data->isp_[Isp::Output0] || > + stream == &data->isp_[Isp::Output1]) > + continue; > /* > * For external streams, we must queue up a set of internal > * buffers to handle the number of drop frames requested by > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() > } > > /* > - * Make sure we have three outputs completed in the case of a dropped > - * frame. > + * Only the ISP statistics output is generated when we are dropping > + * frames on startup. > */ > if (state_ == State::IpaComplete && > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) { > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) { > state_ = State::Idle; > if (dropFrameCount_) { > dropFrameCount_--; > LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" > << dropFrameCount_ << " left)"; > + /* > + * If we have finished dropping startup frames, start > + * streaming on the ISP Output0 and Output1 nodes for > + * normal operation. > + */ > + if (!dropFrameCount_) { > + isp_[Isp::Output0].dev()->streamOn(); > + isp_[Isp::Output1].dev()->streamOn(); > + } > } > } > }
Hi Laurent, Thank you for your feedback. On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via > libcamera-devel wrote: > > If the pipeline handler is required to drop startup frames by the IPA, > do not > > call StreamOn on the ISP Output0 and Output1 device nodes from > > PipelineHandlerRPi::start. This stops the ISP from generating output on > those > > channels giving improving latency, and more crucially does not require > internal > > buffers to be allocated to deal with this condition. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/data/default.json | 2 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++--- > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json > b/src/libcamera/pipeline/raspberrypi/data/default.json > > index d709e31850af..a7ea735c87f4 100644 > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > > @@ -14,7 +14,7 @@ > > # min_total_unicam_buffers > - external buffer count) > > "min_total_unicam_buffers": 4, > > > > - # The number of internal buffers used for ISP Output0. > This must be set to 1. > > + # The number of internal buffers used for ISP Output0. > > Can you explain this change in the commit message ? > > > "num_output0_buffers": 1 > > } > > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index ce411453db0a..86eb43a3a3c5 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > > > data->state_ = RPiCameraData::State::Idle; > > > > - /* Start all streams. */ > > + /* > > + * Start all streams with the exception of ISP Output0 and Output1 > if > > + * we are dropping some start-up frames. This saves a tiny bit of > latency > > + * and avoids the need for allocating an Output0 buffer only to > handle > > + * startup drop frame conditions. > > + */ > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the > buffers at unicam's output without pushing them to the ISP ? Why does > delaying streamOn() help ? > You caught me :-) I would have done exactly this, only our FW interface does not work that way. If the kernel device node is opened, we must provide it with an Output0 buffer to function correctly. This is purely down to the FW and not the kernel driver. Given the fragile nature of the FW, I don't want to make any large scale changes to fix this in FW land. Naush > > > for (auto const stream : data->streams_) { > > + if (data->dropFrameCount_ && > > + (stream == &data->isp_[Isp::Output0] || > > + stream == &data->isp_[Isp::Output1])) > > + continue; > > + > > ret = stream->dev()->streamOn(); > > if (ret) { > > stop(camera); > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera > *camera) > > if (ret < 0) > > return ret; > > } else { > > + /* > > + * We don't enable streaming for Output0 and > Output1 for > > + * startup frame drops, so don't queue any buffers. > > + */ > > + if (stream == &data->isp_[Isp::Output0] || > > + stream == &data->isp_[Isp::Output1]) > > + continue; > > /* > > * For external streams, we must queue up a set of > internal > > * buffers to handle the number of drop frames > requested by > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() > > } > > > > /* > > - * Make sure we have three outputs completed in the case of a > dropped > > - * frame. > > + * Only the ISP statistics output is generated when we are dropping > > + * frames on startup. > > */ > > if (state_ == State::IpaComplete && > > - ((ispOutputCount_ == 3 && dropFrameCount_) || > requestCompleted)) { > > + ((ispOutputCount_ == 1 && dropFrameCount_) || > requestCompleted)) { > > state_ = State::Idle; > > if (dropFrameCount_) { > > dropFrameCount_--; > > LOG(RPI, Debug) << "Dropping frame at the request > of the IPA (" > > << dropFrameCount_ << " left)"; > > + /* > > + * If we have finished dropping startup frames, > start > > + * streaming on the ISP Output0 and Output1 nodes > for > > + * normal operation. > > + */ > > + if (!dropFrameCount_) { > > + isp_[Isp::Output0].dev()->streamOn(); > > + isp_[Isp::Output1].dev()->streamOn(); > > + } > > } > > } > > } > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote: > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote: > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote: > > > If the pipeline handler is required to drop startup frames by the IPA, do not > > > call StreamOn on the ISP Output0 and Output1 device nodes from > > > PipelineHandlerRPi::start. This stops the ISP from generating output on those > > > channels giving improving latency, and more crucially does not require internal > > > buffers to be allocated to deal with this condition. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/data/default.json | 2 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++--- > > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json > > > index d709e31850af..a7ea735c87f4 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > > > @@ -14,7 +14,7 @@ > > > # min_total_unicam_buffers - external buffer count) > > > "min_total_unicam_buffers": 4, > > > > > > - # The number of internal buffers used for ISP Output0. This must be set to 1. > > > + # The number of internal buffers used for ISP Output0. > > > > Can you explain this change in the commit message ? > > > > > "num_output0_buffers": 1 > > > } > > > } > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index ce411453db0a..86eb43a3a3c5 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > - /* Start all streams. */ > > > + /* > > > + * Start all streams with the exception of ISP Output0 and Output1 if > > > + * we are dropping some start-up frames. This saves a tiny bit of latency What latency does this save, and won't we repay that later anyway ? > > > + * and avoids the need for allocating an Output0 buffer only to handle > > > + * startup drop frame conditions. > > > + */ > > > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the > > buffers at unicam's output without pushing them to the ISP ? Why does > > delaying streamOn() help ? > > You caught me :-) > > I would have done exactly this, only our FW interface does not work that way. > If the kernel device node is opened, we must provide it with an Output0 buffer > to function correctly. This patch addresses VIDIOC_STREAMON, do you then mean if the video device node is streaming, not if it is open ? > This is purely down to the FW and not the kernel driver. > Given the fragile nature of the FW, I don't want to make any large scale > changes to fix this in FW land. Sometimes I think it could make things easier if the firmware was open-source ;-) More seriously, it seems this could be addressed in kernelspace, by delaying the vchiq_mmal_port_enable() call until the first buffer is queued, which may be as easy as setting vb2_queue.min_buffers_needed to 1. A quick look at videobuf2 doesn't show any immediate ill side effects, but I may have overlooked something. I also understand that changing the kernel would be more complex (as it would need to be synced with merging this series), and thinking more about it, it may be a bit of a hack as it would only work as expect for the startup frame drop. > > > for (auto const stream : data->streams_) { > > > + if (data->dropFrameCount_ && > > > + (stream == &data->isp_[Isp::Output0] || > > > + stream == &data->isp_[Isp::Output1])) > > > + continue; > > > + > > > ret = stream->dev()->streamOn(); > > > if (ret) { > > > stop(camera); > > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > > if (ret < 0) > > > return ret; > > > } else { > > > + /* > > > + * We don't enable streaming for Output0 and Output1 for Maybe "We haven't enabled streaming yet" to make this clearer ? > > > + * startup frame drops, so don't queue any buffers. > > > + */ > > > + if (stream == &data->isp_[Isp::Output0] || > > > + stream == &data->isp_[Isp::Output1]) > > > + continue; I'd add a blank line here. > > > /* > > > * For external streams, we must queue up a set of internal > > > * buffers to handle the number of drop frames requested by > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() > > > } > > > > > > /* > > > - * Make sure we have three outputs completed in the case of a dropped > > > - * frame. > > > + * Only the ISP statistics output is generated when we are dropping > > > + * frames on startup. > > > */ > > > if (state_ == State::IpaComplete && > > > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) { > > > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) { > > > state_ = State::Idle; > > > if (dropFrameCount_) { > > > dropFrameCount_--; > > > LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" > > > << dropFrameCount_ << " left)"; Here too. > > > + /* > > > + * If we have finished dropping startup frames, start > > > + * streaming on the ISP Output0 and Output1 nodes for > > > + * normal operation. > > > + */ > > > + if (!dropFrameCount_) { > > > + isp_[Isp::Output0].dev()->streamOn(); > > > + isp_[Isp::Output1].dev()->streamOn(); > > > + } This looks good. I think there's a potential issue though. Given that the output queues are not started in start(), but buffers are still queued in queueRequestDevice(), what will happen if the application calls stop() before the video nodes are started ? Won't the buffers stay queued ? > > > } > > > } > > > }
Hi Laurent, On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote: > > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote: > > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via > libcamera-devel wrote: > > > > If the pipeline handler is required to drop startup frames by the > IPA, do not > > > > call StreamOn on the ISP Output0 and Output1 device nodes from > > > > PipelineHandlerRPi::start. This stops the ISP from generating output > on those > > > > channels giving improving latency, and more crucially does not > require internal > > > > buffers to be allocated to deal with this condition. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > .../pipeline/raspberrypi/data/default.json | 2 +- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 > ++++++++++++++++--- > > > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json > b/src/libcamera/pipeline/raspberrypi/data/default.json > > > > index d709e31850af..a7ea735c87f4 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > > > > @@ -14,7 +14,7 @@ > > > > # > min_total_unicam_buffers - external buffer count) > > > > "min_total_unicam_buffers": 4, > > > > > > > > - # The number of internal buffers used for ISP > Output0. This must be set to 1. > > > > + # The number of internal buffers used for ISP > Output0. > > > > > > Can you explain this change in the commit message ? > > > > > > > "num_output0_buffers": 1 > > > > } > > > > } > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index ce411453db0a..86eb43a3a3c5 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > > > - /* Start all streams. */ > > > > + /* > > > > + * Start all streams with the exception of ISP Output0 and > Output1 if > > > > + * we are dropping some start-up frames. This saves a tiny bit > of latency > > What latency does this save, and won't we repay that later anyway ? > I don't have an exact time value, but since the stats are generated at the front of the pipeline, no other processing blocks are run if there is no output. Multiply that by the number (5-8) of convergence frames, and we get a measurable (though still very small) improvement. > > > > > + * and avoids the need for allocating an Output0 buffer only > to handle > > > > + * startup drop frame conditions. > > > > + */ > > > > > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the > > > buffers at unicam's output without pushing them to the ISP ? Why does > > > delaying streamOn() help ? > > > > You caught me :-) > > > > I would have done exactly this, only our FW interface does not work that > way. > > If the kernel device node is opened, we must provide it with an Output0 > buffer > > to function correctly. > > This patch addresses VIDIOC_STREAMON, do you then mean if the video > device node is streaming, not if it is open ? > Sorry, I did mean VIDIOC_STREAMON! > > > This is purely down to the FW and not the kernel driver. > > Given the fragile nature of the FW, I don't want to make any large scale > > changes to fix this in FW land. > > Sometimes I think it could make things easier if the firmware was > open-source ;-) > > More seriously, it seems this could be addressed in kernelspace, by > delaying the vchiq_mmal_port_enable() call until the first buffer is > queued, which may be as easy as setting vb2_queue.min_buffers_needed to > 1. A quick look at videobuf2 doesn't show any immediate ill side > effects, but I may have overlooked something. I also understand that > changing the kernel would be more complex (as it would need to be synced > with merging this series), and thinking more about it, it may be a bit > of a hack as it would only work as expect for the startup frame drop. > Indeed, I opted for the userland fix as it does simplify things quite a bit. > > > > > for (auto const stream : data->streams_) { > > > > + if (data->dropFrameCount_ && > > > > + (stream == &data->isp_[Isp::Output0] || > > > > + stream == &data->isp_[Isp::Output1])) > > > > + continue; > > > > + > > > > ret = stream->dev()->streamOn(); > > > > if (ret) { > > > > stop(camera); > > > > @@ -1500,6 +1510,13 @@ int > PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > > > if (ret < 0) > > > > return ret; > > > > } else { > > > > + /* > > > > + * We don't enable streaming for Output0 and > Output1 for > > Maybe "We haven't enabled streaming yet" to make this clearer ? > Ack. > > > > > + * startup frame drops, so don't queue any > buffers. > > > > + */ > > > > + if (stream == &data->isp_[Isp::Output0] || > > > > + stream == &data->isp_[Isp::Output1]) > > > > + continue; > > I'd add a blank line here. > > > > > /* > > > > * For external streams, we must queue up a > set of internal > > > > * buffers to handle the number of drop frames > requested by > > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() > > > > } > > > > > > > > /* > > > > - * Make sure we have three outputs completed in the case of a > dropped > > > > - * frame. > > > > + * Only the ISP statistics output is generated when we are > dropping > > > > + * frames on startup. > > > > */ > > > > if (state_ == State::IpaComplete && > > > > - ((ispOutputCount_ == 3 && dropFrameCount_) || > requestCompleted)) { > > > > + ((ispOutputCount_ == 1 && dropFrameCount_) || > requestCompleted)) { > > > > state_ = State::Idle; > > > > if (dropFrameCount_) { > > > > dropFrameCount_--; > > > > LOG(RPI, Debug) << "Dropping frame at the > request of the IPA (" > > > > << dropFrameCount_ << " left)"; > > Here too. > > > > > + /* > > > > + * If we have finished dropping startup > frames, start > > > > + * streaming on the ISP Output0 and Output1 > nodes for > > > > + * normal operation. > > > > + */ > > > > + if (!dropFrameCount_) { > > > > + isp_[Isp::Output0].dev()->streamOn(); > > > > + isp_[Isp::Output1].dev()->streamOn(); > > > > + } > > This looks good. > > I think there's a potential issue though. Given that the output queues > are not started in start(), but buffers are still queued in > queueRequestDevice(), what will happen if the application calls stop() > before the video nodes are started ? Won't the buffers stay queued ? > PipelineHandlerRPi::stopDevice() unconditionally calls V4L2VideoDevice::streamOff() for the ISP nodes even if they were not started. I was assuming this would unqueue the buffers and clean up correctly. Do you think this might not be correct? Regards, Naush > > > > > } > > > > } > > > > } > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Mon, Dec 05, 2022 at 11:11:27AM +0000, Naushir Patuck wrote: > On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart wrote: > > On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote: > > > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote: > > > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote: > > > > > If the pipeline handler is required to drop startup frames by the IPA, do not > > > > > call StreamOn on the ISP Output0 and Output1 device nodes from > > > > > PipelineHandlerRPi::start. This stops the ISP from generating output on those > > > > > channels giving improving latency, and more crucially does not require internal > > > > > buffers to be allocated to deal with this condition. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > .../pipeline/raspberrypi/data/default.json | 2 +- > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++--- > > > > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json > > > > > index d709e31850af..a7ea735c87f4 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json > > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json > > > > > @@ -14,7 +14,7 @@ > > > > > # min_total_unicam_buffers - external buffer count) > > > > > "min_total_unicam_buffers": 4, > > > > > > > > > > - # The number of internal buffers used for ISP Output0. This must be set to 1. > > > > > + # The number of internal buffers used for ISP Output0. > > > > > > > > Can you explain this change in the commit message ? > > > > > > > > > "num_output0_buffers": 1 > > > > > } > > > > > } > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index ce411453db0a..86eb43a3a3c5 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > > > > > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > > > > > - /* Start all streams. */ > > > > > + /* > > > > > + * Start all streams with the exception of ISP Output0 and Output1 if > > > > > + * we are dropping some start-up frames. This saves a tiny bit of latency > > > > What latency does this save, and won't we repay that later anyway ? > > I don't have an exact time value, but since the stats are generated at the front of > the pipeline, no other processing blocks are run if there is no output. Multiply that > by the number (5-8) of convergence frames, and we get a measurable (though > still very small) improvement. Ah, I was considering the start() latency, but you're talking about the processing latency. Makes sense. Could you mention that here ? Something along the lines of "a tiny bit of ISP processing latency". > > > > > + * and avoids the need for allocating an Output0 buffer only to handle > > > > > + * startup drop frame conditions. > > > > > + */ > > > > > > > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the > > > > buffers at unicam's output without pushing them to the ISP ? Why does > > > > delaying streamOn() help ? > > > > > > You caught me :-) > > > > > > I would have done exactly this, only our FW interface does not work that way. > > > If the kernel device node is opened, we must provide it with an Output0 buffer > > > to function correctly. > > > > This patch addresses VIDIOC_STREAMON, do you then mean if the video > > device node is streaming, not if it is open ? > > Sorry, I did mean VIDIOC_STREAMON! > > > > This is purely down to the FW and not the kernel driver. > > > Given the fragile nature of the FW, I don't want to make any large scale > > > changes to fix this in FW land. > > > > Sometimes I think it could make things easier if the firmware was > > open-source ;-) > > > > More seriously, it seems this could be addressed in kernelspace, by > > delaying the vchiq_mmal_port_enable() call until the first buffer is > > queued, which may be as easy as setting vb2_queue.min_buffers_needed to > > 1. A quick look at videobuf2 doesn't show any immediate ill side > > effects, but I may have overlooked something. I also understand that > > changing the kernel would be more complex (as it would need to be synced > > with merging this series), and thinking more about it, it may be a bit > > of a hack as it would only work as expect for the startup frame drop. > > Indeed, I opted for the userland fix as it does simplify things quite a > bit. > > > > > > for (auto const stream : data->streams_) { > > > > > + if (data->dropFrameCount_ && > > > > > + (stream == &data->isp_[Isp::Output0] || > > > > > + stream == &data->isp_[Isp::Output1])) > > > > > + continue; > > > > > + > > > > > ret = stream->dev()->streamOn(); > > > > > if (ret) { > > > > > stop(camera); > > > > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) > > > > > if (ret < 0) > > > > > return ret; > > > > > } else { > > > > > + /* > > > > > + * We don't enable streaming for Output0 and Output1 for > > > > Maybe "We haven't enabled streaming yet" to make this clearer ? > > Ack. > > > > > > + * startup frame drops, so don't queue any buffers. > > > > > + */ > > > > > + if (stream == &data->isp_[Isp::Output0] || > > > > > + stream == &data->isp_[Isp::Output1]) > > > > > + continue; > > > > I'd add a blank line here. > > > > > > > /* > > > > > * For external streams, we must queue up a set of internal > > > > > * buffers to handle the number of drop frames requested by > > > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() > > > > > } > > > > > > > > > > /* > > > > > - * Make sure we have three outputs completed in the case of a dropped > > > > > - * frame. > > > > > + * Only the ISP statistics output is generated when we are dropping > > > > > + * frames on startup. > > > > > */ > > > > > if (state_ == State::IpaComplete && > > > > > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) { > > > > > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) { > > > > > state_ = State::Idle; > > > > > if (dropFrameCount_) { > > > > > dropFrameCount_--; > > > > > LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" > > > > > << dropFrameCount_ << " left)"; > > > > Here too. > > > > > > > + /* > > > > > + * If we have finished dropping startup frames, start > > > > > + * streaming on the ISP Output0 and Output1 nodes for > > > > > + * normal operation. > > > > > + */ > > > > > + if (!dropFrameCount_) { > > > > > + isp_[Isp::Output0].dev()->streamOn(); > > > > > + isp_[Isp::Output1].dev()->streamOn(); > > > > > + } > > > > This looks good. > > > > I think there's a potential issue though. Given that the output queues > > are not started in start(), but buffers are still queued in > > queueRequestDevice(), what will happen if the application calls stop() > > before the video nodes are started ? Won't the buffers stay queued ? > > PipelineHandlerRPi::stopDevice() unconditionally calls V4L2VideoDevice::streamOff() > for the ISP nodes even if they were not started. I was assuming this would unqueue > the buffers and clean up correctly. Do you think this might not be correct? I've had another look at the kernel code and the V4L2VideoDevice::streamOff() implementation, and I think it's fine. It would still be nice if you could test it though. > > > > > } > > > > > } > > > > > }
diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json index d709e31850af..a7ea735c87f4 100644 --- a/src/libcamera/pipeline/raspberrypi/data/default.json +++ b/src/libcamera/pipeline/raspberrypi/data/default.json @@ -14,7 +14,7 @@ # min_total_unicam_buffers - external buffer count) "min_total_unicam_buffers": 4, - # The number of internal buffers used for ISP Output0. This must be set to 1. + # The number of internal buffers used for ISP Output0. "num_output0_buffers": 1 } } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index ce411453db0a..86eb43a3a3c5 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) data->state_ = RPiCameraData::State::Idle; - /* Start all streams. */ + /* + * Start all streams with the exception of ISP Output0 and Output1 if + * we are dropping some start-up frames. This saves a tiny bit of latency + * and avoids the need for allocating an Output0 buffer only to handle + * startup drop frame conditions. + */ for (auto const stream : data->streams_) { + if (data->dropFrameCount_ && + (stream == &data->isp_[Isp::Output0] || + stream == &data->isp_[Isp::Output1])) + continue; + ret = stream->dev()->streamOn(); if (ret) { stop(camera); @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) if (ret < 0) return ret; } else { + /* + * We don't enable streaming for Output0 and Output1 for + * startup frame drops, so don't queue any buffers. + */ + if (stream == &data->isp_[Isp::Output0] || + stream == &data->isp_[Isp::Output1]) + continue; /* * For external streams, we must queue up a set of internal * buffers to handle the number of drop frames requested by @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted() } /* - * Make sure we have three outputs completed in the case of a dropped - * frame. + * Only the ISP statistics output is generated when we are dropping + * frames on startup. */ if (state_ == State::IpaComplete && - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) { + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) { state_ = State::Idle; if (dropFrameCount_) { dropFrameCount_--; LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" << dropFrameCount_ << " left)"; + /* + * If we have finished dropping startup frames, start + * streaming on the ISP Output0 and Output1 nodes for + * normal operation. + */ + if (!dropFrameCount_) { + isp_[Isp::Output0].dev()->streamOn(); + isp_[Isp::Output1].dev()->streamOn(); + } } } }