[libcamera-devel] [PATCH] libcamera: pipeline: Add Intel IPU3 pipeline skeleton

[Laurent Pinchart]

Hello,

On Tue, Jan 15, 2019 at 09:38:24PM +0100, Niklas Söderlund wrote:
> On 2019-01-11 17:51:09 +0100, Jacopo Mondi wrote:
> > Add a pipeline handler skeleton for the Intel IPU3 device.
> > Tested with on Soraka, listing detected cameras on the system and
> > verifying the pipeline handler gets properly matched.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > 
> > Let's start by simply matching the pipeline handler with the device
> > it is running on. The here created single camera gets properly enumerated
> > on Soraka by the 'list-cameras' test:
> > 
> > ./test/list-cameras
> > [0:35:35.453249952]   DBG pipeline_handler.cpp:119 Pipeline handler: "PipeHandlerVimc" registered
> > [0:35:35.453538626]   DBG pipeline_handler.cpp:119 Pipeline handler: "PipelineHandlerIPU3" registered
> > [0:35:35.458316459]   DBG device_enumerator.cpp:214 New media device: ipu3-imgu created from: /dev/media1
> > [0:35:35.469071318]   DBG device_enumerator.cpp:214 New media device: ipu3-cio2 created from: /dev/media0
> > [0:35:35.475305874]   DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-cio2
> > [0:35:35.475343991]   DBG device_enumerator.cpp:255 Succesfull match for media device: ipu3-imgu
> > [0:35:35.475354057]   DBG pipeline_handler.cpp:150 Pipeline handler: "PipelineHandlerIPU3" matched
> > - IPU3 Camera
> > 
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp    | 119 ++++++++++++++++++++++++
> >  src/libcamera/pipeline/ipu3/meson.build |   3 +
> >  src/libcamera/pipeline/meson.build      |   2 +
> >  3 files changed, 124 insertions(+)
> >  create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp
> >  create mode 100644 src/libcamera/pipeline/ipu3/meson.build
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > new file mode 100644
> > index 0000000..477a9a2
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp

[snip]

> > +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > +{
> > +	DeviceMatch cio2_dm("ipu3-cio2");
> > +	cio2_dm.add("ipu3-csi2 0");
> > +	cio2_dm.add("ipu3-cio2 0");
> > +	cio2_dm.add("ipu3-csi2 1");
> > +	cio2_dm.add("ipu3-cio2 1");
> > +	cio2_dm.add("ipu3-csi2 2");
> > +	cio2_dm.add("ipu3-cio2 2");
> > +	cio2_dm.add("ipu3-csi2 3");
> > +	cio2_dm.add("ipu3-cio2 3");
> > +
> > +	cio2_ = enumerator->search(cio2_dm);
> > +	if (!cio2_)
> > +		return false;
> > +
> > +	cio2_->acquire();
> > +
> > +	DeviceMatch imgu_dm("ipu3-imgu");
> > +	imgu_dm.add("ipu3-imgu 0");
> > +	imgu_dm.add("ipu3-imgu 0 input");
> > +	imgu_dm.add("ipu3-imgu 0 parameters");
> > +	imgu_dm.add("ipu3-imgu 0 output");
> > +	imgu_dm.add("ipu3-imgu 0 viewfinder");
> > +	imgu_dm.add("ipu3-imgu 0 3a stat");
> > +	imgu_dm.add("ipu3-imgu 1");
> > +	imgu_dm.add("ipu3-imgu 1 input");
> > +	imgu_dm.add("ipu3-imgu 1 parameters");
> > +	imgu_dm.add("ipu3-imgu 1 output");
> > +	imgu_dm.add("ipu3-imgu 1 viewfinder");
> > +	imgu_dm.add("ipu3-imgu 1 3a stat");
> > +
> > +	imgu_ = enumerator->search(imgu_dm);
> > +	if (!imgu_) {
> > +		cio2_->release();
> > +		return false;
> > +	}
> > +
> > +	imgu_->acquire();
> 
> I would reorder this a bit.
> 
>     ...
> 
>     cio2_ = enumerator->search(cio2_dm);
>     if (!cio2_)
>         return false;
> 
>     imgu_ = enumerator->search(imgu_dm);
>     if (!imgu_)
>         return false;
> 
>     cio2_->acquire();
>     imgu_->acquire();
> 
>     ...
> 
> I don't feel strongly about this so if others have a different view I 
> will yield to public opinion :-)

I was about to mention the same, so I can only agree :-)

Furthermore, I think we can match on driver name only in this case,
can't we ?

> > +
> > +	/*
> > +	 * TODO: create cameras. As of now, just create a dummy one
> > +	 * to verify enumeration and matching on IPU3.
> > +	 */
> > +	camera_ = new Camera("IPU3 Camera");
> > +
> > +	return true;
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> > +
> > +} /* namespace libcamera */

[snip]

-- 
Regards,

Laurent Pinchart