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

[Naushir Patuck]

Hi Laurent,

On Tue, 8 Dec 2020 at 11:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

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

Yes, this is a valid concern.  I get around this issue in the staggered
writer by having a reset() mechanism.  Just before stream on, I call
staggered write reset(), and this method write the last set control values
to the device, and throw away all earlier values.  This means that on
start(), there will only ever be one (the latest) set of controls to use.
DelayedControls has implemented a similar reset() mechanism.  However, in
DelayedControls::reset(), it gets given a list of controls as an argument
to replace the existing controls in the queue.  Niklas, please correct me
if this is wrong :)  I did talk about a flush() mechanism that mirrors
closer to what the staggered writer does as well.

Regards,
Naush


>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201208/11b3cb0e/attachment.htm>