[libcamera-devel,v2,0/1] Raspberry Pi: Create empty control lists correctly

14051 mbox series

Hi Laurent, everyone

Here's the v2 of this that I had promised, which patches another
identical problem that occurs only when you stop and restart the
camera (actually passing a ControlList back from the IPA).

I took your suggestion, Laurent, as it was closer to the original, and
also your "reviewed-by" tag in spite of the extra change, but
obviously please still comment if necessary!

As regards the ControlLists, it would be nice if the non-isolated
"thread" version of the IPAProxy could complain in the same
circumstances as the IPC version. Not sure what the best way is to
ensure that, calling the serializer "just for fun" sounds a bit like
overkill. Maybe only for start/configure methods that don't run
repeatedly? Or only in debug builds? Or something else more cunning?
Perhaps a question for another time...

Thanks!

David

David Plowman (1):
  pipeline: raspberrypi: Create empty control lists correctly

 src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++----
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 ++-
 2 files changed, 11 insertions(+), 5 deletions(-)

Hi David,

On Tue, Oct 05, 2021 at 09:56:59AM +0100, David Plowman wrote:
> Hi Laurent, everyone
> 
> Here's the v2 of this that I had promised, which patches another
> identical problem that occurs only when you stop and restart the
> camera (actually passing a ControlList back from the IPA).
> 
> I took your suggestion, Laurent, as it was closer to the original, and
> also your "reviewed-by" tag in spite of the extra change, but
> obviously please still comment if necessary!

I've reviewed v2 and it looks good to me.

> As regards the ControlLists, it would be nice if the non-isolated
> "thread" version of the IPAProxy could complain in the same
> circumstances as the IPC version. Not sure what the best way is to
> ensure that, calling the serializer "just for fun" sounds a bit like
> overkill. Maybe only for start/configure methods that don't run
> repeatedly? Or only in debug builds? Or something else more cunning?
> Perhaps a question for another time...

I see your point, increasing test coverage is always good. Wouldn't this
however be better handled by running a tool such as lc-compliance in
isolated mode, as part of the overall test strategy ?

> David Plowman (1):
>   pipeline: raspberrypi: Create empty control lists correctly
> 
>  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++----
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 ++-
>  2 files changed, 11 insertions(+), 5 deletions(-)

Hi Laurent

On Tue, 5 Oct 2021 at 10:29, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Tue, Oct 05, 2021 at 09:56:59AM +0100, David Plowman wrote:
> > Hi Laurent, everyone
> >
> > Here's the v2 of this that I had promised, which patches another
> > identical problem that occurs only when you stop and restart the
> > camera (actually passing a ControlList back from the IPA).
> >
> > I took your suggestion, Laurent, as it was closer to the original, and
> > also your "reviewed-by" tag in spite of the extra change, but
> > obviously please still comment if necessary!
>
> I've reviewed v2 and it looks good to me.
>
> > As regards the ControlLists, it would be nice if the non-isolated
> > "thread" version of the IPAProxy could complain in the same
> > circumstances as the IPC version. Not sure what the best way is to
> > ensure that, calling the serializer "just for fun" sounds a bit like
> > overkill. Maybe only for start/configure methods that don't run
> > repeatedly? Or only in debug builds? Or something else more cunning?
> > Perhaps a question for another time...
>
> I see your point, increasing test coverage is always good. Wouldn't this
> however be better handled by running a tool such as lc-compliance in
> isolated mode, as part of the overall test strategy ?

Yes, that sounds fair to me. I see it's easy enough to force isolation
mode with that environment variable, so maybe I shall add something to
our own testing too.

David

>
> > David Plowman (1):
> >   pipeline: raspberrypi: Create empty control lists correctly
> >
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++----
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  3 ++-
> >  2 files changed, 11 insertions(+), 5 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart