[libcamera-devel] [PATCH v2 06/10] pipeline: raspberrypi: Reorder startup drop frame initialisation

[Laurent Pinchart]

Hi Naush,

On Fri, Dec 02, 2022 at 02:42:38PM +0000, Naushir Patuck wrote:
> On Fri, 2 Dec 2022 at 13:53, Laurent Pinchart wrote:
> > On Tue, Nov 29, 2022 at 01:45:30PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > Reorder the code such that the IPA requested startup drop frames count is
> > > available before the pipeline handler allocates any stream buffers.
> > >
> > > This will be used in a subsequent change to stop Unicam buffer allocations if
> > > there are no startup drop frames required.
> >
> > Do you mean "if there are no startup drop frames required and the
> > application has configured a raw stream and always provides buffers for
> > it" ?
> 
> Yes!

Could you update the commit message please ?

> > Why is that related ? Can't you avoid allocating internal raw buffers
> > even without startup frames being dropped ? Can't you use the raw buffer
> > from the first request to capture all the dropped frames and the first
> > useful frame ?
> 
> There are a couple of reasons for this:
> 
> 1) I'm working under the assumption that I cannot enqueue the same buffer
> multiple times in v4l2  Because of this, I can only ever queue 1 buffer, have it
> dequeued, processed by the ISP, and requeue it back to Unicam.  This
> effectively halves my overall framerate because of the added latency of running
> the ISP.
> 
> 2) Even if V4L2 does allow enquing the same framebuffer multiple times, there is
> a possible (probable?) HW race condition where the ISP could be processing the
> buffer for frame N, while Unicam is writing out to the buffer for frame N + 1.
> 
> It would be interesting to know if (1) is actually possible.  I don't recall exactly, but
> I think I did try it, and might have got complaints from the v4l2 framework.

(1) should be checked indeed. I had missed in my initial review the fact
that the buffer still needed to be processed by the ISP for algorithms
to converge, so (2) is a valid concern.

In any case, even if we handled frame drop and buffer allocation
differently, this patch wouldn't hurt, so

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 30 +++++++++----------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 86eb43a3a3c5..742521927780 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1044,21 +1044,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >       RPiCameraData *data = cameraData(camera);
> > >       int ret;
> > >
> > > -     for (auto const stream : data->streams_)
> > > -             stream->resetBuffers();
> > > -
> > > -     if (!data->buffersAllocated_) {
> > > -             /* Allocate buffers for internal pipeline usage. */
> > > -             ret = prepareBuffers(camera);
> > > -             if (ret) {
> > > -                     LOG(RPI, Error) << "Failed to allocate buffers";
> > > -                     data->freeBuffers();
> > > -                     stop(camera);
> > > -                     return ret;
> > > -             }
> > > -             data->buffersAllocated_ = true;
> > > -     }
> > > -
> > >       /* Check if a ScalerCrop control was specified. */
> > >       if (controls)
> > >               data->applyScalerCrop(*controls);
> > > @@ -1075,6 +1060,21 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >       /* Configure the number of dropped frames required on startup. */
> > >       data->dropFrameCount_ = startConfig.dropFrameCount;
> > >
> > > +     for (auto const stream : data->streams_)
> > > +             stream->resetBuffers();
> > > +
> > > +     if (!data->buffersAllocated_) {
> > > +             /* Allocate buffers for internal pipeline usage. */
> > > +             ret = prepareBuffers(camera);
> > > +             if (ret) {
> > > +                     LOG(RPI, Error) << "Failed to allocate buffers";
> > > +                     data->freeBuffers();
> > > +                     stop(camera);
> > > +                     return ret;
> > > +             }
> > > +             data->buffersAllocated_ = true;
> > > +     }
> > > +
> > >       /* We need to set the dropFrameCount_ before queueing buffers. */
> > >       ret = queueAllBuffers(camera);
> > >       if (ret) {

-- 
Regards,

Laurent Pinchart