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

[Laurent Pinchart]

Hi Niklas,

On Wed, Jan 16, 2019 at 04:53:58PM +0100, Niklas Söderlund wrote:
> On 2019-01-16 17:42:30 +0200, Laurent Pinchart wrote:
> > 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 :-)

Even better, we should document the usage guidelines for DeviceMatch :-)
Would you like to capture this in a documentation patch ?

-- 
Regards,

Laurent Pinchart