[libcamera-devel] [PATCH v4 1/2] libcamera: ipu3: Move ipa configuration from start() to configure()

[Jean-Michel Hautbois]

Hi Laurent,

On 16/03/2021 21:55, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Tue, Mar 16, 2021 at 04:40:22PM +0100, Jean-Michel Hautbois wrote:
>> IPA was configured after all the pipeline devices were started,
>> including IPA itself...
>> Move it at the end of configure() call.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> 
> The change looks good. Could you make sure it doesn't break anything if
> we call configure() twice without any start() in-between ? This check
> should be added to lc-compliance (once we get it merged), in the
> meantime, you can modify any test application to duplicate the
> configure() call. Bonus points if this can be run under valgrind too.

I have cheated with test/camera/capture.cpp, I get too configure() calls
successfully without any issue.

As I really like bonus points, I also passed valgrind on a 'cam' call:
==28310== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==28310== Command: cam -c1 -C100
==28310==
[29:33:24.510402583] [28310]  INFO Camera camera_manager.cpp:293
libcamera v0.0.0+2409-d179b494-dirty (2021-03-17T13:55:04+01:00)
[29:33:27.824662259] [28313]  INFO IPU3 ipu3.cpp:1128 Registered
Camera[0] "\_SB_.PCI0.LNK1" connected to CSI-2 receiver 1
Using camera \_SB_.PCI0.LNK1
[29:33:28.243120313] [28310]  INFO Camera camera.cpp:890 configuring
streams: (0) 1280x720-NV12
Capture 100 frames
106409.888872 (0.00 fps) stream0 seq: 020006 bytesused: 1382400
106409.896575 (129.82 fps) stream0 seq: 020007 bytesused: 1382400
<snip>
106413.324588 (29.91 fps) stream0 seq: 020104 bytesused: 1382400
106413.357822 (30.09 fps) stream0 seq: 020105 bytesused: 1382400
==28310==
==28310== HEAP SUMMARY:
==28310==     in use at exit: 0 bytes in 0 blocks
==28310==   total heap usage: 18,382 allocs, 18,382 frees, 2,117,494
bytes allocated
==28310==
==28310== All heap blocks were freed -- no leaks are possible
==28310==
==28310== For lists of detected and suppressed errors, rerun with: -s
==28310== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

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

Thank you :-).

> 
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 2ea13ec9..bb61ef4a 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -633,6 +633,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>>  		return ret;
>>  	}
>>  
>> +	std::map<uint32_t, ControlInfoMap> entityControls;
>> +	entityControls.emplace(0, data->cio2_.sensor()->controls());
>> +	data->ipa_->configure(entityControls);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -717,7 +721,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>>  
>>  int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
>>  {
>> -	std::map<uint32_t, ControlInfoMap> entityControls;
>>  	IPU3CameraData *data = cameraData(camera);
>>  	CIO2Device *cio2 = &data->cio2_;
>>  	ImgUDevice *imgu = data->imgu_;
>> @@ -744,9 +747,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>>  	if (ret)
>>  		goto error;
>>  
>> -	entityControls.emplace(0, data->cio2_.sensor()->controls());
>> -	data->ipa_->configure(entityControls);
>> -
>>  	return 0;
>>  
>>  error:
>