| Message ID | 20220307124633.115452-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch! On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > The original use of RPi::Stream::reset() was to clear the external flag state > and free/clear out the framebuffers for the stream. However, the latter is now > done through PipelineHandlerRPi::configure(). Rework > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of > RPi::Stream::reset() to achieve the same thing. > > Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling > logic, where all internally allocated buffers are put back into the queue of > available buffers. This reset() is now called from PipelineHandlerRPi::start(), > allowing the pipeline handler to correctly deal with start()/stop()/start() > sequences and reusing the buffers allocated on the first start(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 ++++- > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 11 +++++------ > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index b17ffa235ac7..193361686d3c 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ > data->freeBuffers(); > for (auto const stream : data->streams_) > - stream->reset(); > + stream->setExternal(false); So I think that replacing the former calls to reset() by setExternal() is a good thing, because "reset" didn't give you much of a clue about what was being reset! I guess my only feeling about repurposing the name "reset" is that it's still rather vague. Could we think of a name that gives someone a clue as to what is being reset? Maybe "resetAvailableBuffers", or just "resetBuffers"? > > BayerFormat::Packing packing = BayerFormat::Packing::CSI2; > Size maxSize, sensorSize; > @@ -985,6 +985,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > RPiCameraData *data = cameraData(camera); > int ret; > > + for (auto const stream : data->streams_) > + stream->reset(); > + > if (data->reconfigured_) { > /* Allocate buffers for internal pipeline usage. */ > ret = prepareBuffers(camera); > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index f446e1ce66c7..0840ec4f54a0 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -28,8 +28,10 @@ std::string Stream::name() const > > void Stream::reset() > { > - external_ = false; > - clearBuffers(); > + /* Add all internal buffers to the queue of usable buffers. */ > + availableBuffers_ = {}; > + for (auto const &buffer : internalBuffers_) > + availableBuffers_.push(buffer.get()); > } > > void Stream::setExternal(bool external) > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count) > > /* Add these exported buffers to the internal/external buffer list. */ > setExportedBuffers(&internalBuffers_); > - > - /* Add these buffers to the queue of internal usable buffers. */ > - for (auto const &buffer : internalBuffers_) > - availableBuffers_.push(buffer.get()); > + reset(); > } > > /* We must import all internal/external exported buffers. */ > -- > 2.25.1 > But either way, with or without any further changes: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David
Hi David, Thank you for your review on this and the other patches in this series! On Thu, 10 Mar 2022 at 10:57, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch! > > On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > The original use of RPi::Stream::reset() was to clear the external flag > state > > and free/clear out the framebuffers for the stream. However, the latter > is now > > done through PipelineHandlerRPi::configure(). Rework > > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() > instead of > > RPi::Stream::reset() to achieve the same thing. > > > > Repurpose RPi::Stream::reset() to instead reset the state of the buffer > handling > > logic, where all internally allocated buffers are put back into the > queue of > > available buffers. This reset() is now called from > PipelineHandlerRPi::start(), > > allowing the pipeline handler to correctly deal with > start()/stop()/start() > > sequences and reusing the buffers allocated on the first start(). > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 ++++- > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 11 +++++------ > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index b17ffa235ac7..193361686d3c 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > /* Start by freeing all buffers and resetting the Unicam and ISP > stream states. */ > > data->freeBuffers(); > > for (auto const stream : data->streams_) > > - stream->reset(); > > + stream->setExternal(false); > > So I think that replacing the former calls to reset() by setExternal() > is a good thing, because "reset" didn't give you much of a clue about > what was being reset! > > I guess my only feeling about repurposing the name "reset" is that > it's still rather vague. Could we think of a name that gives someone a > clue as to what is being reset? Maybe "resetAvailableBuffers", or just > "resetBuffers"? > Sure I'll rename this to resetBuffers in v2 to be clearer of the new function. Regards, Naush > > > > > BayerFormat::Packing packing = BayerFormat::Packing::CSI2; > > Size maxSize, sensorSize; > > @@ -985,6 +985,9 @@ int PipelineHandlerRPi::start(Camera *camera, const > ControlList *controls) > > RPiCameraData *data = cameraData(camera); > > int ret; > > > > + for (auto const stream : data->streams_) > > + stream->reset(); > > + > > if (data->reconfigured_) { > > /* Allocate buffers for internal pipeline usage. */ > > ret = prepareBuffers(camera); > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > index f446e1ce66c7..0840ec4f54a0 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > @@ -28,8 +28,10 @@ std::string Stream::name() const > > > > void Stream::reset() > > { > > - external_ = false; > > - clearBuffers(); > > + /* Add all internal buffers to the queue of usable buffers. */ > > + availableBuffers_ = {}; > > + for (auto const &buffer : internalBuffers_) > > + availableBuffers_.push(buffer.get()); > > } > > > > void Stream::setExternal(bool external) > > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count) > > > > /* Add these exported buffers to the > internal/external buffer list. */ > > setExportedBuffers(&internalBuffers_); > > - > > - /* Add these buffers to the queue of internal > usable buffers. */ > > - for (auto const &buffer : internalBuffers_) > > - availableBuffers_.push(buffer.get()); > > + reset(); > > } > > > > /* We must import all internal/external exported > buffers. */ > > -- > > 2.25.1 > > > > But either way, with or without any further changes: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index b17ffa235ac7..193361686d3c 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -692,7 +692,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) /* Start by freeing all buffers and resetting the Unicam and ISP stream states. */ data->freeBuffers(); for (auto const stream : data->streams_) - stream->reset(); + stream->setExternal(false); BayerFormat::Packing packing = BayerFormat::Packing::CSI2; Size maxSize, sensorSize; @@ -985,6 +985,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) RPiCameraData *data = cameraData(camera); int ret; + for (auto const stream : data->streams_) + stream->reset(); + if (data->reconfigured_) { /* Allocate buffers for internal pipeline usage. */ ret = prepareBuffers(camera); diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index f446e1ce66c7..0840ec4f54a0 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -28,8 +28,10 @@ std::string Stream::name() const void Stream::reset() { - external_ = false; - clearBuffers(); + /* Add all internal buffers to the queue of usable buffers. */ + availableBuffers_ = {}; + for (auto const &buffer : internalBuffers_) + availableBuffers_.push(buffer.get()); } void Stream::setExternal(bool external) @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count) /* Add these exported buffers to the internal/external buffer list. */ setExportedBuffers(&internalBuffers_); - - /* Add these buffers to the queue of internal usable buffers. */ - for (auto const &buffer : internalBuffers_) - availableBuffers_.push(buffer.get()); + reset(); } /* We must import all internal/external exported buffers. */
The original use of RPi::Stream::reset() was to clear the external flag state and free/clear out the framebuffers for the stream. However, the latter is now done through PipelineHandlerRPi::configure(). Rework PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of RPi::Stream::reset() to achieve the same thing. Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling logic, where all internally allocated buffers are put back into the queue of available buffers. This reset() is now called from PipelineHandlerRPi::start(), allowing the pipeline handler to correctly deal with start()/stop()/start() sequences and reusing the buffers allocated on the first start(). Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 ++++- src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 11 +++++------ 2 files changed, 9 insertions(+), 7 deletions(-)