[libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add device IDs
Paul Elder
paul.elder at ideasonboard.com
Mon Oct 7 07:59:27 CEST 2019
Hi Jacopo and Niklas,
Thank you for the review.
On Wed, Oct 02, 2019 at 11:42:45AM +0200, Jacopo Mondi wrote:
> Hi Paul,
> small self reply
>
> On Tue, Oct 01, 2019 at 10:56:57AM +0200, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Mon, Sep 30, 2019 at 01:35:18AM -0400, Paul Elder wrote:
> > > Allow pipeline handlers to declare a list of camera device IDs that they
> > > support. This list will eventually be used to check against the list of
> > > camera devices that the system has, but have yet to be initialized by the
> > > kernel or by udev.
> > >
> > > PipelineHandlerFactory exposes a static method to retrieve the list of
> > > all such camera device IDs from all pipeline handlers.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > > src/libcamera/pipeline/vimc.cpp | 2 +-
> > > src/libcamera/pipeline_handler.cpp | 16 +++++++
> > > 6 files changed, 79 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > > index 1fdef9ce..f16f37ab 100644
> > > --- a/src/libcamera/include/pipeline_handler.h
> > > +++ b/src/libcamera/include/pipeline_handler.h
> > > @@ -30,6 +30,48 @@ class MediaDevice;
> > > class PipelineHandler;
> > > class Request;
> > >
> > > +class DeviceID
> > > +{
> > > +public:
> > > + DeviceID(std::string type)
> > > + : type_(type){};
> > > + ~DeviceID(){};
> > > +
> > > + template<class IDType>
> > > + bool compare(const DeviceID &id) const
> > > + {
> > > + return this->type_.compare(id.type_) ||
> > > + static_cast<const IDType *>(this)->compare(static_cast<const IDType &>(id));
> > > + }
> >
> > Why is this different from defining the base class compare() as virtual,
> > accepting a parameter to the base class in compareDevices() and call the
> > compare method on the thea first parameter ? If you pass there a PCIDeviceID *
> > (as a pointer to the DeviceID base class), the PCIDeviceID::compare()
> > operation, which overrides the base class virtual compare() one, will be
> > called. Isn't this wat you're trying to achieve here or I'm missing a
> > piece?
> >
> > > +
> > > + const std::string type_;
> > > +};
> > > +
> > > +class PCIDeviceID : public DeviceID
> > > +{
> > > +public:
> > > + PCIDeviceID(uint16_t vendor, uint16_t device)
> > > + : DeviceID("pci"), vendor_(vendor), device_(device){};
> > > + ~PCIDeviceID(){};
> > > +
> > > + bool compare(const PCIDeviceID &pci) const
> > > + {
> > > + uint32_t thisID = (vendor_ << 16) + device_;
> > > + uint32_t otherID = (pci.vendor_ << 16) + pci.device_;
> > > +
> > > + return thisID < otherID;
> > > + }
> > > +
> > > + const uint16_t vendor_;
> > > + const uint16_t device_;
> > > +};
> > > +
> > > +template<class IDType>
> > > +bool compareDevices(const DeviceID &id1, const DeviceID &id2)
> > > +{
> > > + return id1.compare<IDType>(id2);
> > > +}
> > > +
> > > class CameraData
> > > {
> > > public:
> > > @@ -117,6 +159,10 @@ public:
> > >
> > > static void registerType(PipelineHandlerFactory *factory);
> > > static std::vector<PipelineHandlerFactory *> &factories();
> > > + static std::vector<std::reference_wrapper<DeviceID>> &getDeviceIDs();
> > > +
> > > +protected:
> > > + virtual std::vector<DeviceID> &deviceIDs() = 0;
> > >
> > > private:
> > > virtual PipelineHandler *createInstance(CameraManager *manager) = 0;
> > > @@ -124,12 +170,22 @@ private:
> > > std::string name_;
> > > };
> > >
> > > -#define REGISTER_PIPELINE_HANDLER(handler) \
> > > +#define P99_PROTECT(...) __VA_ARGS__
> >
> > I see where it comes from, but P99 refers to C99. If we like to keep
> > this in the macro, please consider changing its name.
> > > +
> > > +#define REGISTER_PIPELINE_HANDLER(handler, cameras) \
> > > class handler##Factory final : public PipelineHandlerFactory \
> > > { \
> > > public: \
> > > handler##Factory() : PipelineHandlerFactory(#handler) {} \
> > > \
> > > +protected: \
> > > + std::vector<DeviceID> &deviceIDs() \
> > > + { \
> > > + static std::vector<DeviceID> handler##Cameras = \
> > > + std::vector<DeviceID>(cameras); \
> > > + return handler##Cameras; \
> > > + } \
> > > + \
> > > private: \
> > > PipelineHandler *createInstance(CameraManager *manager) \
> > > { \
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 827906d5..562ee9fb 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)
> > > }
> > > }
> > >
> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,
> > > + P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),
> > > + PCIDeviceID(0x8086, 0x9D32) }));
> >
> > What do these IDs refers to ? I haven't found them in the Soraka ACPI
> > tables (probablt they're not specified there at all).
> >
> > Are those the IPU3 pipe instances (does the IPU3 firmware exposes two
> > PCI devices?).
> >
>
> One is the CIO2 and the other one is IMGU, according to the properties
> exposed in sysfs.
>
> While it guarantees the device nodes for those two entities have been
> created (are they permissions set?) this does not cover the sensors
> (and eventual lenses).
>
> Also, in the next patch you check for
> std::set_intersection(supportedDevices.begin(), supportedDevices.end(),
> availableDevices.begin(), availableDevices.end(),
> back_inserter(intersection),
> compareDevices<PCIDeviceID>);
>
> if (cm->cameras().size() < intersection.size()) {
> LOG(DeviceEnumerator, Warning) << "Not enough cameras!";
> return false;
> }
Pasting in what Niklas sent:
> To me this seems a bit too strict. Say that I have a Soraka device, run
> a kernel on it without IPU3 support, With this change libcamera would
> not function for a USB camera.
I think there's another problem that if you start up the Soraka with a
USB camera plugged in, libcamera would say that enough cameras have
started up and not wait for the IPU3.
> Also comparing against cm->cameras().size() might be skewed if you run
> on a kernel with vivid and/or vimc support built in.
That too.
Anyway, with this, I think we can see that we can't just compare the
number of expected cameras and actual cameras, and we have to actually
check a list of specific cameras. I think this calls for platform
configs :/
>
> Where cm->cameras() is populated by the pipeline handlers with
> registerCamera(), while the intersection would contain entries for
> CIO2 and IMGU. Am I wrong or are you comparing two different things
...I might be :S
> here (also, using Soraka as an example, if not CIO2 is registered, no
> camera would ever been created, as they depend on the presence of the
> the CIO2 subdevice nodes).
>
> If we're now only interested in platform PCI devices only, I would
> move this check to CameraManager::start(), before match() is called
> (as it attempts to create cameras and if no subdevice device node is
> registered, none will be created).
I'm not sure what you mean.
> Then, there is a different question that is how we deal with
> device-specific requirements: CIO2 and IMGU are platform properties,
> they are there for all IPU3 device, while the number of cameras
> depends on the number of sensor installed on the device. For Soraka
> there are two cameras, other IPU3 device might have just one. I fear
> we will need compatilble-like strings for pipeline handlers which
> contains device-specific information. In example, just thinking out
> loud:
>
> struct PipelineData sorakaCameraData = {
> .platform = {
> "0x8086, 0x1919",
> "0x8086, 0x9D32",
> },
> .sensors = {
> "ov5670 4-0036",
> "ov13858 2-0018",
> },
> };
> struct PipelineDeviceIds[] = {
> { .compatible = "ipu3,soraka", .data = &soraka_camera_data },
> };
>
> And you could match the "sensors" with /sys/class/video4linux/*/name
> If we have everything we need, then match() can be called.
We still need to call match() in CameraManager::start() though. The only
thing that cares if we don't have the expected cameras initialized is
the camera HAL; the rest of libcamera doesn't care if it expected some
cameras to be enumerated but weren't.
> Where to specify the compatible string "ipu3,soraka" is the question,
> as it might require a pipeline configuration file somewhere.
I think we will need it, though :/
> Just thinking out loud...
>
Thanks,
Paul
>
> > Thanks
> > j
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index de4ab523..85225f9b 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> > > completeRequest(activeCamera_, request);
> > > }
> > >
> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);
> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 89652105..376eb92a 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)
> > > pipe_->completeRequest(camera_, request);
> > > }
> > >
> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index f26a91f8..75d0236f 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)
> > > pipe_->completeRequest(camera_, request);
> > > }
> > >
> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 3e54aa23..9113bf18 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -593,6 +593,22 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories()
> > > return factories;
> > > }
> > >
> > > +std::vector<std::reference_wrapper<DeviceID>> &PipelineHandlerFactory::getDeviceIDs()
> > > +{
> > > + static std::vector<std::reference_wrapper<DeviceID>> devices;
> > > + if (!devices.empty())
> > > + return devices;
> > > +
> > > + std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> > > +
> > > + for (PipelineHandlerFactory *factory : factories) {
> > > + std::vector<DeviceID> &factoryDevices = factory->deviceIDs();
> > > + devices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());
> > > + }
> > > +
> > > + return devices;
> > > +}
> > > +
> > > /**
> > > * \fn PipelineHandlerFactory::createInstance()
> > > * \brief Create an instance of the PipelineHandler corresponding to the factory
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
More information about the libcamera-devel
mailing list