[libcamera-devel] [RFC PATCH v2 4/5] libcamera: ipu3: Apply a requested test pattern mode

[Laurent Pinchart]

Hi Hiro,

On Wed, Jun 23, 2021 at 05:08:38PM +0900, Hirokazu Honda wrote:
> On Wed, Jun 23, 2021 at 4:36 PM Hirokazu Honda wrote:
> > On Tue, Jun 22, 2021 at 7:42 PM Jacopo Mondi wrote:
> > > On Tue, Jun 22, 2021 at 11:36:53AM +0900, Hirokazu Honda wrote:
> > > > Apply a camera sensor a requested test pattern mode. The test
> > >
> > > Apply to the ...
> > >
> > > > pattern mode can be specified per frame.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/cio2.cpp | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > index 8548f749..9d1ff07d 100644
> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > @@ -12,6 +12,7 @@
> > > >  #include <libcamera/control_ids.h>
> > > >  #include <libcamera/formats.h>
> > > >  #include <libcamera/geometry.h>
> > > > +#include <libcamera/request.h>
> > > >  #include <libcamera/stream.h>
> > > >
> > > >  #include "libcamera/internal/camera_sensor.h"
> > > > @@ -289,6 +290,20 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> > > >               buffer->setRequest(request);
> > > >       }
> > > >
> > > > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > > > +             const uint8_t testPatternMode =
> > > > +                     static_cast<uint8_t>(request->controls().get(
> > > > +                             controls::draft::TestPatternMode));
> > > > +             int ret = sensor_->setTestPatternMode(testPatternMode);
> > > > +             if (ret) {
> > > > +                     LOG(IPU3, Error)
> > > > +                             << "Failed to set test pattern mode: " << ret;
> > > > +             } else {
> > > > +                     request->metadata().set(controls::draft::TestPatternMode,
> > > > +                                             testPatternMode);
> > > > +             }
> > > > +     }
> > > > +
> > >
> > > Correct me if I'm wrong, but if we have 3 buffers queued to the CIO2,
> > > we queue a 4th one with test pattern enabled and we enable it on the
> > > sensor here, won't the 3 frames that complete before this one contain
> > > the test pattern ? Cc-ed Laurent to rope him in for this question.
> > >
> > > Also, can we enable/disable test patter while streaming ? Have you
> > > tested this ?
> >
> > I haven't tested that. We don't have such a test.
> > I have the same question. My guess is applying the test pattern mode
> > on fly affects the preceding queued buffers.
> > In intel HAL implementation, it waits until the preceding queued
> > buffers have been dequeued before applying the test pattern mode.
> > We have to do that in libcamera too.

Yes, this needs to be handled correctly to work in per-frame-control
mode.

> Laurent, what do you think the best to do this?
> I am thinking to wait in each pipeline handler by queueing pending
> capture requests until that time.

Implementing this in the pipeline handler is the way to go. The IPA also
needs to implement such a mechanism for most controls. We could handle
the test pattern mode the same way by passing it to the IPA that would
pass it back at the right time, but this isn't implemented in the
open-source IPU3 IPA yet, and it would likely complicate support of the
closed-source IPA. I'm thus thinking it may be better to handle the test
pattern mode in the pipeline handler. The implementation should keep
requests in a queue, and apply the correct control value in the handler
of the CIO2 frameStart signal. I'd do this in ipu3.cpp, not cio2.cpp.
The frameStart signal is currently connected to the
DelayedControls::applyControls() function, you will need to connect it
to a function of IPU3CameraData, call DelayedControls::applyControls()
there, and apply the test pattern control too.

> > > >       int ret = output_->queueBuffer(buffer);
> > > >       if (ret)
> > > >               return nullptr;

-- 
Regards,

Laurent Pinchart