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

[Laurent Pinchart]

Hi Naush,

On Tue, Dec 08, 2020 at 09:57:17AM +0000, Naushir Patuck wrote:
> On Mon, 7 Dec 2020 at 23:35, Laurent Pinchart wrote:
> > 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.

Right, I understand that. My question was whether we shouldn't instead
skip RPi::IPA_CONFIG_SENSOR at configure() time, and do it
unconditionally at start() time. Now that I've read your answer, I
understand we don't necessarily want to apply new control values at
start() time when restarting the camera without reconfiguring it, but
I'm concerned about how this will interact with staggered writes (or the
new DelayedControls class). For instance, if an application calls
Camera::configure() multiple times in a row before Camera::start(),
there will be multiple sensor control writes enqueued in the staggered
writes mechanism, which I think could create issues. Wouldn't it be
simpler, from a pipeline handler point of view, if every time the camera
is started the IPA provided initial sensor controls once and once only ?

> 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.
> 
> > > 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