[libcamera-devel] [PATCH v3 19/22] libcamera: pipeline: rkisp1: Move start and stop of path to RkISP1Path
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Sep 29 00:57:53 CEST 2020
Hi Laurent,
Thanks for your feedback.
On 2020-09-29 01:46:47 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Sep 25, 2020 at 03:42:04AM +0200, Niklas Söderlund wrote:
> > Move the start and stop of a path to RkISP1Path. This allows the
> > importing of buffers to be moved closer the path start/stop simplifying
> > the code. Also by adding a simple running tracker the error logic in
> > PipelineHandlerRkISP1 can be simplified as stop() can always be called.
> >
> > This also removes all external users of RkISP1Path::video_ so it can be
> > made private.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 53 ++------------------
> > src/libcamera/pipeline/rkisp1/rkisp1path.cpp | 42 +++++++++++++++-
> > src/libcamera/pipeline/rkisp1/rkisp1path.h | 8 +--
> > 3 files changed, 50 insertions(+), 53 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 7dd4fb11c39dd811..6158fbee41339c32 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -770,20 +770,6 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > data->selfPathStream_.configuration().bufferCount,
> > });
> >
> > - if (data->mainPathActive_) {
> > - ret = mainPath_.video_->importBuffers(
> > - data->mainPathStream_.configuration().bufferCount);
> > - if (ret < 0)
> > - goto error;
> > - }
> > -
> > - if (data->selfPathActive_) {
> > - ret = selfPath_.video_->importBuffers(
> > - data->selfPathStream_.configuration().bufferCount);
> > - if (ret < 0)
> > - goto error;
> > - }
> > -
> > ret = param_->allocateBuffers(maxCount, ¶mBuffers_);
> > if (ret < 0)
> > goto error;
> > @@ -813,8 +799,6 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > error:
> > paramBuffers_.clear();
> > statBuffers_.clear();
> > - mainPath_.video_->releaseBuffers();
> > - selfPath_.video_->releaseBuffers();
> >
> > return ret;
> > }
> > @@ -845,12 +829,6 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> > if (stat_->releaseBuffers())
> > LOG(RkISP1, Error) << "Failed to release stat buffers";
> >
> > - if (mainPath_.video_->releaseBuffers())
> > - LOG(RkISP1, Error) << "Failed to release main path buffers";
> > -
> > - if (selfPath_.video_->releaseBuffers())
> > - LOG(RkISP1, Error) << "Failed to release self path buffers";
> > -
> > return 0;
> > }
> >
> > @@ -896,15 +874,12 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > std::map<unsigned int, IPAStream> streamConfig;
> >
> > if (data->mainPathActive_) {
> > - ret = mainPath_.video_->streamOn();
> > + ret = mainPath_.start();
> > if (ret) {
> > param_->streamOff();
> > stat_->streamOff();
> > data->ipa_->stop();
> > freeBuffers(camera);
> > -
> > - LOG(RkISP1, Error)
> > - << "Failed to start main path " << camera->id();
> > return ret;
> > }
> >
> > @@ -915,18 +890,13 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > }
> >
> > if (data->selfPathActive_) {
> > - ret = selfPath_.video_->streamOn();
> > + ret = selfPath_.start();
> > if (ret) {
> > - if (data->mainPathActive_)
> > - mainPath_.video_->streamOff();
> > -
> > + mainPath_.stop();
> > param_->streamOff();
> > stat_->streamOff();
> > data->ipa_->stop();
> > freeBuffers(camera);
> > -
> > - LOG(RkISP1, Error)
> > - << "Failed to start self path " << camera->id();
> > return ret;
> > }
> >
> > @@ -963,21 +933,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> > RkISP1CameraData *data = cameraData(camera);
> > int ret;
> >
> > - if (data->selfPathActive_) {
> > - ret = selfPath_.video_->streamOff();
> > - if (ret)
> > - LOG(RkISP1, Warning)
> > - << "Failed to stop self path for "
> > - << camera->id();
> > - }
> > -
> > - if (data->mainPathActive_) {
> > - ret = mainPath_.video_->streamOff();
> > - if (ret)
> > - LOG(RkISP1, Warning)
> > - << "Failed to stop main path for "
> > - << camera->id();
> > - }
> > + selfPath_.stop();
> > + mainPath_.stop();
> >
> > ret = stat_->streamOff();
> > if (ret)
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> > index 74eaa5d32388184b..b2dd669f4caf3269 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.cpp
> > @@ -20,9 +20,9 @@ LOG_DECLARE_CATEGORY(RkISP1)
> >
> > RkISP1Path::RkISP1Path(const char *name, const std::vector<PixelFormat> &formats,
> > const Size &minResolution, const Size &maxResolution)
> > - : video_(nullptr), name_(name), formats_(formats),
> > + : name_(name), running_(false), formats_(formats),
> > minResolution_(minResolution), maxResolution_(maxResolution),
> > - resizer_(nullptr)
> > + resizer_(nullptr), video_(nullptr)
> > {
> > }
> >
> > @@ -155,6 +155,44 @@ int RkISP1Path::exportBuffers(unsigned int bufferCount,
> > return video_->exportBuffers(bufferCount, buffers);
> > }
> >
> > +int RkISP1Path::start()
> > +{
> > + int ret;
> > +
> > + if (running_)
> > + return -EBUSY;
> > +
> > + ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
>
> This is a functional change that needs to be documented in the commit
> message (the number of buffers is now hardcoded). Is it right ?
No it's not the validate() function have always forced the number of
buffers 4 or at other parts in the code to RKISP1_BUFFER_COUNT (which is
defined as 4). This change only makes this more explicit.
I agree this is not the optimum behavior and we should rework the
pipeline to accept a configurable number of buffers from the
application. I wanted to keep the changes to a minimum as this series is
already too large, but I have noted this in my todo file.
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = video_->streamOn();
> > + if (ret) {
> > + LOG(RkISP1, Error)
> > + << "Failed to start " << name_ << "path";
>
> Should it be " path" ? Same below.
Yes, thanks good catch.
>
> > +
> > + video_->releaseBuffers();
> > + return ret;
> > + }
> > +
> > + running_ = true;
> > +
> > + return 0;
> > +}
> > +
> > +void RkISP1Path::stop()
> > +{
> > + if (!running_)
> > + return;
> > +
> > + if (video_->streamOff())
> > + LOG(RkISP1, Warning) << "Failed to stop " << name_ << "path";
> > +
> > + video_->releaseBuffers();
> > +
> > + running_ = false;
> > +}
> > +
> > RkISP1MainPath::RkISP1MainPath()
> > : RkISP1Path("main",
> > {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1path.h b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> > index 1315b2c64feac681..9c2ecb620375683b 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1path.h
> > @@ -41,22 +41,24 @@ public:
> > int exportBuffers(unsigned int bufferCount,
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >
> > + int start();
> > + void stop();
> > +
> > int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }
> > Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; }
> >
> > - /* \todo Make video private. */
> > - V4L2VideoDevice *video_;
> > -
> > private:
> > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >
> > const char *name_;
> > + bool running_;
> >
> > const std::vector<PixelFormat> formats_;
> > const Size minResolution_;
> > const Size maxResolution_;
> >
> > V4L2Subdevice *resizer_;
> > + V4L2VideoDevice *video_;
> > };
> >
> > class RkISP1MainPath : public RkISP1Path
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list