[libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices without self path
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 20 10:53:56 CEST 2022
Hi Jacopo,
On Wed, Jul 20, 2022 at 10:36:57AM +0200, Jacopo Mondi via libcamera-devel wrote:
> 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.
I thought about this, but didn't check the IPU3 implementation when
reviewing this patch. The generateConfiguration() documentation states
* \brief Generate a default camera configuration according to stream roles
* \param[in] roles A list of stream roles
*
* Generate a camera configuration for a set of desired stream roles. The caller
* specifies a list of stream roles and the camera returns a configuration
* containing suitable streams and their suggested default configurations. An
* empty list of roles is valid, and will generate an empty configuration that
* can be filled by the caller.
*
* \context This function is \threadsafe.
*
* \return A CameraConfiguration if the requested roles can be satisfied, or a
* null pointer otherwise. The ownership of the returned configuration is
* passed to the caller.
The expected behaviour isn't very explicitly documented, but the
documentation of the return value hints that if the requested roles
can't be satisfied, nullptr should be returned. Maybe we should modify
the IPU3 pipeline handler to match this ? Or modify the documentation ?
> > 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 ?
I think I commented in the review of the previous version that those two
cases should never happen :-) I don't mind keeping the checks, it's up
to you and Paul.
> > 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.
True, but I don't think there's a risk of a false positive match here
when removing the self path.
> > 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);
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list