| Message ID | 20190111165109.26803-1-jacopo@jmondi.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2019-01-11 17:51:09 +0100, Jacopo Mondi wrote: > Add a pipeline handler skeleton for the Intel IPU3 device. > Tested with on Soraka, listing detected cameras on the system and > verifying the pipeline handler gets properly matched. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > > Let's start by simply matching the pipeline handler with the device > it is running on. The here created single camera gets properly enumerated > on Soraka by the 'list-cameras' test: > > ./test/list-cameras > [0:35:35.453249952] DBG pipeline_handler.cpp:119 Pipeline handler: "PipeHandlerVimc" registered > [0:35:35.453538626] DBG pipeline_handler.cpp:119 Pipeline handler: "PipelineHandlerIPU3" registered > [0:35:35.458316459] DBG device_enumerator.cpp:214 New media device: ipu3-imgu created from: /dev/media1 > [0:35:35.469071318] DBG device_enumerator.cpp:214 New media device: ipu3-cio2 created from: /dev/media0 > [0:35:35.475305874] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-cio2 > [0:35:35.475343991] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-imgu > [0:35:35.475354057] DBG pipeline_handler.cpp:150 Pipeline handler: "PipelineHandlerIPU3" matched > - IPU3 Camera > > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 119 ++++++++++++++++++++++++ > src/libcamera/pipeline/ipu3/meson.build | 3 + > src/libcamera/pipeline/meson.build | 2 + > 3 files changed, 124 insertions(+) > create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp > create mode 100644 src/libcamera/pipeline/ipu3/meson.build > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > new file mode 100644 > index 0000000..477a9a2 > --- /dev/null > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -0,0 +1,119 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipu3.cpp - Pipeline handler for Intel IPU3 > + */ > + > +#include <libcamera/camera.h> > + > +#include "device_enumerator.h" > +#include "media_device.h" > +#include "pipeline_handler.h" > + > +#include "log.h" > + > +namespace libcamera { > + > +class PipelineHandlerIPU3 : public PipelineHandler > +{ > +public: > + PipelineHandlerIPU3(); > + ~PipelineHandlerIPU3(); > + > + bool match(DeviceEnumerator *enumerator); > + > + unsigned int count(); > + Camera *camera(unsigned int id) final; > + > +private: > + MediaDevice *cio2_; > + MediaDevice *imgu_; > + > + Camera *camera_; > +}; > + > +PipelineHandlerIPU3::PipelineHandlerIPU3() > + : cio2_(nullptr), imgu_(nullptr), camera_(nullptr) > +{ > +} > + > +PipelineHandlerIPU3::~PipelineHandlerIPU3() > +{ > + if (cio2_) > + cio2_->release(); > + if (imgu_) > + imgu_->release(); > + if (camera_) > + camera_->put(); > + > + cio2_ = nullptr; > + imgu_ = nullptr; > + camera_ = nullptr; > +} > + > +unsigned int PipelineHandlerIPU3::count() > +{ > + return 1; > +} > + > +Camera *PipelineHandlerIPU3::camera(unsigned int id) > +{ > + if (id != 0) > + return nullptr; > + > + return camera_; > +} > + > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > +{ > + DeviceMatch cio2_dm("ipu3-cio2"); > + cio2_dm.add("ipu3-csi2 0"); > + cio2_dm.add("ipu3-cio2 0"); > + cio2_dm.add("ipu3-csi2 1"); > + cio2_dm.add("ipu3-cio2 1"); > + cio2_dm.add("ipu3-csi2 2"); > + cio2_dm.add("ipu3-cio2 2"); > + cio2_dm.add("ipu3-csi2 3"); > + cio2_dm.add("ipu3-cio2 3"); > + > + cio2_ = enumerator->search(cio2_dm); > + if (!cio2_) > + return false; > + > + cio2_->acquire(); > + > + DeviceMatch imgu_dm("ipu3-imgu"); > + imgu_dm.add("ipu3-imgu 0"); > + imgu_dm.add("ipu3-imgu 0 input"); > + imgu_dm.add("ipu3-imgu 0 parameters"); > + imgu_dm.add("ipu3-imgu 0 output"); > + imgu_dm.add("ipu3-imgu 0 viewfinder"); > + imgu_dm.add("ipu3-imgu 0 3a stat"); > + imgu_dm.add("ipu3-imgu 1"); > + imgu_dm.add("ipu3-imgu 1 input"); > + imgu_dm.add("ipu3-imgu 1 parameters"); > + imgu_dm.add("ipu3-imgu 1 output"); > + imgu_dm.add("ipu3-imgu 1 viewfinder"); > + imgu_dm.add("ipu3-imgu 1 3a stat"); > + > + imgu_ = enumerator->search(imgu_dm); > + if (!imgu_) { > + cio2_->release(); > + return false; > + } > + > + imgu_->acquire(); I would reorder this a bit. ... cio2_ = enumerator->search(cio2_dm); if (!cio2_) return false; imgu_ = enumerator->search(imgu_dm); if (!imgu_) return false; cio2_->acquire(); imgu_->acquire(); ... I don't feel strongly about this so if others have a different view I will yield to public opinion :-) > + > + /* > + * TODO: create cameras. As of now, just create a dummy one > + * to verify enumeration and matching on IPU3. > + */ > + camera_ = new Camera("IPU3 Camera"); > + > + return true; > +} > + > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build > new file mode 100644 > index 0000000..0ab766a > --- /dev/null > +++ b/src/libcamera/pipeline/ipu3/meson.build > @@ -0,0 +1,3 @@ > +libcamera_sources += files([ > + 'ipu3.cpp', > +]) > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build > index 615ecd2..811c075 100644 > --- a/src/libcamera/pipeline/meson.build > +++ b/src/libcamera/pipeline/meson.build > @@ -1,3 +1,5 @@ > libcamera_sources += files([ > 'vimc.cpp', > ]) > + > +subdir('ipu3') > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Tue, Jan 15, 2019 at 09:38:24PM +0100, Niklas Söderlund wrote: > On 2019-01-11 17:51:09 +0100, Jacopo Mondi wrote: > > Add a pipeline handler skeleton for the Intel IPU3 device. > > Tested with on Soraka, listing detected cameras on the system and > > verifying the pipeline handler gets properly matched. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > > > Let's start by simply matching the pipeline handler with the device > > it is running on. The here created single camera gets properly enumerated > > on Soraka by the 'list-cameras' test: > > > > ./test/list-cameras > > [0:35:35.453249952] DBG pipeline_handler.cpp:119 Pipeline handler: "PipeHandlerVimc" registered > > [0:35:35.453538626] DBG pipeline_handler.cpp:119 Pipeline handler: "PipelineHandlerIPU3" registered > > [0:35:35.458316459] DBG device_enumerator.cpp:214 New media device: ipu3-imgu created from: /dev/media1 > > [0:35:35.469071318] DBG device_enumerator.cpp:214 New media device: ipu3-cio2 created from: /dev/media0 > > [0:35:35.475305874] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-cio2 > > [0:35:35.475343991] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-imgu > > [0:35:35.475354057] DBG pipeline_handler.cpp:150 Pipeline handler: "PipelineHandlerIPU3" matched > > - IPU3 Camera > > > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 119 ++++++++++++++++++++++++ > > src/libcamera/pipeline/ipu3/meson.build | 3 + > > src/libcamera/pipeline/meson.build | 2 + > > 3 files changed, 124 insertions(+) > > create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp > > create mode 100644 src/libcamera/pipeline/ipu3/meson.build > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > new file mode 100644 > > index 0000000..477a9a2 > > --- /dev/null > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp [snip] > > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > +{ > > + DeviceMatch cio2_dm("ipu3-cio2"); > > + cio2_dm.add("ipu3-csi2 0"); > > + cio2_dm.add("ipu3-cio2 0"); > > + cio2_dm.add("ipu3-csi2 1"); > > + cio2_dm.add("ipu3-cio2 1"); > > + cio2_dm.add("ipu3-csi2 2"); > > + cio2_dm.add("ipu3-cio2 2"); > > + cio2_dm.add("ipu3-csi2 3"); > > + cio2_dm.add("ipu3-cio2 3"); > > + > > + cio2_ = enumerator->search(cio2_dm); > > + if (!cio2_) > > + return false; > > + > > + cio2_->acquire(); > > + > > + DeviceMatch imgu_dm("ipu3-imgu"); > > + imgu_dm.add("ipu3-imgu 0"); > > + imgu_dm.add("ipu3-imgu 0 input"); > > + imgu_dm.add("ipu3-imgu 0 parameters"); > > + imgu_dm.add("ipu3-imgu 0 output"); > > + imgu_dm.add("ipu3-imgu 0 viewfinder"); > > + imgu_dm.add("ipu3-imgu 0 3a stat"); > > + imgu_dm.add("ipu3-imgu 1"); > > + imgu_dm.add("ipu3-imgu 1 input"); > > + imgu_dm.add("ipu3-imgu 1 parameters"); > > + imgu_dm.add("ipu3-imgu 1 output"); > > + imgu_dm.add("ipu3-imgu 1 viewfinder"); > > + imgu_dm.add("ipu3-imgu 1 3a stat"); > > + > > + imgu_ = enumerator->search(imgu_dm); > > + if (!imgu_) { > > + cio2_->release(); > > + return false; > > + } > > + > > + imgu_->acquire(); > > I would reorder this a bit. > > ... > > cio2_ = enumerator->search(cio2_dm); > if (!cio2_) > return false; > > imgu_ = enumerator->search(imgu_dm); > if (!imgu_) > return false; > > cio2_->acquire(); > imgu_->acquire(); > > ... > > I don't feel strongly about this so if others have a different view I > will yield to public opinion :-) I was about to mention the same, so I can only agree :-) Furthermore, I think we can match on driver name only in this case, can't we ? > > + > > + /* > > + * TODO: create cameras. As of now, just create a dummy one > > + * to verify enumeration and matching on IPU3. > > + */ > > + camera_ = new Camera("IPU3 Camera"); > > + > > + return true; > > +} > > + > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); > > + > > +} /* namespace libcamera */ [snip]
Hi, First off, sorry Jacopo I now see there is a later version of this patch posted. I should have commented on that one. On 2019-01-15 23:25:07 +0200, Laurent Pinchart wrote: [snip] > > > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > > +{ > > > + DeviceMatch cio2_dm("ipu3-cio2"); > > > + cio2_dm.add("ipu3-csi2 0"); > > > + cio2_dm.add("ipu3-cio2 0"); > > > + cio2_dm.add("ipu3-csi2 1"); > > > + cio2_dm.add("ipu3-cio2 1"); > > > + cio2_dm.add("ipu3-csi2 2"); > > > + cio2_dm.add("ipu3-cio2 2"); > > > + cio2_dm.add("ipu3-csi2 3"); > > > + cio2_dm.add("ipu3-cio2 3"); > > > + > > > + cio2_ = enumerator->search(cio2_dm); > > > + if (!cio2_) > > > + return false; > > > + > > > + cio2_->acquire(); > > > + > > > + DeviceMatch imgu_dm("ipu3-imgu"); > > > + imgu_dm.add("ipu3-imgu 0"); > > > + imgu_dm.add("ipu3-imgu 0 input"); > > > + imgu_dm.add("ipu3-imgu 0 parameters"); > > > + imgu_dm.add("ipu3-imgu 0 output"); > > > + imgu_dm.add("ipu3-imgu 0 viewfinder"); > > > + imgu_dm.add("ipu3-imgu 0 3a stat"); > > > + imgu_dm.add("ipu3-imgu 1"); > > > + imgu_dm.add("ipu3-imgu 1 input"); > > > + imgu_dm.add("ipu3-imgu 1 parameters"); > > > + imgu_dm.add("ipu3-imgu 1 output"); > > > + imgu_dm.add("ipu3-imgu 1 viewfinder"); > > > + imgu_dm.add("ipu3-imgu 1 3a stat"); > > > + > > > + imgu_ = enumerator->search(imgu_dm); > > > + if (!imgu_) { > > > + cio2_->release(); > > > + return false; > > > + } > > > + > > > + imgu_->acquire(); > > > > I would reorder this a bit. > > > > ... > > > > cio2_ = enumerator->search(cio2_dm); > > if (!cio2_) > > return false; > > > > imgu_ = enumerator->search(imgu_dm); > > if (!imgu_) > > return false; > > > > cio2_->acquire(); > > imgu_->acquire(); > > > > ... > > > > I don't feel strongly about this so if others have a different view I > > will yield to public opinion :-) > > I was about to mention the same, so I can only agree :-) > > Furthermore, I think we can match on driver name only in this case, > can't we ? I think we need to design some sort of guide lines here. It's true we could match on driver name only here. By doing so we no longer ensures that the media devices have all the entities we expect it to have, and may therefor accept a media device which is 1. From a different kernel version/patch series. 2. Not probed completely. 3. Expresses a variation of hardware with more or less capabilities then the one the pipeline handler was developed for. Reason 1 should not be a real issue, but API breakages do happen... This could then be solved with a check of the running kernel version which the matching logic could be expanded to cover. I'm thinking here of UVC where a lot of the entity names are not static and we would need to depend on the driver name alone. Reason 2 is valid and a issue discussed at some V4L2 conference and is the main blocker for video devices can't be register at probe time. Once the kernel have a way to express that a media graph is complete we should use that in libcamera. Reason 3 is a real problem as we don't know what variations of a hardware block will be released in a slightly different form in the future which a driver would be extended to support.
Hi Niklas, On Tue, Jan 15, 2019 at 10:43:49PM +0100, Niklas Söderlund wrote: > Hi, > > First off, sorry Jacopo I now see there is a later version of this patch > posted. I should have commented on that one. > > On 2019-01-15 23:25:07 +0200, Laurent Pinchart wrote: > > [snip] > > >>> +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > >>> +{ > >>> + DeviceMatch cio2_dm("ipu3-cio2"); > >>> + cio2_dm.add("ipu3-csi2 0"); > >>> + cio2_dm.add("ipu3-cio2 0"); > >>> + cio2_dm.add("ipu3-csi2 1"); > >>> + cio2_dm.add("ipu3-cio2 1"); > >>> + cio2_dm.add("ipu3-csi2 2"); > >>> + cio2_dm.add("ipu3-cio2 2"); > >>> + cio2_dm.add("ipu3-csi2 3"); > >>> + cio2_dm.add("ipu3-cio2 3"); > >>> + > >>> + cio2_ = enumerator->search(cio2_dm); > >>> + if (!cio2_) > >>> + return false; > >>> + > >>> + cio2_->acquire(); > >>> + > >>> + DeviceMatch imgu_dm("ipu3-imgu"); > >>> + imgu_dm.add("ipu3-imgu 0"); > >>> + imgu_dm.add("ipu3-imgu 0 input"); > >>> + imgu_dm.add("ipu3-imgu 0 parameters"); > >>> + imgu_dm.add("ipu3-imgu 0 output"); > >>> + imgu_dm.add("ipu3-imgu 0 viewfinder"); > >>> + imgu_dm.add("ipu3-imgu 0 3a stat"); > >>> + imgu_dm.add("ipu3-imgu 1"); > >>> + imgu_dm.add("ipu3-imgu 1 input"); > >>> + imgu_dm.add("ipu3-imgu 1 parameters"); > >>> + imgu_dm.add("ipu3-imgu 1 output"); > >>> + imgu_dm.add("ipu3-imgu 1 viewfinder"); > >>> + imgu_dm.add("ipu3-imgu 1 3a stat"); > >>> + > >>> + imgu_ = enumerator->search(imgu_dm); > >>> + if (!imgu_) { > >>> + cio2_->release(); > >>> + return false; > >>> + } > >>> + > >>> + imgu_->acquire(); > >> > >> I would reorder this a bit. > >> > >> ... > >> > >> cio2_ = enumerator->search(cio2_dm); > >> if (!cio2_) > >> return false; > >> > >> imgu_ = enumerator->search(imgu_dm); > >> if (!imgu_) > >> return false; > >> > >> cio2_->acquire(); > >> imgu_->acquire(); > >> > >> ... > >> > >> I don't feel strongly about this so if others have a different view I > >> will yield to public opinion :-) > > > > I was about to mention the same, so I can only agree :-) > > > > Furthermore, I think we can match on driver name only in this case, > > can't we ? > > I think we need to design some sort of guide lines here. It's true we > could match on driver name only here. By doing so we no longer ensures > that the media devices have all the entities we expect it to have, and > may therefor accept a media device which is > > 1. From a different kernel version/patch series. > 2. Not probed completely. > 3. Expresses a variation of hardware with more or less capabilities > then the one the pipeline handler was developed for. > > Reason 1 should not be a real issue, but API breakages do happen... This > could then be solved with a check of the running kernel version which > the matching logic could be expanded to cover. I'm thinking here of UVC > where a lot of the entity names are not static and we would need to > depend on the driver name alone. I think we should add a kernel version (or rather media device version) check in the DeviceMatch in any case, it can be useful. > Reason 2 is valid and a issue discussed at some V4L2 conference and is > the main blocker for video devices can't be register at probe time. Once > the kernel have a way to express that a media graph is complete we > should use that in libcamera. It is valid, but not for all devices. For the ImgU for instance, we have no external entity, so it shouldn't be an issue. Furthermore, for the CIO2, we indeed want to make sure all entities are successfully probed, but we don't know what sensor are present in the system, so the only entities we can match on anyway are internal to the CIO2, which seems a bit pointless to me. > Reason 3 is a real problem as we don't know what variations of a > hardware block will be released in a slightly different form in the > future which a driver would be extended to support. Same as for reason 2, this is valid, but not for all devices. In the IPU3 case I believe matching on entities would only be useful to handle the first issue you pointed out, and I wonder if we need to do so now.
HI Laurent, On 2019-01-16 17:42:30 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Tue, Jan 15, 2019 at 10:43:49PM +0100, Niklas Söderlund wrote: > > Hi, > > > > First off, sorry Jacopo I now see there is a later version of this patch > > posted. I should have commented on that one. > > > > On 2019-01-15 23:25:07 +0200, Laurent Pinchart wrote: > > > > [snip] > > > > >>> +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > >>> +{ > > >>> + DeviceMatch cio2_dm("ipu3-cio2"); > > >>> + cio2_dm.add("ipu3-csi2 0"); > > >>> + cio2_dm.add("ipu3-cio2 0"); > > >>> + cio2_dm.add("ipu3-csi2 1"); > > >>> + cio2_dm.add("ipu3-cio2 1"); > > >>> + cio2_dm.add("ipu3-csi2 2"); > > >>> + cio2_dm.add("ipu3-cio2 2"); > > >>> + cio2_dm.add("ipu3-csi2 3"); > > >>> + cio2_dm.add("ipu3-cio2 3"); > > >>> + > > >>> + cio2_ = enumerator->search(cio2_dm); > > >>> + if (!cio2_) > > >>> + return false; > > >>> + > > >>> + cio2_->acquire(); > > >>> + > > >>> + DeviceMatch imgu_dm("ipu3-imgu"); > > >>> + imgu_dm.add("ipu3-imgu 0"); > > >>> + imgu_dm.add("ipu3-imgu 0 input"); > > >>> + imgu_dm.add("ipu3-imgu 0 parameters"); > > >>> + imgu_dm.add("ipu3-imgu 0 output"); > > >>> + imgu_dm.add("ipu3-imgu 0 viewfinder"); > > >>> + imgu_dm.add("ipu3-imgu 0 3a stat"); > > >>> + imgu_dm.add("ipu3-imgu 1"); > > >>> + imgu_dm.add("ipu3-imgu 1 input"); > > >>> + imgu_dm.add("ipu3-imgu 1 parameters"); > > >>> + imgu_dm.add("ipu3-imgu 1 output"); > > >>> + imgu_dm.add("ipu3-imgu 1 viewfinder"); > > >>> + imgu_dm.add("ipu3-imgu 1 3a stat"); > > >>> + > > >>> + imgu_ = enumerator->search(imgu_dm); > > >>> + if (!imgu_) { > > >>> + cio2_->release(); > > >>> + return false; > > >>> + } > > >>> + > > >>> + imgu_->acquire(); > > >> > > >> I would reorder this a bit. > > >> > > >> ... > > >> > > >> cio2_ = enumerator->search(cio2_dm); > > >> if (!cio2_) > > >> return false; > > >> > > >> imgu_ = enumerator->search(imgu_dm); > > >> if (!imgu_) > > >> return false; > > >> > > >> cio2_->acquire(); > > >> imgu_->acquire(); > > >> > > >> ... > > >> > > >> I don't feel strongly about this so if others have a different view I > > >> will yield to public opinion :-) > > > > > > I was about to mention the same, so I can only agree :-) > > > > > > Furthermore, I think we can match on driver name only in this case, > > > can't we ? > > > > I think we need to design some sort of guide lines here. It's true we > > could match on driver name only here. By doing so we no longer ensures > > that the media devices have all the entities we expect it to have, and > > may therefor accept a media device which is > > > > 1. From a different kernel version/patch series. > > 2. Not probed completely. > > 3. Expresses a variation of hardware with more or less capabilities > > then the one the pipeline handler was developed for. > > > > Reason 1 should not be a real issue, but API breakages do happen... This > > could then be solved with a check of the running kernel version which > > the matching logic could be expanded to cover. I'm thinking here of UVC > > where a lot of the entity names are not static and we would need to > > depend on the driver name alone. > > I think we should add a kernel version (or rather media device version) > check in the DeviceMatch in any case, it can be useful. > > > Reason 2 is valid and a issue discussed at some V4L2 conference and is > > the main blocker for video devices can't be register at probe time. Once > > the kernel have a way to express that a media graph is complete we > > should use that in libcamera. > > It is valid, but not for all devices. For the ImgU for instance, we have > no external entity, so it shouldn't be an issue. > > Furthermore, for the CIO2, we indeed want to make sure all entities are > successfully probed, but we don't know what sensor are present in the > system, so the only entities we can match on anyway are internal to the > CIO2, which seems a bit pointless to me. > > > Reason 3 is a real problem as we don't know what variations of a > > hardware block will be released in a slightly different form in the > > future which a driver would be extended to support. > > Same as for reason 2, this is valid, but not for all devices. > > In the IPU3 case I believe matching on entities would only be useful to > handle the first issue you pointed out, and I wonder if we need to do so > now. I agree with your comments. My point is that we should think about these issues when developing the first pipeline handlers. Even if it's not strictly needed for the IPU3 it might be a good idea to match on all entities we know should be there (or not). Cargo cult programming is areal thing :-)
Hi Niklas, On Wed, Jan 16, 2019 at 04:53:58PM +0100, Niklas Söderlund wrote: > On 2019-01-16 17:42:30 +0200, Laurent Pinchart wrote: > > On Tue, Jan 15, 2019 at 10:43:49PM +0100, Niklas Söderlund wrote: > >> Hi, > >> > >> First off, sorry Jacopo I now see there is a later version of this patch > >> posted. I should have commented on that one. > >> > >> On 2019-01-15 23:25:07 +0200, Laurent Pinchart wrote: > >> > >> [snip] > >> > >>>>> +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > >>>>> +{ > >>>>> + DeviceMatch cio2_dm("ipu3-cio2"); > >>>>> + cio2_dm.add("ipu3-csi2 0"); > >>>>> + cio2_dm.add("ipu3-cio2 0"); > >>>>> + cio2_dm.add("ipu3-csi2 1"); > >>>>> + cio2_dm.add("ipu3-cio2 1"); > >>>>> + cio2_dm.add("ipu3-csi2 2"); > >>>>> + cio2_dm.add("ipu3-cio2 2"); > >>>>> + cio2_dm.add("ipu3-csi2 3"); > >>>>> + cio2_dm.add("ipu3-cio2 3"); > >>>>> + > >>>>> + cio2_ = enumerator->search(cio2_dm); > >>>>> + if (!cio2_) > >>>>> + return false; > >>>>> + > >>>>> + cio2_->acquire(); > >>>>> + > >>>>> + DeviceMatch imgu_dm("ipu3-imgu"); > >>>>> + imgu_dm.add("ipu3-imgu 0"); > >>>>> + imgu_dm.add("ipu3-imgu 0 input"); > >>>>> + imgu_dm.add("ipu3-imgu 0 parameters"); > >>>>> + imgu_dm.add("ipu3-imgu 0 output"); > >>>>> + imgu_dm.add("ipu3-imgu 0 viewfinder"); > >>>>> + imgu_dm.add("ipu3-imgu 0 3a stat"); > >>>>> + imgu_dm.add("ipu3-imgu 1"); > >>>>> + imgu_dm.add("ipu3-imgu 1 input"); > >>>>> + imgu_dm.add("ipu3-imgu 1 parameters"); > >>>>> + imgu_dm.add("ipu3-imgu 1 output"); > >>>>> + imgu_dm.add("ipu3-imgu 1 viewfinder"); > >>>>> + imgu_dm.add("ipu3-imgu 1 3a stat"); > >>>>> + > >>>>> + imgu_ = enumerator->search(imgu_dm); > >>>>> + if (!imgu_) { > >>>>> + cio2_->release(); > >>>>> + return false; > >>>>> + } > >>>>> + > >>>>> + imgu_->acquire(); > >>>> > >>>> I would reorder this a bit. > >>>> > >>>> ... > >>>> > >>>> cio2_ = enumerator->search(cio2_dm); > >>>> if (!cio2_) > >>>> return false; > >>>> > >>>> imgu_ = enumerator->search(imgu_dm); > >>>> if (!imgu_) > >>>> return false; > >>>> > >>>> cio2_->acquire(); > >>>> imgu_->acquire(); > >>>> > >>>> ... > >>>> > >>>> I don't feel strongly about this so if others have a different view I > >>>> will yield to public opinion :-) > >>> > >>> I was about to mention the same, so I can only agree :-) > >>> > >>> Furthermore, I think we can match on driver name only in this case, > >>> can't we ? > >> > >> I think we need to design some sort of guide lines here. It's true we > >> could match on driver name only here. By doing so we no longer ensures > >> that the media devices have all the entities we expect it to have, and > >> may therefor accept a media device which is > >> > >> 1. From a different kernel version/patch series. > >> 2. Not probed completely. > >> 3. Expresses a variation of hardware with more or less capabilities > >> then the one the pipeline handler was developed for. > >> > >> Reason 1 should not be a real issue, but API breakages do happen... This > >> could then be solved with a check of the running kernel version which > >> the matching logic could be expanded to cover. I'm thinking here of UVC > >> where a lot of the entity names are not static and we would need to > >> depend on the driver name alone. > > > > I think we should add a kernel version (or rather media device version) > > check in the DeviceMatch in any case, it can be useful. > > > >> Reason 2 is valid and a issue discussed at some V4L2 conference and is > >> the main blocker for video devices can't be register at probe time. Once > >> the kernel have a way to express that a media graph is complete we > >> should use that in libcamera. > > > > It is valid, but not for all devices. For the ImgU for instance, we have > > no external entity, so it shouldn't be an issue. > > > > Furthermore, for the CIO2, we indeed want to make sure all entities are > > successfully probed, but we don't know what sensor are present in the > > system, so the only entities we can match on anyway are internal to the > > CIO2, which seems a bit pointless to me. > > > >> Reason 3 is a real problem as we don't know what variations of a > >> hardware block will be released in a slightly different form in the > >> future which a driver would be extended to support. > > > > Same as for reason 2, this is valid, but not for all devices. > > > > In the IPU3 case I believe matching on entities would only be useful to > > handle the first issue you pointed out, and I wonder if we need to do so > > now. > > I agree with your comments. > > My point is that we should think about these issues when developing the > first pipeline handlers. Even if it's not strictly needed for the IPU3 > it might be a good idea to match on all entities we know should be there > (or not). Cargo cult programming is areal thing :-) Even better, we should document the usage guidelines for DeviceMatch :-) Would you like to capture this in a documentation patch ?
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp new file mode 100644 index 0000000..477a9a2 --- /dev/null +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -0,0 +1,119 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipu3.cpp - Pipeline handler for Intel IPU3 + */ + +#include <libcamera/camera.h> + +#include "device_enumerator.h" +#include "media_device.h" +#include "pipeline_handler.h" + +#include "log.h" + +namespace libcamera { + +class PipelineHandlerIPU3 : public PipelineHandler +{ +public: + PipelineHandlerIPU3(); + ~PipelineHandlerIPU3(); + + bool match(DeviceEnumerator *enumerator); + + unsigned int count(); + Camera *camera(unsigned int id) final; + +private: + MediaDevice *cio2_; + MediaDevice *imgu_; + + Camera *camera_; +}; + +PipelineHandlerIPU3::PipelineHandlerIPU3() + : cio2_(nullptr), imgu_(nullptr), camera_(nullptr) +{ +} + +PipelineHandlerIPU3::~PipelineHandlerIPU3() +{ + if (cio2_) + cio2_->release(); + if (imgu_) + imgu_->release(); + if (camera_) + camera_->put(); + + cio2_ = nullptr; + imgu_ = nullptr; + camera_ = nullptr; +} + +unsigned int PipelineHandlerIPU3::count() +{ + return 1; +} + +Camera *PipelineHandlerIPU3::camera(unsigned int id) +{ + if (id != 0) + return nullptr; + + return camera_; +} + +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) +{ + DeviceMatch cio2_dm("ipu3-cio2"); + cio2_dm.add("ipu3-csi2 0"); + cio2_dm.add("ipu3-cio2 0"); + cio2_dm.add("ipu3-csi2 1"); + cio2_dm.add("ipu3-cio2 1"); + cio2_dm.add("ipu3-csi2 2"); + cio2_dm.add("ipu3-cio2 2"); + cio2_dm.add("ipu3-csi2 3"); + cio2_dm.add("ipu3-cio2 3"); + + cio2_ = enumerator->search(cio2_dm); + if (!cio2_) + return false; + + cio2_->acquire(); + + DeviceMatch imgu_dm("ipu3-imgu"); + imgu_dm.add("ipu3-imgu 0"); + imgu_dm.add("ipu3-imgu 0 input"); + imgu_dm.add("ipu3-imgu 0 parameters"); + imgu_dm.add("ipu3-imgu 0 output"); + imgu_dm.add("ipu3-imgu 0 viewfinder"); + imgu_dm.add("ipu3-imgu 0 3a stat"); + imgu_dm.add("ipu3-imgu 1"); + imgu_dm.add("ipu3-imgu 1 input"); + imgu_dm.add("ipu3-imgu 1 parameters"); + imgu_dm.add("ipu3-imgu 1 output"); + imgu_dm.add("ipu3-imgu 1 viewfinder"); + imgu_dm.add("ipu3-imgu 1 3a stat"); + + imgu_ = enumerator->search(imgu_dm); + if (!imgu_) { + cio2_->release(); + return false; + } + + imgu_->acquire(); + + /* + * TODO: create cameras. As of now, just create a dummy one + * to verify enumeration and matching on IPU3. + */ + camera_ = new Camera("IPU3 Camera"); + + return true; +} + +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3); + +} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build new file mode 100644 index 0000000..0ab766a --- /dev/null +++ b/src/libcamera/pipeline/ipu3/meson.build @@ -0,0 +1,3 @@ +libcamera_sources += files([ + 'ipu3.cpp', +]) diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build index 615ecd2..811c075 100644 --- a/src/libcamera/pipeline/meson.build +++ b/src/libcamera/pipeline/meson.build @@ -1,3 +1,5 @@ libcamera_sources += files([ 'vimc.cpp', ]) + +subdir('ipu3')
Add a pipeline handler skeleton for the Intel IPU3 device. Tested with on Soraka, listing detected cameras on the system and verifying the pipeline handler gets properly matched. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- Let's start by simply matching the pipeline handler with the device it is running on. The here created single camera gets properly enumerated on Soraka by the 'list-cameras' test: ./test/list-cameras [0:35:35.453249952] DBG pipeline_handler.cpp:119 Pipeline handler: "PipeHandlerVimc" registered [0:35:35.453538626] DBG pipeline_handler.cpp:119 Pipeline handler: "PipelineHandlerIPU3" registered [0:35:35.458316459] DBG device_enumerator.cpp:214 New media device: ipu3-imgu created from: /dev/media1 [0:35:35.469071318] DBG device_enumerator.cpp:214 New media device: ipu3-cio2 created from: /dev/media0 [0:35:35.475305874] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-cio2 [0:35:35.475343991] DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-imgu [0:35:35.475354057] DBG pipeline_handler.cpp:150 Pipeline handler: "PipelineHandlerIPU3" matched - IPU3 Camera --- src/libcamera/pipeline/ipu3/ipu3.cpp | 119 ++++++++++++++++++++++++ src/libcamera/pipeline/ipu3/meson.build | 3 + src/libcamera/pipeline/meson.build | 2 + 3 files changed, 124 insertions(+) create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp create mode 100644 src/libcamera/pipeline/ipu3/meson.build -- 2.20.1