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

[Kieran Bingham]

Hi Jacopo,

On 13/06/2019 12:20, 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 at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 29 ++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1c0a9825b4cd..d13bdfcce54e 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"
>  #include "v4l2_subdevice.h"
>  #include "v4l2_videodevice.h"
>  
> @@ -29,6 +31,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +#define IPU3_PIPE_MODE_CTRL	0x009819c1

I see this value documented at
  https://www.kernel.org/doc/html/latest/media/v4l-drivers/ipu3.html

Is there a way to define custom controls "correctly"?
 rather than just putting in the raw value?

Once we have a real header of course it should come from that...
 (isn't this in a header somewhere already?)

It might be nice to define an enum to go with this control at least
internally

enum IPU3PipeMode {
	VideoMode = 0,
	StillMode = 1,
}

> +
>  class ImgUDevice
>  {
>  public:
> @@ -536,6 +540,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * the configuration of the active one for that purpose (there should
>  	 * be at least one active stream in the configuration request).
>  	 */
> +	unsigned int pipeMode = 0;

 = VideoMode; ?

>  	if (!outStream->active_) {
>  		ret = imgu->configureOutput(outStream->device_, config->at(0));
>  		if (ret)
> @@ -546,6 +551,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		ret = imgu->configureOutput(vfStream->device_, config->at(0));
>  		if (ret)
>  			return ret;
> +
> +		/*
> +		 * \todo: This works as long as only two concurrent streams
> +		 * per pipe are supported.
> +		 */
> +		pipeMode = 1;

 = StillMode;


>  	}
>  
>  	/*
> @@ -559,6 +570,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(IPU3_PIPE_MODE_CTRL, 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(IPU3_PIPE_MODE_CTRL);
> +	LOG(IPU3, Debug) << "ImgU pipe mode set to: "
> +			 << (pipeMode ? "'Still Capture'" : "'Video'");


Otherwise, this does show a nice easy way of handling controls IMO.

Do you expect to handle setting a single control through a setControl()
/ getControl() api which doesn't require the V4L2Controls object at all?


(perhaps we decide later if we need that separately)

>  	return 0;
>  }
>  
> 

-- 
Regards
--
Kieran