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

[Laurent Pinchart]

Hi David,

On Mon, Oct 04, 2021 at 03:46:30PM +0100, David Plowman wrote:
> On Mon, 4 Oct 2021 at 15:18, Laurent Pinchart wrote:
> > 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?

No, you're right, it's my mistake, I thought we supported nullable
arguments already for IPC but that's not the case yet (unless I've
missed something, Paul can correct me here).

By the way, the above could be written

	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
			  &startConfig);

Up to you. With or without that change,

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

> (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!)

That's good feedback, thanks. We need to harden the interface.

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

-- 
Regards,

Laurent Pinchart