[libcamera-devel] [PATCH 1/3] libcamera: ipu3: Fix RAW+YUV capture

[Laurent Pinchart]

On Tue, Sep 29, 2020 at 02:20:27AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Thu, Sep 24, 2020 at 04:51:41PM +0200, Jacopo Mondi wrote:
> > When requesting one RAW stream and one YUV stream the
> > StreamConfiguration assigned to the RAW stream is the first one
> > added to the CameraConfiguration, while the YUV stream gets assigned to
> > the main output.
> > 
> > At configure() time the viewfinder output needs to be configured with
> > the same format as the main output, but since the introduction of RAW
> > capture support, the pipeline has not been updated and still assumes
> > the main output configuration is the first one in the
> > CameraConfiguration. This causes the viewfinder to be configured
> > with the same format as the raw stream, breaking capture operations.
> 
> Oops... You mentioned no CTS regression, is there also no CTS tests
> fixed by this ?
> 
> > Before this commit the following command fails and the ImgU does not
> > produce frames:
> > cam -srole=stillraw -srole=viewfinder -c2 -C
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 221259c7fe61..9cea6c7e9611 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -476,25 +476,23 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  		return ret;
> >  
> >  	/* Apply the format to the configured streams output devices. */
> > -	bool outActive = false;
> > -	bool vfActive = false;
> > +	StreamConfiguration *mainCfg = nullptr;
> > +	StreamConfiguration *vfCfg = nullptr;
> >  
> >  	for (unsigned int i = 0; i < config->size(); ++i) {
> >  		StreamConfiguration &cfg = (*config)[i];
> >  		Stream *stream = cfg.stream();
> >  
> >  		if (stream == outStream) {
> > +			mainCfg = &cfg;
> >  			ret = imgu->configureOutput(cfg, &outputFormat);
> >  			if (ret)
> >  				return ret;
> > -
> > -			outActive = true;
> >  		} else if (stream == vfStream) {
> > +			vfCfg = &cfg;
> >  			ret = imgu->configureViewfinder(cfg, &outputFormat);
> >  			if (ret)
> >  				return ret;
> > -
> > -			vfActive = true;
> >  		}
> >  	}
> >  
> > @@ -503,14 +501,14 @@ 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).
> >  	 */
> > -	if (!outActive) {
> > -		ret = imgu->configureOutput(config->at(0), &outputFormat);
> > +	if (!mainCfg) {
> > +		ret = imgu->configureOutput(*vfCfg, &outputFormat);
> 
> What if the only stream requested is the raw stream ? Won't both mainCfg
> and vfCfg be null ?

Scratch this, there's a check above the returns early if only a raw
stream is requested.

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


> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> > -	if (!vfActive) {
> > -		ret = imgu->configureViewfinder(config->at(0), &outputFormat);
> > +	if (!vfCfg) {
> > +		ret = imgu->configureViewfinder(*mainCfg, &outputFormat);
> >  		if (ret)
> >  			return ret;
> >  	}
> > @@ -529,7 +527,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> >  	ControlList ctrls(imgu->imgu_->controls());
> >  	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> > -		  static_cast<int32_t>(vfActive ? IPU3PipeModeVideo :
> > +		  static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo :
> >  				       IPU3PipeModeStillCapture));
> >  	ret = imgu->imgu_->setControls(&ctrls);
> >  	if (ret) {

-- 
Regards,

Laurent Pinchart