[libcamera-devel] [RFC PATCH 1/3] libcamera: pipeline: Add device IDs
Paul Elder
paul.elder at ideasonboard.com
Mon Oct 7 07:59:39 CEST 2019
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 at ideasonboard.com>
> > ---
> > src/libcamera/include/pipeline_handler.h | 58 +++++++++++++++++++++++-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > src/libcamera/pipeline_handler.cpp | 16 +++++++
> > 6 files changed, 79 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 1fdef9ce..f16f37ab 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -30,6 +30,48 @@ class MediaDevice;
> > class PipelineHandler;
> > class Request;
> >
> > +class DeviceID
> > +{
> > +public:
> > + DeviceID(std::string type)
> > + : type_(type){};
> > + ~DeviceID(){};
> > +
> > + template<class IDType>
> > + bool compare(const DeviceID &id) const
> > + {
> > + return this->type_.compare(id.type_) ||
> > + static_cast<const IDType *>(this)->compare(static_cast<const IDType &>(id));
> > + }
>
> Why is this different from defining the base class compare() as virtual,
> accepting a parameter to the base class in compareDevices() and call the
> compare method on the thea first parameter ? If you pass there a PCIDeviceID *
> (as a pointer to the DeviceID base class), the PCIDeviceID::compare()
> operation, which overrides the base class virtual compare() one, will be
> called. Isn't this wat you're trying to achieve here or I'm missing a
> piece?
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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list