[libcamera-devel] [RFC PATCH v1] libcamera: device_enumerator_udev: remove devnum from dependency map

[Jacopo Mondi]

Hi Barnabás

On Mon, Apr 17, 2023 at 05:41:58PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> Hi
>
>
> 2023. április 17., hétfő 8:30 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > [...]
> > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > > index 5317afbd..a63cd360 100644
> > > --- a/src/libcamera/device_enumerator_udev.cpp
> > > +++ b/src/libcamera/device_enumerator_udev.cpp
> > > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> > >  	 * enumerator.
> > >  	 */
> > >  	deps->deps_.erase(devnum);
> > > +	devMap_.erase(it);
> >
> > Shouldn't this be done only when all the dependencies are actually
> > satisfied ?
> >
>
> Every dependency has its own entry in `devMap_` with a pointer to
> the corresponding `MediaDeviceDeps` object. I think when one dependency
> appears it should be removed from the map, just like it is removed
> from `MediaDeviceDeps::deps_` (in the previous line).
>
>
> > >
> > >  	if (deps->deps_.empty()) {
> >
> > Likely inside this if() clause ?
>
> `deps->deps_` is empty at this point, so we cannot efficiently find all
> entries in `devMap_` that belong to `deps`.
>
>
> >
> > Otherwise, if there still are dependencies to meet for the devnum at
> > hand, the next time addV4L2Device(devnum) is called it won't find any item
> > on the devMap_ for that key.
>
> My reading of the code suggests that devnum is unique and each v4l2 device
> can be a dependency of at most 1 media device. Is that not correct?
>

Indeed, from a superficial read I thought devMap_ was associating
media devices with their dependencies while instead, as you said, each
dependency has its own entry in devMap_.

Sorry for the noise, it makes sense to me now

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> >
> > >  		LOG(DeviceEnumerator, Debug)
> > > --
> > > 2.40.0
> > >
> > >
> >
>
>
> Regards,
> Barnabás Pőcze