| Message ID | 20210324150125.1318325-7-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote: > The configure operation is synchronous and should not send events back > to the pipeline handler. > > If information needs to be returned from configure it should be handled > through the interface directly. > > Move the initial call to setControls() out of configure() and into the > start() method which is called after the IPA running_ state is updated. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index a5c5e029f465..34a907f23ef5 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -32,7 +32,7 @@ public: > { > return 0; > } > - int start() override { return 0; } > + int start() override; > void stop() override {} > > void configure(const std::map<uint32_t, ControlInfoMap> &entityControls, > @@ -63,6 +63,13 @@ private: > uint32_t maxGain_; > }; > > +int IPAIPU3::start() > +{ > + setControls(0); This will send an asynchronous event to the pipeline handler, which will process it after starting the device. It defeats a little bit the point of setting initial controls, doesn't it ? The IPU3 IPA protocol seems ill-defined here, as it uses a frame counter, and it's not clear if that counter is 0-based or 1-based. All this doesn't matter much right now as we never call setControls() at runtime, so we could merge the patch as is, but please keep in mind that the mechanism is likely broken, and JM would then need to fix it (including redesigning part of the IPA protocol) as part of his pending IPU3 IPA series. This will involve figuring out how to properly interact with delayed controls. > + > + return 0; > +} > + > void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls, > [[maybe_unused]] const Size &bdsOutputSize) > { > @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls > minGain_ = std::max(itGain->second.min().get<int32_t>(), 1); > maxGain_ = itGain->second.max().get<int32_t>(); > gain_ = maxGain_; > - > - setControls(0); > } > > void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
Hi Laurent, On 24/03/2021 15:47, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote: >> The configure operation is synchronous and should not send events back >> to the pipeline handler. >> >> If information needs to be returned from configure it should be handled >> through the interface directly. >> >> Move the initial call to setControls() out of configure() and into the >> start() method which is called after the IPA running_ state is updated. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index a5c5e029f465..34a907f23ef5 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -32,7 +32,7 @@ public: >> { >> return 0; >> } >> - int start() override { return 0; } >> + int start() override; >> void stop() override {} >> >> void configure(const std::map<uint32_t, ControlInfoMap> &entityControls, >> @@ -63,6 +63,13 @@ private: >> uint32_t maxGain_; >> }; >> >> +int IPAIPU3::start() >> +{ >> + setControls(0); > > This will send an asynchronous event to the pipeline handler, which will > process it after starting the device. It defeats a little bit the point > of setting initial controls, doesn't it ? But it will set the controls before the first request, which was what I thought was the aim. I believe setControls() was just an initial skeleton implementation to demonstrate how to set controls. I don't think it's really been thought about correctly at all indeed. But the aim of this patch isn't to 'fix' the design, just stop the call being made in an invalid state. The actual design process is separate, and on-going, and I don't think this is the place to design how the controls should be set. The actual design of the IPA should determine that. > The IPU3 IPA protocol seems ill-defined here, as it uses a frame > counter, and it's not clear if that counter is 0-based or 1-based. > > All this doesn't matter much right now as we never call setControls() at > runtime, so we could merge the patch as is, but please keep in mind that > the mechanism is likely broken, and JM would then need to fix it > (including redesigning part of the IPA protocol) as part of his pending > IPU3 IPA series. This will involve figuring out how to properly interact > with delayed controls. This was detailed in the cover letter: Finally patch [6/6] moves an asynchronous callback out of the synchronous configure() call, and into start(). This is not expected to be a substantial change in functionality currently, but should be considered in follow on developments with the IPA. > >> + >> + return 0; >> +} >> + >> void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls, >> [[maybe_unused]] const Size &bdsOutputSize) >> { >> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls >> minGain_ = std::max(itGain->second.min().get<int32_t>(), 1); >> maxGain_ = itGain->second.max().get<int32_t>(); >> gain_ = maxGain_; >> - >> - setControls(0); >> } >> >> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers) >
Hi Kieran, On Wed, Mar 24, 2021 at 04:14:43PM +0000, Kieran Bingham wrote: > On 24/03/2021 15:47, Laurent Pinchart wrote: > > On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote: > >> The configure operation is synchronous and should not send events back > >> to the pipeline handler. > >> > >> If information needs to be returned from configure it should be handled > >> through the interface directly. > >> > >> Move the initial call to setControls() out of configure() and into the > >> start() method which is called after the IPA running_ state is updated. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/ipa/ipu3/ipu3.cpp | 11 ++++++++--- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index a5c5e029f465..34a907f23ef5 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -32,7 +32,7 @@ public: > >> { > >> return 0; > >> } > >> - int start() override { return 0; } > >> + int start() override; > >> void stop() override {} > >> > >> void configure(const std::map<uint32_t, ControlInfoMap> &entityControls, > >> @@ -63,6 +63,13 @@ private: > >> uint32_t maxGain_; > >> }; > >> > >> +int IPAIPU3::start() > >> +{ > >> + setControls(0); > > > > This will send an asynchronous event to the pipeline handler, which will > > process it after starting the device. It defeats a little bit the point > > of setting initial controls, doesn't it ? > > But it will set the controls before the first request, which was what I > thought was the aim. That depends on how fast the first request is queued after start() though, as the event from the IPA could race with the application and camera manager thread. Even if the controls are set before the first request is queued, if the sensor is started before setting controls, then the first controls will be subject to delays, while if they're set when the sensor is stopped, they will be taken into account immediately. We don't have to handle all of this as part of this patch, but I want everybody to be aware of this issue as it will become a problem pretty quickly. > I believe setControls() was just an initial skeleton implementation to > demonstrate how to set controls. I don't think it's really been thought > about correctly at all indeed. We agree on that, yes. One option could be to just drop the setControls() call too, until Jean-Michel's patches get merged :-) > But the aim of this patch isn't to 'fix' the design, just stop the call > being made in an invalid state. > > The actual design process is separate, and on-going, and I don't think > this is the place to design how the controls should be set. > > The actual design of the IPA should determine that. > > > The IPU3 IPA protocol seems ill-defined here, as it uses a frame > > counter, and it's not clear if that counter is 0-based or 1-based. > > > > All this doesn't matter much right now as we never call setControls() at > > runtime, so we could merge the patch as is, but please keep in mind that > > the mechanism is likely broken, and JM would then need to fix it > > (including redesigning part of the IPA protocol) as part of his pending > > IPU3 IPA series. This will involve figuring out how to properly interact > > with delayed controls. > > This was detailed in the cover letter: > > Finally patch [6/6] moves an asynchronous callback out of the > synchronous configure() call, and into start(). This is not expected to > be a substantial change in functionality currently, but should be > considered in follow on developments with the IPA. I should have read the cover letter first :-S Now that we're all aware that there's an issue that needs to be fixed later, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> As an initial idea, fixing the issue correctly could involve returning initial controls from the start() function instead of going through a separate async call. Another thing that needs to be taken into account is that, in the isolated case, the event sent by setControls() (with this patch applied) will be received by the libcamera process before the IPC reply to the start() call. This means that we may want to forbid sending any asynchronous events from start() (with appropriate documentation). > >> + > >> + return 0; > >> +} > >> + > >> void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls, > >> [[maybe_unused]] const Size &bdsOutputSize) > >> { > >> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls > >> minGain_ = std::max(itGain->second.min().get<int32_t>(), 1); > >> maxGain_ = itGain->second.max().get<int32_t>(); > >> gain_ = maxGain_; > >> - > >> - setControls(0); > >> } > >> > >> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
Hi Laurent, On 24/03/2021 16:40, Laurent Pinchart wrote: > Hi Kieran, > > On Wed, Mar 24, 2021 at 04:14:43PM +0000, Kieran Bingham wrote: >> On 24/03/2021 15:47, Laurent Pinchart wrote: >>> On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote: >>>> The configure operation is synchronous and should not send events back >>>> to the pipeline handler. >>>> >>>> If information needs to be returned from configure it should be handled >>>> through the interface directly. >>>> >>>> Move the initial call to setControls() out of configure() and into the >>>> start() method which is called after the IPA running_ state is updated. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/ipu3.cpp | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index a5c5e029f465..34a907f23ef5 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -32,7 +32,7 @@ public: >>>> { >>>> return 0; >>>> } >>>> - int start() override { return 0; } >>>> + int start() override; >>>> void stop() override {} >>>> >>>> void configure(const std::map<uint32_t, ControlInfoMap> &entityControls, >>>> @@ -63,6 +63,13 @@ private: >>>> uint32_t maxGain_; >>>> }; >>>> >>>> +int IPAIPU3::start() >>>> +{ >>>> + setControls(0); >>> >>> This will send an asynchronous event to the pipeline handler, which will >>> process it after starting the device. It defeats a little bit the point >>> of setting initial controls, doesn't it ? >> >> But it will set the controls before the first request, which was what I >> thought was the aim. > > That depends on how fast the first request is queued after start() > though, as the event from the IPA could race with the application and > camera manager thread. Even if the controls are set before the first > request is queued, if the sensor is started before setting controls, > then the first controls will be subject to delays, while if they're set > when the sensor is stopped, they will be taken into account immediately. > > We don't have to handle all of this as part of this patch, but I want > everybody to be aware of this issue as it will become a problem pretty > quickly. > >> I believe setControls() was just an initial skeleton implementation to >> demonstrate how to set controls. I don't think it's really been thought >> about correctly at all indeed. > > We agree on that, yes. One option could be to just drop the > setControls() call too, until Jean-Michel's patches get merged :-) > >> But the aim of this patch isn't to 'fix' the design, just stop the call >> being made in an invalid state. >> >> The actual design process is separate, and on-going, and I don't think >> this is the place to design how the controls should be set. >> >> The actual design of the IPA should determine that. >> >>> The IPU3 IPA protocol seems ill-defined here, as it uses a frame >>> counter, and it's not clear if that counter is 0-based or 1-based. >>> >>> All this doesn't matter much right now as we never call setControls() at >>> runtime, so we could merge the patch as is, but please keep in mind that >>> the mechanism is likely broken, and JM would then need to fix it >>> (including redesigning part of the IPA protocol) as part of his pending >>> IPU3 IPA series. This will involve figuring out how to properly interact >>> with delayed controls. >> >> This was detailed in the cover letter: >> >> Finally patch [6/6] moves an asynchronous callback out of the >> synchronous configure() call, and into start(). This is not expected to >> be a substantial change in functionality currently, but should be >> considered in follow on developments with the IPA. > > I should have read the cover letter first :-S > > Now that we're all aware that there's an issue that needs to be fixed > later, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > As an initial idea, fixing the issue correctly could involve returning > initial controls from the start() function instead of going through a > separate async call. > > Another thing that needs to be taken into account is that, in the > isolated case, the event sent by setControls() (with this patch applied) > will be received by the libcamera process before the IPC reply to the > start() call. This means that we may want to forbid sending any > asynchronous events from start() (with appropriate documentation). If we want to do that, we can change the running_ state after start() completes, rather than before. (There's already a patch to change running_ to state_ in my next series, so lets try to remember to discuss that there). >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls, >>>> [[maybe_unused]] const Size &bdsOutputSize) >>>> { >>>> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls >>>> minGain_ = std::max(itGain->second.min().get<int32_t>(), 1); >>>> maxGain_ = itGain->second.max().get<int32_t>(); >>>> gain_ = maxGain_; >>>> - >>>> - setControls(0); >>>> } >>>> >>>> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers) >
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index a5c5e029f465..34a907f23ef5 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -32,7 +32,7 @@ public: { return 0; } - int start() override { return 0; } + int start() override; void stop() override {} void configure(const std::map<uint32_t, ControlInfoMap> &entityControls, @@ -63,6 +63,13 @@ private: uint32_t maxGain_; }; +int IPAIPU3::start() +{ + setControls(0); + + return 0; +} + void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls, [[maybe_unused]] const Size &bdsOutputSize) { @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls minGain_ = std::max(itGain->second.min().get<int32_t>(), 1); maxGain_ = itGain->second.max().get<int32_t>(); gain_ = maxGain_; - - setControls(0); } void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
The configure operation is synchronous and should not send events back to the pipeline handler. If information needs to be returned from configure it should be handled through the interface directly. Move the initial call to setControls() out of configure() and into the start() method which is called after the IPA running_ state is updated. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)