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

[Niklas Söderlund]

Hi,

First off, sorry Jacopo I now see there is a later version of this patch 
posted. I should have commented on that one.

On 2019-01-15 23:25:07 +0200, Laurent Pinchart wrote:

[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 ?

I think we need to design some sort of guide lines here. It's true we 
could match on driver name only here. By doing so we no longer ensures 
that the media devices have all the entities we expect it to have, and 
may therefor accept a media device which is

1.  From a different kernel version/patch series.
2.  Not probed completely.
3.  Expresses a variation of hardware with more or less capabilities 
    then the one the pipeline handler was developed for.

Reason 1 should not be a real issue, but API breakages do happen... This 
could then be solved with a check of the running kernel version which 
the matching logic could be expanded to cover. I'm thinking here of UVC 
where a lot of the entity names are not static and we would need to 
depend on the driver name alone.

Reason 2 is valid and a issue discussed at some V4L2 conference and is 
the main blocker for video devices can't be register at probe time. Once 
the kernel have a way to express that a media graph is complete we 
should use that in libcamera.

Reason 3 is a real problem as we don't know what variations of a 
hardware block will be released in a slightly different form in the 
future which a driver would be extended to support.

-- 
Regards,
Niklas Söderlund