[libcamera-devel,v4,5/6] libcamera: ipu3: Set pipe_mode based on stream configuration

1478 diff mbox series

Set the ImgU pipe_mode control based on the active stream configuration.
Use 'Video' pipe mode unless the viewfinder stream is not active.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 34 ++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 19, 2019 at 01:08:57PM +0200, Jacopo Mondi wrote:
> Set the ImgU pipe_mode control based on the active stream configuration.
> Use 'Video' pipe mode unless the viewfinder stream is not active.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 34 ++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1c0a9825b4cd..8c26d2c8c60c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -22,6 +22,8 @@
>  #include "media_device.h"
>  #include "pipeline_handler.h"
>  #include "utils.h"
> +#include "v4l2_controls.h"
> +#include "v4l2_device.h"

Is v4l2_device.h needed ? It should be included by both v4l2_subdevice.h
and v4l2_videodevice.h.

>  #include "v4l2_subdevice.h"
>  #include "v4l2_videodevice.h"
>  
> @@ -194,6 +196,12 @@ private:
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> +	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> +	enum IPU3PipeModes {
> +		IPU3PipeModeVideo = 0,
> +		IPU3PipeModeStillCapture = 1,
> +	};
> +
>  	PipelineHandlerIPU3(CameraManager *manager);
>  
>  	CameraConfiguration *generateConfiguration(Camera *camera,
> @@ -535,7 +543,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * As we need to set format also on the non-active streams, use
>  	 * the configuration of the active one for that purpose (there should
>  	 * be at least one active stream in the configuration request).
> +	 *
> +	 * Also set the IPU3 pipe mode: video mode by default, unless the only
> +	 * requested stream is the still capture output one.
> +	 * \todo: This works as long as only two concurrent streams per pipe
> +	 * are supported.

s/todo:/todo/

>  	 */
> +	int pipeMode = IPU3PipeModeVideo;

Would it be easier to read if written

	int pipeMode = vfStream->active_ ? IPU3PipeModeVideo : IPU3PipeModeStillCapture;

(and moved further down, where used) ?

>  	if (!outStream->active_) {
>  		ret = imgu->configureOutput(outStream->device_, config->at(0));
>  		if (ret)
> @@ -546,6 +560,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		ret = imgu->configureOutput(vfStream->device_, config->at(0));
>  		if (ret)
>  			return ret;
> +
> +		pipeMode = IPU3PipeModeStillCapture;
>  	}
>  
>  	/*
> @@ -559,6 +575,24 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret)
>  		return ret;
>  
> +	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> +	V4L2Controls ctrls;
> +	ctrls.add(V4L2_CID_IPU3_PIPE_MODE, pipeMode);
> +	ret = imgu->imgu_->setControls(&ctrls);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> +		return ret;
> +	}
> +
> +	ret = imgu->imgu_->getControls(&ctrls);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Unable to get pipe_mode control value";
> +		return ret;
> +	}
> +
> +	pipeMode = ctrls.get(V4L2_CID_IPU3_PIPE_MODE);
> +	LOG(IPU3, Debug) << "ImgU pipe mode set to: "
> +			 << (pipeMode ? "'Still Capture'" : "'Video'");

Do we really need the get part ?

>  	return 0;
>  }
>