[libcamera-devel] [PATCH v3] pipeline: rkisp1: Support devices without self path
Jacopo Mondi
jacopo at jmondi.org
Wed Jul 20 11:07:04 CEST 2022
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'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.
> > > 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