[libcamera-devel] [PATCH v3 0/3] Pass controls on camera:start()

[Naushir Patuck]

Hi Laurent,

On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Fri, Dec 04, 2020 at 03:31:18PM +0000, Naushir Patuck wrote:
> > Hi,
> >
> > Here is patchset v3 for the work on passing controls on
> > camera::start().  I have addressed Jacopo's review comments, and
> > rebased to the top of master.
>
> This looks good to me. I've replied with minor comments on 1/3 and 2/3
> which I could address when applying the series. For 3/3, there's an open
> question of whether we should drop the initial control set at
> configure() time. If you send a v4 with this change, could you
> incorporate the review feedback for the other patches too ?
>

Thanks for the review feedback for all the patches.  Regarding the question
of removing the initial control set at configure() time, I think it still
is needed.  That set at configure() applies the default exposure time/gain
to the sensor at start of today, whereas the set at start() only applies
the exposure/gain if the controller wants a change to those values.  On the
first run, the controller will have no state history, so will not provide
values to set in start().  The only time this is not true is if a user has
provided some manual exposure/gain controls for the agc to use.  Then we do
the RPi::IPA_CONFIG_SENSOR twice, once with the default values, then
overriding that with the user values.

If you are ok with everything else, feel free to apply all your suggested
changes to the patches and merge.  If instead you would prefer me to create
a v4 with all the changes, happy to do so as well, let me know.

Regards,
Naush



> > Naushir Patuck (3):
> >   libcamera: pipeline: Pass libcamera controls into
> >     pipeline_handler::start()
> >   libcamera: ipa: Pass a set of controls and return results from
> >     ipa::start()
> >   pipeline: ipa: raspberrypi: Pass controls to IPA on start
> >
> >  Documentation/guides/pipeline-handler.rst     |  4 +-
> >  include/libcamera/camera.h                    |  2 +-
> >  .../libcamera/internal/ipa_context_wrapper.h  |  3 +-
> >  include/libcamera/internal/pipeline_handler.h |  2 +-
> >  include/libcamera/ipa/ipa_interface.h         |  3 +-
> >  include/libcamera/ipa/raspberrypi.h           |  1 +
> >  src/ipa/libipa/ipa_interface_wrapper.cpp      |  4 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 55 ++++++++++++-------
> >  src/ipa/rkisp1/rkisp1.cpp                     |  3 +-
> >  src/ipa/vimc/vimc.cpp                         |  6 +-
> >  src/libcamera/camera.cpp                      | 11 ++--
> >  src/libcamera/ipa_context_wrapper.cpp         |  5 +-
> >  src/libcamera/ipa_interface.cpp               |  7 +++
> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |  4 +-
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 22 ++++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  7 ++-
> >  src/libcamera/pipeline/simple/simple.cpp      |  4 +-
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  4 +-
> >  src/libcamera/pipeline/vimc/vimc.cpp          |  7 ++-
> >  src/libcamera/pipeline_handler.cpp            |  1 +
> >  src/libcamera/proxy/ipa_proxy_linux.cpp       |  3 +-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp      | 13 +++--
> >  test/ipa/ipa_interface_test.cpp               |  3 +-
> >  test/ipa/ipa_wrappers_test.cpp                |  5 +-
> >  24 files changed, 117 insertions(+), 62 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201208/94c14e16/attachment-0001.htm>