| Message ID | 20190930053520.2711-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
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@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?). 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thanks for your work. On 2019-09-30 01:35:18 -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@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)); > + } > + > + 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__ > + > +#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) })); Can't we get these IDs from the kernel? They must be described for the ipu3 kernel module right? > > } /* 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Thu, Oct 03, 2019 at 09:59:36PM +0200, Niklas Söderlund wrote: > Hi Paul, > > Thanks for your work. > > On 2019-09-30 01:35:18 -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@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)); > > + } > > + > > + 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__ > > + > > +#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) })); > > Can't we get these IDs from the kernel? They must be described for the > ipu3 kernel module right? I'm not sure. We can't get it from sysfs, since we're trying to specify what we're looking for in sysfs. There are no headers that define IMGU's, but there is one for CIO2. Thanks, Paul > > > > } /* 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Jacopo, Thank you for the review. 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@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? It's not different. That's exactly what I was trying to achieve and getting stuck. > > + > > + 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. Okay :/ ...PPP03? Thanks, Paul > > + > > +#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?). > > 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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)); + } + + 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__ + +#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) })); } /* 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
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@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(-)