Message ID | 20250707075400.9079-5-stefan.klug@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Quoting Stefan Klug (2025-07-07 08:53:38) > The bufferCount is reset to a hardcoded value of 4 in > RkISP1Path::validate(). Keep the default value of 4 but do not reset > it, if it was changed. This allows the user to set bufferCount to an > arbitrary number of buffers which then can be allocated for example by > the FrameBufferAllocator. Internally maxQueuedRequestsDevice_ is used as > buffer count which is the maximum number of buffers queued to the device > at once. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v1: > - Removed todo comment that was solved by this change > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++-- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++----- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +--- > 3 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index fff42359cbff..bdaeb935ee06 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -790,6 +790,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > return nullptr; > > cfg.colorSpace = colorSpace; > + cfg.bufferCount = kPipelineDepth; > config->addConfiguration(cfg); > } > > @@ -1123,14 +1124,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL > } > > if (data->mainPath_->isEnabled()) { > - ret = mainPath_.start(); > + ret = mainPath_.start(maxQueuedRequestsDevice_); > if (ret) > return ret; > actions += [&]() { mainPath_.stop(); }; > } > > if (hasSelfPath_ && data->selfPath_->isEnabled()) { > - ret = selfPath_.start(); > + ret = selfPath_.start(maxQueuedRequestsDevice_); > if (ret) > return ret; > } > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 64018dc5b2f4..8ea5500d4080 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, > StreamConfiguration cfg(formats); > cfg.pixelFormat = format; > cfg.size = streamSize; > - cfg.bufferCount = RKISP1_BUFFER_COUNT; > > return cfg; > } > @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor, > > cfg->size.boundTo(maxResolution); > cfg->size.expandTo(minResolution); > - cfg->bufferCount = RKISP1_BUFFER_COUNT; > > V4L2DeviceFormat format; > format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat); > @@ -480,15 +478,14 @@ int RkISP1Path::configure(const StreamConfiguration &config, > return 0; > } > > -int RkISP1Path::start() > +int RkISP1Path::start(unsigned int bufferCount) > { > int ret; > > if (running_) > return -EBUSY; > > - /* \todo Make buffer count user configurable. */ > - ret = video_->importBuffers(RKISP1_BUFFER_COUNT); > + ret = video_->importBuffers(bufferCount); Eeek. Sorry - this might be wrong here. We're now letting applications allocate cfg.bufferCount size ... isn't that what should be passed in here ? This impacts the v4l2 buffer cache handling - where we track dma bufs to v4l2_buf objects. It would still work - but I think we'll be cycling through permanantly hitting cache flush/misses now with this if the application sets cfg->bufferCount > kPipelineDepth. The allocation that happens in this call is still small - and I still think we should be 'importing' the maximum number V4L2 lets us in most cases - the existing v4l2 buffer cache we have would then manage these accordingly. I think for the time being - this series could instead pass in the cfg.Buffer count to the two paths... perhaps with a todo to suggest that it could easily just be the maximum allowed by v4l2 instead. -- Kieran > if (ret) > return ret; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 430181d371a7..0b60c499ac64 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -58,7 +58,7 @@ public: > return video_->exportBuffers(bufferCount, buffers); > } > > - int start(); > + int start(unsigned int bufferCount); > void stop(); > > int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); } > @@ -69,8 +69,6 @@ private: > void populateFormats(); > Size filterSensorResolution(const CameraSensor *sensor); > > - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > - > const char *name_; > bool running_; > > -- > 2.48.1 >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index fff42359cbff..bdaeb935ee06 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -790,6 +790,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, return nullptr; cfg.colorSpace = colorSpace; + cfg.bufferCount = kPipelineDepth; config->addConfiguration(cfg); } @@ -1123,14 +1124,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL } if (data->mainPath_->isEnabled()) { - ret = mainPath_.start(); + ret = mainPath_.start(maxQueuedRequestsDevice_); if (ret) return ret; actions += [&]() { mainPath_.stop(); }; } if (hasSelfPath_ && data->selfPath_->isEnabled()) { - ret = selfPath_.start(); + ret = selfPath_.start(maxQueuedRequestsDevice_); if (ret) return ret; } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 64018dc5b2f4..8ea5500d4080 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, StreamConfiguration cfg(formats); cfg.pixelFormat = format; cfg.size = streamSize; - cfg.bufferCount = RKISP1_BUFFER_COUNT; return cfg; } @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor, cfg->size.boundTo(maxResolution); cfg->size.expandTo(minResolution); - cfg->bufferCount = RKISP1_BUFFER_COUNT; V4L2DeviceFormat format; format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat); @@ -480,15 +478,14 @@ int RkISP1Path::configure(const StreamConfiguration &config, return 0; } -int RkISP1Path::start() +int RkISP1Path::start(unsigned int bufferCount) { int ret; if (running_) return -EBUSY; - /* \todo Make buffer count user configurable. */ - ret = video_->importBuffers(RKISP1_BUFFER_COUNT); + ret = video_->importBuffers(bufferCount); if (ret) return ret; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 430181d371a7..0b60c499ac64 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -58,7 +58,7 @@ public: return video_->exportBuffers(bufferCount, buffers); } - int start(); + int start(unsigned int bufferCount); void stop(); int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); } @@ -69,8 +69,6 @@ private: void populateFormats(); Size filterSensorResolution(const CameraSensor *sensor); - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; - const char *name_; bool running_;
The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate(). Keep the default value of 4 but do not reset it, if it was changed. This allows the user to set bufferCount to an arbitrary number of buffers which then can be allocated for example by the FrameBufferAllocator. Internally maxQueuedRequestsDevice_ is used as buffer count which is the maximum number of buffers queued to the device at once. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v1: - Removed todo comment that was solved by this change --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++-- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++----- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +--- 3 files changed, 6 insertions(+), 10 deletions(-)