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

[Niklas Söderlund]

HI Laurent,

On 2019-01-16 17:42:30 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Tue, Jan 15, 2019 at 10:43:49PM +0100, Niklas Söderlund wrote:
> > 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.
> 
> I think we should add a kernel version (or rather media device version)
> check in the DeviceMatch in any case, it can be useful.
> 
> > 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.
> 
> It is valid, but not for all devices. For the ImgU for instance, we have
> no external entity, so it shouldn't be an issue.
> 
> Furthermore, for the CIO2, we indeed want to make sure all entities are
> successfully probed, but we don't know what sensor are present in the
> system, so the only entities we can match on anyway are internal to the
> CIO2, which seems a bit pointless to me.
> 
> > 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.
> 
> Same as for reason 2, this is valid, but not for all devices.
> 
> In the IPU3 case I believe matching on entities would only be useful to
> handle the first issue you pointed out, and I wonder if we need to do so
> now.

I agree with your comments.

My point is that we should think about these issues when developing the 
first pipeline handlers. Even if it's not strictly needed for the IPU3 
it might be a good idea to match on all entities we know should be there 
(or not). Cargo cult programming is areal thing :-)

-- 
Regards,
Niklas Söderlund