[libcamera-devel] [PATCH v2] libcamera: ipu3: set V4L2_CID_INTEL_IPU3_MODE

[Kieran Bingham]

Hi Jean-Michel,

On 12/02/2021 15:27, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Fri, Feb 12, 2021 at 04:17:49PM +0200, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> Thank you for the patch.
>>
>> On Fri, Feb 12, 2021 at 02:18:51PM +0100, Jean-Michel Hautbois wrote:
>>> In order to get the stats back, the imgu subdev needs to have the
>>> V4L2_CID_INTEL_IPU3_MODE control set.
>>> Set it to video mode by default to get the stats at each frame.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 61f7bf43..6957e3a9 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -551,9 +551,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>>>
>>>  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
>>>  	ControlList ctrls(imgu->imgu_->controls());
>>> +	/*
>>> +	 * \todo Set the ImgU pipe mode to 'Video' unconditionally to have statistics
>>> +	 * generated and re-consider if 'Still Capture' should be used for
>>> +	 * high quality RAW capture operations that do not involve the IPA.
>>
>> Raw images are captured at the output of the CIO2, the ImgU won't be in
>> the pipeline. However, when capturing raw frames we still want to run
>> the IPA, so we'll need statistics, which will be produced by the ImgU.
>> The output images will be discarded in that case.
> 
> Correct, sorry I was confused, of course RAW won't go through the ImgU
> 
>>
>> I however agree that we need to check what the use cases for still
>> capture mode are. If the mode exists, it's probably meant for something
>> (although one never knows, I think you have first hand experience with
>> this). How about the following comment ?
>>
>> 	/*
>> 	 * Set the ImgU pipe mode to 'Video' unconditionally to have statistics
>> 	 * generated.
>> 	 *
>> 	 * \todo Figure out what the 'Still Capture' mode is meant for, and use
>> 	 * it accordingly.
>> 	 */
> 
> Agree. The question stays, what is still capture for if it does not
> generate statistics and goes through the ImgU...

Are you happy with the updated comment? Will you post a v3? or should I
just apply Laurent's suggestion before merging?

--
Kieran


> 
>>
>> With this,
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>>> +	 */
>>>  	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
>>> -		  static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo :
>>> -				       IPU3PipeModeStillCapture));
>>> +		  static_cast<int32_t>(IPU3PipeModeVideo));
>>>  	ret = imgu->imgu_->setControls(&ctrls);
>>>  	if (ret) {
>>>  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran