| Message ID | 20210312130304.34698-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
Hello Jean-Micheal, On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote: > The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to > calculate the statistics grids. > This patch makes it possible for PipelineHandlerIPU3::start() to access > the BDS configuration and later pass the rectangle to the IPU3 IPA > configure method. > The alternative would be having the pipe configuration retrieved from the ImgU device at start() which would require having the ImgUDevice stateful, which will be very painful to implement due to the awful pipe configuration procedure. Removing 'const' from the data_ field doesn't worry me too much, but smells like we're taking a shortcut. I'm fine with that for the time being. > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 00da2bb2..0d133031 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -70,6 +70,8 @@ public: > CIO2Device cio2_; > ImgUDevice *imgu_; > > + ImgUDevice::PipeConfig pipeConfig_; > + > Stream outStream_; > Stream vfStream_; > Stream rawStream_; > @@ -97,7 +99,6 @@ public: > Status validate() override; > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > /* Cache the combinedTransform_ that will be applied to the sensor */ > Transform combinedTransform_; > @@ -108,10 +109,9 @@ private: > * corresponding Camera instance is valid. In order to borrow a > * reference to the camera data, store a new reference to the camera. > */ > - const IPU3CameraData *data_; > + IPU3CameraData *data_; > > StreamConfiguration cio2Configuration_; > - ImgUDevice::PipeConfig pipeConfig_; > }; > > class PipelineHandlerIPU3 : public PipelineHandler > @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > /* Only compute the ImgU configuration if a YUV stream has been requested. */ > if (yuvCount) { > - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); > - if (pipeConfig_.isNull()) { > + ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe); Why do you need to go through an intermediate variable ? Can't you assign data_->pipeConfig_ directly here ? Thanks j > + if (imguConfig.isNull()) { > LOG(IPU3, Error) << "Failed to calculate pipe configuration: " > << "unsupported resolutions."; > return Invalid; > } > + data_->pipeConfig_ = imguConfig; > } > > return status; > @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * stream has been requested: return here to skip the ImgU configuration > * part. > */ > - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); > + ImgUDevice::PipeConfig imguConfig = data->pipeConfig_; > if (imguConfig.isNull()) > return 0; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 13/03/2021 09:31, Jacopo Mondi wrote: > Hello Jean-Micheal, > > On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote: >> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to >> calculate the statistics grids. >> This patch makes it possible for PipelineHandlerIPU3::start() to access >> the BDS configuration and later pass the rectangle to the IPU3 IPA >> configure method. >> > > The alternative would be having the pipe configuration retrieved from > the ImgU device at start() which would require having the ImgUDevice > stateful, which will be very painful to implement due to the awful > pipe configuration procedure. > > Removing 'const' from the data_ field doesn't worry me too much, but > smells like we're taking a shortcut. I'm fine with that for the time > being. Yes, there is no real easy straightforward way unfortunately... >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 00da2bb2..0d133031 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -70,6 +70,8 @@ public: >> CIO2Device cio2_; >> ImgUDevice *imgu_; >> >> + ImgUDevice::PipeConfig pipeConfig_; >> + >> Stream outStream_; >> Stream vfStream_; >> Stream rawStream_; >> @@ -97,7 +99,6 @@ public: >> Status validate() override; >> >> const StreamConfiguration &cio2Format() const { return cio2Configuration_; } >> - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } >> >> /* Cache the combinedTransform_ that will be applied to the sensor */ >> Transform combinedTransform_; >> @@ -108,10 +109,9 @@ private: >> * corresponding Camera instance is valid. In order to borrow a >> * reference to the camera data, store a new reference to the camera. >> */ >> - const IPU3CameraData *data_; >> + IPU3CameraData *data_; >> >> StreamConfiguration cio2Configuration_; >> - ImgUDevice::PipeConfig pipeConfig_; >> }; >> >> class PipelineHandlerIPU3 : public PipelineHandler >> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >> >> /* Only compute the ImgU configuration if a YUV stream has been requested. */ >> if (yuvCount) { >> - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); >> - if (pipeConfig_.isNull()) { >> + ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe); > > Why do you need to go through an intermediate variable ? > Can't you assign data_->pipeConfig_ directly here ? Obviously yes, I can, thanks :-). > Thanks > j > >> + if (imguConfig.isNull()) { >> LOG(IPU3, Error) << "Failed to calculate pipe configuration: " >> << "unsupported resolutions."; >> return Invalid; >> } >> + data_->pipeConfig_ = imguConfig; >> } >> >> return status; >> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) >> * stream has been requested: return here to skip the ImgU configuration >> * part. >> */ >> - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); >> + ImgUDevice::PipeConfig imguConfig = data->pipeConfig_; >> if (imguConfig.isNull()) >> return 0; >> >> -- >> 2.27.0 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Jean-Michel, Thank you for the patch. On Sun, Mar 14, 2021 at 08:19:01PM +0100, Jean-Michel Hautbois wrote: > On 13/03/2021 09:31, Jacopo Mondi wrote: > > On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote: > >> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to > >> calculate the statistics grids. Blank line. > >> This patch makes it possible for PipelineHandlerIPU3::start() to access > >> the BDS configuration and later pass the rectangle to the IPU3 IPA > >> configure method. > > > > The alternative would be having the pipe configuration retrieved from > > the ImgU device at start() which would require having the ImgUDevice > > stateful, which will be very painful to implement due to the awful > > pipe configuration procedure. > > > > Removing 'const' from the data_ field doesn't worry me too much, but > > smells like we're taking a shortcut. I'm fine with that for the time > > being. > > Yes, there is no real easy straightforward way unfortunately... > > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------ > >> 1 file changed, 7 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 00da2bb2..0d133031 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -70,6 +70,8 @@ public: > >> CIO2Device cio2_; > >> ImgUDevice *imgu_; > >> > >> + ImgUDevice::PipeConfig pipeConfig_; > >> + > >> Stream outStream_; > >> Stream vfStream_; > >> Stream rawStream_; > >> @@ -97,7 +99,6 @@ public: > >> Status validate() override; > >> > >> const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > >> - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > >> > >> /* Cache the combinedTransform_ that will be applied to the sensor */ > >> Transform combinedTransform_; > >> @@ -108,10 +109,9 @@ private: > >> * corresponding Camera instance is valid. In order to borrow a > >> * reference to the camera data, store a new reference to the camera. > >> */ > >> - const IPU3CameraData *data_; > >> + IPU3CameraData *data_; > >> > >> StreamConfiguration cio2Configuration_; > >> - ImgUDevice::PipeConfig pipeConfig_; > >> }; > >> > >> class PipelineHandlerIPU3 : public PipelineHandler > >> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > >> > >> /* Only compute the ImgU configuration if a YUV stream has been requested. */ > >> if (yuvCount) { > >> - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); > >> - if (pipeConfig_.isNull()) { > >> + ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe); > > > > Why do you need to go through an intermediate variable ? > > Can't you assign data_->pipeConfig_ directly here ? > > Obviously yes, I can, thanks :-). > > >> + if (imguConfig.isNull()) { > >> LOG(IPU3, Error) << "Failed to calculate pipe configuration: " > >> << "unsupported resolutions."; > >> return Invalid; > >> } > >> + data_->pipeConfig_ = imguConfig; I'm sorry, that's a no-go. validate() isn't supposed to modify the current state of the camera. An application can configure the camera, generate a new configuration, validate it, and then throw it away. You would end up storing the second configuration here. You could instead store the pipe config in IPU3CameraData in PipelineHandlerIPU3::configure(), that would be simpler. A better option would be to move the IPA configure() call from PipelineHandlerIPU3::start() to PipelineHandlerIPU3::configure(). > >> } > >> > >> return status; > >> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > >> * stream has been requested: return here to skip the ImgU configuration > >> * part. > >> */ > >> - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); > >> + ImgUDevice::PipeConfig imguConfig = data->pipeConfig_; > >> if (imguConfig.isNull()) > >> return 0; > >>
Hi All, On 15/03/2021 02:53, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Sun, Mar 14, 2021 at 08:19:01PM +0100, Jean-Michel Hautbois wrote: >> On 13/03/2021 09:31, Jacopo Mondi wrote: >>> On Fri, Mar 12, 2021 at 02:03:03PM +0100, Jean-Michel Hautbois wrote: >>>> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to >>>> calculate the statistics grids. > > Blank line. > >>>> This patch makes it possible for PipelineHandlerIPU3::start() to access >>>> the BDS configuration and later pass the rectangle to the IPU3 IPA >>>> configure method. >>> >>> The alternative would be having the pipe configuration retrieved from >>> the ImgU device at start() which would require having the ImgUDevice >>> stateful, which will be very painful to implement due to the awful >>> pipe configuration procedure. >>> >>> Removing 'const' from the data_ field doesn't worry me too much, but >>> smells like we're taking a shortcut. I'm fine with that for the time >>> being. >> >> Yes, there is no real easy straightforward way unfortunately... >> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> index 00da2bb2..0d133031 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -70,6 +70,8 @@ public: >>>> CIO2Device cio2_; >>>> ImgUDevice *imgu_; >>>> >>>> + ImgUDevice::PipeConfig pipeConfig_; >>>> + >>>> Stream outStream_; >>>> Stream vfStream_; >>>> Stream rawStream_; >>>> @@ -97,7 +99,6 @@ public: >>>> Status validate() override; >>>> >>>> const StreamConfiguration &cio2Format() const { return cio2Configuration_; } >>>> - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } >>>> >>>> /* Cache the combinedTransform_ that will be applied to the sensor */ >>>> Transform combinedTransform_; >>>> @@ -108,10 +109,9 @@ private: >>>> * corresponding Camera instance is valid. In order to borrow a >>>> * reference to the camera data, store a new reference to the camera. >>>> */ >>>> - const IPU3CameraData *data_; >>>> + IPU3CameraData *data_; >>>> >>>> StreamConfiguration cio2Configuration_; >>>> - ImgUDevice::PipeConfig pipeConfig_; >>>> }; >>>> >>>> class PipelineHandlerIPU3 : public PipelineHandler >>>> @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>>> >>>> /* Only compute the ImgU configuration if a YUV stream has been requested. */ >>>> if (yuvCount) { >>>> - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); >>>> - if (pipeConfig_.isNull()) { >>>> + ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe); >>> >>> Why do you need to go through an intermediate variable ? >>> Can't you assign data_->pipeConfig_ directly here ? >> >> Obviously yes, I can, thanks :-). >> >>>> + if (imguConfig.isNull()) { >>>> LOG(IPU3, Error) << "Failed to calculate pipe configuration: " >>>> << "unsupported resolutions."; >>>> return Invalid; >>>> } >>>> + data_->pipeConfig_ = imguConfig; > > I'm sorry, that's a no-go. validate() isn't supposed to modify the > current state of the camera. An application can configure the camera, > generate a new configuration, validate it, and then throw it away. You > would end up storing the second configuration here. > > You could instead store the pipe config in IPU3CameraData in > PipelineHandlerIPU3::configure(), that would be simpler. A better option > would be to move the IPA configure() call from > PipelineHandlerIPU3::start() to PipelineHandlerIPU3::configure(). I think this is the right way forwards. Our interfaces throughout should be consistent, so we should configure in configure(). In this instance I think I recall a message from Laurent highlighting that the IPU3 is currently buggy because it calls configure after it has started which is incorrect. So first we need to move the call to configure the IPA before we start it, and then ensure we configure everything correctly before starting anything ;-) -- Kieran > >>>> } >>>> >>>> return status; >>>> @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) >>>> * stream has been requested: return here to skip the ImgU configuration >>>> * part. >>>> */ >>>> - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); >>>> + ImgUDevice::PipeConfig imguConfig = data->pipeConfig_; >>>> if (imguConfig.isNull()) >>>> return 0; >>>> >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 00da2bb2..0d133031 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -70,6 +70,8 @@ public: CIO2Device cio2_; ImgUDevice *imgu_; + ImgUDevice::PipeConfig pipeConfig_; + Stream outStream_; Stream vfStream_; Stream rawStream_; @@ -97,7 +99,6 @@ public: Status validate() override; const StreamConfiguration &cio2Format() const { return cio2Configuration_; } - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } /* Cache the combinedTransform_ that will be applied to the sensor */ Transform combinedTransform_; @@ -108,10 +109,9 @@ private: * corresponding Camera instance is valid. In order to borrow a * reference to the camera data, store a new reference to the camera. */ - const IPU3CameraData *data_; + IPU3CameraData *data_; StreamConfiguration cio2Configuration_; - ImgUDevice::PipeConfig pipeConfig_; }; class PipelineHandlerIPU3 : public PipelineHandler @@ -375,12 +375,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() /* Only compute the ImgU configuration if a YUV stream has been requested. */ if (yuvCount) { - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); - if (pipeConfig_.isNull()) { + ImgUDevice::PipeConfig imguConfig = data_->imgu_->calculatePipeConfig(&pipe); + if (imguConfig.isNull()) { LOG(IPU3, Error) << "Failed to calculate pipe configuration: " << "unsupported resolutions."; return Invalid; } + data_->pipeConfig_ = imguConfig; } return status; @@ -576,7 +577,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * stream has been requested: return here to skip the ImgU configuration * part. */ - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); + ImgUDevice::PipeConfig imguConfig = data->pipeConfig_; if (imguConfig.isNull()) return 0;
The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to calculate the statistics grids. This patch makes it possible for PipelineHandlerIPU3::start() to access the BDS configuration and later pass the rectangle to the IPU3 IPA configure method. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)