[libcamera-devel] [PATCH] libcamera: CameraManager, PipelineHandler: Allow multiple devnums per camera

[Paul Elder]

Hi Laurent,

Thank you for the review.

On Fri, Jun 05, 2020 at 10:02:37PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 

<snip>

> >  /**
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 53aeebdc..b3824a5f 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> >  	data->camera_ = camera.get();
> >  	cameraData_[camera.get()] = std::move(data);
> >  	cameras_.push_back(camera);
> > -	manager_->addCamera(std::move(camera), devnum);
> > +
> > +	std::vector<dev_t> devnums;
> > +	if (devnum != 0)
> > +		devnums.push_back(devnum);
> 
> Should we allow this, or always use all the capture video nodes ?

It doesn't seem to me that applications are very likely to use
CameraManager::get(devnum) to get cameras, and only those that deal with
V4L2 to a certain extent (eg. the V4L2 compatibility layer), will. In
that sense, I think that it won't be bad to disallow declaring a devnum
and always map the devnums automatically like below. In addition, having
all the capture video nodes mapped to a camera would be prevent
confusion on the rules surrounding which "video node is mapped to which
camera when using the V4L2 compatibility layer".

On the other hand, I think it's good to still provide the ability to
pipeline handlers to declare their own mapping if they want to. The
default (ie. less work) is the auto-mapping anyway. This might cause
confusion to users of the V4L2 compatilibity layer, if they expect that
all capture nodes can be used when the pipeline handler has disagreed.

So it's ease of use for the users of the V4L2 compatiblity layer, or
freedom to pipeline handlers. Maybe user-friendliness is more important,
and the pipeline handlers don't care to declare devnum mappings. I think
I'll go with the former then, until there are objections.


> > +	else
> > +		for (const std::shared_ptr<MediaDevice> media : mediaDevices_)
> > +			for (const MediaEntity *entity : media->entities())
> > +				devnums.push_back(makedev(entity->deviceMajor(),
> > +							  entity->deviceMinor()));
> 
> You need to limit entities to the capture video nodes. You can handle
> that through a combination of entity flags (to check if it's a video
> node) and pad flags (to check if it's a capture video node, by looking
> at the direction of the pad).
> 
> A short comment to explain what's going on would be useful.

ack

> > +
> > +	manager_->addCamera(std::move(camera), devnums);
> >  }
> >  
> >  /**


Thanks,

Paul