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

[paul.elder at ideasonboard.com]

Hello,

On Tue, Oct 05, 2021 at 12:34:19AM +0300, Laurent Pinchart wrote:
> 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).

We don't. We can't pass in pointers either.

> 
> By the way, the above could be written
> 
> 	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> 			  &startConfig);

Yeah that should work.

> 
> Up to you. With or without that change,

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

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