| Message ID | 20210209150335.26773-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi JM, On 09/02/2021 15:03, Jean-Michel Hautbois wrote: > Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first > frame but there is no need to force it for all frames. > It will be called later by the 3A algorithms. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index b11b03ef..ae7e8c0b 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > op.operation = IPU3_IPA_ACTION_PARAM_FILLED; > > queueFrameAction.emit(frame, op); > - > - /* \todo Calculate new values for exposure_ and gain_. */ > - setControls(frame); Does this leave the setControls() function unused? I wonder if it should be removed in that case. I hear that this patch is anticipated to be able to stop the exposure being set to max, without any means of setting it so I'm not opposed to this going in as a fix anyway. Especially as this will all change soon enough with your other work on top. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > void IPAIPU3::parseStatistics(unsigned int frame, >
On Fri, Feb 12, 2021 at 03:36:36PM +0000, Kieran Bingham wrote: > On 09/02/2021 15:03, Jean-Michel Hautbois wrote: > > Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first > > frame but there is no need to force it for all frames. > > It will be called later by the 3A algorithms. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/ipu3.cpp | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index b11b03ef..ae7e8c0b 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > > op.operation = IPU3_IPA_ACTION_PARAM_FILLED; > > > > queueFrameAction.emit(frame, op); > > - > > - /* \todo Calculate new values for exposure_ and gain_. */ > > - setControls(frame); > > Does this leave the setControls() function unused? I wonder if it should > be removed in that case. > > I hear that this patch is anticipated to be able to stop the exposure > being set to max, without any means of setting it so I'm not opposed to > this going in as a fix anyway. > > Especially as this will all change soon enough with your other work on top. Depending on how "soon" that would be, we could also implement support for controls::Exposure right away, so that it could be set from applications. This shouldn't take long, but we would however need to also implement support for controls in qcam and cam, and that part is a bigger piece of work. Probably not the priority right now, so this patch should be fine. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > } > > > > void IPAIPU3::parseStatistics(unsigned int frame, > >
Hi Kieran, On 12/02/2021 16:36, Kieran Bingham wrote: > Hi JM, > > On 09/02/2021 15:03, Jean-Michel Hautbois wrote: >> Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first >> frame but there is no need to force it for all frames. >> It will be called later by the 3A algorithms. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index b11b03ef..ae7e8c0b 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >> op.operation = IPU3_IPA_ACTION_PARAM_FILLED; >> >> queueFrameAction.emit(frame, op); >> - >> - /* \todo Calculate new values for exposure_ and gain_. */ >> - setControls(frame); > > Does this leave the setControls() function unused? I wonder if it should > be removed in that case. It is still called at configure()... > I hear that this patch is anticipated to be able to stop the exposure > being set to max, without any means of setting it so I'm not opposed to > this going in as a fix anyway. Well, Laurent pointed that we could implement support for controls::Exposure and it would definitely be better, but I can't find enough time for it this week :-/. > Especially as this will all change soon enough with your other work on top. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> } >> >> void IPAIPU3::parseStatistics(unsigned int frame, >> >
Hi Jean-Michel, On Fri, Feb 12, 2021 at 04:59:01PM +0100, Jean-Michel Hautbois wrote: > On 12/02/2021 16:36, Kieran Bingham wrote: > > On 09/02/2021 15:03, Jean-Michel Hautbois wrote: > >> Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first > >> frame but there is no need to force it for all frames. > >> It will be called later by the 3A algorithms. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/ipu3.cpp | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index b11b03ef..ae7e8c0b 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > >> op.operation = IPU3_IPA_ACTION_PARAM_FILLED; > >> > >> queueFrameAction.emit(frame, op); > >> - > >> - /* \todo Calculate new values for exposure_ and gain_. */ > >> - setControls(frame); > > > > Does this leave the setControls() function unused? I wonder if it should > > be removed in that case. > > It is still called at configure()... > > > I hear that this patch is anticipated to be able to stop the exposure > > being set to max, without any means of setting it so I'm not opposed to > > this going in as a fix anyway. > > Well, Laurent pointed that we could implement support for > controls::Exposure and it would definitely be better, but I can't find > enough time for it this week :-/. For the record, I'm fine with this patch as a very short term interim measure. > > Especially as this will all change soon enough with your other work on top. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> } > >> > >> void IPAIPU3::parseStatistics(unsigned int frame, > >>
Hi Laurent, On 12/02/2021 16:50, Laurent Pinchart wrote: > Hi Jean-Michel, > > On Fri, Feb 12, 2021 at 04:59:01PM +0100, Jean-Michel Hautbois wrote: >> On 12/02/2021 16:36, Kieran Bingham wrote: >>> On 09/02/2021 15:03, Jean-Michel Hautbois wrote: >>>> Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first >>>> frame but there is no need to force it for all frames. >>>> It will be called later by the 3A algorithms. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/ipu3.cpp | 3 --- >>>> 1 file changed, 3 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index b11b03ef..ae7e8c0b 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >>>> op.operation = IPU3_IPA_ACTION_PARAM_FILLED; >>>> >>>> queueFrameAction.emit(frame, op); >>>> - >>>> - /* \todo Calculate new values for exposure_ and gain_. */ >>>> - setControls(frame); >>> >>> Does this leave the setControls() function unused? I wonder if it should >>> be removed in that case. >> >> It is still called at configure()... >> >>> I hear that this patch is anticipated to be able to stop the exposure >>> being set to max, without any means of setting it so I'm not opposed to >>> this going in as a fix anyway. >> >> Well, Laurent pointed that we could implement support for >> controls::Exposure and it would definitely be better, but I can't find >> enough time for it this week :-/. > > For the record, I'm fine with this patch as a very short term interim > measure. I'll push this soon then, as even when we have IPA patches posted on top, we'll expect more time for review cycles etc, so this can fill the gap for a couple of weeks, as I'm aware it may be affecting current developers working on sensor drivers attached to the IPU3. -- Kieran >>> Especially as this will all change soon enough with your other work on top. >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> } >>>> >>>> void IPAIPU3::parseStatistics(unsigned int frame, >>>> >
Hi Laurent, On 12/02/2021 17:50, Laurent Pinchart wrote: > Hi Jean-Michel, > > On Fri, Feb 12, 2021 at 04:59:01PM +0100, Jean-Michel Hautbois wrote: >> On 12/02/2021 16:36, Kieran Bingham wrote: >>> On 09/02/2021 15:03, Jean-Michel Hautbois wrote: >>>> Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first >>>> frame but there is no need to force it for all frames. >>>> It will be called later by the 3A algorithms. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/ipu3.cpp | 3 --- >>>> 1 file changed, 3 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index b11b03ef..ae7e8c0b 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) >>>> op.operation = IPU3_IPA_ACTION_PARAM_FILLED; >>>> >>>> queueFrameAction.emit(frame, op); >>>> - >>>> - /* \todo Calculate new values for exposure_ and gain_. */ >>>> - setControls(frame); >>> >>> Does this leave the setControls() function unused? I wonder if it should >>> be removed in that case. >> >> It is still called at configure()... >> >>> I hear that this patch is anticipated to be able to stop the exposure >>> being set to max, without any means of setting it so I'm not opposed to >>> this going in as a fix anyway. >> >> Well, Laurent pointed that we could implement support for >> controls::Exposure and it would definitely be better, but I can't find >> enough time for it this week :-/. > > For the record, I'm fine with this patch as a very short term interim > measure. I can push a v2 with a default value instead of the max value if you prefer too ?
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index b11b03ef..ae7e8c0b 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) op.operation = IPU3_IPA_ACTION_PARAM_FILLED; queueFrameAction.emit(frame, op); - - /* \todo Calculate new values for exposure_ and gain_. */ - setControls(frame); } void IPAIPU3::parseStatistics(unsigned int frame,
Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first frame but there is no need to force it for all frames. It will be called later by the 3A algorithms. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 3 --- 1 file changed, 3 deletions(-)