| Message ID | 20221129134534.2933-4-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:27PM +0000, Naushir Patuck via libcamera-devel wrote: > Add a new config parameter numOutput0Buffers specifying the number of internally > allocated ISP Output0 buffers. This parameter defaults to 1. > > Split out the buffer allocation logic so that the buffer count for ISP Output0 > can be different from the ISP Output1 and Statistics streams. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 4486d31ea78d..f2b695af2399 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -305,6 +305,11 @@ public: > * the Unicam Image steram. > */ > unsigned int minTotalUnicamBuffers; > + /* > + * The number of internal buffers allocated for the ISP Output0 > + * stream. I'd comment here that the value can only be 0 or 1. > + */ > + unsigned int numOutput0Buffers; > }; > > Config config_; > @@ -1418,6 +1423,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > config = { > .minUnicamBuffers = 2, > .minTotalUnicamBuffers = 4, > + .numOutput0Buffers = 1, > }; > > return 0; > @@ -1502,12 +1508,19 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > * so allocate the minimum required to avoid frame drops. > */ > numBuffers = minBuffers; > - } else { > + } else if (stream == &data->isp_[Isp::Output0]) { > /* > * Since the ISP runs synchronous with the IPA and requests, > - * we only ever need one set of internal buffers. Any buffers > - * the application wants to hold onto will already be exported > - * through PipelineHandlerRPi::exportFrameBuffers(). > + * we only ever need a maximum of one internal buffer. Any > + * buffers the application wants to hold onto will already > + * be exported through PipelineHandlerRPi::exportFrameBuffers(). > + */ > + numBuffers = std::min(1u, data->config_.numOutput0Buffers); Is it OK to call Stream::prepareBuffers() below if numBuffers is 0 ? > + } else { > + /* > + * Same reasoning as above, we only ever need a maximum > + * of one internal buffer for Output1 (required for colour > + * denoise) and ISP statistics. > */ > numBuffers = 1; > }
Hi Laurent, Thank you for your feedback! On Fri, 2 Dec 2022 at 12:19, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Tue, Nov 29, 2022 at 01:45:27PM +0000, Naushir Patuck via > libcamera-devel wrote: > > Add a new config parameter numOutput0Buffers specifying the number of > internally > > allocated ISP Output0 buffers. This parameter defaults to 1. > > > > Split out the buffer allocation logic so that the buffer count for ISP > Output0 > > can be different from the ISP Output1 and Statistics streams. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 4486d31ea78d..f2b695af2399 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -305,6 +305,11 @@ public: > > * the Unicam Image steram. > > */ > > unsigned int minTotalUnicamBuffers; > > + /* > > + * The number of internal buffers allocated for the ISP > Output0 > > + * stream. > > I'd comment here that the value can only be 0 or 1. > Ack. > > > + */ > > + unsigned int numOutput0Buffers; > > }; > > > > Config config_; > > @@ -1418,6 +1423,7 @@ int > PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) > > config = { > > .minUnicamBuffers = 2, > > .minTotalUnicamBuffers = 4, > > + .numOutput0Buffers = 1, > > }; > > > > return 0; > > @@ -1502,12 +1508,19 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > > * so allocate the minimum required to avoid frame > drops. > > */ > > numBuffers = minBuffers; > > - } else { > > + } else if (stream == &data->isp_[Isp::Output0]) { > > /* > > * Since the ISP runs synchronous with the IPA and > requests, > > - * we only ever need one set of internal buffers. > Any buffers > > - * the application wants to hold onto will already > be exported > > - * through > PipelineHandlerRPi::exportFrameBuffers(). > > + * we only ever need a maximum of one internal > buffer. Any > > + * buffers the application wants to hold onto will > already > > + * be exported through > PipelineHandlerRPi::exportFrameBuffers(). > > + */ > > + numBuffers = std::min(1u, > data->config_.numOutput0Buffers); > > Is it OK to call Stream::prepareBuffers() below if numBuffers is 0 ? > Yes it is. The count param tells the stream how many (if any) buffers to allocate for internal use. Stream::prepareBuffers will call V4L2VideoDevice::importBuffers for externally/internally allocated buffers. Regards, Naush > > > + } else { > > + /* > > + * Same reasoning as above, we only ever need a > maximum > > + * of one internal buffer for Output1 (required > for colour > > + * denoise) and ISP statistics. > > */ > > numBuffers = 1; > > } > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 4486d31ea78d..f2b695af2399 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -305,6 +305,11 @@ public: * the Unicam Image steram. */ unsigned int minTotalUnicamBuffers; + /* + * The number of internal buffers allocated for the ISP Output0 + * stream. + */ + unsigned int numOutput0Buffers; }; Config config_; @@ -1418,6 +1423,7 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data) config = { .minUnicamBuffers = 2, .minTotalUnicamBuffers = 4, + .numOutput0Buffers = 1, }; return 0; @@ -1502,12 +1508,19 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) * so allocate the minimum required to avoid frame drops. */ numBuffers = minBuffers; - } else { + } else if (stream == &data->isp_[Isp::Output0]) { /* * Since the ISP runs synchronous with the IPA and requests, - * we only ever need one set of internal buffers. Any buffers - * the application wants to hold onto will already be exported - * through PipelineHandlerRPi::exportFrameBuffers(). + * we only ever need a maximum of one internal buffer. Any + * buffers the application wants to hold onto will already + * be exported through PipelineHandlerRPi::exportFrameBuffers(). + */ + numBuffers = std::min(1u, data->config_.numOutput0Buffers); + } else { + /* + * Same reasoning as above, we only ever need a maximum + * of one internal buffer for Output1 (required for colour + * denoise) and ISP statistics. */ numBuffers = 1; }