[libcamera-devel] [PATCH v3] libcamera: device_enumerator: ensure deviceNode is not empty
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 11 15:45:56 CEST 2023
Quoting Laurent Pinchart via libcamera-devel (2023-07-06 16:53:44)
> Hi Benjamin,
>
> Thank you for the patch.
>
> On Thu, Jul 06, 2023 at 01:41:11PM +0200, Benjamin Bara via libcamera-devel wrote:
> > From: Benjamin Bara <benjamin.bara at skidata.com>
> >
> > When activating both ISP nodes on the i.MX8MP, but only connecting one
> > camera sensor, libcamera aborts because it couldn't find the chosen
> > entity's device node:
>
> Why is this ? I assume that's because the kernel driver only registers
> subdevs once connected sensors have been found, given that video nodes
> are there. It would be nice to record that in the commit message.
>
> > [37:54:40.779902250] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:252 Added device /dev/media1: rkisp1
> > [37:54:40.780196750] [3631] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media0 found
> > [37:54:40.780237875] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:252 Added device /dev/media0: rkisp1
> > [37:54:40.780505125] [3631] DEBUG Camera camera_manager.cpp:152 Found registered pipeline handler 'PipelineHandlerRkISP1'
> > [37:54:40.780599875] [3631] DEBUG DeviceEnumerator device_enumerator.cpp:312 Successful match for media device "rkisp1"
> > [37:54:40.780731375] [3631] ERROR V4L2 v4l2_device.cpp:93 'rkisp1_isp': Failed to open V4L2 device '': No such file or directory
> >
> > Fix this by skipping empty device nodes:
> >
> > [37:49:05.172672000] [3603] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media1 found
> > [37:49:05.172720625] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:256 Added device /dev/media1: rkisp1
> > [37:49:05.172973875] [3603] DEBUG DeviceEnumerator device_enumerator_udev.cpp:322 All dependencies for media device /dev/media0 found
> > [37:49:05.173012125] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:256 Added device /dev/media0: rkisp1
> > [37:49:05.173281625] [3603] DEBUG Camera camera_manager.cpp:152 Found registered pipeline handler 'PipelineHandlerRkISP1'
> > [37:49:05.173376875] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:107 Skip rkisp1_isp: no device node
> > [37:49:05.173414375] [3603] DEBUG DeviceEnumerator device_enumerator.cpp:316 Successful match for media device "rkisp1"
> > [37:49:05.173671250] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video1[15:cap]: Opened device platform:rkisp1: rkisp1: rkisp1_stats
> > [37:49:05.173775125] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video2[16:out]: Opened device platform:rkisp1: rkisp1: rkisp1_params
> > [37:49:05.173880500] [3603] DEBUG V4L2 v4l2_videodevice.cpp:632 /dev/video0[18:cap]: Opened device platform:rkisp1: rkisp1: rkisp1
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Benjamin Bara <benjamin.bara at skidata.com>
> > ---
> > Hi!
> >
> > The dots look like this:
> >
> > /dev/media0:
> > digraph board {
> > rankdir=TB
> > n00000001 [label="{{<port0> 0 | <port1> 1} | rkisp1_isp\n | {<port2> 2 | <port3> 3}}", shape=Mrecord, style=filled, fillcolor=green]
> > n00000001:port2 -> n00000006:port0
> > n00000001:port3 -> n0000000d [style=bold]
> > n00000006 [label="{{<port0> 0} | rkisp1_resizer_mainpath\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
> > n00000006:port1 -> n00000009 [style=bold]
> > n00000009 [label="rkisp1_mainpath\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > n0000000d [label="rkisp1_stats\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
> > n00000011 [label="rkisp1_params\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
> > n00000011 -> n00000001:port1 [style=bold]
> > n0000001d [label="{{<port0> 0} | csis-32e40000.csi\n | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
> > n0000001d:port1 -> n00000001:port0
> > }
> >
> > /dev/media1:
> > digraph board {
> > rankdir=TB
> > n00000001 [label="{{<port0> 0 | <port1> 1} | rkisp1_isp\n/dev/v4l-subdev0 | {<port2> 2 | <port3> 3}}", shape=Mrecord, style=filled, fillcolor=green]
> > n00000001:port2 -> n00000006:port0
> > n00000001:port3 -> n0000000d [style=bold]
> > n00000006 [label="{{<port0> 0} | rkisp1_resizer_mainpath\n/dev/v4l-subdev1 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
> > n00000006:port1 -> n00000009 [style=bold]
> > n00000009 [label="rkisp1_mainpath\n/dev/video3", shape=box, style=filled, fillcolor=yellow]
> > n0000000d [label="rkisp1_stats\n/dev/video4", shape=box, style=filled, fillcolor=yellow]
> > n00000011 [label="rkisp1_params\n/dev/video5", shape=box, style=filled, fillcolor=yellow]
> > n00000011 -> n00000001:port1 [style=bold]
> > n0000001d [label="{{<port0> 0} | csis-32e50000.csi\n/dev/v4l-subdev2 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
> > n0000001d:port1 -> n00000001:port0
> > n00000022 [label="{{} | imx327 8-001a\n/dev/v4l-subdev3 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
> > n00000022:port0 -> n0000001d:port0
> > }
Thanks - that's helpful to see what was going on.
> > ---
> > Changes in v3:
> > - adapt check (thx to Kieran)
> > - Link to v2: https://lists.libcamera.org/pipermail/libcamera-devel/2023-June/038182.html
> >
> > Changes in v2:
> > - adapt commit message
> > - Link to v1: https://lists.libcamera.org/pipermail/libcamera-devel/2023-June/038181.html
> > ---
> > src/libcamera/device_enumerator.cpp | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> > index f2e055de..42b5ba6c 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -101,8 +101,14 @@ bool DeviceMatch::match(const MediaDevice *device) const
> >
> > for (const MediaEntity *entity : device->entities()) {
> > if (name == entity->name()) {
> > - found = true;
> > - break;
> > + if (!entity->deviceNode().empty()) {
> > + found = true;
> > + break;
> > + } else {
> > + LOG(DeviceEnumerator, Debug)
> > + << "Skip " << entity->name()
> > + << ": no device node";
> > + }
> > }
> > }
>
> Should we break when the device node list is empty ? We can't have two
> entities in the graph with the same name. Something along the lines of
>
> for (const MediaEntity *entity : device->entities()) {
> if (name == entity->name()) {
> if (entity->deviceNode().empty())
> LOG(DeviceEnumerator, Debug)
> << "Skip " << entity->name()
> << ": no device node";
> else
> found = true;
> break;
> }
> }
>
> or
>
> for (const MediaEntity *entity : device->entities()) {
> if (name != entity->name())
> continue;
>
> if (entity->deviceNode().empty())
> LOG(DeviceEnumerator, Debug)
> << "Skip " << entity->name()
> << ": no device node";
> else
> found = true;
> break;
> }
>
Benjamin,
The second option here looks better to me overall. Do you have a
preference? I can update the patch while applying - no need to send a
new patch - but the final choice is yours as it's your patch.
As this fixes a user facing issue - I'd like to get this in soon so it's
included in the next release.
--
Kieran
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >
> >
> > ---
> > base-commit: 0ee9339331c648232e87d2de2ccd5a92cc61cab2
> > change-id: 20230609-empty-devnode-36dd2647cde0
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list