| Message ID | 20210212190557.13905-1-dafna.hirschfeld@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Dafna, Thank you for the patch. On Fri, Feb 12, 2021 at 08:05:57PM +0100, Dafna Hirschfeld wrote: > Currently the call to the configure method of rkisp1 IPA > is called upon the 'start' of the pipeline. This should be done > in the 'configure' method instead. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------ > 1 file changed, 31 insertions(+), 32 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index e85979a7..f15efa9f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > << "ISP output pad configured with " << format.toString() > << " crop " << rect.toString(); > > + std::map<unsigned int, IPAStream> streamConfig; > + > for (const StreamConfiguration &cfg : *config) { > - if (cfg.stream() == &data->mainPathStream_) > + if (cfg.stream() == &data->mainPathStream_) { > ret = mainPath_.configure(cfg, format); > - else > + if (data->mainPath_->isEnabled()) > + streamConfig[0] = { > + .pixelFormat = cfg.pixelFormat, > + .size = cfg.size, > + }; > + } else { > ret = selfPath_.configure(cfg, format); > + if (data->selfPath_->isEnabled()) > + streamConfig[1] = { > + .pixelFormat = cfg.pixelFormat, > + .size = cfg.size, > + }; > + } > > if (ret) > return ret; > @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > + /* Inform IPA of stream configuration and sensor controls. */ > + CameraSensorInfo sensorInfo = {}; > + ret = data->sensor_->sensorInfo(&sensorInfo); > + if (ret) { > + /* \todo Turn this in an hard failure. */ > + LOG(RkISP1, Warning) << "Camera sensor information not available"; > + sensorInfo = {}; > + ret = 0; > + } > + > + std::map<unsigned int, const ControlInfoMap &> entityControls; > + entityControls.emplace(0, data->sensor_->controls()); > + > + IPAOperationData ipaConfig; > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); > + > return 0; > } > > @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > return ret; > } > > - std::map<unsigned int, IPAStream> streamConfig; > - > if (data->mainPath_->isEnabled()) { > ret = mainPath_.start(); > if (ret) { > @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } > - > - streamConfig[0] = { > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > - .size = data->mainPathStream_.configuration().size, > - }; > } > > if (data->selfPath_->isEnabled()) { > @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } > - > - streamConfig[1] = { > - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > - .size = data->selfPathStream_.configuration().size, > - }; > } > > isp_->setFrameStartEnabled(true); > > activeCamera_ = camera; > - > - /* Inform IPA of stream configuration and sensor controls. */ > - CameraSensorInfo sensorInfo = {}; > - ret = data->sensor_->sensorInfo(&sensorInfo); > - if (ret) { > - /* \todo Turn this in an hard failure. */ > - LOG(RkISP1, Warning) << "Camera sensor information not available"; > - sensorInfo = {}; > - ret = 0; > - } > - > - std::map<unsigned int, const ControlInfoMap &> entityControls; > - entityControls.emplace(0, data->sensor_->controls()); > - > - IPAOperationData ipaConfig; > - data->ipa_->configure(sensorInfo, streamConfig, entityControls, > - ipaConfig, nullptr); > - > return ret; > } > > -- > 2.17.1 >
Hey Dafna, Thank you for the patch. On 12.02.2021 20:05, Dafna Hirschfeld wrote: >Currently the call to the configure method of rkisp1 IPA >is called upon the 'start' of the pipeline. This should be done >in the 'configure' method instead. I assume that the reason for this change is consistency right? I believe the raspberry Pi has the most mature pipeline and IPA implementation and it makes sense to have some kind of consistent pattern. In order to find out why it is necessary to configure the IPA in configure instead of start, I first looked into `src/cam/capture.cpp`, where both methods are called. In between the configure and the start call, the buffers are allocated and the initial set of requests is created. Additionally, I have compared both the RkISP1 and Raspberry Pi implementation of the IPA configure method. Currently, the RkISP1 IPA doesn't do a lot so it is difficult to decide, which parts might be added. The Raspberry Pi currently does the following actions that are missing in the RkISP1: - Setup and validate the ISP controls - Setup metadata controls - Load device specific information from a helper - Load the lens shading table - Prepare the algorithms by setting the camera mode - Load the tuning file for the sensor What they both do is initialize the analog gain and exposure controls. (With the difference that RkISP1 sets both to their respective minimum and Raspberry Pi provides default values). This means, that the different order of the IPA configure method can only have an impact on the actions happening in between the two states configured and start (allocation of buffers and creation of the inital requests). So, the current advantage/disadvantage is that the controls are reset to their minimum for the initial set of requests as well. From my point of view this seems to be a good preparation for the following changes to the RkISP1 IPA, even though it currently doesn't seem to do to much. Have I missed anything else that makes this order of actions essential? Besides that little analysis I have tested the patch by building it and performing a test capture. Both works without problems. Greetings, Sebastian I fixed a small typo below that existed before as well. > >Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >--- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------ > 1 file changed, 31 insertions(+), 32 deletions(-) > >diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >index e85979a7..f15efa9f 100644 >--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >@@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > << "ISP output pad configured with " << format.toString() > << " crop " << rect.toString(); > >+ std::map<unsigned int, IPAStream> streamConfig; >+ > for (const StreamConfiguration &cfg : *config) { >- if (cfg.stream() == &data->mainPathStream_) >+ if (cfg.stream() == &data->mainPathStream_) { > ret = mainPath_.configure(cfg, format); >- else >+ if (data->mainPath_->isEnabled()) >+ streamConfig[0] = { >+ .pixelFormat = cfg.pixelFormat, >+ .size = cfg.size, >+ }; >+ } else { > ret = selfPath_.configure(cfg, format); >+ if (data->selfPath_->isEnabled()) >+ streamConfig[1] = { >+ .pixelFormat = cfg.pixelFormat, >+ .size = cfg.size, >+ }; >+ } > > if (ret) > return ret; >@@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > >+ /* Inform IPA of stream configuration and sensor controls. */ >+ CameraSensorInfo sensorInfo = {}; >+ ret = data->sensor_->sensorInfo(&sensorInfo); >+ if (ret) { >+ /* \todo Turn this in an hard failure. */ s/Turn this in an hard failure./Turn this into a hard failure./ (or maybe: Make this a hard failure) >+ LOG(RkISP1, Warning) << "Camera sensor information not available"; >+ sensorInfo = {}; >+ ret = 0; >+ } >+ >+ std::map<unsigned int, const ControlInfoMap &> entityControls; >+ entityControls.emplace(0, data->sensor_->controls()); >+ >+ IPAOperationData ipaConfig; >+ data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); >+ > return 0; > } > >@@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > return ret; > } > >- std::map<unsigned int, IPAStream> streamConfig; >- > if (data->mainPath_->isEnabled()) { > ret = mainPath_.start(); > if (ret) { >@@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } >- >- streamConfig[0] = { >- .pixelFormat = data->mainPathStream_.configuration().pixelFormat, >- .size = data->mainPathStream_.configuration().size, >- }; > } > > if (data->selfPath_->isEnabled()) { >@@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } >- >- streamConfig[1] = { >- .pixelFormat = data->selfPathStream_.configuration().pixelFormat, >- .size = data->selfPathStream_.configuration().size, >- }; > } > > isp_->setFrameStartEnabled(true); > > activeCamera_ = camera; >- >- /* Inform IPA of stream configuration and sensor controls. */ >- CameraSensorInfo sensorInfo = {}; >- ret = data->sensor_->sensorInfo(&sensorInfo); >- if (ret) { >- /* \todo Turn this in an hard failure. */ >- LOG(RkISP1, Warning) << "Camera sensor information not available"; >- sensorInfo = {}; >- ret = 0; >- } >- >- std::map<unsigned int, const ControlInfoMap &> entityControls; >- entityControls.emplace(0, data->sensor_->controls()); >- >- IPAOperationData ipaConfig; >- data->ipa_->configure(sensorInfo, streamConfig, entityControls, >- ipaConfig, nullptr); >- > return ret; > } > >-- >2.17.1 > >_______________________________________________ >libcamera-devel mailing list >libcamera-devel@lists.libcamera.org >https://lists.libcamera.org/listinfo/libcamera-devel
Hi Dafna, Thank you for the patch. On Fri, Feb 12, 2021 at 08:05:57PM +0100, Dafna Hirschfeld wrote: > Currently the call to the configure method of rkisp1 IPA > is called upon the 'start' of the pipeline. This should be done > in the 'configure' method instead. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------ > 1 file changed, 31 insertions(+), 32 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index e85979a7..f15efa9f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > << "ISP output pad configured with " << format.toString() > << " crop " << rect.toString(); > > + std::map<unsigned int, IPAStream> streamConfig; > + > for (const StreamConfiguration &cfg : *config) { > - if (cfg.stream() == &data->mainPathStream_) > + if (cfg.stream() == &data->mainPathStream_) { > ret = mainPath_.configure(cfg, format); > - else > + if (data->mainPath_->isEnabled()) I think you can drop this check. isEnabled() will return true if PipelineHandlerRkISP1::initLinks() has enabled the path with setEnabled(), which is done when cfg.stream() == &data->mainPathStream_. This being said, I think there's a bug in the pipeline handler as links are never reset, but that's out of scope for this patch. > + streamConfig[0] = { > + .pixelFormat = cfg.pixelFormat, > + .size = cfg.size, > + }; > + } else { > ret = selfPath_.configure(cfg, format); > + if (data->selfPath_->isEnabled()) Same here. > + streamConfig[1] = { > + .pixelFormat = cfg.pixelFormat, > + .size = cfg.size, > + }; > + } > > if (ret) > return ret; > @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > + /* Inform IPA of stream configuration and sensor controls. */ > + CameraSensorInfo sensorInfo = {}; > + ret = data->sensor_->sensorInfo(&sensorInfo); > + if (ret) { > + /* \todo Turn this in an hard failure. */ > + LOG(RkISP1, Warning) << "Camera sensor information not available"; > + sensorInfo = {}; > + ret = 0; > + } > + > + std::map<unsigned int, const ControlInfoMap &> entityControls; > + entityControls.emplace(0, data->sensor_->controls()); > + > + IPAOperationData ipaConfig; > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); This could be wrapped to shorten the line. With these issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > return 0; > } > > @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > return ret; > } > > - std::map<unsigned int, IPAStream> streamConfig; > - > if (data->mainPath_->isEnabled()) { > ret = mainPath_.start(); > if (ret) { > @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } > - > - streamConfig[0] = { > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > - .size = data->mainPathStream_.configuration().size, > - }; > } > > if (data->selfPath_->isEnabled()) { > @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > freeBuffers(camera); > return ret; > } > - > - streamConfig[1] = { > - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > - .size = data->selfPathStream_.configuration().size, > - }; > } > > isp_->setFrameStartEnabled(true); > > activeCamera_ = camera; > - > - /* Inform IPA of stream configuration and sensor controls. */ > - CameraSensorInfo sensorInfo = {}; > - ret = data->sensor_->sensorInfo(&sensorInfo); > - if (ret) { > - /* \todo Turn this in an hard failure. */ > - LOG(RkISP1, Warning) << "Camera sensor information not available"; > - sensorInfo = {}; > - ret = 0; > - } > - > - std::map<unsigned int, const ControlInfoMap &> entityControls; > - entityControls.emplace(0, data->sensor_->controls()); > - > - IPAOperationData ipaConfig; > - data->ipa_->configure(sensorInfo, streamConfig, entityControls, > - ipaConfig, nullptr); > - > return ret; > } >
Hi Sebastian, On Sat, Feb 13, 2021 at 10:12:17AM +0100, Sebastian Fricke wrote: > On 12.02.2021 20:05, Dafna Hirschfeld wrote: > > Currently the call to the configure method of rkisp1 IPA > > is called upon the 'start' of the pipeline. This should be done > > in the 'configure' method instead. > > I assume that the reason for this change is consistency right? I believe > the raspberry Pi has the most mature pipeline and IPA implementation and > it makes sense to have some kind of consistent pattern. > > In order to find out why it is necessary to configure the IPA in > configure instead of start, I first looked into `src/cam/capture.cpp`, > where both methods are called. In between the configure and the start > call, the buffers are allocated and the initial set of requests is > created. > Additionally, I have compared both the RkISP1 and Raspberry Pi > implementation of the IPA configure method. Currently, the RkISP1 IPA > doesn't do a lot so it is difficult to decide, which parts might be added. > The Raspberry Pi currently does the following actions that are missing in > the RkISP1: > - Setup and validate the ISP controls > - Setup metadata controls > - Load device specific information from a helper > - Load the lens shading table > - Prepare the algorithms by setting the camera mode > - Load the tuning file for the sensor > > What they both do is initialize the analog gain and exposure controls. > (With the difference that RkISP1 sets both to their respective minimum > and Raspberry Pi provides default values). > > This means, that the different order of the IPA configure method can > only have an impact on the actions happening in between the two states > configured and start (allocation of buffers and creation of the inital > requests). So, the current advantage/disadvantage is that the controls > are reset to their minimum for the initial set of requests as well. Nice and detailed analysis :-) > From my point of view this seems to be a good preparation for the > following changes to the RkISP1 IPA, even though it currently doesn't > seem to do to much. Have I missed anything else that makes this order of > actions essential? Right now, no, but with Paul's series that implements the IPA IPC protocol, IPA functions called after start() must be asynchronous. The IPA configure() function is a synchronous call, so this won't work well. This patch is in preparation for the IPA IPC (beside improving consistency, which is usually good). > Besides that little analysis I have tested the patch by building it and > performing a test capture. Both works without problems. > > Greetings, > Sebastian > > I fixed a small typo below that existed before as well. > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------ > > 1 file changed, 31 insertions(+), 32 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index e85979a7..f15efa9f 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > << "ISP output pad configured with " << format.toString() > > << " crop " << rect.toString(); > > > > + std::map<unsigned int, IPAStream> streamConfig; > > + > > for (const StreamConfiguration &cfg : *config) { > > - if (cfg.stream() == &data->mainPathStream_) > > + if (cfg.stream() == &data->mainPathStream_) { > > ret = mainPath_.configure(cfg, format); > > - else > > + if (data->mainPath_->isEnabled()) > > + streamConfig[0] = { > > + .pixelFormat = cfg.pixelFormat, > > + .size = cfg.size, > > + }; > > + } else { > > ret = selfPath_.configure(cfg, format); > > + if (data->selfPath_->isEnabled()) > > + streamConfig[1] = { > > + .pixelFormat = cfg.pixelFormat, > > + .size = cfg.size, > > + }; > > + } > > > > if (ret) > > return ret; > > @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > if (ret) > > return ret; > > > > + /* Inform IPA of stream configuration and sensor controls. */ > > + CameraSensorInfo sensorInfo = {}; > > + ret = data->sensor_->sensorInfo(&sensorInfo); > > + if (ret) { > > + /* \todo Turn this in an hard failure. */ > > s/Turn this in an hard failure./Turn this into a hard failure./ > (or maybe: Make this a hard failure) > > > + LOG(RkISP1, Warning) << "Camera sensor information not available"; > > + sensorInfo = {}; > > + ret = 0; > > + } > > + > > + std::map<unsigned int, const ControlInfoMap &> entityControls; > > + entityControls.emplace(0, data->sensor_->controls()); > > + > > + IPAOperationData ipaConfig; > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); > > + > > return 0; > > } > > > > @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > return ret; > > } > > > > - std::map<unsigned int, IPAStream> streamConfig; > > - > > if (data->mainPath_->isEnabled()) { > > ret = mainPath_.start(); > > if (ret) { > > @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > freeBuffers(camera); > > return ret; > > } > > - > > - streamConfig[0] = { > > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > > - .size = data->mainPathStream_.configuration().size, > > - }; > > } > > > > if (data->selfPath_->isEnabled()) { > > @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > freeBuffers(camera); > > return ret; > > } > > - > > - streamConfig[1] = { > > - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > > - .size = data->selfPathStream_.configuration().size, > > - }; > > } > > > > isp_->setFrameStartEnabled(true); > > > > activeCamera_ = camera; > > - > > - /* Inform IPA of stream configuration and sensor controls. */ > > - CameraSensorInfo sensorInfo = {}; > > - ret = data->sensor_->sensorInfo(&sensorInfo); > > - if (ret) { > > - /* \todo Turn this in an hard failure. */ > > - LOG(RkISP1, Warning) << "Camera sensor information not available"; > > - sensorInfo = {}; > > - ret = 0; > > - } > > - > > - std::map<unsigned int, const ControlInfoMap &> entityControls; > > - entityControls.emplace(0, data->sensor_->controls()); > > - > > - IPAOperationData ipaConfig; > > - data->ipa_->configure(sensorInfo, streamConfig, entityControls, > > - ipaConfig, nullptr); > > - > > return ret; > > } > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index e85979a7..f15efa9f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) << "ISP output pad configured with " << format.toString() << " crop " << rect.toString(); + std::map<unsigned int, IPAStream> streamConfig; + for (const StreamConfiguration &cfg : *config) { - if (cfg.stream() == &data->mainPathStream_) + if (cfg.stream() == &data->mainPathStream_) { ret = mainPath_.configure(cfg, format); - else + if (data->mainPath_->isEnabled()) + streamConfig[0] = { + .pixelFormat = cfg.pixelFormat, + .size = cfg.size, + }; + } else { ret = selfPath_.configure(cfg, format); + if (data->selfPath_->isEnabled()) + streamConfig[1] = { + .pixelFormat = cfg.pixelFormat, + .size = cfg.size, + }; + } if (ret) return ret; @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret) return ret; + /* Inform IPA of stream configuration and sensor controls. */ + CameraSensorInfo sensorInfo = {}; + ret = data->sensor_->sensorInfo(&sensorInfo); + if (ret) { + /* \todo Turn this in an hard failure. */ + LOG(RkISP1, Warning) << "Camera sensor information not available"; + sensorInfo = {}; + ret = 0; + } + + std::map<unsigned int, const ControlInfoMap &> entityControls; + entityControls.emplace(0, data->sensor_->controls()); + + IPAOperationData ipaConfig; + data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr); + return 0; } @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c return ret; } - std::map<unsigned int, IPAStream> streamConfig; - if (data->mainPath_->isEnabled()) { ret = mainPath_.start(); if (ret) { @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c freeBuffers(camera); return ret; } - - streamConfig[0] = { - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, - .size = data->mainPathStream_.configuration().size, - }; } if (data->selfPath_->isEnabled()) { @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c freeBuffers(camera); return ret; } - - streamConfig[1] = { - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, - .size = data->selfPathStream_.configuration().size, - }; } isp_->setFrameStartEnabled(true); activeCamera_ = camera; - - /* Inform IPA of stream configuration and sensor controls. */ - CameraSensorInfo sensorInfo = {}; - ret = data->sensor_->sensorInfo(&sensorInfo); - if (ret) { - /* \todo Turn this in an hard failure. */ - LOG(RkISP1, Warning) << "Camera sensor information not available"; - sensorInfo = {}; - ret = 0; - } - - std::map<unsigned int, const ControlInfoMap &> entityControls; - entityControls.emplace(0, data->sensor_->controls()); - - IPAOperationData ipaConfig; - data->ipa_->configure(sensorInfo, streamConfig, entityControls, - ipaConfig, nullptr); - return ret; }
Currently the call to the configure method of rkisp1 IPA is called upon the 'start' of the pipeline. This should be done in the 'configure' method instead. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------ 1 file changed, 31 insertions(+), 32 deletions(-)