[libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices without self path
Jacopo Mondi
jacopo at jmondi.org
Wed Jul 20 10:36:57 CEST 2022
Hi Paul,
On Tue, Jul 19, 2022 at 04:30:12PM +0900, Paul Elder via libcamera-devel wrote:
> Some hardware supported by the rkisp1 driver, such as the ISP in the
> i.MX8MP, don't have a self path. Although at the moment the driver still
> exposes the self path, prepare the rkisp1 pipeline handler for when the
> self path will be removed for devices that don't support it.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - add more guards
> - add pathCount guard to generateConfiguration to allow making self path
> unavailable Just Work
>
> Changes in v2:
> - simplify matching
> - clean up self path availability in validate()
> - fix configure(), return -ENODEV if multiple roles but no self path
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 49 ++++++++++++++++--------
> 1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 212fc76a..99eecd44 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -180,6 +180,8 @@ private:
> std::unique_ptr<V4L2VideoDevice> stat_;
> std::unique_ptr<V4L2Subdevice> csi_;
>
> + bool hasSelfPath_;
> +
> RkISP1MainPath mainPath_;
> RkISP1SelfPath selfPath_;
>
> @@ -364,7 +366,7 @@ void RkISP1CameraData::paramFilled(unsigned int frame)
> if (info->mainPathBuffer)
> mainPath_->queueBuffer(info->mainPathBuffer);
>
> - if (info->selfPathBuffer)
> + if (selfPath_ && info->selfPathBuffer)
> selfPath_->queueBuffer(info->selfPathBuffer);
> }
>
> @@ -403,7 +405,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> return false;
>
> config = cfg;
> - if (data_->selfPath_->validate(&config) != Valid)
> + if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid)
> return false;
>
> return true;
> @@ -412,6 +414,7 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> {
> const CameraSensor *sensor = data_->sensor_.get();
> + unsigned int pathCount = data_->selfPath_ ? 2 : 1;
> Status status = Valid;
>
> if (config_.empty())
> @@ -423,8 +426,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> }
>
> /* Cap the number of entries to the available streams. */
> - if (config_.size() > 2) {
> - config_.resize(2);
> + if (config_.size() > pathCount) {
> + config_.resize(pathCount);
> status = Adjusted;
> }
>
> @@ -441,7 +444,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> std::reverse(order.begin(), order.end());
>
> bool mainPathAvailable = true;
> - bool selfPathAvailable = true;
> + bool selfPathAvailable = data_->selfPath_;
> for (unsigned int index : order) {
> StreamConfiguration &cfg = config_[index];
>
> @@ -520,7 +523,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> }
>
> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> - : PipelineHandler(manager)
> + : PipelineHandler(manager), hasSelfPath_(true)
> {
> }
>
> @@ -532,12 +535,19 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> const StreamRoles &roles)
> {
> RkISP1CameraData *data = cameraData(camera);
> +
> + unsigned int pathCount = data->selfPath_ ? 2 : 1;
> + if (roles.size() > pathCount) {
> + LOG(RkISP1, Error) << "Too many stream roles requested";
> + return nullptr;
> + }
> +
If I'm not mistaken, the IPU3 works differently. It accepts all roles,
and then lets validate() to shrink it down to the number of actually
supported ones.
> CameraConfiguration *config = new RkISP1CameraConfiguration(camera, data);
> if (roles.empty())
> return config;
>
> bool mainPathAvailable = true;
> - bool selfPathAvailable = true;
> + bool selfPathAvailable = data->selfPath_;
> for (const StreamRole role : roles) {
> bool useMainPath;
>
> @@ -646,10 +656,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> ret = mainPath_.configure(cfg, format);
> streamConfig[0] = IPAStream(cfg.pixelFormat,
> cfg.size);
> - } else {
> + } else if (hasSelfPath_) {
> ret = selfPath_.configure(cfg, format);
> streamConfig[1] = IPAStream(cfg.pixelFormat,
> cfg.size);
> + } else {
> + return -ENODEV;
> }
This -shouldn't- be necessary. Configurations are validated before
being passed to Camera::configure() and the number of streams has
already been reduced to 1 if !selfPath, and the only stream available
should be the mainPath one.
>
> if (ret)
> @@ -697,7 +709,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, S
>
> if (stream == &data->mainPathStream_)
> return mainPath_.exportBuffers(count, buffers);
> - else if (stream == &data->selfPathStream_)
> + else if (hasSelfPath_ && stream == &data->selfPathStream_)
> return selfPath_.exportBuffers(count, buffers);
>
> return -EINVAL;
> @@ -826,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> }
> }
>
> - if (data->selfPath_->isEnabled()) {
> + if (hasSelfPath_ && data->selfPath_->isEnabled()) {
Same reasoning, a validate configuration shouldn't have selfPath_
enabled ? Have I missed how this can happen ?
> ret = selfPath_.start();
> if (ret) {
> mainPath_.stop();
> @@ -853,7 +865,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>
> data->ipa_->stop();
>
> - selfPath_.stop();
> + if (hasSelfPath_)
> + selfPath_.stop();
> mainPath_.stop();
>
> ret = stat_->streamOff();
> @@ -934,7 +947,7 @@ int PipelineHandlerRkISP1::initLinks(Camera *camera,
> for (const StreamConfiguration &cfg : config) {
> if (cfg.stream() == &data->mainPathStream_)
> ret = data->mainPath_->setEnabled(true);
> - else if (cfg.stream() == &data->selfPathStream_)
> + else if (hasSelfPath_ && cfg.stream() == &data->selfPathStream_)
> ret = data->selfPath_->setEnabled(true);
> else
> return -EINVAL;
> @@ -951,7 +964,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> int ret;
>
> std::unique_ptr<RkISP1CameraData> data =
> - std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);
> + std::make_unique<RkISP1CameraData>(this, &mainPath_,
> + hasSelfPath_ ? &selfPath_ : nullptr);
>
> ControlInfoMap::Map ctrls;
> ctrls.emplace(std::piecewise_construct,
> @@ -1007,9 +1021,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>
> DeviceMatch dm("rkisp1");
> dm.add("rkisp1_isp");
> - dm.add("rkisp1_resizer_selfpath");
> dm.add("rkisp1_resizer_mainpath");
> - dm.add("rkisp1_selfpath");
It's a shame we can't verify in advance if the platform has a
self-path and add the above entities to the DeviceMatch conditionally.
> dm.add("rkisp1_mainpath");
> dm.add("rkisp1_stats");
> dm.add("rkisp1_params");
> @@ -1024,6 +1036,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> return false;
> }
>
> + hasSelfPath_ = !!media_->getEntityByName("rkisp1_selfpath");
> +
> /* Create the V4L2 subdevices we will need. */
> isp_ = V4L2Subdevice::fromEntityName(media_, "rkisp1_isp");
> if (isp_->open() < 0)
> @@ -1058,11 +1072,12 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> if (!mainPath_.init(media_))
> return false;
>
> - if (!selfPath_.init(media_))
> + if (hasSelfPath_ && !selfPath_.init(media_))
> return false;
>
> mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> - selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> + if (hasSelfPath_)
> + selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>
> --
> 2.30.2
>
More information about the libcamera-devel
mailing list