[libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices without self path
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Jul 20 11:19:51 CEST 2022
Hi,
On Wed, Jul 20, 2022 at 11:07:04AM +0200, Jacopo Mondi wrote:
> Hi Laurent
>
> On Wed, Jul 20, 2022 at 11:53:56AM +0300, Laurent Pinchart wrote:
> > 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 ?
> >
>
> FYI raspberrypi and simple behave like IPU3, if I'm not mistaken.
I looked at raspberrypi while doing this. They return invalid/nullptr if
more streams than supported are requested ((rawCount > 1 || outCount > 2)),
so this was meant to be analogous to that.
> I'm debated, I like the idea of returning a valid configuration, but
> the Adjusted flag can be easily ignored and the application might find
> itself dealing with something it doesn't expect.
>
> > > > 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.
> >
>
> Don't know, I don't feel strong on this. Generally avoiding testing
> for conditions known to be false could save the reader from wondering why
> the check is there and chase a code path that doesn't exist.
>
> But this is minimal so I'll let Paul decide.
I was just being extra safe...
Plus it's pushed anyway soooo...
Thanks,
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.
> >
>
> Yeah, and unless we introduce something similar to
> device_get_match_data() where to hardcode that information there's no
> way we can know that in advance.
>
> Thanks
> j
>
> > > > 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