[libcamera-devel,v2] libcamera: ipu3: set V4L2_CID_INTEL_IPU3_MODE
diff mbox series

Message ID 20210212131851.22059-1-jeanmichel.hautbois@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] libcamera: ipu3: set V4L2_CID_INTEL_IPU3_MODE
Related show

Commit Message

Jean-Michel Hautbois Feb. 12, 2021, 1:18 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Feb. 12, 2021, 2:17 p.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@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.

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.
	 */

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@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";
Jacopo Mondi Feb. 12, 2021, 3:27 p.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@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...

>
> With this,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Feb. 19, 2021, 2:12 p.m. UTC | #3 
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@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo@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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Jean-Michel Hautbois Feb. 19, 2021, 2:19 p.m. UTC | #4 
Hi Kieran,

On 19/02/2021 15:12, Kieran Bingham wrote:
> 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@ideasonboard.com>
>>>> Reviewed-by: Jacopo Mondi <jacopo@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?

Oh dear, forgot about it :-(.
Yes, Laurent's comment it better, I can send a v3 or let you apply it
with updated comment, as you prefer !

JM

> --
> Kieran
> 
> 
>>
>>>
>>> With this,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>
Kieran Bingham Feb. 19, 2021, 3:48 p.m. UTC | #5 
On 19/02/2021 14:19, Jean-Michel Hautbois wrote:
> Hi Kieran,
> 
> On 19/02/2021 15:12, Kieran Bingham wrote:
>> 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@ideasonboard.com>
>>>>> Reviewed-by: Jacopo Mondi <jacopo@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?
> 
> Oh dear, forgot about it :-(.
> Yes, Laurent's comment it better, I can send a v3 or let you apply it
> with updated comment, as you prefer !

Pushed with the updated comment.
--
Kieran


> 
> JM
> 
>> --
>> Kieran
>>
>>
>>>
>>>>
>>>> With this,
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>> _______________________________________________
>>> libcamera-devel mailing list
>>> libcamera-devel@lists.libcamera.org
>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>>
>>
>

Patch
diff mbox series 

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.
+	 */
 	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";