[libcamera-devel] [PATCH 03/11] libcamera: ipu3: imgu: Configure the stat video device as part of configure()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Nov 09, 2020 at 11:18:50AM +0100, Jacopo Mondi wrote:
> On Thu, Nov 05, 2020 at 01:15:38AM +0100, Niklas Söderlund wrote:
> > There is no reason to expose and call a separate configureStat() when
> > the statistics video device can be configured with the exact same
> > parameters as part of configure(). Move the configuration internally to
> > the ImgUDevice simplifying the interface, there is no functional change.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp |  7 +++++++
> >  src/libcamera/pipeline/ipu3/imgu.h   |  7 -------
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 11 -----------
> >  3 files changed, 7 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index a4d74a62f69a0c97..0a3bf62020fd23fb 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -477,6 +477,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF
> >
> >  	LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString();
> >
> > +	StreamConfiguration statCfg = {};
> > +	statCfg.size = inputFormat->size;
> > +	V4L2DeviceFormat statFormat;
> > +	ret = configureVideoDevice(stat_.get(), PAD_STAT, statCfg, &statFormat);

Maybe we should make the last parameter optional at some point, not
sure.

> > +	if (ret)
> > +		return ret;
> > +
> 
> This was in the same call path before, but just to make sure, I read
> in ImgUDevice::configureVideoDevice():
> 
> 	/* No need to apply format to the stat node. */
> 	if (dev == stat_.get())
> 		return 0;
> 
> No size need to be configured on the stat video device, but only on
> the ImgU stat source pad ?

The ImgU driver sets .vidioc_s_fmt_meta_cap to imgu_vidioc_g_meta_fmt(),
so there's indeed no need to set the format on the video node. We may
want to capture this in a comment in ImgUDevice::configureVideoDevice()
(strictly speaking out of scope for this patch, but I wouldn't mind
adding it here as a separate patch seems a bit overkill - I however know
how much Niklas likes to split patches :-)).

I've had a quick look at the subdev configuration, and it seems to
format on the stats pad is completely ignored, so we could even simplify
the code further. This needs to be double-checked though, I'm not very
familiar with the driver.

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

> >  	return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > index c73ac5a5a37cfe0e..37f5ae77c99ff8fe 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -61,13 +61,6 @@ public:
> >  					    outputFormat);
> >  	}
> >
> > -	int configureStat(const StreamConfiguration &cfg,
> > -			  V4L2DeviceFormat *outputFormat)
> > -	{
> > -		return configureVideoDevice(stat_.get(), PAD_STAT, cfg,
> > -					    outputFormat);
> > -	}
> > -
> >  	int allocateBuffers(unsigned int bufferCount);
> >  	void freeBuffers();
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 3f0232bc1eaad048..c559d160084f87e7 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -518,17 +518,6 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  			return ret;
> >  	}
> >
> > -	/*
> > -	 * Apply the largest available format to the stat node.
> > -	 * \todo Revise this when we'll actually use the stat node.
> > -	 */
> > -	StreamConfiguration statCfg = {};
> > -	statCfg.size = cio2Format.size;
> > -
> > -	ret = imgu->configureStat(statCfg, &outputFormat);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> >  	ControlList ctrls(imgu->imgu_->controls());
> >  	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,

-- 
Regards,

Laurent Pinchart