[libcamera-devel] [PATCH 1/5] libcamera: media_device: Provide the default entity

[Laurent Pinchart]

Hi Kieran,

On Mon, Feb 11, 2019 at 11:47:35AM +0000, Kieran Bingham wrote:
> On 08/02/2019 15:44, Laurent Pinchart wrote:
> > On Thu, Feb 07, 2019 at 11:06:20PM +0100, Jacopo Mondi wrote:
> >> Hi Kieran,
> >>     ignorant question, is FL_DEFAULT an UVC specific thing?
> > 
> > It isn't, but it isn't widely used either. It would thus be fine using
> > the default entity in UVC-specific tests or in the UVC pipeline handler,
> > but not in the rest of the code, at least not without a fallback.
> 
> The only places that use this helper are the uvcvideo pipeline, and the

There it makes complete sense.

> V4L2DeviceTest which is currently coded to skip if not operated on a UVC
> device...
> 
> When this stops being coded to a UVC device - the helper can be removed
> or updated as necessary.

Fine with me.

> >> A quick grep on kernel sources returns:
> >>
> >> $ git grep FL_DEFAULT drivers/media/
> >> drivers/media/platform/omap3isp/ispresizer.c:   res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT;
> >> drivers/media/usb/uvc/uvc_entity.c:             entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
> >>
> >> So I guess not, as omap3 seems also omap3 uses that as well.
> >>
> >> If that's not a UVC specificity, I welcome this change.
> >>
> >> On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote:
> >>> Add a helper to identify the default entity from a media device.
> >>>
> >>> Utilise it in the two places which iterate the entities manually, to allocated
> >>> a V4L2Device from the default entity.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>> ---
> >>>  src/libcamera/include/media_device.h  |  1 +
> >>>  src/libcamera/media_device.cpp        | 15 +++++++++++++++
> >>>  src/libcamera/pipeline/uvcvideo.cpp   | 10 ++++------
> >>>  test/v4l2_device/v4l2_device_test.cpp | 12 +++++-------
> >>>  4 files changed, 25 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> >>> index 9f038093b2b2..361e8f4a4b86 100644
> >>> --- a/src/libcamera/include/media_device.h
> >>> +++ b/src/libcamera/include/media_device.h
> >>> @@ -42,6 +42,7 @@ public:
> >>>
> >>>  	const std::vector<MediaEntity *> &entities() const { return entities_; }
> >>>  	MediaEntity *getEntityByName(const std::string &name) const;
> >>> +	MediaEntity *defaultEntity() const;
> > 
> > This should take an entity type as a parameter as the default flag is
> > defined as "Default entity for its type". It could also be used, for
> > instance, to discover the default camera sensor.
> 
> I don't understand how that would be implemented.
> 
> What extra item does it then check against? the entity type?

The entity type, yes.

> >>>  	MediaLink *link(const std::string &sourceName, unsigned int sourceIdx,
> >>>  			const std::string &sinkName, unsigned int sinkIdx);
> >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> >>> index 800ed330fe6d..4af90e1590a1 100644
> >>> --- a/src/libcamera/media_device.cpp
> >>> +++ b/src/libcamera/media_device.cpp
> >>> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const
> >>>  	return nullptr;
> >>>  }
> >>>
> >>> +/**
> >>> + * \brief Return the default MediaEntity within a MediaDevice
> >>> + * \return The default entity if specified, or a nullptr otherwise
> >>> + */
> >>> +MediaEntity *MediaDevice::defaultEntity() const
> >>> +{
> >>> +	for (MediaEntity *entity : entities_) {
> >>> +		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> >>> +			return entity;
> >>> +		}
> >>
> >> You could drop the inner braces { } here
> >>> +	}
> >>> +
> >>> +	return nullptr;
> >>> +}
> >>> +
> >>>  /**
> >>>   * \brief Retrieve the MediaLink connecting two pads, identified by entity
> >>>   * names and pad indexes
> >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >>> index fc31c52c0ecd..ed8228bb2fc6 100644
> >>> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >>> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >>>
> >>>  	media_->acquire();
> >>>
> >>> -	for (MediaEntity *entity : media_->entities()) {
> >>> -		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> >>> -			video_ = new V4L2Device(entity);
> >>> -			break;
> >>> -		}
> >>> -	}
> >>> +	MediaEntity *entity = media_->defaultEntity();
> >>> +	if (!entity)
> >>
> >> Looking at existing code if you don't find any DEFAULT entity, you
> >> return false as you do here, but also release the media device.
> >>
> >> Should the same be done here?
> > 
> > I think it should, and a message should likely be logged too.
> 
> Ack.
> 
> >>> +		return false;
> >>>
> >>> +	video_ = new V4L2Device(entity);
> >>>  	if (!video_ || video_->open()) {
> >>>  		if (!video_)
> >>>  			LOG(UVC, Error) << "Could not find a default video device";
> >>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> >>> index 18d014caf4c8..dd28cccada6b 100644
> >>> --- a/test/v4l2_device/v4l2_device_test.cpp
> >>> +++ b/test/v4l2_device/v4l2_device_test.cpp
> >>> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init()
> >>>
> >>>  	media_->acquire();
> >>>
> >>> -	for (MediaEntity *entity : media_->entities()) {
> >>> -		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> >>> -			dev_ = new V4L2Device(entity);
> >>> -			break;
> >>> -		}
> >>> -	}
> >>> +	MediaEntity *entity = media_->defaultEntity();
> >>> +	if (!entity)
> >>> +		return TestFail;
> > 
> > Note that we're planning to use vivid instead of uvcvideo for V4L2Device
> > tests, so this may not be a good idea.
> 
> Ok - so that should change at that point. Here to me it's a failure,
> because we will not know which UVC device to check against.
> 
> I can add a comment to say that this check is UVC specific.

That should be enough for now.

> >>> +	dev_ = new V4L2Device(entity);
> >>>  	if (!dev_)
> >>> -		return TestSkip;
> >>> +		return TestFail;
> >>>
> >>>  	return dev_->open();
> >>>  }

-- 
Regards,

Laurent Pinchart