| Message ID | 20190603231637.28554-10-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Mon, Jun 03, 2019 at 07:16:36PM -0400, Paul Elder wrote: > After the pipeline handler acquires an IPA, it should save it for future > use. Store it in the pipeline data. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > New patch > > src/libcamera/include/pipeline_handler.h | 3 +++ > src/libcamera/pipeline_handler.cpp | 8 ++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index 67971b3..a2d9624 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -14,6 +14,7 @@ > #include <string> > #include <vector> > > +#include <libcamera/ipa_interface.h> > #include <libcamera/stream.h> > > namespace libcamera { > @@ -93,6 +94,8 @@ protected: > const char *name_; > uint32_t version_; > > + std::unique_ptr<IPAInterface> ipa_; > + > private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index b18f14d..d6e68b0 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -530,6 +530,14 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) > * \brief Pipeline handler version > */ > > +/** > + * \var PipelineHandler::ipa_ > + * \brief The IPA that the pipeline handler will use "The IPA interface instance for this pipeline handler" ? And thinking about it, will we always have a 1:1 mapping between pipeline handlers and IPA instances ? In the IPU3 case, if we support capturing from two cameras simultaneously, I think we'll have one IPA instance per camera, while both cameras will be handled by the same pipeline handler. We could store the IPA instance in the camera data instead. Or, for now, until we figure the details out, store it in the vimc pipeline handler in patch 10/10 (thus dropping this patch). What do you think ? > + * > + * The pipeline handler can acquire an IPA from the IPAManager. Once it is > + * acquired, it will be stored here. > + */ > + > /** > * \class PipelineHandlerFactory > * \brief Registration of PipelineHandler classes and creation of instances
Hi Laurent, On Tue, Jun 04, 2019 at 03:07:44PM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. Thank you for the review. > On Mon, Jun 03, 2019 at 07:16:36PM -0400, Paul Elder wrote: > > After the pipeline handler acquires an IPA, it should save it for future > > use. Store it in the pipeline data. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > New patch > > > > src/libcamera/include/pipeline_handler.h | 3 +++ > > src/libcamera/pipeline_handler.cpp | 8 ++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > index 67971b3..a2d9624 100644 > > --- a/src/libcamera/include/pipeline_handler.h > > +++ b/src/libcamera/include/pipeline_handler.h > > @@ -14,6 +14,7 @@ > > #include <string> > > #include <vector> > > > > +#include <libcamera/ipa_interface.h> > > #include <libcamera/stream.h> > > > > namespace libcamera { > > @@ -93,6 +94,8 @@ protected: > > const char *name_; > > uint32_t version_; > > > > + std::unique_ptr<IPAInterface> ipa_; > > + > > private: > > void mediaDeviceDisconnected(MediaDevice *media); > > virtual void disconnect(); > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index b18f14d..d6e68b0 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -530,6 +530,14 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) > > * \brief Pipeline handler version > > */ > > > > +/** > > + * \var PipelineHandler::ipa_ > > + * \brief The IPA that the pipeline handler will use > > "The IPA interface instance for this pipeline handler" ? > > And thinking about it, will we always have a 1:1 mapping between > pipeline handlers and IPA instances ? In the IPU3 case, if we support tbh, I was doubting a 1:1 mapping between pipeline handlers and IPA instances. > capturing from two cameras simultaneously, I think we'll have one IPA > instance per camera, while both cameras will be handled by the same > pipeline handler. We could store the IPA instance in the camera data > instead. Or, for now, until we figure the details out, store it in the > vimc pipeline handler in patch 10/10 (thus dropping this patch). What do > you think ? If the mapping is 1:1 between cameras and IPA instances then I think we can put the IPA instance in the camera data. > > + * > > + * The pipeline handler can acquire an IPA from the IPAManager. Once it is > > + * acquired, it will be stored here. > > + */ > > + > > /** > > * \class PipelineHandlerFactory > > * \brief Registration of PipelineHandler classes and creation of instances Thanks, Paul
Hi Paul, On Tue, Jun 04, 2019 at 12:24:03PM -0400, Paul Elder wrote: > On Tue, Jun 04, 2019 at 03:07:44PM +0300, Laurent Pinchart wrote: > > Hi Paul, > > > > Thank you for the patch. > > Thank you for the review. > > > On Mon, Jun 03, 2019 at 07:16:36PM -0400, Paul Elder wrote: > >> After the pipeline handler acquires an IPA, it should save it for future > >> use. Store it in the pipeline data. > >> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >> --- > >> New patch > >> > >> src/libcamera/include/pipeline_handler.h | 3 +++ > >> src/libcamera/pipeline_handler.cpp | 8 ++++++++ > >> 2 files changed, 11 insertions(+) > >> > >> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > >> index 67971b3..a2d9624 100644 > >> --- a/src/libcamera/include/pipeline_handler.h > >> +++ b/src/libcamera/include/pipeline_handler.h > >> @@ -14,6 +14,7 @@ > >> #include <string> > >> #include <vector> > >> > >> +#include <libcamera/ipa_interface.h> > >> #include <libcamera/stream.h> > >> > >> namespace libcamera { > >> @@ -93,6 +94,8 @@ protected: > >> const char *name_; > >> uint32_t version_; > >> > >> + std::unique_ptr<IPAInterface> ipa_; > >> + > >> private: > >> void mediaDeviceDisconnected(MediaDevice *media); > >> virtual void disconnect(); > >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > >> index b18f14d..d6e68b0 100644 > >> --- a/src/libcamera/pipeline_handler.cpp > >> +++ b/src/libcamera/pipeline_handler.cpp > >> @@ -530,6 +530,14 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) > >> * \brief Pipeline handler version > >> */ > >> > >> +/** > >> + * \var PipelineHandler::ipa_ > >> + * \brief The IPA that the pipeline handler will use > > > > "The IPA interface instance for this pipeline handler" ? > > > > And thinking about it, will we always have a 1:1 mapping between > > pipeline handlers and IPA instances ? In the IPU3 case, if we support > > tbh, I was doubting a 1:1 mapping between pipeline handlers and IPA > instances. > > > capturing from two cameras simultaneously, I think we'll have one IPA > > instance per camera, while both cameras will be handled by the same > > pipeline handler. We could store the IPA instance in the camera data > > instead. Or, for now, until we figure the details out, store it in the > > vimc pipeline handler in patch 10/10 (thus dropping this patch). What do > > you think ? > > If the mapping is 1:1 between cameras and IPA instances then I think we > can put the IPA instance in the camera data. I don't think it will be the case either. The rkisp1 can handle two cameras, but not at the same time, so they will likely be a single IPA instance (although that's actually debatable). How about not trying to make the code too generic yet and move the ipa_ member to the PipelineHandlerVimc class for now ? > >> + * > >> + * The pipeline handler can acquire an IPA from the IPAManager. Once it is > >> + * acquired, it will be stored here. > >> + */ > >> + > >> /** > >> * \class PipelineHandlerFactory > >> * \brief Registration of PipelineHandler classes and creation of instances
diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index 67971b3..a2d9624 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -14,6 +14,7 @@ #include <string> #include <vector> +#include <libcamera/ipa_interface.h> #include <libcamera/stream.h> namespace libcamera { @@ -93,6 +94,8 @@ protected: const char *name_; uint32_t version_; + std::unique_ptr<IPAInterface> ipa_; + private: void mediaDeviceDisconnected(MediaDevice *media); virtual void disconnect(); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index b18f14d..d6e68b0 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -530,6 +530,14 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) * \brief Pipeline handler version */ +/** + * \var PipelineHandler::ipa_ + * \brief The IPA that the pipeline handler will use + * + * The pipeline handler can acquire an IPA from the IPAManager. Once it is + * acquired, it will be stored here. + */ + /** * \class PipelineHandlerFactory * \brief Registration of PipelineHandler classes and creation of instances
After the pipeline handler acquires an IPA, it should save it for future use. Store it in the pipeline data. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New patch src/libcamera/include/pipeline_handler.h | 3 +++ src/libcamera/pipeline_handler.cpp | 8 ++++++++ 2 files changed, 11 insertions(+)