| Message ID | 20200813005246.3265807-11-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote: > Allow for both the main and self path streams to be configured. This > change adds the self path as an internal stream to the pipeline handler. > It is not exposed as a Camera stream so it can not yet be used. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- > 1 file changed, 139 insertions(+), 58 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 132878c13b10b555..671098e11a2e2750 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -133,7 +133,8 @@ public: > V4L2VideoDevice *selfPathVideo) > : CameraData(pipe), sensor_(nullptr), frame_(0), > frameInfo_(pipe), mainPathVideo_(mainPathVideo), > - selfPathVideo_(selfPathVideo) > + selfPathVideo_(selfPathVideo), mainPathActive_(false), > + selfPathActive_(false) > { > } > > @@ -145,6 +146,7 @@ public: > int loadIPA(); > > Stream mainPathStream_; > + Stream selfPathStream_; Would this, and all other duplicated fields be nicer is stored as arrays and accessed with fixed indexes ? I don't mind the way it is today though > CameraSensor *sensor_; > unsigned int frame_; > std::vector<IPABuffer> ipaBuffers_; > @@ -154,6 +156,9 @@ public: > V4L2VideoDevice *mainPathVideo_; > V4L2VideoDevice *selfPathVideo_; > > + bool mainPathActive_; > + bool selfPathActive_; > + > private: > void queueFrameAction(unsigned int frame, > const IPAOperationData &action); > @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > RkISP1CameraConfiguration *config = > static_cast<RkISP1CameraConfiguration *>(c); > RkISP1CameraData *data = cameraData(camera); > - StreamConfiguration &cfg = config->at(0); > CameraSensor *sensor = data->sensor_; > int ret; > > @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); > > - ret = mainPathResizer_->setFormat(0, &format); > - if (ret < 0) > - return ret; > - > - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); > - > - format.size = cfg.size; > - > - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); > - > - ret = mainPathResizer_->setFormat(1, &format); > - if (ret < 0) > - return ret; > - > - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); > - > - V4L2DeviceFormat outputFormat = {}; > - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); > - outputFormat.size = cfg.size; > - outputFormat.planesCount = 2; > - > - ret = mainPathVideo_->setFormat(&outputFormat); > - if (ret) > - return ret; > - > - if (outputFormat.size != cfg.size || > - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { > - LOG(RkISP1, Error) > - << "Unable to configure capture in " << cfg.toString(); > - return -EINVAL; > + data->mainPathActive_ = false; > + data->selfPathActive_ = false; > + for (const StreamConfiguration &cfg : *config) { > + V4L2SubdeviceFormat ispFormat = format; > + V4L2Subdevice *resizer; > + V4L2VideoDevice *video; > + > + if (cfg.stream() == &data->mainPathStream_) { > + resizer = mainPathResizer_; > + video = mainPathVideo_; > + data->mainPathActive_ = true; > + } else { > + resizer = selfPathResizer_; > + video = selfPathVideo_; > + data->selfPathActive_ = true; > + } > + > + ret = resizer->setFormat(0, &ispFormat); > + if (ret < 0) > + return ret; > + > + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); > + > + ispFormat.size = cfg.size; > + > + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); > + > + ret = resizer->setFormat(1, &ispFormat); > + if (ret < 0) > + return ret; > + > + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); > + > + V4L2DeviceFormat outputFormat = {}; > + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); > + outputFormat.size = cfg.size; > + outputFormat.planesCount = 2; Doesn't the planes count depend on the format ? > + > + ret = video->setFormat(&outputFormat); > + if (ret) > + return ret; > + > + if (outputFormat.size != cfg.size || > + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { > + LOG(RkISP1, Error) > + << "Unable to configure capture in " << cfg.toString(); > + return -EINVAL; > + } > } > > V4L2DeviceFormat paramFormat = {}; > @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > - cfg.setStream(&data->mainPathStream_); > - > return 0; > } > > int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > + RkISP1CameraData *data = cameraData(camera); > unsigned int count = stream->configuration().bufferCount; > - return mainPathVideo_->exportBuffers(count, buffers); > + > + if (stream == &data->mainPathStream_) > + return mainPathVideo_->exportBuffers(count, buffers); > + > + if (stream == &data->selfPathStream_) > + return selfPathVideo_->exportBuffers(count, buffers); > + > + return -EINVAL; > } > > int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > { > RkISP1CameraData *data = cameraData(camera); > - unsigned int count = data->mainPathStream_.configuration().bufferCount; > unsigned int ipaBufferId = 1; > int ret; > > - ret = mainPathVideo_->importBuffers(count); > - if (ret < 0) > - goto error; > + unsigned int count = std::max({ > + data->mainPathStream_.configuration().bufferCount, > + data->selfPathStream_.configuration().bufferCount, > + }); > + > + if (data->mainPathActive_) { > + ret = mainPathVideo_->importBuffers(count); > + if (ret < 0) > + goto error; Is it safe to call releaseBuffers if importing fails ? > + } > + > + if (data->selfPathActive_) { > + ret = selfPathVideo_->importBuffers(count); > + if (ret < 0) > + goto error; > + } > > ret = param_->allocateBuffers(count, ¶mBuffers_); > if (ret < 0) > @@ -754,6 +794,7 @@ error: > paramBuffers_.clear(); > statBuffers_.clear(); > mainPathVideo_->releaseBuffers(); > + selfPathVideo_->releaseBuffers(); > > return ret; > } > @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) > if (mainPathVideo_->releaseBuffers()) > LOG(RkISP1, Error) << "Failed to release main path buffers"; > > + if (selfPathVideo_->releaseBuffers()) > + LOG(RkISP1, Error) << "Failed to release self path buffers"; > + > return 0; > } > > @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) > return ret; > } > > - ret = mainPathVideo_->streamOn(); > - if (ret) { > - param_->streamOff(); > - stat_->streamOff(); > - data->ipa_->stop(); > - freeBuffers(camera); > - > - LOG(RkISP1, Error) > - << "Failed to start camera " << camera->id(); > + std::map<unsigned int, IPAStream> streamConfig; > + > + if (data->mainPathActive_) { > + ret = mainPathVideo_->streamOn(); > + if (ret) { > + param_->streamOff(); > + stat_->streamOff(); > + data->ipa_->stop(); > + freeBuffers(camera); > + > + LOG(RkISP1, Error) > + << "Failed to start main path " << camera->id(); > + return ret; > + } > + > + streamConfig[0] = { > + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > + .size = data->mainPathStream_.configuration().size, > + }; What's this for ? And why assume [0] is assigned to main path ? > + } > + > + if (data->selfPathActive_) { > + ret = selfPathVideo_->streamOn(); > + if (ret) { > + if (data->mainPathActive_) > + mainPathVideo_->streamOff(); > + > + param_->streamOff(); > + stat_->streamOff(); > + data->ipa_->stop(); > + freeBuffers(camera); > + > + LOG(RkISP1, Error) > + << "Failed to start self path " << camera->id(); > + return ret; > + } > + > + streamConfig[1] = { > + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > + .size = data->selfPathStream_.configuration().size, > + }; > } > > activeCamera_ = camera; > @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) > ret = 0; > } > > - std::map<unsigned int, IPAStream> streamConfig; > - streamConfig[0] = { > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > - .size = data->mainPathStream_.configuration().size, > - }; > - > std::map<unsigned int, const ControlInfoMap &> entityControls; > entityControls.emplace(0, data->sensor_->controls()); > > @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > RkISP1CameraData *data = cameraData(camera); > int ret; > > - ret = mainPathVideo_->streamOff(); > - if (ret) > - LOG(RkISP1, Warning) > - << "Failed to stop camera " << camera->id(); > + if (data->selfPathActive_) { > + ret = selfPathVideo_->streamOff(); > + if (ret) > + LOG(RkISP1, Warning) > + << "Failed to stop self path " << camera->id(); > + } Can we call streamOff unconditionally ? It depends on the driver implementation I guess Overall the patch looks good! Thanks j > + > + if (data->mainPathActive_) { > + ret = mainPathVideo_->streamOff(); > + if (ret) > + LOG(RkISP1, Warning) > + << "Failed to stop main path " << camera->id(); > + } > > ret = stat_->streamOff(); > if (ret) > @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, > std::string resizer; > if (cfg.stream() == &data->mainPathStream_) > resizer = "rkisp1_resizer_mainpath"; > + else if (cfg.stream() == &data->selfPathStream_) > + resizer = "rkisp1_resizer_selfpath"; > else > return -EINVAL; > > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote: > > Allow for both the main and self path streams to be configured. This > > change adds the self path as an internal stream to the pipeline handler. > > It is not exposed as a Camera stream so it can not yet be used. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- > > 1 file changed, 139 insertions(+), 58 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 132878c13b10b555..671098e11a2e2750 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -133,7 +133,8 @@ public: > > V4L2VideoDevice *selfPathVideo) > > : CameraData(pipe), sensor_(nullptr), frame_(0), > > frameInfo_(pipe), mainPathVideo_(mainPathVideo), > > - selfPathVideo_(selfPathVideo) > > + selfPathVideo_(selfPathVideo), mainPathActive_(false), > > + selfPathActive_(false) > > { > > } > > > > @@ -145,6 +146,7 @@ public: > > int loadIPA(); > > > > Stream mainPathStream_; > > + Stream selfPathStream_; > > Would this, and all other duplicated fields be nicer is stored as > arrays and accessed with fixed indexes ? I don't mind the way it is > today though I guess this is another bikeshedding topic. I don't see any clear advantage of one over the other. mainPathStream_ vs streams_[RkISP1::MainPAth] If we are to switch to an array approach I do think we should do so before or after this series. I picked this solution as it was the one already used in the pipeline. > > > CameraSensor *sensor_; > > unsigned int frame_; > > std::vector<IPABuffer> ipaBuffers_; > > @@ -154,6 +156,9 @@ public: > > V4L2VideoDevice *mainPathVideo_; > > V4L2VideoDevice *selfPathVideo_; > > > > + bool mainPathActive_; > > + bool selfPathActive_; > > + > > private: > > void queueFrameAction(unsigned int frame, > > const IPAOperationData &action); > > @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > RkISP1CameraConfiguration *config = > > static_cast<RkISP1CameraConfiguration *>(c); > > RkISP1CameraData *data = cameraData(camera); > > - StreamConfiguration &cfg = config->at(0); > > CameraSensor *sensor = data->sensor_; > > int ret; > > > > @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > > LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); > > > > - ret = mainPathResizer_->setFormat(0, &format); > > - if (ret < 0) > > - return ret; > > - > > - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); > > - > > - format.size = cfg.size; > > - > > - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); > > - > > - ret = mainPathResizer_->setFormat(1, &format); > > - if (ret < 0) > > - return ret; > > - > > - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); > > - > > - V4L2DeviceFormat outputFormat = {}; > > - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); > > - outputFormat.size = cfg.size; > > - outputFormat.planesCount = 2; > > - > > - ret = mainPathVideo_->setFormat(&outputFormat); > > - if (ret) > > - return ret; > > - > > - if (outputFormat.size != cfg.size || > > - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { > > - LOG(RkISP1, Error) > > - << "Unable to configure capture in " << cfg.toString(); > > - return -EINVAL; > > + data->mainPathActive_ = false; > > + data->selfPathActive_ = false; > > + for (const StreamConfiguration &cfg : *config) { > > + V4L2SubdeviceFormat ispFormat = format; > > + V4L2Subdevice *resizer; > > + V4L2VideoDevice *video; > > + > > + if (cfg.stream() == &data->mainPathStream_) { > > + resizer = mainPathResizer_; > > + video = mainPathVideo_; > > + data->mainPathActive_ = true; > > + } else { > > + resizer = selfPathResizer_; > > + video = selfPathVideo_; > > + data->selfPathActive_ = true; > > + } > > + > > + ret = resizer->setFormat(0, &ispFormat); > > + if (ret < 0) > > + return ret; > > + > > + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); > > + > > + ispFormat.size = cfg.size; > > + > > + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); > > + > > + ret = resizer->setFormat(1, &ispFormat); > > + if (ret < 0) > > + return ret; > > + > > + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); > > + > > + V4L2DeviceFormat outputFormat = {}; > > + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); > > + outputFormat.size = cfg.size; > > + outputFormat.planesCount = 2; > > Doesn't the planes count depend on the format ? Good catch. This is a preexisting bug in the pipeline. I will add a separate patch to this series to sort this out. > > > + > > + ret = video->setFormat(&outputFormat); > > + if (ret) > > + return ret; > > + > > + if (outputFormat.size != cfg.size || > > + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { > > + LOG(RkISP1, Error) > > + << "Unable to configure capture in " << cfg.toString(); > > + return -EINVAL; > > + } > > } > > > > V4L2DeviceFormat paramFormat = {}; > > @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > if (ret) > > return ret; > > > > - cfg.setStream(&data->mainPathStream_); > > - > > return 0; > > } > > > > int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > + RkISP1CameraData *data = cameraData(camera); > > unsigned int count = stream->configuration().bufferCount; > > - return mainPathVideo_->exportBuffers(count, buffers); > > + > > + if (stream == &data->mainPathStream_) > > + return mainPathVideo_->exportBuffers(count, buffers); > > + > > + if (stream == &data->selfPathStream_) > > + return selfPathVideo_->exportBuffers(count, buffers); > > + > > + return -EINVAL; > > } > > > > int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > > { > > RkISP1CameraData *data = cameraData(camera); > > - unsigned int count = data->mainPathStream_.configuration().bufferCount; > > unsigned int ipaBufferId = 1; > > int ret; > > > > - ret = mainPathVideo_->importBuffers(count); > > - if (ret < 0) > > - goto error; > > + unsigned int count = std::max({ > > + data->mainPathStream_.configuration().bufferCount, > > + data->selfPathStream_.configuration().bufferCount, > > + }); > > + > > + if (data->mainPathActive_) { > > + ret = mainPathVideo_->importBuffers(count); > > + if (ret < 0) > > + goto error; > > Is it safe to call releaseBuffers if importing fails ? Yes, is it not always safe to call releaseBuffers? The idea is that if allocateBuffers() fails it should undo all operations. Remember we depend on buffer orphaning so buffers allocated by for example the FrameBufferAllocater are still OK. > > > + } > > + > > + if (data->selfPathActive_) { > > + ret = selfPathVideo_->importBuffers(count); > > + if (ret < 0) > > + goto error; > > + } > > > > ret = param_->allocateBuffers(count, ¶mBuffers_); > > if (ret < 0) > > @@ -754,6 +794,7 @@ error: > > paramBuffers_.clear(); > > statBuffers_.clear(); > > mainPathVideo_->releaseBuffers(); > > + selfPathVideo_->releaseBuffers(); > > > > return ret; > > } > > @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) > > if (mainPathVideo_->releaseBuffers()) > > LOG(RkISP1, Error) << "Failed to release main path buffers"; > > > > + if (selfPathVideo_->releaseBuffers()) > > + LOG(RkISP1, Error) << "Failed to release self path buffers"; > > + > > return 0; > > } > > > > @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > return ret; > > } > > > > - ret = mainPathVideo_->streamOn(); > > - if (ret) { > > - param_->streamOff(); > > - stat_->streamOff(); > > - data->ipa_->stop(); > > - freeBuffers(camera); > > - > > - LOG(RkISP1, Error) > > - << "Failed to start camera " << camera->id(); > > + std::map<unsigned int, IPAStream> streamConfig; > > + > > + if (data->mainPathActive_) { > > + ret = mainPathVideo_->streamOn(); > > + if (ret) { > > + param_->streamOff(); > > + stat_->streamOff(); > > + data->ipa_->stop(); > > + freeBuffers(camera); > > + > > + LOG(RkISP1, Error) > > + << "Failed to start main path " << camera->id(); > > + return ret; > > + } > > + > > + streamConfig[0] = { > > + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > > + .size = data->mainPathStream_.configuration().size, > > + }; > > What's this for ? And why assume [0] is assigned to main path ? This is part of the yet undocumented IPA protocl for the RkISP1. The stream configuration map is mainpath = 0 and selfpath = 1. When we get the new IPC framework in place all of this should of course be documented. In reality this is have currently no impact as the RkISP1 IPA does not examine the stream configuration at all. > > > + } > > + > > + if (data->selfPathActive_) { > > + ret = selfPathVideo_->streamOn(); > > + if (ret) { > > + if (data->mainPathActive_) > > + mainPathVideo_->streamOff(); > > + > > + param_->streamOff(); > > + stat_->streamOff(); > > + data->ipa_->stop(); > > + freeBuffers(camera); > > + > > + LOG(RkISP1, Error) > > + << "Failed to start self path " << camera->id(); > > + return ret; > > + } > > + > > + streamConfig[1] = { > > + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > > + .size = data->selfPathStream_.configuration().size, > > + }; > > } > > > > activeCamera_ = camera; > > @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > ret = 0; > > } > > > > - std::map<unsigned int, IPAStream> streamConfig; > > - streamConfig[0] = { > > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > > - .size = data->mainPathStream_.configuration().size, > > - }; > > - > > std::map<unsigned int, const ControlInfoMap &> entityControls; > > entityControls.emplace(0, data->sensor_->controls()); > > > > @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > > RkISP1CameraData *data = cameraData(camera); > > int ret; > > > > - ret = mainPathVideo_->streamOff(); > > - if (ret) > > - LOG(RkISP1, Warning) > > - << "Failed to stop camera " << camera->id(); > > + if (data->selfPathActive_) { > > + ret = selfPathVideo_->streamOff(); > > + if (ret) > > + LOG(RkISP1, Warning) > > + << "Failed to stop self path " << camera->id(); > > + } > > Can we call streamOff unconditionally ? It depends on the driver > implementation I guess We can call it unconditionally, I think even v4l2-compliance tests multiple s_stream(1) and s_stream(0) calls does it not? But I agree there might of course be driver specific quirks. > > Overall the patch looks good! > Thanks > j > > > + > > + if (data->mainPathActive_) { > > + ret = mainPathVideo_->streamOff(); > > + if (ret) > > + LOG(RkISP1, Warning) > > + << "Failed to stop main path " << camera->id(); > > + } > > > > ret = stat_->streamOff(); > > if (ret) > > @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, > > std::string resizer; > > if (cfg.stream() == &data->mainPathStream_) > > resizer = "rkisp1_resizer_mainpath"; > > + else if (cfg.stream() == &data->selfPathStream_) > > + resizer = "rkisp1_resizer_selfpath"; > > else > > return -EINVAL; > > > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 14/09/2020 12:09, Niklas Söderlund wrote: > Hi Jacopo, > > On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote: >> Hi Niklas, >> >> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote: >>> Allow for both the main and self path streams to be configured. This >>> change adds the self path as an internal stream to the pipeline handler. >>> It is not exposed as a Camera stream so it can not yet be used. >>> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>> --- >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- >>> 1 file changed, 139 insertions(+), 58 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> index 132878c13b10b555..671098e11a2e2750 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> @@ -133,7 +133,8 @@ public: >>> V4L2VideoDevice *selfPathVideo) >>> : CameraData(pipe), sensor_(nullptr), frame_(0), >>> frameInfo_(pipe), mainPathVideo_(mainPathVideo), >>> - selfPathVideo_(selfPathVideo) >>> + selfPathVideo_(selfPathVideo), mainPathActive_(false), >>> + selfPathActive_(false) >>> { >>> } >>> >>> @@ -145,6 +146,7 @@ public: >>> int loadIPA(); >>> >>> Stream mainPathStream_; >>> + Stream selfPathStream_; >> >> Would this, and all other duplicated fields be nicer is stored as >> arrays and accessed with fixed indexes ? I don't mind the way it is >> today though > > I guess this is another bikeshedding topic. I don't see any clear > advantage of one over the other. > > mainPathStream_ > > vs > > streams_[RkISP1::MainPAth] > > If we are to switch to an array approach I do think we should do so > before or after this series. I picked this solution as it was the one > already used in the pipeline. Jumping into the bikeshed mostly because it really stood out in the title of this and the preceding patch. The 'self' path sounds horrible. I assume it derives from 'selfie' ... but really it's just a secondary ... I would probably prefer the array route (can be refactored anytime, doesn't have to be here). If one path is more feature full than the other then perhaps naming them makes sense, but otherwise I'd just have them as numerical pipelines. And rather than have an array of streams_ and an array of V4L2VideoDevice and an array of Sensors etc... I think this can be moddelled as a class, and then it's just a single array of that class. (Or if there's only two, It could actually be just two instances of that class at that point) RkISPPipe main; RkISPPipe secondary; edit: ohh yuck. I see > > + selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath"); so it's named the 'self path' at the ISP ... -- Kieran >> >>> CameraSensor *sensor_; >>> unsigned int frame_; >>> std::vector<IPABuffer> ipaBuffers_; >>> @@ -154,6 +156,9 @@ public: >>> V4L2VideoDevice *mainPathVideo_; >>> V4L2VideoDevice *selfPathVideo_; >>> >>> + bool mainPathActive_; >>> + bool selfPathActive_; >>> + >>> private: >>> void queueFrameAction(unsigned int frame, >>> const IPAOperationData &action); >>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>> RkISP1CameraConfiguration *config = >>> static_cast<RkISP1CameraConfiguration *>(c); >>> RkISP1CameraData *data = cameraData(camera); >>> - StreamConfiguration &cfg = config->at(0); >>> CameraSensor *sensor = data->sensor_; >>> int ret; >>> >>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>> >>> LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); >>> >>> - ret = mainPathResizer_->setFormat(0, &format); >>> - if (ret < 0) >>> - return ret; >>> - >>> - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); >>> - >>> - format.size = cfg.size; >>> - >>> - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); >>> - >>> - ret = mainPathResizer_->setFormat(1, &format); >>> - if (ret < 0) >>> - return ret; >>> - >>> - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); >>> - >>> - V4L2DeviceFormat outputFormat = {}; >>> - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); >>> - outputFormat.size = cfg.size; >>> - outputFormat.planesCount = 2; >>> - >>> - ret = mainPathVideo_->setFormat(&outputFormat); >>> - if (ret) >>> - return ret; >>> - >>> - if (outputFormat.size != cfg.size || >>> - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { >>> - LOG(RkISP1, Error) >>> - << "Unable to configure capture in " << cfg.toString(); >>> - return -EINVAL; >>> + data->mainPathActive_ = false; >>> + data->selfPathActive_ = false; >>> + for (const StreamConfiguration &cfg : *config) { >>> + V4L2SubdeviceFormat ispFormat = format; >>> + V4L2Subdevice *resizer; >>> + V4L2VideoDevice *video; >>> + >>> + if (cfg.stream() == &data->mainPathStream_) { >>> + resizer = mainPathResizer_; >>> + video = mainPathVideo_; >>> + data->mainPathActive_ = true; >>> + } else { >>> + resizer = selfPathResizer_; >>> + video = selfPathVideo_; >>> + data->selfPathActive_ = true; >>> + } >>> + >>> + ret = resizer->setFormat(0, &ispFormat); >>> + if (ret < 0) >>> + return ret; >>> + >>> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); >>> + >>> + ispFormat.size = cfg.size; >>> + >>> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); >>> + >>> + ret = resizer->setFormat(1, &ispFormat); >>> + if (ret < 0) >>> + return ret; >>> + >>> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); >>> + >>> + V4L2DeviceFormat outputFormat = {}; >>> + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); >>> + outputFormat.size = cfg.size; >>> + outputFormat.planesCount = 2; >> >> Doesn't the planes count depend on the format ? > > Good catch. This is a preexisting bug in the pipeline. I will add a > separate patch to this series to sort this out. > >> >>> + >>> + ret = video->setFormat(&outputFormat); >>> + if (ret) >>> + return ret; >>> + >>> + if (outputFormat.size != cfg.size || >>> + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { >>> + LOG(RkISP1, Error) >>> + << "Unable to configure capture in " << cfg.toString(); >>> + return -EINVAL; >>> + } >>> } >>> >>> V4L2DeviceFormat paramFormat = {}; >>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>> if (ret) >>> return ret; >>> >>> - cfg.setStream(&data->mainPathStream_); >>> - >>> return 0; >>> } >>> >>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, >>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) >>> { >>> + RkISP1CameraData *data = cameraData(camera); >>> unsigned int count = stream->configuration().bufferCount; >>> - return mainPathVideo_->exportBuffers(count, buffers); >>> + >>> + if (stream == &data->mainPathStream_) >>> + return mainPathVideo_->exportBuffers(count, buffers); >>> + >>> + if (stream == &data->selfPathStream_) >>> + return selfPathVideo_->exportBuffers(count, buffers); >>> + >>> + return -EINVAL; >>> } >>> >>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) >>> { >>> RkISP1CameraData *data = cameraData(camera); >>> - unsigned int count = data->mainPathStream_.configuration().bufferCount; >>> unsigned int ipaBufferId = 1; >>> int ret; >>> >>> - ret = mainPathVideo_->importBuffers(count); >>> - if (ret < 0) >>> - goto error; >>> + unsigned int count = std::max({ >>> + data->mainPathStream_.configuration().bufferCount, >>> + data->selfPathStream_.configuration().bufferCount, >>> + }); >>> + >>> + if (data->mainPathActive_) { >>> + ret = mainPathVideo_->importBuffers(count); >>> + if (ret < 0) >>> + goto error; >> >> Is it safe to call releaseBuffers if importing fails ? > > Yes, is it not always safe to call releaseBuffers? The idea is that if > allocateBuffers() fails it should undo all operations. Remember we > depend on buffer orphaning so buffers allocated by for example the > FrameBufferAllocater are still OK. > >> >>> + } >>> + >>> + if (data->selfPathActive_) { >>> + ret = selfPathVideo_->importBuffers(count); >>> + if (ret < 0) >>> + goto error; >>> + } >>> >>> ret = param_->allocateBuffers(count, ¶mBuffers_); >>> if (ret < 0) >>> @@ -754,6 +794,7 @@ error: >>> paramBuffers_.clear(); >>> statBuffers_.clear(); >>> mainPathVideo_->releaseBuffers(); >>> + selfPathVideo_->releaseBuffers(); >>> >>> return ret; >>> } >>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) >>> if (mainPathVideo_->releaseBuffers()) >>> LOG(RkISP1, Error) << "Failed to release main path buffers"; >>> >>> + if (selfPathVideo_->releaseBuffers()) >>> + LOG(RkISP1, Error) << "Failed to release self path buffers"; >>> + >>> return 0; >>> } >>> >>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) >>> return ret; >>> } >>> >>> - ret = mainPathVideo_->streamOn(); >>> - if (ret) { >>> - param_->streamOff(); >>> - stat_->streamOff(); >>> - data->ipa_->stop(); >>> - freeBuffers(camera); >>> - >>> - LOG(RkISP1, Error) >>> - << "Failed to start camera " << camera->id(); >>> + std::map<unsigned int, IPAStream> streamConfig; >>> + >>> + if (data->mainPathActive_) { >>> + ret = mainPathVideo_->streamOn(); >>> + if (ret) { >>> + param_->streamOff(); >>> + stat_->streamOff(); >>> + data->ipa_->stop(); >>> + freeBuffers(camera); >>> + >>> + LOG(RkISP1, Error) >>> + << "Failed to start main path " << camera->id(); >>> + return ret; >>> + } >>> + >>> + streamConfig[0] = { >>> + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, >>> + .size = data->mainPathStream_.configuration().size, >>> + }; >> >> What's this for ? And why assume [0] is assigned to main path ? > > This is part of the yet undocumented IPA protocl for the RkISP1. The > stream configuration map is mainpath = 0 and selfpath = 1. When we get > the new IPC framework in place all of this should of course be > documented. In reality this is have currently no impact as the RkISP1 > IPA does not examine the stream configuration at all. > >> >>> + } >>> + >>> + if (data->selfPathActive_) { >>> + ret = selfPathVideo_->streamOn(); >>> + if (ret) { >>> + if (data->mainPathActive_) >>> + mainPathVideo_->streamOff(); >>> + >>> + param_->streamOff(); >>> + stat_->streamOff(); >>> + data->ipa_->stop(); >>> + freeBuffers(camera); >>> + >>> + LOG(RkISP1, Error) >>> + << "Failed to start self path " << camera->id(); >>> + return ret; >>> + } >>> + >>> + streamConfig[1] = { >>> + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, >>> + .size = data->selfPathStream_.configuration().size, >>> + }; >>> } >>> >>> activeCamera_ = camera; >>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) >>> ret = 0; >>> } >>> >>> - std::map<unsigned int, IPAStream> streamConfig; >>> - streamConfig[0] = { >>> - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, >>> - .size = data->mainPathStream_.configuration().size, >>> - }; >>> - >>> std::map<unsigned int, const ControlInfoMap &> entityControls; >>> entityControls.emplace(0, data->sensor_->controls()); >>> >>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) >>> RkISP1CameraData *data = cameraData(camera); >>> int ret; >>> >>> - ret = mainPathVideo_->streamOff(); >>> - if (ret) >>> - LOG(RkISP1, Warning) >>> - << "Failed to stop camera " << camera->id(); >>> + if (data->selfPathActive_) { >>> + ret = selfPathVideo_->streamOff(); >>> + if (ret) >>> + LOG(RkISP1, Warning) >>> + << "Failed to stop self path " << camera->id(); >>> + } >> >> Can we call streamOff unconditionally ? It depends on the driver >> implementation I guess > > We can call it unconditionally, I think even v4l2-compliance tests > multiple s_stream(1) and s_stream(0) calls does it not? But I agree > there might of course be driver specific quirks. > >> >> Overall the patch looks good! >> Thanks >> j >> >>> + >>> + if (data->mainPathActive_) { >>> + ret = mainPathVideo_->streamOff(); >>> + if (ret) >>> + LOG(RkISP1, Warning) >>> + << "Failed to stop main path " << camera->id(); >>> + } >>> >>> ret = stat_->streamOff(); >>> if (ret) >>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, >>> std::string resizer; >>> if (cfg.stream() == &data->mainPathStream_) >>> resizer = "rkisp1_resizer_mainpath"; >>> + else if (cfg.stream() == &data->selfPathStream_) >>> + resizer = "rkisp1_resizer_selfpath"; >>> else >>> return -EINVAL; >>> >>> -- >>> 2.28.0 >>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Niklas, On 14/09/2020 12:19, Kieran Bingham wrote: > Hi Niklas, > > On 14/09/2020 12:09, Niklas Söderlund wrote: >> Hi Jacopo, >> >> On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote: >>> Hi Niklas, >>> >>> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote: >>>> Allow for both the main and self path streams to be configured. This >>>> change adds the self path as an internal stream to the pipeline handler. >>>> It is not exposed as a Camera stream so it can not yet be used. >>>> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>>> --- >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- >>>> 1 file changed, 139 insertions(+), 58 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> index 132878c13b10b555..671098e11a2e2750 100644 >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>>> @@ -133,7 +133,8 @@ public: >>>> V4L2VideoDevice *selfPathVideo) >>>> : CameraData(pipe), sensor_(nullptr), frame_(0), >>>> frameInfo_(pipe), mainPathVideo_(mainPathVideo), >>>> - selfPathVideo_(selfPathVideo) >>>> + selfPathVideo_(selfPathVideo), mainPathActive_(false), >>>> + selfPathActive_(false) >>>> { >>>> } >>>> >>>> @@ -145,6 +146,7 @@ public: >>>> int loadIPA(); >>>> >>>> Stream mainPathStream_; >>>> + Stream selfPathStream_; >>> >>> Would this, and all other duplicated fields be nicer is stored as >>> arrays and accessed with fixed indexes ? I don't mind the way it is >>> today though >> >> I guess this is another bikeshedding topic. I don't see any clear >> advantage of one over the other. >> >> mainPathStream_ >> >> vs >> >> streams_[RkISP1::MainPAth] >> >> If we are to switch to an array approach I do think we should do so >> before or after this series. I picked this solution as it was the one >> already used in the pipeline. > > > Jumping into the bikeshed mostly because it really stood out in the > title of this and the preceding patch. > > The 'self' path sounds horrible. > I assume it derives from 'selfie' ... but really it's just a secondary ... > > > I would probably prefer the array route (can be refactored anytime, > doesn't have to be here). If one path is more feature full than the > other then perhaps naming them makes sense, but otherwise I'd just have > them as numerical pipelines. > > And rather than have an array of streams_ and an array of > V4L2VideoDevice and an array of Sensors etc... I think this can be > moddelled as a class, and then it's just a single array of that class. > > (Or if there's only two, It could actually be just two instances of that > class at that point) > > RkISPPipe main; > RkISPPipe secondary; > > > edit: ohh yuck. I see >> >> + selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath"); > > so it's named the 'self path' at the ISP ... > Re-edit: Aha, I think I mis-understood this slightly. I thought this was dealing with multiple camera paths ... but it might be dealing with multiple stream paths from a single camera... so it's not separate pipes necessarily ... :-S Perhaps it's an RkISPStream object instead though ... > -- > Kieran > > > >>> >>>> CameraSensor *sensor_; >>>> unsigned int frame_; >>>> std::vector<IPABuffer> ipaBuffers_; >>>> @@ -154,6 +156,9 @@ public: >>>> V4L2VideoDevice *mainPathVideo_; >>>> V4L2VideoDevice *selfPathVideo_; >>>> >>>> + bool mainPathActive_; >>>> + bool selfPathActive_; >>>> + >>>> private: >>>> void queueFrameAction(unsigned int frame, >>>> const IPAOperationData &action); >>>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>>> RkISP1CameraConfiguration *config = >>>> static_cast<RkISP1CameraConfiguration *>(c); >>>> RkISP1CameraData *data = cameraData(camera); >>>> - StreamConfiguration &cfg = config->at(0); >>>> CameraSensor *sensor = data->sensor_; >>>> int ret; >>>> >>>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>>> >>>> LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); >>>> >>>> - ret = mainPathResizer_->setFormat(0, &format); >>>> - if (ret < 0) >>>> - return ret; >>>> - >>>> - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); >>>> - >>>> - format.size = cfg.size; >>>> - >>>> - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); >>>> - >>>> - ret = mainPathResizer_->setFormat(1, &format); >>>> - if (ret < 0) >>>> - return ret; >>>> - >>>> - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); >>>> - >>>> - V4L2DeviceFormat outputFormat = {}; >>>> - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); >>>> - outputFormat.size = cfg.size; >>>> - outputFormat.planesCount = 2; >>>> - >>>> - ret = mainPathVideo_->setFormat(&outputFormat); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - if (outputFormat.size != cfg.size || >>>> - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { >>>> - LOG(RkISP1, Error) >>>> - << "Unable to configure capture in " << cfg.toString(); >>>> - return -EINVAL; >>>> + data->mainPathActive_ = false; >>>> + data->selfPathActive_ = false; >>>> + for (const StreamConfiguration &cfg : *config) { >>>> + V4L2SubdeviceFormat ispFormat = format; >>>> + V4L2Subdevice *resizer; >>>> + V4L2VideoDevice *video; >>>> + >>>> + if (cfg.stream() == &data->mainPathStream_) { >>>> + resizer = mainPathResizer_; >>>> + video = mainPathVideo_; >>>> + data->mainPathActive_ = true; >>>> + } else { >>>> + resizer = selfPathResizer_; >>>> + video = selfPathVideo_; >>>> + data->selfPathActive_ = true; >>>> + } >>>> + >>>> + ret = resizer->setFormat(0, &ispFormat); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); >>>> + >>>> + ispFormat.size = cfg.size; >>>> + >>>> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); >>>> + >>>> + ret = resizer->setFormat(1, &ispFormat); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); >>>> + >>>> + V4L2DeviceFormat outputFormat = {}; >>>> + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); >>>> + outputFormat.size = cfg.size; >>>> + outputFormat.planesCount = 2; >>> >>> Doesn't the planes count depend on the format ? >> >> Good catch. This is a preexisting bug in the pipeline. I will add a >> separate patch to this series to sort this out. >> >>> >>>> + >>>> + ret = video->setFormat(&outputFormat); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (outputFormat.size != cfg.size || >>>> + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { >>>> + LOG(RkISP1, Error) >>>> + << "Unable to configure capture in " << cfg.toString(); >>>> + return -EINVAL; >>>> + } >>>> } >>>> >>>> V4L2DeviceFormat paramFormat = {}; >>>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>>> if (ret) >>>> return ret; >>>> >>>> - cfg.setStream(&data->mainPathStream_); >>>> - >>>> return 0; >>>> } >>>> >>>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) >>>> { >>>> + RkISP1CameraData *data = cameraData(camera); >>>> unsigned int count = stream->configuration().bufferCount; >>>> - return mainPathVideo_->exportBuffers(count, buffers); >>>> + >>>> + if (stream == &data->mainPathStream_) >>>> + return mainPathVideo_->exportBuffers(count, buffers); >>>> + >>>> + if (stream == &data->selfPathStream_) >>>> + return selfPathVideo_->exportBuffers(count, buffers); >>>> + >>>> + return -EINVAL; >>>> } >>>> >>>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) >>>> { >>>> RkISP1CameraData *data = cameraData(camera); >>>> - unsigned int count = data->mainPathStream_.configuration().bufferCount; >>>> unsigned int ipaBufferId = 1; >>>> int ret; >>>> >>>> - ret = mainPathVideo_->importBuffers(count); >>>> - if (ret < 0) >>>> - goto error; >>>> + unsigned int count = std::max({ >>>> + data->mainPathStream_.configuration().bufferCount, >>>> + data->selfPathStream_.configuration().bufferCount, >>>> + }); >>>> + >>>> + if (data->mainPathActive_) { >>>> + ret = mainPathVideo_->importBuffers(count); >>>> + if (ret < 0) >>>> + goto error; >>> >>> Is it safe to call releaseBuffers if importing fails ? >> >> Yes, is it not always safe to call releaseBuffers? The idea is that if >> allocateBuffers() fails it should undo all operations. Remember we >> depend on buffer orphaning so buffers allocated by for example the >> FrameBufferAllocater are still OK. >> >>> >>>> + } >>>> + >>>> + if (data->selfPathActive_) { >>>> + ret = selfPathVideo_->importBuffers(count); >>>> + if (ret < 0) >>>> + goto error; >>>> + } >>>> >>>> ret = param_->allocateBuffers(count, ¶mBuffers_); >>>> if (ret < 0) >>>> @@ -754,6 +794,7 @@ error: >>>> paramBuffers_.clear(); >>>> statBuffers_.clear(); >>>> mainPathVideo_->releaseBuffers(); >>>> + selfPathVideo_->releaseBuffers(); >>>> >>>> return ret; >>>> } >>>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) >>>> if (mainPathVideo_->releaseBuffers()) >>>> LOG(RkISP1, Error) << "Failed to release main path buffers"; >>>> >>>> + if (selfPathVideo_->releaseBuffers()) >>>> + LOG(RkISP1, Error) << "Failed to release self path buffers"; >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) >>>> return ret; >>>> } >>>> >>>> - ret = mainPathVideo_->streamOn(); >>>> - if (ret) { >>>> - param_->streamOff(); >>>> - stat_->streamOff(); >>>> - data->ipa_->stop(); >>>> - freeBuffers(camera); >>>> - >>>> - LOG(RkISP1, Error) >>>> - << "Failed to start camera " << camera->id(); >>>> + std::map<unsigned int, IPAStream> streamConfig; >>>> + >>>> + if (data->mainPathActive_) { >>>> + ret = mainPathVideo_->streamOn(); >>>> + if (ret) { >>>> + param_->streamOff(); >>>> + stat_->streamOff(); >>>> + data->ipa_->stop(); >>>> + freeBuffers(camera); >>>> + >>>> + LOG(RkISP1, Error) >>>> + << "Failed to start main path " << camera->id(); >>>> + return ret; >>>> + } >>>> + >>>> + streamConfig[0] = { >>>> + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, >>>> + .size = data->mainPathStream_.configuration().size, >>>> + }; >>> >>> What's this for ? And why assume [0] is assigned to main path ? >> >> This is part of the yet undocumented IPA protocl for the RkISP1. The >> stream configuration map is mainpath = 0 and selfpath = 1. When we get >> the new IPC framework in place all of this should of course be >> documented. In reality this is have currently no impact as the RkISP1 >> IPA does not examine the stream configuration at all. >> >>> >>>> + } >>>> + >>>> + if (data->selfPathActive_) { >>>> + ret = selfPathVideo_->streamOn(); >>>> + if (ret) { >>>> + if (data->mainPathActive_) >>>> + mainPathVideo_->streamOff(); >>>> + >>>> + param_->streamOff(); >>>> + stat_->streamOff(); >>>> + data->ipa_->stop(); >>>> + freeBuffers(camera); >>>> + >>>> + LOG(RkISP1, Error) >>>> + << "Failed to start self path " << camera->id(); >>>> + return ret; >>>> + } >>>> + >>>> + streamConfig[1] = { >>>> + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, >>>> + .size = data->selfPathStream_.configuration().size, >>>> + }; >>>> } >>>> >>>> activeCamera_ = camera; >>>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) >>>> ret = 0; >>>> } >>>> >>>> - std::map<unsigned int, IPAStream> streamConfig; >>>> - streamConfig[0] = { >>>> - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, >>>> - .size = data->mainPathStream_.configuration().size, >>>> - }; >>>> - >>>> std::map<unsigned int, const ControlInfoMap &> entityControls; >>>> entityControls.emplace(0, data->sensor_->controls()); >>>> >>>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) >>>> RkISP1CameraData *data = cameraData(camera); >>>> int ret; >>>> >>>> - ret = mainPathVideo_->streamOff(); >>>> - if (ret) >>>> - LOG(RkISP1, Warning) >>>> - << "Failed to stop camera " << camera->id(); >>>> + if (data->selfPathActive_) { >>>> + ret = selfPathVideo_->streamOff(); >>>> + if (ret) >>>> + LOG(RkISP1, Warning) >>>> + << "Failed to stop self path " << camera->id(); >>>> + } >>> >>> Can we call streamOff unconditionally ? It depends on the driver >>> implementation I guess >> >> We can call it unconditionally, I think even v4l2-compliance tests >> multiple s_stream(1) and s_stream(0) calls does it not? But I agree >> there might of course be driver specific quirks. >> >>> >>> Overall the patch looks good! >>> Thanks >>> j >>> >>>> + >>>> + if (data->mainPathActive_) { >>>> + ret = mainPathVideo_->streamOff(); >>>> + if (ret) >>>> + LOG(RkISP1, Warning) >>>> + << "Failed to stop main path " << camera->id(); >>>> + } >>>> >>>> ret = stat_->streamOff(); >>>> if (ret) >>>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, >>>> std::string resizer; >>>> if (cfg.stream() == &data->mainPathStream_) >>>> resizer = "rkisp1_resizer_mainpath"; >>>> + else if (cfg.stream() == &data->selfPathStream_) >>>> + resizer = "rkisp1_resizer_selfpath"; >>>> else >>>> return -EINVAL; >>>> >>>> -- >>>> 2.28.0 >>>> >>>> _______________________________________________ >>>> libcamera-devel mailing list >>>> libcamera-devel@lists.libcamera.org >>>> https://lists.libcamera.org/listinfo/libcamera-devel >> >
Hi Kieran, On 2020-09-14 12:19:54 +0100, Kieran Bingham wrote: > Hi Niklas, > > On 14/09/2020 12:09, Niklas Söderlund wrote: > > Hi Jacopo, > > > > On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote: > >> Hi Niklas, > >> > >> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote: > >>> Allow for both the main and self path streams to be configured. This > >>> change adds the self path as an internal stream to the pipeline handler. > >>> It is not exposed as a Camera stream so it can not yet be used. > >>> > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> --- > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- > >>> 1 file changed, 139 insertions(+), 58 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> index 132878c13b10b555..671098e11a2e2750 100644 > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> @@ -133,7 +133,8 @@ public: > >>> V4L2VideoDevice *selfPathVideo) > >>> : CameraData(pipe), sensor_(nullptr), frame_(0), > >>> frameInfo_(pipe), mainPathVideo_(mainPathVideo), > >>> - selfPathVideo_(selfPathVideo) > >>> + selfPathVideo_(selfPathVideo), mainPathActive_(false), > >>> + selfPathActive_(false) > >>> { > >>> } > >>> > >>> @@ -145,6 +146,7 @@ public: > >>> int loadIPA(); > >>> > >>> Stream mainPathStream_; > >>> + Stream selfPathStream_; > >> > >> Would this, and all other duplicated fields be nicer is stored as > >> arrays and accessed with fixed indexes ? I don't mind the way it is > >> today though > > > > I guess this is another bikeshedding topic. I don't see any clear > > advantage of one over the other. > > > > mainPathStream_ > > > > vs > > > > streams_[RkISP1::MainPAth] > > > > If we are to switch to an array approach I do think we should do so > > before or after this series. I picked this solution as it was the one > > already used in the pipeline. > > > Jumping into the bikeshed mostly because it really stood out in the > title of this and the preceding patch. > > The 'self' path sounds horrible. > I assume it derives from 'selfie' ... but really it's just a secondary ... > > > I would probably prefer the array route (can be refactored anytime, > doesn't have to be here). If one path is more feature full than the > other then perhaps naming them makes sense, but otherwise I'd just have > them as numerical pipelines. > > And rather than have an array of streams_ and an array of > V4L2VideoDevice and an array of Sensors etc... I think this can be > moddelled as a class, and then it's just a single array of that class. > > (Or if there's only two, It could actually be just two instances of that > class at that point) > > RkISPPipe main; > RkISPPipe secondary; This have been suggested to another patch in this series and I agree it's a good idea. I think that work should go on-top (or as a series before) tho as it will be quiet large in LoC and I don't wish to do that work in this already large series. > > > edit: ohh yuck. I see > > > > + selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath"); > > so it's named the 'self path' at the ISP ... I know :-( So I think we are stuck with the man and self path names. > > -- > Kieran > > > > >> > >>> CameraSensor *sensor_; > >>> unsigned int frame_; > >>> std::vector<IPABuffer> ipaBuffers_; > >>> @@ -154,6 +156,9 @@ public: > >>> V4L2VideoDevice *mainPathVideo_; > >>> V4L2VideoDevice *selfPathVideo_; > >>> > >>> + bool mainPathActive_; > >>> + bool selfPathActive_; > >>> + > >>> private: > >>> void queueFrameAction(unsigned int frame, > >>> const IPAOperationData &action); > >>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >>> RkISP1CameraConfiguration *config = > >>> static_cast<RkISP1CameraConfiguration *>(c); > >>> RkISP1CameraData *data = cameraData(camera); > >>> - StreamConfiguration &cfg = config->at(0); > >>> CameraSensor *sensor = data->sensor_; > >>> int ret; > >>> > >>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >>> > >>> LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); > >>> > >>> - ret = mainPathResizer_->setFormat(0, &format); > >>> - if (ret < 0) > >>> - return ret; > >>> - > >>> - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); > >>> - > >>> - format.size = cfg.size; > >>> - > >>> - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); > >>> - > >>> - ret = mainPathResizer_->setFormat(1, &format); > >>> - if (ret < 0) > >>> - return ret; > >>> - > >>> - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); > >>> - > >>> - V4L2DeviceFormat outputFormat = {}; > >>> - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); > >>> - outputFormat.size = cfg.size; > >>> - outputFormat.planesCount = 2; > >>> - > >>> - ret = mainPathVideo_->setFormat(&outputFormat); > >>> - if (ret) > >>> - return ret; > >>> - > >>> - if (outputFormat.size != cfg.size || > >>> - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { > >>> - LOG(RkISP1, Error) > >>> - << "Unable to configure capture in " << cfg.toString(); > >>> - return -EINVAL; > >>> + data->mainPathActive_ = false; > >>> + data->selfPathActive_ = false; > >>> + for (const StreamConfiguration &cfg : *config) { > >>> + V4L2SubdeviceFormat ispFormat = format; > >>> + V4L2Subdevice *resizer; > >>> + V4L2VideoDevice *video; > >>> + > >>> + if (cfg.stream() == &data->mainPathStream_) { > >>> + resizer = mainPathResizer_; > >>> + video = mainPathVideo_; > >>> + data->mainPathActive_ = true; > >>> + } else { > >>> + resizer = selfPathResizer_; > >>> + video = selfPathVideo_; > >>> + data->selfPathActive_ = true; > >>> + } > >>> + > >>> + ret = resizer->setFormat(0, &ispFormat); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); > >>> + > >>> + ispFormat.size = cfg.size; > >>> + > >>> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); > >>> + > >>> + ret = resizer->setFormat(1, &ispFormat); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); > >>> + > >>> + V4L2DeviceFormat outputFormat = {}; > >>> + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); > >>> + outputFormat.size = cfg.size; > >>> + outputFormat.planesCount = 2; > >> > >> Doesn't the planes count depend on the format ? > > > > Good catch. This is a preexisting bug in the pipeline. I will add a > > separate patch to this series to sort this out. > > > >> > >>> + > >>> + ret = video->setFormat(&outputFormat); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + if (outputFormat.size != cfg.size || > >>> + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { > >>> + LOG(RkISP1, Error) > >>> + << "Unable to configure capture in " << cfg.toString(); > >>> + return -EINVAL; > >>> + } > >>> } > >>> > >>> V4L2DeviceFormat paramFormat = {}; > >>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > >>> if (ret) > >>> return ret; > >>> > >>> - cfg.setStream(&data->mainPathStream_); > >>> - > >>> return 0; > >>> } > >>> > >>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, > >>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) > >>> { > >>> + RkISP1CameraData *data = cameraData(camera); > >>> unsigned int count = stream->configuration().bufferCount; > >>> - return mainPathVideo_->exportBuffers(count, buffers); > >>> + > >>> + if (stream == &data->mainPathStream_) > >>> + return mainPathVideo_->exportBuffers(count, buffers); > >>> + > >>> + if (stream == &data->selfPathStream_) > >>> + return selfPathVideo_->exportBuffers(count, buffers); > >>> + > >>> + return -EINVAL; > >>> } > >>> > >>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > >>> { > >>> RkISP1CameraData *data = cameraData(camera); > >>> - unsigned int count = data->mainPathStream_.configuration().bufferCount; > >>> unsigned int ipaBufferId = 1; > >>> int ret; > >>> > >>> - ret = mainPathVideo_->importBuffers(count); > >>> - if (ret < 0) > >>> - goto error; > >>> + unsigned int count = std::max({ > >>> + data->mainPathStream_.configuration().bufferCount, > >>> + data->selfPathStream_.configuration().bufferCount, > >>> + }); > >>> + > >>> + if (data->mainPathActive_) { > >>> + ret = mainPathVideo_->importBuffers(count); > >>> + if (ret < 0) > >>> + goto error; > >> > >> Is it safe to call releaseBuffers if importing fails ? > > > > Yes, is it not always safe to call releaseBuffers? The idea is that if > > allocateBuffers() fails it should undo all operations. Remember we > > depend on buffer orphaning so buffers allocated by for example the > > FrameBufferAllocater are still OK. > > > >> > >>> + } > >>> + > >>> + if (data->selfPathActive_) { > >>> + ret = selfPathVideo_->importBuffers(count); > >>> + if (ret < 0) > >>> + goto error; > >>> + } > >>> > >>> ret = param_->allocateBuffers(count, ¶mBuffers_); > >>> if (ret < 0) > >>> @@ -754,6 +794,7 @@ error: > >>> paramBuffers_.clear(); > >>> statBuffers_.clear(); > >>> mainPathVideo_->releaseBuffers(); > >>> + selfPathVideo_->releaseBuffers(); > >>> > >>> return ret; > >>> } > >>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) > >>> if (mainPathVideo_->releaseBuffers()) > >>> LOG(RkISP1, Error) << "Failed to release main path buffers"; > >>> > >>> + if (selfPathVideo_->releaseBuffers()) > >>> + LOG(RkISP1, Error) << "Failed to release self path buffers"; > >>> + > >>> return 0; > >>> } > >>> > >>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) > >>> return ret; > >>> } > >>> > >>> - ret = mainPathVideo_->streamOn(); > >>> - if (ret) { > >>> - param_->streamOff(); > >>> - stat_->streamOff(); > >>> - data->ipa_->stop(); > >>> - freeBuffers(camera); > >>> - > >>> - LOG(RkISP1, Error) > >>> - << "Failed to start camera " << camera->id(); > >>> + std::map<unsigned int, IPAStream> streamConfig; > >>> + > >>> + if (data->mainPathActive_) { > >>> + ret = mainPathVideo_->streamOn(); > >>> + if (ret) { > >>> + param_->streamOff(); > >>> + stat_->streamOff(); > >>> + data->ipa_->stop(); > >>> + freeBuffers(camera); > >>> + > >>> + LOG(RkISP1, Error) > >>> + << "Failed to start main path " << camera->id(); > >>> + return ret; > >>> + } > >>> + > >>> + streamConfig[0] = { > >>> + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > >>> + .size = data->mainPathStream_.configuration().size, > >>> + }; > >> > >> What's this for ? And why assume [0] is assigned to main path ? > > > > This is part of the yet undocumented IPA protocl for the RkISP1. The > > stream configuration map is mainpath = 0 and selfpath = 1. When we get > > the new IPC framework in place all of this should of course be > > documented. In reality this is have currently no impact as the RkISP1 > > IPA does not examine the stream configuration at all. > > > >> > >>> + } > >>> + > >>> + if (data->selfPathActive_) { > >>> + ret = selfPathVideo_->streamOn(); > >>> + if (ret) { > >>> + if (data->mainPathActive_) > >>> + mainPathVideo_->streamOff(); > >>> + > >>> + param_->streamOff(); > >>> + stat_->streamOff(); > >>> + data->ipa_->stop(); > >>> + freeBuffers(camera); > >>> + > >>> + LOG(RkISP1, Error) > >>> + << "Failed to start self path " << camera->id(); > >>> + return ret; > >>> + } > >>> + > >>> + streamConfig[1] = { > >>> + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > >>> + .size = data->selfPathStream_.configuration().size, > >>> + }; > >>> } > >>> > >>> activeCamera_ = camera; > >>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) > >>> ret = 0; > >>> } > >>> > >>> - std::map<unsigned int, IPAStream> streamConfig; > >>> - streamConfig[0] = { > >>> - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > >>> - .size = data->mainPathStream_.configuration().size, > >>> - }; > >>> - > >>> std::map<unsigned int, const ControlInfoMap &> entityControls; > >>> entityControls.emplace(0, data->sensor_->controls()); > >>> > >>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > >>> RkISP1CameraData *data = cameraData(camera); > >>> int ret; > >>> > >>> - ret = mainPathVideo_->streamOff(); > >>> - if (ret) > >>> - LOG(RkISP1, Warning) > >>> - << "Failed to stop camera " << camera->id(); > >>> + if (data->selfPathActive_) { > >>> + ret = selfPathVideo_->streamOff(); > >>> + if (ret) > >>> + LOG(RkISP1, Warning) > >>> + << "Failed to stop self path " << camera->id(); > >>> + } > >> > >> Can we call streamOff unconditionally ? It depends on the driver > >> implementation I guess > > > > We can call it unconditionally, I think even v4l2-compliance tests > > multiple s_stream(1) and s_stream(0) calls does it not? But I agree > > there might of course be driver specific quirks. > > > >> > >> Overall the patch looks good! > >> Thanks > >> j > >> > >>> + > >>> + if (data->mainPathActive_) { > >>> + ret = mainPathVideo_->streamOff(); > >>> + if (ret) > >>> + LOG(RkISP1, Warning) > >>> + << "Failed to stop main path " << camera->id(); > >>> + } > >>> > >>> ret = stat_->streamOff(); > >>> if (ret) > >>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, > >>> std::string resizer; > >>> if (cfg.stream() == &data->mainPathStream_) > >>> resizer = "rkisp1_resizer_mainpath"; > >>> + else if (cfg.stream() == &data->selfPathStream_) > >>> + resizer = "rkisp1_resizer_selfpath"; > >>> else > >>> return -EINVAL; > >>> > >>> -- > >>> 2.28.0 > >>> > >>> _______________________________________________ > >>> libcamera-devel mailing list > >>> libcamera-devel@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 132878c13b10b555..671098e11a2e2750 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -133,7 +133,8 @@ public: V4L2VideoDevice *selfPathVideo) : CameraData(pipe), sensor_(nullptr), frame_(0), frameInfo_(pipe), mainPathVideo_(mainPathVideo), - selfPathVideo_(selfPathVideo) + selfPathVideo_(selfPathVideo), mainPathActive_(false), + selfPathActive_(false) { } @@ -145,6 +146,7 @@ public: int loadIPA(); Stream mainPathStream_; + Stream selfPathStream_; CameraSensor *sensor_; unsigned int frame_; std::vector<IPABuffer> ipaBuffers_; @@ -154,6 +156,9 @@ public: V4L2VideoDevice *mainPathVideo_; V4L2VideoDevice *selfPathVideo_; + bool mainPathActive_; + bool selfPathActive_; + private: void queueFrameAction(unsigned int frame, const IPAOperationData &action); @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) RkISP1CameraConfiguration *config = static_cast<RkISP1CameraConfiguration *>(c); RkISP1CameraData *data = cameraData(camera); - StreamConfiguration &cfg = config->at(0); CameraSensor *sensor = data->sensor_; int ret; @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString(); - ret = mainPathResizer_->setFormat(0, &format); - if (ret < 0) - return ret; - - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString(); - - format.size = cfg.size; - - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString(); - - ret = mainPathResizer_->setFormat(1, &format); - if (ret < 0) - return ret; - - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString(); - - V4L2DeviceFormat outputFormat = {}; - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat); - outputFormat.size = cfg.size; - outputFormat.planesCount = 2; - - ret = mainPathVideo_->setFormat(&outputFormat); - if (ret) - return ret; - - if (outputFormat.size != cfg.size || - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) { - LOG(RkISP1, Error) - << "Unable to configure capture in " << cfg.toString(); - return -EINVAL; + data->mainPathActive_ = false; + data->selfPathActive_ = false; + for (const StreamConfiguration &cfg : *config) { + V4L2SubdeviceFormat ispFormat = format; + V4L2Subdevice *resizer; + V4L2VideoDevice *video; + + if (cfg.stream() == &data->mainPathStream_) { + resizer = mainPathResizer_; + video = mainPathVideo_; + data->mainPathActive_ = true; + } else { + resizer = selfPathResizer_; + video = selfPathVideo_; + data->selfPathActive_ = true; + } + + ret = resizer->setFormat(0, &ispFormat); + if (ret < 0) + return ret; + + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString(); + + ispFormat.size = cfg.size; + + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString(); + + ret = resizer->setFormat(1, &ispFormat); + if (ret < 0) + return ret; + + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString(); + + V4L2DeviceFormat outputFormat = {}; + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat); + outputFormat.size = cfg.size; + outputFormat.planesCount = 2; + + ret = video->setFormat(&outputFormat); + if (ret) + return ret; + + if (outputFormat.size != cfg.size || + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) { + LOG(RkISP1, Error) + << "Unable to configure capture in " << cfg.toString(); + return -EINVAL; + } } V4L2DeviceFormat paramFormat = {}; @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret) return ret; - cfg.setStream(&data->mainPathStream_); - return 0; } int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers) { + RkISP1CameraData *data = cameraData(camera); unsigned int count = stream->configuration().bufferCount; - return mainPathVideo_->exportBuffers(count, buffers); + + if (stream == &data->mainPathStream_) + return mainPathVideo_->exportBuffers(count, buffers); + + if (stream == &data->selfPathStream_) + return selfPathVideo_->exportBuffers(count, buffers); + + return -EINVAL; } int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) { RkISP1CameraData *data = cameraData(camera); - unsigned int count = data->mainPathStream_.configuration().bufferCount; unsigned int ipaBufferId = 1; int ret; - ret = mainPathVideo_->importBuffers(count); - if (ret < 0) - goto error; + unsigned int count = std::max({ + data->mainPathStream_.configuration().bufferCount, + data->selfPathStream_.configuration().bufferCount, + }); + + if (data->mainPathActive_) { + ret = mainPathVideo_->importBuffers(count); + if (ret < 0) + goto error; + } + + if (data->selfPathActive_) { + ret = selfPathVideo_->importBuffers(count); + if (ret < 0) + goto error; + } ret = param_->allocateBuffers(count, ¶mBuffers_); if (ret < 0) @@ -754,6 +794,7 @@ error: paramBuffers_.clear(); statBuffers_.clear(); mainPathVideo_->releaseBuffers(); + selfPathVideo_->releaseBuffers(); return ret; } @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera) if (mainPathVideo_->releaseBuffers()) LOG(RkISP1, Error) << "Failed to release main path buffers"; + if (selfPathVideo_->releaseBuffers()) + LOG(RkISP1, Error) << "Failed to release self path buffers"; + return 0; } @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera) return ret; } - ret = mainPathVideo_->streamOn(); - if (ret) { - param_->streamOff(); - stat_->streamOff(); - data->ipa_->stop(); - freeBuffers(camera); - - LOG(RkISP1, Error) - << "Failed to start camera " << camera->id(); + std::map<unsigned int, IPAStream> streamConfig; + + if (data->mainPathActive_) { + ret = mainPathVideo_->streamOn(); + if (ret) { + param_->streamOff(); + stat_->streamOff(); + data->ipa_->stop(); + freeBuffers(camera); + + LOG(RkISP1, Error) + << "Failed to start main path " << camera->id(); + return ret; + } + + streamConfig[0] = { + .pixelFormat = data->mainPathStream_.configuration().pixelFormat, + .size = data->mainPathStream_.configuration().size, + }; + } + + if (data->selfPathActive_) { + ret = selfPathVideo_->streamOn(); + if (ret) { + if (data->mainPathActive_) + mainPathVideo_->streamOff(); + + param_->streamOff(); + stat_->streamOff(); + data->ipa_->stop(); + freeBuffers(camera); + + LOG(RkISP1, Error) + << "Failed to start self path " << camera->id(); + return ret; + } + + streamConfig[1] = { + .pixelFormat = data->selfPathStream_.configuration().pixelFormat, + .size = data->selfPathStream_.configuration().size, + }; } activeCamera_ = camera; @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera) ret = 0; } - std::map<unsigned int, IPAStream> streamConfig; - streamConfig[0] = { - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, - .size = data->mainPathStream_.configuration().size, - }; - std::map<unsigned int, const ControlInfoMap &> entityControls; entityControls.emplace(0, data->sensor_->controls()); @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera) RkISP1CameraData *data = cameraData(camera); int ret; - ret = mainPathVideo_->streamOff(); - if (ret) - LOG(RkISP1, Warning) - << "Failed to stop camera " << camera->id(); + if (data->selfPathActive_) { + ret = selfPathVideo_->streamOff(); + if (ret) + LOG(RkISP1, Warning) + << "Failed to stop self path " << camera->id(); + } + + if (data->mainPathActive_) { + ret = mainPathVideo_->streamOff(); + if (ret) + LOG(RkISP1, Warning) + << "Failed to stop main path " << camera->id(); + } ret = stat_->streamOff(); if (ret) @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera, std::string resizer; if (cfg.stream() == &data->mainPathStream_) resizer = "rkisp1_resizer_mainpath"; + else if (cfg.stream() == &data->selfPathStream_) + resizer = "rkisp1_resizer_selfpath"; else return -EINVAL;
Allow for both the main and self path streams to be configured. This change adds the self path as an internal stream to the pipeline handler. It is not exposed as a Camera stream so it can not yet be used. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++------- 1 file changed, 139 insertions(+), 58 deletions(-)