| Message ID | 20211004135207.2936-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > > /* Apply any gain/exposure settings that the IPA may have passed back. */ > if (!startConfig.controls.empty())
Hi Laurent Thanks for the feedback. On Mon, 4 Oct 2021 at 15:18, Laurent Pinchart <laurent.pinchart@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@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
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@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@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())
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@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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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); /* Apply any gain/exposure settings that the IPA may have passed back. */ if (!startConfig.controls.empty())
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@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)