| Message ID | 20211108131350.130665-11-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-11-08 13:13:38) > We can call configure multiple times and we don't need to reset > delayedControls for each of the configure calls. Reset it after the IPA > is started. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 6a7f5b9a..3fcfa777 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -667,8 +667,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > - data->delayedCtrls_->reset(); > - > return updateControls(data); > } > > @@ -769,6 +767,12 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret) > goto error; > > + /* > + * Reset the delayed controls with the gain and exposure values set by > + * the IPA. > + */ > + data->delayedCtrls_->reset(); > + Should this be squashed into 2/22? Han-Lin - Does this affect your use case at all? (I suspect it's ok) -- Kieran > /* > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > -- > 2.32.0 >
Perhaps put libcamera: pipeline: ipu3: in the $SUBJECT? $ git log --oneline ./src/libcamera/pipeline shows a bit of a mix of styles, but I think we can be more specific than just ipu3: as that doesn't make it clear which side is being changed. Quoting Kieran Bingham (2021-11-08 13:49:35) > Quoting Jean-Michel Hautbois (2021-11-08 13:13:38) > > We can call configure multiple times and we don't need to reset > > delayedControls for each of the configure calls. Reset it after the IPA > > is started. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 6a7f5b9a..3fcfa777 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -667,8 +667,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > } > > > > - data->delayedCtrls_->reset(); > > - > > return updateControls(data); > > } > > > > @@ -769,6 +767,12 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > > if (ret) > > goto error; > > > > + /* > > + * Reset the delayed controls with the gain and exposure values set by > > + * the IPA. > > + */ > > + data->delayedCtrls_->reset(); > > + > > Should this be squashed into 2/22? In fact, it probably stands alone from 2/22 and could be 'before' it. (With the addition of the ->reset in current 2/22 not being required in that patch). > > Han-Lin - Does this affect your use case at all? (I suspect it's ok) > -- > Kieran > > > > /* > > * Start the ImgU video devices, buffers will be queued to the > > * ImgU output and viewfinder when requests will be queued. > > -- > > 2.32.0 > >
Thanks for the patch Jean-Michel, On Mon, Nov 8, 2021 at 9:49 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Jean-Michel Hautbois (2021-11-08 13:13:38) > > We can call configure multiple times and we don't need to reset > > delayedControls for each of the configure calls. Reset it after the IPA > > is started. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 6a7f5b9a..3fcfa777 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -667,8 +667,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > } > > > > - data->delayedCtrls_->reset(); > > - > > return updateControls(data); > > } > > > > @@ -769,6 +767,12 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > > if (ret) > > goto error; > > > > + /* > > + * Reset the delayed controls with the gain and exposure values set by > > + * the IPA. > > + */ > > + data->delayedCtrls_->reset(); > > + > > Should this be squashed into 2/22? > > Han-Lin - Does this affect your use case at all? (I suspect it's ok) I think it should be okay as long as we reset it before the first frame queuing into the sensor node. > -- > Kieran > > > > /* > > * Start the ImgU video devices, buffers will be queued to the > > * ImgU output and viewfinder when requests will be queued. > > -- > > 2.32.0 > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 6a7f5b9a..3fcfa777 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -667,8 +667,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; } - data->delayedCtrls_->reset(); - return updateControls(data); } @@ -769,6 +767,12 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret) goto error; + /* + * Reset the delayed controls with the gain and exposure values set by + * the IPA. + */ + data->delayedCtrls_->reset(); + /* * Start the ImgU video devices, buffers will be queued to the * ImgU output and viewfinder when requests will be queued.
We can call configure multiple times and we don't need to reset delayedControls for each of the configure calls. Reset it after the IPA is started. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)