[libcamera-devel] [PATCH] pipeline: raspberrypi: Create empty control list correctly

[David Plowman]

Hi Laurent

Thanks for the feedback.

On Mon, 4 Oct 2021 at 15:18, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Mon, Oct 04, 2021 at 02:52:07PM +0100, David Plowman wrote:
> > When the start() method is supplied with a NULL list of controls, we
> > send an empty control list to the IPA. When the IPA is running in
> > isolated mode the control list goes through the data serializer, for
> > which it must be marked correctly as a list of "controls::controls",
> > otherwise the IPA process will abort.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0bdfa727..87836996 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -825,7 +825,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> >
> >       /* Start the IPA. */
> >       ipa::RPi::StartConfig startConfig;
> > -     data->ipa_->start(controls ? *controls : ControlList{}, &startConfig);
> > +     ControlList emptyControls(controls::controls);
> > +     data->ipa_->start(controls ? *controls : emptyControls, &startConfig);
>
> Wouldn't it be better to change the IPA start() method to take a
> ControlList pointer, and pass nullptr when no controls are provided ?

But I'm thinking that, because we're going across the IPC mechanism,
someone has to turn the pointer into a real (but possibly empty)
ControlList at some point, so if it doesn't happen here then it
happens later. Or have I misunderstood?

David

(PS. I've found another problem where the IPA tries to send a
ControlList back and this fails too in isolation mode. So I sent this
patch a bit too soon and there's a v2 incoming in any case. But
passing ControlLists across the IPC interface is more of a minefield
than I realised!)

>
> >
> >       /* Apply any gain/exposure settings that the IPA may have passed back. */
> >       if (!startConfig.controls.empty())
>
> --
> Regards,
>
> Laurent Pinchart