[libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add device IDs
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 2 11:42:45 CEST 2019
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;
}
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
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).
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.
Where to specify the compatible string "ipu3,soraka" is the question,
as it might require a pipeline configuration file somewhere.
Just thinking out loud...
Thanks
j
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191002/f26c7148/attachment.sig>
More information about the libcamera-devel
mailing list