[{"id":2739,"web_url":"https://patchwork.libcamera.org/comment/2739/","msgid":"<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","date":"2019-10-01T08:56:57","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Mon, Sep 30, 2019 at 01:35:18AM -0400, Paul Elder wrote:\n> Allow pipeline handlers to declare a list of camera device IDs that they\n> support. This list will eventually be used to check against the list of\n> camera devices that the system has, but have yet to be initialized by the\n> kernel or by udev.\n>\n> PipelineHandlerFactory exposes a static method to retrieve the list of\n> all such camera device IDs from all pipeline handlers.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 16 +++++++\n> 6 files changed, 79 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 1fdef9ce..f16f37ab 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -30,6 +30,48 @@ class MediaDevice;\n> class PipelineHandler;\n> class Request;\n>\n> +class DeviceID\n> +{\n> +public:\n> +\tDeviceID(std::string type)\n> +\t\t: type_(type){};\n> +\t~DeviceID(){};\n> +\n> +\ttemplate\n> +\tbool compare(const DeviceID &id) const\n> +\t{\n> +\t\treturn this->type_.compare(id.type_) ||\n> +\t\t static_cast(this)->compare(static_cast(id));\n> +\t}\n\nWhy is this different from defining the base class compare() as virtual,\naccepting a parameter to the base class in compareDevices() and call the\ncompare method on the thea first parameter ? If you pass there a PCIDeviceID *\n(as a pointer to the DeviceID base class), the PCIDeviceID::compare()\noperation, which overrides the base class virtual compare() one, will be\ncalled. Isn't this wat you're trying to achieve here or I'm missing a\npiece?\n\n> +\n> +\tconst std::string type_;\n> +};\n> +\n> +class PCIDeviceID : public DeviceID\n> +{\n> +public:\n> +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> +\t~PCIDeviceID(){};\n> +\n> +\tbool compare(const PCIDeviceID &pci) const\n> +\t{\n> +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> +\n> +\t\treturn thisID < otherID;\n> +\t}\n> +\n> +\tconst uint16_t vendor_;\n> +\tconst uint16_t device_;\n> +};\n> +\n> +template\n> +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> +{\n> +\treturn id1.compare(id2);\n> +}\n> +\n> class CameraData\n> {\n> public:\n> @@ -117,6 +159,10 @@ public:\n>\n> \tstatic void registerType(PipelineHandlerFactory *factory);\n> \tstatic std::vector &factories();\n> +\tstatic std::vector> &getDeviceIDs();\n> +\n> +protected:\n> +\tvirtual std::vector &deviceIDs() = 0;\n>\n> private:\n> \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> @@ -124,12 +170,22 @@ private:\n> \tstd::string name_;\n> };\n>\n> -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> +#define P99_PROTECT(...) __VA_ARGS__\n\nI see where it comes from, but P99 refers to C99. If we like to keep\nthis in the macro, please consider changing its name.\n> +\n> +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> public:\t\t\t\t\t\t\t\t\t\\\n> \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> \t\t\t\t\t\t\t\t\t\\\n> +protected:\t\t\t\t\t\t\t\t\\\n> +\tstd::vector &deviceIDs()\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\tstatic std::vector handler##Cameras =\t\t\\\n> +\t\t std::vector(cameras);\t\t\t\\\n> +\t\treturn handler##Cameras;\t\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> private:\t\t\t\t\t\t\t\t\\\n> \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> \t{\t\t\t\t\t\t\t\t\\\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 827906d5..562ee9fb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> \t}\n> }\n>\n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n\nWhat do these IDs refers to ? I haven't found them in the Soraka ACPI\ntables (probablt they're not specified there at all).\n\nAre those the IPU3 pipe instances (does the IPU3 firmware exposes two\nPCI devices?).\n\nThanks\n j\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index de4ab523..85225f9b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> \tcompleteRequest(activeCamera_, request);\n> }\n>\n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 89652105..376eb92a 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> \tpipe_->completeRequest(camera_, request);\n> }\n>\n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index f26a91f8..75d0236f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> \tpipe_->completeRequest(camera_, request);\n> }\n>\n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 3e54aa23..9113bf18 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> \treturn factories;\n> }\n>\n> +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> +{\n> +\tstatic std::vector> devices;\n> +\tif (!devices.empty())\n> +\t\treturn devices;\n> +\n> +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\n> +\tfor (PipelineHandlerFactory *factory : factories) {\n> +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> +\t}\n> +\n> +\treturn devices;\n> +}\n> +\n> /**\n> * \\fn PipelineHandlerFactory::createInstance()\n> * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58BED61656\n\tfor ;\n\tTue, 1 Oct 2019 10:55:18 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id C3DF2200010;\n\tTue, 1 Oct 2019 08:55:17 +0000 (UTC)"],"Date":"Tue, 1 Oct 2019 10:56:57 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vnlsablwuq45j3ho\"","Content-Disposition":"inline","In-Reply-To":"<20190930053520.2711-1-paul.elder@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 01 Oct 2019 08:55:18 -0000"}},{"id":2742,"web_url":"https://patchwork.libcamera.org/comment/2742/","msgid":"<20191002094245.iwkfh7rhqdgkqo5w@uno.localdomain>","date":"2019-10-02T09:42:45","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n small self reply\n\nOn Tue, Oct 01, 2019 at 10:56:57AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n>\n> On Mon, Sep 30, 2019 at 01:35:18AM -0400, Paul Elder wrote:\n> > Allow pipeline handlers to declare a list of camera device IDs that they\n> > support. This list will eventually be used to check against the list of\n> > camera devices that the system has, but have yet to be initialized by the\n> > kernel or by udev.\n> >\n> > PipelineHandlerFactory exposes a static method to retrieve the list of\n> > all such camera device IDs from all pipeline handlers.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 16 +++++++\n> > 6 files changed, 79 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 1fdef9ce..f16f37ab 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -30,6 +30,48 @@ class MediaDevice;\n> > class PipelineHandler;\n> > class Request;\n> >\n> > +class DeviceID\n> > +{\n> > +public:\n> > +\tDeviceID(std::string type)\n> > +\t\t: type_(type){};\n> > +\t~DeviceID(){};\n> > +\n> > +\ttemplate\n> > +\tbool compare(const DeviceID &id) const\n> > +\t{\n> > +\t\treturn this->type_.compare(id.type_) ||\n> > +\t\t static_cast(this)->compare(static_cast(id));\n> > +\t}\n>\n> Why is this different from defining the base class compare() as virtual,\n> accepting a parameter to the base class in compareDevices() and call the\n> compare method on the thea first parameter ? If you pass there a PCIDeviceID *\n> (as a pointer to the DeviceID base class), the PCIDeviceID::compare()\n> operation, which overrides the base class virtual compare() one, will be\n> called. Isn't this wat you're trying to achieve here or I'm missing a\n> piece?\n>\n> > +\n> > +\tconst std::string type_;\n> > +};\n> > +\n> > +class PCIDeviceID : public DeviceID\n> > +{\n> > +public:\n> > +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> > +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> > +\t~PCIDeviceID(){};\n> > +\n> > +\tbool compare(const PCIDeviceID &pci) const\n> > +\t{\n> > +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> > +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> > +\n> > +\t\treturn thisID < otherID;\n> > +\t}\n> > +\n> > +\tconst uint16_t vendor_;\n> > +\tconst uint16_t device_;\n> > +};\n> > +\n> > +template\n> > +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> > +{\n> > +\treturn id1.compare(id2);\n> > +}\n> > +\n> > class CameraData\n> > {\n> > public:\n> > @@ -117,6 +159,10 @@ public:\n> >\n> > \tstatic void registerType(PipelineHandlerFactory *factory);\n> > \tstatic std::vector &factories();\n> > +\tstatic std::vector> &getDeviceIDs();\n> > +\n> > +protected:\n> > +\tvirtual std::vector &deviceIDs() = 0;\n> >\n> > private:\n> > \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> > @@ -124,12 +170,22 @@ private:\n> > \tstd::string name_;\n> > };\n> >\n> > -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> > +#define P99_PROTECT(...) __VA_ARGS__\n>\n> I see where it comes from, but P99 refers to C99. If we like to keep\n> this in the macro, please consider changing its name.\n> > +\n> > +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> > class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> > {\t\t\t\t\t\t\t\t\t\\\n> > public:\t\t\t\t\t\t\t\t\t\\\n> > \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> > \t\t\t\t\t\t\t\t\t\\\n> > +protected:\t\t\t\t\t\t\t\t\\\n> > +\tstd::vector &deviceIDs()\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\tstatic std::vector handler##Cameras =\t\t\\\n> > +\t\t std::vector(cameras);\t\t\t\\\n> > +\t\treturn handler##Cameras;\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > private:\t\t\t\t\t\t\t\t\\\n> > \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> > \t{\t\t\t\t\t\t\t\t\\\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 827906d5..562ee9fb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> > \t}\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> > +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> > +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n>\n> What do these IDs refers to ? I haven't found them in the Soraka ACPI\n> tables (probablt they're not specified there at all).\n>\n> Are those the IPU3 pipe instances (does the IPU3 firmware exposes two\n> PCI devices?).\n>\n\nOne is the CIO2 and the other one is IMGU, according to the properties\nexposed in sysfs.\n\nWhile it guarantees the device nodes for those two entities have been\ncreated (are they permissions set?) this does not cover the sensors\n(and eventual lenses).\n\nAlso, in the next patch you check for\n std::set_intersection(supportedDevices.begin(), supportedDevices.end(),\n availableDevices.begin(), availableDevices.end(),\n back_inserter(intersection),\n compareDevices);\n\n if (cm->cameras().size() < intersection.size()) {\n LOG(DeviceEnumerator, Warning) << \"Not enough cameras!\";\n return false;\n }\n\nWhere cm->cameras() is populated by the pipeline handlers with\nregisterCamera(), while the intersection would contain entries for\nCIO2 and IMGU. Am I wrong or are you comparing two different things\nhere (also, using Soraka as an example, if not CIO2 is registered, no\ncamera would ever been created, as they depend on the presence of the\nthe CIO2 subdevice nodes).\n\nIf we're now only interested in platform PCI devices only, I would\nmove this check to CameraManager::start(), before match() is called\n(as it attempts to create cameras and if no subdevice device node is\nregistered, none will be created).\n\nThen, there is a different question that is how we deal with\ndevice-specific requirements: CIO2 and IMGU are platform properties,\nthey are there for all IPU3 device, while the number of cameras\ndepends on the number of sensor installed on the device. For Soraka\nthere are two cameras, other IPU3 device might have just one. I fear\nwe will need compatilble-like strings for pipeline handlers which\ncontains device-specific information. In example, just thinking out\nloud:\n\n struct PipelineData sorakaCameraData = {\n .platform = {\n \"0x8086, 0x1919\",\n \"0x8086, 0x9D32\",\n },\n .sensors = {\n \"ov5670 4-0036\",\n \"ov13858 2-0018\",\n },\n };\n struct PipelineDeviceIds[] = {\n { .compatible = \"ipu3,soraka\", .data = &soraka_camera_data },\n };\n\nAnd you could match the \"sensors\" with /sys/class/video4linux/*/name\nIf we have everything we need, then match() can be called.\n\nWhere to specify the compatible string \"ipu3,soraka\" is the question,\nas it might require a pipeline configuration file somewhere.\n\nJust thinking out loud...\n\nThanks\n j\n\n\n> Thanks\n> j\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index de4ab523..85225f9b 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> > \tcompleteRequest(activeCamera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 89652105..376eb92a 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index f26a91f8..75d0236f 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 3e54aa23..9113bf18 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> > \treturn factories;\n> > }\n> >\n> > +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> > +{\n> > +\tstatic std::vector> devices;\n> > +\tif (!devices.empty())\n> > +\t\treturn devices;\n> > +\n> > +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > +\n> > +\tfor (PipelineHandlerFactory *factory : factories) {\n> > +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> > +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> > +\t}\n> > +\n> > +\treturn devices;\n> > +}\n> > +\n> > /**\n> > * \\fn PipelineHandlerFactory::createInstance()\n> > * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n\n\n\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA53260BE9\n\tfor ;\n\tWed, 2 Oct 2019 11:41:07 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 6BCFE20014;\n\tWed, 2 Oct 2019 09:41:07 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 2 Oct 2019 11:42:45 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191002094245.iwkfh7rhqdgkqo5w@uno.localdomain>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>\n\t<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"hgewnexl76a335x5\"","Content-Disposition":"inline","In-Reply-To":"<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Oct 2019 09:41:08 -0000"}},{"id":2762,"web_url":"https://patchwork.libcamera.org/comment/2762/","msgid":"<20191003195936.GQ1322@bigcity.dyn.berto.se>","date":"2019-10-03T19:59:36","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2019-09-30 01:35:18 -0400, Paul Elder wrote:\n> Allow pipeline handlers to declare a list of camera device IDs that they\n> support. This list will eventually be used to check against the list of\n> camera devices that the system has, but have yet to be initialized by the\n> kernel or by udev.\n> \n> PipelineHandlerFactory exposes a static method to retrieve the list of\n> all such camera device IDs from all pipeline handlers.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 16 +++++++\n> 6 files changed, 79 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 1fdef9ce..f16f37ab 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -30,6 +30,48 @@ class MediaDevice;\n> class PipelineHandler;\n> class Request;\n> \n> +class DeviceID\n> +{\n> +public:\n> +\tDeviceID(std::string type)\n> +\t\t: type_(type){};\n> +\t~DeviceID(){};\n> +\n> +\ttemplate\n> +\tbool compare(const DeviceID &id) const\n> +\t{\n> +\t\treturn this->type_.compare(id.type_) ||\n> +\t\t static_cast(this)->compare(static_cast(id));\n> +\t}\n> +\n> +\tconst std::string type_;\n> +};\n> +\n> +class PCIDeviceID : public DeviceID\n> +{\n> +public:\n> +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> +\t~PCIDeviceID(){};\n> +\n> +\tbool compare(const PCIDeviceID &pci) const\n> +\t{\n> +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> +\n> +\t\treturn thisID < otherID;\n> +\t}\n> +\n> +\tconst uint16_t vendor_;\n> +\tconst uint16_t device_;\n> +};\n> +\n> +template\n> +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> +{\n> +\treturn id1.compare(id2);\n> +}\n> +\n> class CameraData\n> {\n> public:\n> @@ -117,6 +159,10 @@ public:\n> \n> \tstatic void registerType(PipelineHandlerFactory *factory);\n> \tstatic std::vector &factories();\n> +\tstatic std::vector> &getDeviceIDs();\n> +\n> +protected:\n> +\tvirtual std::vector &deviceIDs() = 0;\n> \n> private:\n> \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> @@ -124,12 +170,22 @@ private:\n> \tstd::string name_;\n> };\n> \n> -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> +#define P99_PROTECT(...) __VA_ARGS__\n> +\n> +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> public:\t\t\t\t\t\t\t\t\t\\\n> \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> \t\t\t\t\t\t\t\t\t\\\n> +protected:\t\t\t\t\t\t\t\t\\\n> +\tstd::vector &deviceIDs()\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\tstatic std::vector handler##Cameras =\t\t\\\n> +\t\t std::vector(cameras);\t\t\t\\\n> +\t\treturn handler##Cameras;\t\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> private:\t\t\t\t\t\t\t\t\\\n> \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> \t{\t\t\t\t\t\t\t\t\\\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 827906d5..562ee9fb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> \t}\n> }\n> \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n\nCan't we get these IDs from the kernel? They must be described for the \nipu3 kernel module right?\n\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index de4ab523..85225f9b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> \tcompleteRequest(activeCamera_, request);\n> }\n> \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 89652105..376eb92a 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> \tpipe_->completeRequest(camera_, request);\n> }\n> \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index f26a91f8..75d0236f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> \tpipe_->completeRequest(camera_, request);\n> }\n> \n> -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 3e54aa23..9113bf18 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> \treturn factories;\n> }\n> \n> +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> +{\n> +\tstatic std::vector> devices;\n> +\tif (!devices.empty())\n> +\t\treturn devices;\n> +\n> +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\n> +\tfor (PipelineHandlerFactory *factory : factories) {\n> +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> +\t}\n> +\n> +\treturn devices;\n> +}\n> +\n> /**\n> * \\fn PipelineHandlerFactory::createInstance()\n> * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> -- \n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9266A60BE8\n\tfor ;\n\tThu, 3 Oct 2019 21:59:39 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id x80so2786067lff.3\n\tfor ;\n\tThu, 03 Oct 2019 12:59:39 -0700 (PDT)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\ti6sm627240lfo.83.2019.10.03.12.59.38\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 03 Oct 2019 12:59:38 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=7U+D8mHi5idunkOJCiZKOSDKSCMbSW+J0Gsr0XRvCEk=;\n\tb=JtpC6jrhpxs/gpYC1KGOsGRmfk0o4qOv8VU//8ve21isgo5ASw/+XsAWPcbTlHiuon\n\t5dwqOp/S+H1whB8LVH9X3wcWCEyjQxnc5XbtN/JmS8n+tYNmWq1BmyDlOU2v6NM5TOEq\n\tSapPZgY5XRi0CzhDxL9NACLPOT/yb75TaI+kOfvvjtBM3P2d9Y/sSRGQxXbUMINarzuV\n\tHEw6DIj/z5Cg6JIlzXbsv5TzYXJZTCEWziIdHOFi+aOpig8ulOB7k3vyqNYrnkAJiL1Y\n\tGHGodjfqdhDv8fxJaj/iamSn2lxWVxyfKGF0kpKXwWW2+n5FgxQNQ+0iQlvnh21DhaWH\n\tVzuA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=7U+D8mHi5idunkOJCiZKOSDKSCMbSW+J0Gsr0XRvCEk=;\n\tb=aLbekZqx8ytIL00VtU8qWR2gcpAN0BBQKBS8cj/XBx4YKI1PUI1zxvCjW71RFDms+7\n\twju1By2H3zq4zvamLIXki0wjJc2KOo2vREVbb/Xb82+cjMmdZZuWUP26Lry0739L5SGX\n\tBzx9MEevPHuftgSCEJd+xeZci6nEl6gSkZociEVHngx3/q+cCI4PMUPopj24mZ9KlmEa\n\t6XYRTsmWb/iI1cpQwDt2vEr8h3fh558TUxDJVwPRI52EQj5Lb17qOJkFv8h1GJbVUwLU\n\tVlm33Tpb3clPDoccl2tBKXJxLDfs/JiuTHl2z1xCy0he8xKzMtEirNuMNQ84Z2Q8BE5u\n\tTayg==","X-Gm-Message-State":"APjAAAUP/KhsrRMo0g7BDJum2/lJ5wbvuJMCf70dEvE/neXyf61I1hw/\n\t+KMSmqIWS8VSDBPcq+EHevYpew==","X-Google-Smtp-Source":"APXvYqwSjfjdBFGrDV/5wkdMtUROqtgZCHrQSaWdBodXGXcJ/OWmg5Ep0DH0q0JxDmU805w57uvmyg==","X-Received":"by 2002:ac2:4289:: with SMTP id m9mr3893156lfh.139.1570132778844;\n\tThu, 03 Oct 2019 12:59:38 -0700 (PDT)","Date":"Thu, 3 Oct 2019 21:59:36 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191003195936.GQ1322@bigcity.dyn.berto.se>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190930053520.2711-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 03 Oct 2019 19:59:39 -0000"}},{"id":2817,"web_url":"https://patchwork.libcamera.org/comment/2817/","msgid":"<20191007055913.GC5103@localhost.localdomain>","date":"2019-10-07T05:59:13","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Thu, Oct 03, 2019 at 09:59:36PM +0200, Niklas Söderlund wrote:\n> Hi Paul,\n> \n> Thanks for your work.\n> \n> On 2019-09-30 01:35:18 -0400, Paul Elder wrote:\n> > Allow pipeline handlers to declare a list of camera device IDs that they\n> > support. This list will eventually be used to check against the list of\n> > camera devices that the system has, but have yet to be initialized by the\n> > kernel or by udev.\n> > \n> > PipelineHandlerFactory exposes a static method to retrieve the list of\n> > all such camera device IDs from all pipeline handlers.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 16 +++++++\n> > 6 files changed, 79 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 1fdef9ce..f16f37ab 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -30,6 +30,48 @@ class MediaDevice;\n> > class PipelineHandler;\n> > class Request;\n> > \n> > +class DeviceID\n> > +{\n> > +public:\n> > +\tDeviceID(std::string type)\n> > +\t\t: type_(type){};\n> > +\t~DeviceID(){};\n> > +\n> > +\ttemplate\n> > +\tbool compare(const DeviceID &id) const\n> > +\t{\n> > +\t\treturn this->type_.compare(id.type_) ||\n> > +\t\t static_cast(this)->compare(static_cast(id));\n> > +\t}\n> > +\n> > +\tconst std::string type_;\n> > +};\n> > +\n> > +class PCIDeviceID : public DeviceID\n> > +{\n> > +public:\n> > +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> > +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> > +\t~PCIDeviceID(){};\n> > +\n> > +\tbool compare(const PCIDeviceID &pci) const\n> > +\t{\n> > +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> > +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> > +\n> > +\t\treturn thisID < otherID;\n> > +\t}\n> > +\n> > +\tconst uint16_t vendor_;\n> > +\tconst uint16_t device_;\n> > +};\n> > +\n> > +template\n> > +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> > +{\n> > +\treturn id1.compare(id2);\n> > +}\n> > +\n> > class CameraData\n> > {\n> > public:\n> > @@ -117,6 +159,10 @@ public:\n> > \n> > \tstatic void registerType(PipelineHandlerFactory *factory);\n> > \tstatic std::vector &factories();\n> > +\tstatic std::vector> &getDeviceIDs();\n> > +\n> > +protected:\n> > +\tvirtual std::vector &deviceIDs() = 0;\n> > \n> > private:\n> > \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> > @@ -124,12 +170,22 @@ private:\n> > \tstd::string name_;\n> > };\n> > \n> > -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> > +#define P99_PROTECT(...) __VA_ARGS__\n> > +\n> > +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> > class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> > {\t\t\t\t\t\t\t\t\t\\\n> > public:\t\t\t\t\t\t\t\t\t\\\n> > \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> > \t\t\t\t\t\t\t\t\t\\\n> > +protected:\t\t\t\t\t\t\t\t\\\n> > +\tstd::vector &deviceIDs()\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\tstatic std::vector handler##Cameras =\t\t\\\n> > +\t\t std::vector(cameras);\t\t\t\\\n> > +\t\treturn handler##Cameras;\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > private:\t\t\t\t\t\t\t\t\\\n> > \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> > \t{\t\t\t\t\t\t\t\t\\\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 827906d5..562ee9fb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> > \t}\n> > }\n> > \n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> > +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> > +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n> \n> Can't we get these IDs from the kernel? They must be described for the \n> ipu3 kernel module right?\n\nI'm not sure. We can't get it from sysfs, since we're trying to specify what\nwe're looking for in sysfs. There are no headers that define IMGU's, but\nthere is one for CIO2.\n\n\nThanks,\n\nPaul\n\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index de4ab523..85225f9b 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> > \tcompleteRequest(activeCamera_, request);\n> > }\n> > \n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 89652105..376eb92a 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> > \n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index f26a91f8..75d0236f 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> > \n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 3e54aa23..9113bf18 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> > \treturn factories;\n> > }\n> > \n> > +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> > +{\n> > +\tstatic std::vector> devices;\n> > +\tif (!devices.empty())\n> > +\t\treturn devices;\n> > +\n> > +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > +\n> > +\tfor (PipelineHandlerFactory *factory : factories) {\n> > +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> > +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> > +\t}\n> > +\n> > +\treturn devices;\n> > +}\n> > +\n> > /**\n> > * \\fn PipelineHandlerFactory::createInstance()\n> > * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> > -- \n> > 2.23.0\n> > \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D58D860BC6\n\tfor ;\n\tMon, 7 Oct 2019 07:59:20 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 14470DD;\n\tMon, 7 Oct 2019 07:59:19 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570427960;\n\tbh=6uh+Aji4K8CyIJGKae+AfIH/pouT68/MI9eybHOBt0E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J8VFXVZBacX0mkowOJWUWTkbVazXZ3H/rok9rS4e+PeYM+SYYp31uE3HvTHyAwgJe\n\taQ8C1SkXxkuaHPimjKfyKozMuOLZNHj7K2qqsEBGxofQZ8dtd0x6tYMclZVhfMvE9U\n\tgLJAMl1LzuV4H5K4Z7zyn/BHTNA4fFMGhQ4IpkNI=","Date":"Mon, 7 Oct 2019 01:59:13 -0400","From":"Paul Elder ","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191007055913.GC5103@localhost.localdomain>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>\n\t<20191003195936.GQ1322@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191003195936.GQ1322@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Oct 2019 05:59:21 -0000"}},{"id":2818,"web_url":"https://patchwork.libcamera.org/comment/2818/","msgid":"<20191007055927.GD5103@localhost.localdomain>","date":"2019-10-07T05:59:27","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo and Niklas,\n\nThank you for the review.\n\nOn Wed, Oct 02, 2019 at 11:42:45AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> small self reply\n> \n> On Tue, Oct 01, 2019 at 10:56:57AM +0200, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Mon, Sep 30, 2019 at 01:35:18AM -0400, Paul Elder wrote:\n> > > Allow pipeline handlers to declare a list of camera device IDs that they\n> > > support. This list will eventually be used to check against the list of\n> > > camera devices that the system has, but have yet to be initialized by the\n> > > kernel or by udev.\n> > >\n> > > PipelineHandlerFactory exposes a static method to retrieve the list of\n> > > all such camera device IDs from all pipeline handlers.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > > src/libcamera/pipeline_handler.cpp | 16 +++++++\n> > > 6 files changed, 79 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > > index 1fdef9ce..f16f37ab 100644\n> > > --- a/src/libcamera/include/pipeline_handler.h\n> > > +++ b/src/libcamera/include/pipeline_handler.h\n> > > @@ -30,6 +30,48 @@ class MediaDevice;\n> > > class PipelineHandler;\n> > > class Request;\n> > >\n> > > +class DeviceID\n> > > +{\n> > > +public:\n> > > +\tDeviceID(std::string type)\n> > > +\t\t: type_(type){};\n> > > +\t~DeviceID(){};\n> > > +\n> > > +\ttemplate\n> > > +\tbool compare(const DeviceID &id) const\n> > > +\t{\n> > > +\t\treturn this->type_.compare(id.type_) ||\n> > > +\t\t static_cast(this)->compare(static_cast(id));\n> > > +\t}\n> >\n> > Why is this different from defining the base class compare() as virtual,\n> > accepting a parameter to the base class in compareDevices() and call the\n> > compare method on the thea first parameter ? If you pass there a PCIDeviceID *\n> > (as a pointer to the DeviceID base class), the PCIDeviceID::compare()\n> > operation, which overrides the base class virtual compare() one, will be\n> > called. Isn't this wat you're trying to achieve here or I'm missing a\n> > piece?\n> >\n> > > +\n> > > +\tconst std::string type_;\n> > > +};\n> > > +\n> > > +class PCIDeviceID : public DeviceID\n> > > +{\n> > > +public:\n> > > +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> > > +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> > > +\t~PCIDeviceID(){};\n> > > +\n> > > +\tbool compare(const PCIDeviceID &pci) const\n> > > +\t{\n> > > +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> > > +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> > > +\n> > > +\t\treturn thisID < otherID;\n> > > +\t}\n> > > +\n> > > +\tconst uint16_t vendor_;\n> > > +\tconst uint16_t device_;\n> > > +};\n> > > +\n> > > +template\n> > > +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> > > +{\n> > > +\treturn id1.compare(id2);\n> > > +}\n> > > +\n> > > class CameraData\n> > > {\n> > > public:\n> > > @@ -117,6 +159,10 @@ public:\n> > >\n> > > \tstatic void registerType(PipelineHandlerFactory *factory);\n> > > \tstatic std::vector &factories();\n> > > +\tstatic std::vector> &getDeviceIDs();\n> > > +\n> > > +protected:\n> > > +\tvirtual std::vector &deviceIDs() = 0;\n> > >\n> > > private:\n> > > \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> > > @@ -124,12 +170,22 @@ private:\n> > > \tstd::string name_;\n> > > };\n> > >\n> > > -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> > > +#define P99_PROTECT(...) __VA_ARGS__\n> >\n> > I see where it comes from, but P99 refers to C99. If we like to keep\n> > this in the macro, please consider changing its name.\n> > > +\n> > > +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> > > class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> > > {\t\t\t\t\t\t\t\t\t\\\n> > > public:\t\t\t\t\t\t\t\t\t\\\n> > > \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> > > \t\t\t\t\t\t\t\t\t\\\n> > > +protected:\t\t\t\t\t\t\t\t\\\n> > > +\tstd::vector &deviceIDs()\t\t\t\t\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\tstatic std::vector handler##Cameras =\t\t\\\n> > > +\t\t std::vector(cameras);\t\t\t\\\n> > > +\t\treturn handler##Cameras;\t\t\t\t\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > private:\t\t\t\t\t\t\t\t\\\n> > > \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> > > \t{\t\t\t\t\t\t\t\t\\\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 827906d5..562ee9fb 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> > > \t}\n> > > }\n> > >\n> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> > > +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> > > +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n> >\n> > What do these IDs refers to ? I haven't found them in the Soraka ACPI\n> > tables (probablt they're not specified there at all).\n> >\n> > Are those the IPU3 pipe instances (does the IPU3 firmware exposes two\n> > PCI devices?).\n> >\n> \n> One is the CIO2 and the other one is IMGU, according to the properties\n> exposed in sysfs.\n> \n> While it guarantees the device nodes for those two entities have been\n> created (are they permissions set?) this does not cover the sensors\n> (and eventual lenses).\n> \n> Also, in the next patch you check for\n> std::set_intersection(supportedDevices.begin(), supportedDevices.end(),\n> availableDevices.begin(), availableDevices.end(),\n> back_inserter(intersection),\n> compareDevices);\n> \n> if (cm->cameras().size() < intersection.size()) {\n> LOG(DeviceEnumerator, Warning) << \"Not enough cameras!\";\n> return false;\n> }\n\nPasting in what Niklas sent:\n> To me this seems a bit too strict. Say that I have a Soraka device, run \n> a kernel on it without IPU3 support, With this change libcamera would \n> not function for a USB camera.\n\nI think there's another problem that if you start up the Soraka with a\nUSB camera plugged in, libcamera would say that enough cameras have\nstarted up and not wait for the IPU3.\n\n> Also comparing against cm->cameras().size() might be skewed if you run \n> on a kernel with vivid and/or vimc support built in.\n\nThat too.\n\nAnyway, with this, I think we can see that we can't just compare the\nnumber of expected cameras and actual cameras, and we have to actually\ncheck a list of specific cameras. I think this calls for platform\nconfigs :/\n\n\n> \n> Where cm->cameras() is populated by the pipeline handlers with\n> registerCamera(), while the intersection would contain entries for\n> CIO2 and IMGU. Am I wrong or are you comparing two different things\n\n...I might be :S\n\n> here (also, using Soraka as an example, if not CIO2 is registered, no\n> camera would ever been created, as they depend on the presence of the\n> the CIO2 subdevice nodes).\n> \n> If we're now only interested in platform PCI devices only, I would\n> move this check to CameraManager::start(), before match() is called\n> (as it attempts to create cameras and if no subdevice device node is\n> registered, none will be created).\n\nI'm not sure what you mean.\n\n> Then, there is a different question that is how we deal with\n> device-specific requirements: CIO2 and IMGU are platform properties,\n> they are there for all IPU3 device, while the number of cameras\n> depends on the number of sensor installed on the device. For Soraka\n> there are two cameras, other IPU3 device might have just one. I fear\n> we will need compatilble-like strings for pipeline handlers which\n> contains device-specific information. In example, just thinking out\n> loud:\n> \n> struct PipelineData sorakaCameraData = {\n> .platform = {\n> \"0x8086, 0x1919\",\n> \"0x8086, 0x9D32\",\n> },\n> .sensors = {\n> \"ov5670 4-0036\",\n> \"ov13858 2-0018\",\n> },\n> };\n> struct PipelineDeviceIds[] = {\n> { .compatible = \"ipu3,soraka\", .data = &soraka_camera_data },\n> };\n> \n> And you could match the \"sensors\" with /sys/class/video4linux/*/name\n> If we have everything we need, then match() can be called.\n\nWe still need to call match() in CameraManager::start() though. The only\nthing that cares if we don't have the expected cameras initialized is\nthe camera HAL; the rest of libcamera doesn't care if it expected some\ncameras to be enumerated but weren't.\n\n> Where to specify the compatible string \"ipu3,soraka\" is the question,\n> as it might require a pipeline configuration file somewhere.\n\nI think we will need it, though :/\n\n> Just thinking out loud...\n> \n\n\nThanks,\n\nPaul\n \n> \n> > Thanks\n> > j\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index de4ab523..85225f9b 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> > > \tcompleteRequest(activeCamera_, request);\n> > > }\n> > >\n> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > > index 89652105..376eb92a 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> > > \tpipe_->completeRequest(camera_, request);\n> > > }\n> > >\n> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index f26a91f8..75d0236f 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> > > \tpipe_->completeRequest(camera_, request);\n> > > }\n> > >\n> > > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 3e54aa23..9113bf18 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> > > \treturn factories;\n> > > }\n> > >\n> > > +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> > > +{\n> > > +\tstatic std::vector> devices;\n> > > +\tif (!devices.empty())\n> > > +\t\treturn devices;\n> > > +\n> > > +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > > +\n> > > +\tfor (PipelineHandlerFactory *factory : factories) {\n> > > +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> > > +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> > > +\t}\n> > > +\n> > > +\treturn devices;\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn PipelineHandlerFactory::createInstance()\n> > > * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> > > --\n> > > 2.23.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> \n> \n> \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D44B760BC6\n\tfor ;\n\tMon, 7 Oct 2019 07:59:33 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4B112DD;\n\tMon, 7 Oct 2019 07:59:33 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570427973;\n\tbh=Z3X+pp4WODGeX8/TmTaXh7XjbhUSvG0ZaUtCfCwf6Gg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=alpR6KTvb0l9tTFkm2fW0e3A2oRN2UTB6CvcZDrO+JVxPnzTezX1gugMSCglwvlby\n\tOh3o/4T5/RL/hMhuhs8J0H3zXWZn3xRgVi3r8BLoP/nNPq3pJ2pMALuve2lt6jcH/d\n\t8hUYA6XSKm7aniOI9OA33iO98qlQwOrLyFELidUo=","Date":"Mon, 7 Oct 2019 01:59:27 -0400","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191007055927.GD5103@localhost.localdomain>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>\n\t<20191001085657.5plqgianl5nzz4cq@uno.localdomain>\n\t<20191002094245.iwkfh7rhqdgkqo5w@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20191002094245.iwkfh7rhqdgkqo5w@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Oct 2019 05:59:34 -0000"}},{"id":2819,"web_url":"https://patchwork.libcamera.org/comment/2819/","msgid":"<20191007055939.GE5103@localhost.localdomain>","date":"2019-10-07T05:59:39","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review.\n\nOn Tue, Oct 01, 2019 at 10:56:57AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Mon, Sep 30, 2019 at 01:35:18AM -0400, Paul Elder wrote:\n> > Allow pipeline handlers to declare a list of camera device IDs that they\n> > support. This list will eventually be used to check against the list of\n> > camera devices that the system has, but have yet to be initialized by the\n> > kernel or by udev.\n> >\n> > PipelineHandlerFactory exposes a static method to retrieve the list of\n> > all such camera device IDs from all pipeline handlers.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/pipeline_handler.cpp | 16 +++++++\n> > 6 files changed, 79 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 1fdef9ce..f16f37ab 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -30,6 +30,48 @@ class MediaDevice;\n> > class PipelineHandler;\n> > class Request;\n> >\n> > +class DeviceID\n> > +{\n> > +public:\n> > +\tDeviceID(std::string type)\n> > +\t\t: type_(type){};\n> > +\t~DeviceID(){};\n> > +\n> > +\ttemplate\n> > +\tbool compare(const DeviceID &id) const\n> > +\t{\n> > +\t\treturn this->type_.compare(id.type_) ||\n> > +\t\t static_cast(this)->compare(static_cast(id));\n> > +\t}\n> \n> Why is this different from defining the base class compare() as virtual,\n> accepting a parameter to the base class in compareDevices() and call the\n> compare method on the thea first parameter ? If you pass there a PCIDeviceID *\n> (as a pointer to the DeviceID base class), the PCIDeviceID::compare()\n> operation, which overrides the base class virtual compare() one, will be\n> called. Isn't this wat you're trying to achieve here or I'm missing a\n> piece?\n\nIt's not different. That's exactly what I was trying to achieve and\ngetting stuck.\n\n> > +\n> > +\tconst std::string type_;\n> > +};\n> > +\n> > +class PCIDeviceID : public DeviceID\n> > +{\n> > +public:\n> > +\tPCIDeviceID(uint16_t vendor, uint16_t device)\n> > +\t\t: DeviceID(\"pci\"), vendor_(vendor), device_(device){};\n> > +\t~PCIDeviceID(){};\n> > +\n> > +\tbool compare(const PCIDeviceID &pci) const\n> > +\t{\n> > +\t\tuint32_t thisID = (vendor_ << 16) + device_;\n> > +\t\tuint32_t otherID = (pci.vendor_ << 16) + pci.device_;\n> > +\n> > +\t\treturn thisID < otherID;\n> > +\t}\n> > +\n> > +\tconst uint16_t vendor_;\n> > +\tconst uint16_t device_;\n> > +};\n> > +\n> > +template\n> > +bool compareDevices(const DeviceID &id1, const DeviceID &id2)\n> > +{\n> > +\treturn id1.compare(id2);\n> > +}\n> > +\n> > class CameraData\n> > {\n> > public:\n> > @@ -117,6 +159,10 @@ public:\n> >\n> > \tstatic void registerType(PipelineHandlerFactory *factory);\n> > \tstatic std::vector &factories();\n> > +\tstatic std::vector> &getDeviceIDs();\n> > +\n> > +protected:\n> > +\tvirtual std::vector &deviceIDs() = 0;\n> >\n> > private:\n> > \tvirtual PipelineHandler *createInstance(CameraManager *manager) = 0;\n> > @@ -124,12 +170,22 @@ private:\n> > \tstd::string name_;\n> > };\n> >\n> > -#define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> > +#define P99_PROTECT(...) __VA_ARGS__\n> \n> I see where it comes from, but P99 refers to C99. If we like to keep\n> this in the macro, please consider changing its name.\n\nOkay :/\n...PPP03?\n\n\nThanks,\n\nPaul\n\n> > +\n> > +#define REGISTER_PIPELINE_HANDLER(handler, cameras)\t\t\t\\\n> > class handler##Factory final : public PipelineHandlerFactory\t\t\\\n> > {\t\t\t\t\t\t\t\t\t\\\n> > public:\t\t\t\t\t\t\t\t\t\\\n> > \thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> > \t\t\t\t\t\t\t\t\t\\\n> > +protected:\t\t\t\t\t\t\t\t\\\n> > +\tstd::vector &deviceIDs()\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\tstatic std::vector handler##Cameras =\t\t\\\n> > +\t\t std::vector(cameras);\t\t\t\\\n> > +\t\treturn handler##Cameras;\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > private:\t\t\t\t\t\t\t\t\\\n> > \tPipelineHandler *createInstance(CameraManager *manager)\t\t\\\n> > \t{\t\t\t\t\t\t\t\t\\\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 827906d5..562ee9fb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1504,6 +1504,8 @@ int CIO2Device::mediaBusToFormat(unsigned int code)\n> > \t}\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3,\n> > +\t\t\t P99_PROTECT({ PCIDeviceID(0x8086, 0x1919),\n> > +\t\t\t\t\tPCIDeviceID(0x8086, 0x9D32) }));\n> \n> What do these IDs refers to ? I haven't found them in the Soraka ACPI\n> tables (probablt they're not specified there at all).\n> \n> Are those the IPU3 pipe instances (does the IPU3 firmware exposes two\n> PCI devices?).\n> \n> Thanks\n> j\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index de4ab523..85225f9b 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -513,6 +513,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> > \tcompleteRequest(activeCamera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 89652105..376eb92a 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -390,6 +390,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index f26a91f8..75d0236f 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -459,6 +459,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)\n> > \tpipe_->completeRequest(camera_, request);\n> > }\n> >\n> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, {});\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 3e54aa23..9113bf18 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -593,6 +593,22 @@ std::vector &PipelineHandlerFactory::factories()\n> > \treturn factories;\n> > }\n> >\n> > +std::vector> &PipelineHandlerFactory::getDeviceIDs()\n> > +{\n> > +\tstatic std::vector> devices;\n> > +\tif (!devices.empty())\n> > +\t\treturn devices;\n> > +\n> > +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > +\n> > +\tfor (PipelineHandlerFactory *factory : factories) {\n> > +\t\tstd::vector &factoryDevices = factory->deviceIDs();\n> > +\t\tdevices.insert(devices.end(), factoryDevices.begin(), factoryDevices.end());\n> > +\t}\n> > +\n> > +\treturn devices;\n> > +}\n> > +\n> > /**\n> > * \\fn PipelineHandlerFactory::createInstance()\n> > * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D35E60BC6\n\tfor ;\n\tMon, 7 Oct 2019 07:59:45 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C54FBDD;\n\tMon, 7 Oct 2019 07:59:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570427985;\n\tbh=PVP+oDRs0eL3HZVb8i8z7wxtZCmLJ9+bqMLHA3aOL14=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C4uYandi1F3P8G603wz9ITM8H9WF92GiIRYot9b/iCcGz/wJXLYFLuTNWip0q7EU2\n\ttfXDqxNiVZ7+6InMgmn4SNVJNCbeirLAMZRivqUCAa7FoAxma76RXst/wz3Ao3e5T4\n\tmDTtcMMMNbDhLS5MA5ECgjkF0EjkjHDVh9l3XYG0=","Date":"Mon, 7 Oct 2019 01:59:39 -0400","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191007055939.GE5103@localhost.localdomain>","References":"<20190930053520.2711-1-paul.elder@ideasonboard.com>\n\t<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20191001085657.5plqgianl5nzz4cq@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add\n\tdevice IDs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 07 Oct 2019 05:59:45 -0000"}}]