| Message ID | 20250820132316.1033443-2-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Dan, Thank you for the patch. On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: > At the moment populateEntities() just returns true or false to indicate > success or failure. Although the populate() function assumes that the > topology is settled once the version number stops incrementing, that's > not guaranteed to be the case. One indicator that it's likely to increase > again in the future is the absence of interfaces for an entity, which Not all entities necessarily expose an interface to userspace, the code has to cope with that. > would only be created for V4L2 subdevices once the v4l2-async framework > had run to completion. How is that the case ? If no subdevide is created, there should be no entity in the media graph. > > Update populateEntities() to return -EAGAIN in the event an entity has > no associated interface yet and pass that error out from populate to > indicate to callers that they should re-try the population at a later > time. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > include/libcamera/internal/media_device.h | 2 +- > src/libcamera/media_device.cpp | 27 ++++++++++++++--------- > 2 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h > index b3a48b98d..d3c5eaa02 100644 > --- a/include/libcamera/internal/media_device.h > +++ b/include/libcamera/internal/media_device.h > @@ -70,7 +70,7 @@ private: > > struct media_v2_interface *findInterface(const struct media_v2_topology &topology, > unsigned int entityId); > - bool populateEntities(const struct media_v2_topology &topology); > + int populateEntities(const struct media_v2_topology &topology); > bool populatePads(const struct media_v2_topology &topology); > bool populateLinks(const struct media_v2_topology &topology); > void fixupEntityFlags(struct media_v2_entity *entity); > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index 353f34a81..6d58f09eb 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -257,12 +257,13 @@ int MediaDevice::populate() You need to update the function documemntation. > } > > /* Populate entities, pads and links. */ > - if (populateEntities(topology) && > - populatePads(topology) && > - populateLinks(topology)) > - valid_ = true; > + if (!(ret = populateEntities(topology))) { > + if (populatePads(topology) && populateLinks(topology)) > + valid_ = true; > + else > + ret = -EINVAL; > + } > > - ret = 0; This looks convoluted. Let's improve the function first: - Turn the ents, interfaces, links and pads arrays into std::vector instances. That will avoid having to delete the array manually in the exit path of the function. - Use utils::scope_exit to perform the cleanup, calling close(); if (!valie) clear(); That will allow you to get rid of the gotos. The above code can then be simplified and made clearer. > done: > close(); > > @@ -271,10 +272,8 @@ done: > delete[] pads; > delete[] links; > > - if (!valid_) { > + if (!valid_) > clear(); > - return -EINVAL; > - } > > return ret; > } > @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo > * For each entity in the media graph create a MediaEntity and store a > * reference in the media device objects map and entities list. > */ > -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > +int MediaDevice::populateEntities(const struct media_v2_topology &topology) > { > struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> > (topology.ptr_entities); > @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > */ > struct media_v2_interface *iface = > findInterface(topology, ent->id); > + if (!iface) { > + LOG(MediaDevice, Debug) > + << "Entity " << ent->name << " has no interface yet"; > + return -EAGAIN; > + } > + > MediaEntity *entity = new MediaEntity(this, ent, iface); > > if (!addObject(entity)) { > delete entity; > - return false; > + return -EINVAL; > } > > entities_.push_back(entity); > } > > - return true; > + return 0; > } > > bool MediaDevice::populatePads(const struct media_v2_topology &topology)
Hi Laurent - thanks for the comments On 20/08/2025 20:39, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: >> At the moment populateEntities() just returns true or false to indicate >> success or failure. Although the populate() function assumes that the >> topology is settled once the version number stops incrementing, that's >> not guaranteed to be the case. One indicator that it's likely to increase >> again in the future is the absence of interfaces for an entity, which > > Not all entities necessarily expose an interface to userspace, the code > has to cope with that. Ah, good point...can I just check for MEDIA_ENTITY_TYPE_V4L2_SUBDEV? That ought to be enough I think right? > >> would only be created for V4L2 subdevices once the v4l2-async framework >> had run to completion. > > How is that the case ? If no subdevide is created, there should be no > entity in the media graph. Indeed; the subdevice's entity exists, it's the interface that doesn't seem to. > >> >> Update populateEntities() to return -EAGAIN in the event an entity has >> no associated interface yet and pass that error out from populate to >> indicate to callers that they should re-try the population at a later >> time. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> include/libcamera/internal/media_device.h | 2 +- >> src/libcamera/media_device.cpp | 27 ++++++++++++++--------- >> 2 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h >> index b3a48b98d..d3c5eaa02 100644 >> --- a/include/libcamera/internal/media_device.h >> +++ b/include/libcamera/internal/media_device.h >> @@ -70,7 +70,7 @@ private: >> >> struct media_v2_interface *findInterface(const struct media_v2_topology &topology, >> unsigned int entityId); >> - bool populateEntities(const struct media_v2_topology &topology); >> + int populateEntities(const struct media_v2_topology &topology); >> bool populatePads(const struct media_v2_topology &topology); >> bool populateLinks(const struct media_v2_topology &topology); >> void fixupEntityFlags(struct media_v2_entity *entity); >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >> index 353f34a81..6d58f09eb 100644 >> --- a/src/libcamera/media_device.cpp >> +++ b/src/libcamera/media_device.cpp >> @@ -257,12 +257,13 @@ int MediaDevice::populate() > > You need to update the function documemntation. Ack > >> } >> >> /* Populate entities, pads and links. */ >> - if (populateEntities(topology) && >> - populatePads(topology) && >> - populateLinks(topology)) >> - valid_ = true; >> + if (!(ret = populateEntities(topology))) { >> + if (populatePads(topology) && populateLinks(topology)) >> + valid_ = true; >> + else >> + ret = -EINVAL; >> + } >> >> - ret = 0; > > This looks convoluted. Let's improve the function first: > > - Turn the ents, interfaces, links and pads arrays into std::vector > instances. That will avoid having to delete the array manually in the > exit path of the function. > > - Use utils::scope_exit to perform the cleanup, calling > > close(); > if (!valie) > clear(); > > That will allow you to get rid of the gotos. > > The above code can then be simplified and made clearer. Oooh; TIL of utils::scope_exit - thanks! I'll take a look. > >> done: >> close(); >> >> @@ -271,10 +272,8 @@ done: >> delete[] pads; >> delete[] links; >> >> - if (!valid_) { >> + if (!valid_) >> clear(); >> - return -EINVAL; >> - } >> >> return ret; >> } >> @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo >> * For each entity in the media graph create a MediaEntity and store a >> * reference in the media device objects map and entities list. >> */ >> -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >> +int MediaDevice::populateEntities(const struct media_v2_topology &topology) >> { >> struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> >> (topology.ptr_entities); >> @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >> */ >> struct media_v2_interface *iface = >> findInterface(topology, ent->id); >> + if (!iface) { >> + LOG(MediaDevice, Debug) >> + << "Entity " << ent->name << " has no interface yet"; >> + return -EAGAIN; >> + } >> + >> MediaEntity *entity = new MediaEntity(this, ent, iface); >> >> if (!addObject(entity)) { >> delete entity; >> - return false; >> + return -EINVAL; >> } >> >> entities_.push_back(entity); >> } >> >> - return true; >> + return 0; >> } >> >> bool MediaDevice::populatePads(const struct media_v2_topology &topology) >
Hi Laurent On 20/08/2025 22:33, Dan Scally wrote: > Hi Laurent - thanks for the comments > > On 20/08/2025 20:39, Laurent Pinchart wrote: >> Hi Dan, >> >> Thank you for the patch. >> >> On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: >>> At the moment populateEntities() just returns true or false to indicate >>> success or failure. Although the populate() function assumes that the >>> topology is settled once the version number stops incrementing, that's >>> not guaranteed to be the case. One indicator that it's likely to increase >>> again in the future is the absence of interfaces for an entity, which >> >> Not all entities necessarily expose an interface to userspace, the code >> has to cope with that. > > Ah, good point...can I just check for MEDIA_ENTITY_TYPE_V4L2_SUBDEV? That ought to be enough I think > right? Having investigated this a bit, it turns out that the type field that's returned from MEDIA_IOC_ENUM_ENTITIES for an entity isn't actually a value from MEDIA_ENTITY_TYPE_* but instead is the function, which I don't think map specifically to subdevices? Or if they do we'd likely have to have a large list of possible entity functions that we wanted to treat as qualifying here which seems suboptimal. I'm not sure how to proceed with identifying entities that are embedded within a subdevice without that type field...we could update struct media_entity_desc so that one of the reserved fields returns the obj_type to give us those MEDIA_ENTITY_TYPE_V4L2_SUBDEV / TYPE_VIDEO_DEVICE values but that won't help in the short term - do you have any thoughts? Thanks Dan > >> >>> would only be created for V4L2 subdevices once the v4l2-async framework >>> had run to completion. >> >> How is that the case ? If no subdevide is created, there should be no >> entity in the media graph. > > Indeed; the subdevice's entity exists, it's the interface that doesn't seem to. > >> >>> >>> Update populateEntities() to return -EAGAIN in the event an entity has >>> no associated interface yet and pass that error out from populate to >>> indicate to callers that they should re-try the population at a later >>> time. >>> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>> --- >>> include/libcamera/internal/media_device.h | 2 +- >>> src/libcamera/media_device.cpp | 27 ++++++++++++++--------- >>> 2 files changed, 17 insertions(+), 12 deletions(-) >>> >>> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h >>> index b3a48b98d..d3c5eaa02 100644 >>> --- a/include/libcamera/internal/media_device.h >>> +++ b/include/libcamera/internal/media_device.h >>> @@ -70,7 +70,7 @@ private: >>> struct media_v2_interface *findInterface(const struct media_v2_topology &topology, >>> unsigned int entityId); >>> - bool populateEntities(const struct media_v2_topology &topology); >>> + int populateEntities(const struct media_v2_topology &topology); >>> bool populatePads(const struct media_v2_topology &topology); >>> bool populateLinks(const struct media_v2_topology &topology); >>> void fixupEntityFlags(struct media_v2_entity *entity); >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >>> index 353f34a81..6d58f09eb 100644 >>> --- a/src/libcamera/media_device.cpp >>> +++ b/src/libcamera/media_device.cpp >>> @@ -257,12 +257,13 @@ int MediaDevice::populate() >> >> You need to update the function documemntation. > > Ack > >> >>> } >>> /* Populate entities, pads and links. */ >>> - if (populateEntities(topology) && >>> - populatePads(topology) && >>> - populateLinks(topology)) >>> - valid_ = true; >>> + if (!(ret = populateEntities(topology))) { >>> + if (populatePads(topology) && populateLinks(topology)) >>> + valid_ = true; >>> + else >>> + ret = -EINVAL; >>> + } >>> - ret = 0; >> >> This looks convoluted. Let's improve the function first: >> >> - Turn the ents, interfaces, links and pads arrays into std::vector >> instances. That will avoid having to delete the array manually in the >> exit path of the function. >> >> - Use utils::scope_exit to perform the cleanup, calling >> >> close(); >> if (!valie) >> clear(); >> >> That will allow you to get rid of the gotos. >> >> The above code can then be simplified and made clearer. > > Oooh; TIL of utils::scope_exit - thanks! I'll take a look. > >> >>> done: >>> close(); >>> @@ -271,10 +272,8 @@ done: >>> delete[] pads; >>> delete[] links; >>> - if (!valid_) { >>> + if (!valid_) >>> clear(); >>> - return -EINVAL; >>> - } >>> return ret; >>> } >>> @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo >>> * For each entity in the media graph create a MediaEntity and store a >>> * reference in the media device objects map and entities list. >>> */ >>> -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >>> +int MediaDevice::populateEntities(const struct media_v2_topology &topology) >>> { >>> struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> >>> (topology.ptr_entities); >>> @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >>> */ >>> struct media_v2_interface *iface = >>> findInterface(topology, ent->id); >>> + if (!iface) { >>> + LOG(MediaDevice, Debug) >>> + << "Entity " << ent->name << " has no interface yet"; >>> + return -EAGAIN; >>> + } >>> + >>> MediaEntity *entity = new MediaEntity(this, ent, iface); >>> if (!addObject(entity)) { >>> delete entity; >>> - return false; >>> + return -EINVAL; >>> } >>> entities_.push_back(entity); >>> } >>> - return true; >>> + return 0; >>> } >>> bool MediaDevice::populatePads(const struct media_v2_topology &topology) >> >
On Thu, Aug 21, 2025 at 11:52:29AM +0100, Daniel Scally wrote: > On 20/08/2025 22:33, Dan Scally wrote: > > On 20/08/2025 20:39, Laurent Pinchart wrote: > >> On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: > >>> At the moment populateEntities() just returns true or false to indicate > >>> success or failure. Although the populate() function assumes that the > >>> topology is settled once the version number stops incrementing, that's > >>> not guaranteed to be the case. One indicator that it's likely to increase > >>> again in the future is the absence of interfaces for an entity, which > >> > >> Not all entities necessarily expose an interface to userspace, the code > >> has to cope with that. > > > > Ah, good point...can I just check for MEDIA_ENTITY_TYPE_V4L2_SUBDEV? > > That ought to be enough I think right? That would probably cover the vast majority of cases in practice, but there's nothing in the spec that forces subdevs to have an interface. > Having investigated this a bit, it turns out that the type field > that's returned from MEDIA_IOC_ENUM_ENTITIES for an entity isn't > actually a value from MEDIA_ENTITY_TYPE_* but instead is the function, > which I don't think map specifically to subdevices? Or if they do we'd > likely have to have a large list of possible entity functions that we > wanted to treat as qualifying here which seems suboptimal. > > I'm not sure how to proceed with identifying entities that are > embedded within a subdevice without that type field...we could update > struct media_entity_desc so that one of the reserved fields returns > the obj_type to give us those MEDIA_ENTITY_TYPE_V4L2_SUBDEV / > TYPE_VIDEO_DEVICE values but that won't help in the short term - do > you have any thoughts? As subdevs are not required to have an entity, I don't think that would be enough. One option would be to create the interface at the same time the entity is registered. We won't know the major/minor at that point, and they could stay 0 until the userspace-facing device is registered. This would allow userspace to know if an interface is eventually expected to appear. I haven't investigated how difficult this would be on the kernel side. Another thing that would likely help is adding support for events in the MC API, to report updates to the topology. That way libcamera wouldn't need to reject an incomplete media device, it could instead update it. > >>> would only be created for V4L2 subdevices once the v4l2-async framework > >>> had run to completion. > >> > >> How is that the case ? If no subdevide is created, there should be no > >> entity in the media graph. > > > > Indeed; the subdevice's entity exists, it's the interface that doesn't seem to. > > > >>> Update populateEntities() to return -EAGAIN in the event an entity has > >>> no associated interface yet and pass that error out from populate to > >>> indicate to callers that they should re-try the population at a later > >>> time. > >>> > >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >>> --- > >>> include/libcamera/internal/media_device.h | 2 +- > >>> src/libcamera/media_device.cpp | 27 ++++++++++++++--------- > >>> 2 files changed, 17 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h > >>> index b3a48b98d..d3c5eaa02 100644 > >>> --- a/include/libcamera/internal/media_device.h > >>> +++ b/include/libcamera/internal/media_device.h > >>> @@ -70,7 +70,7 @@ private: > >>> struct media_v2_interface *findInterface(const struct media_v2_topology &topology, > >>> unsigned int entityId); > >>> - bool populateEntities(const struct media_v2_topology &topology); > >>> + int populateEntities(const struct media_v2_topology &topology); > >>> bool populatePads(const struct media_v2_topology &topology); > >>> bool populateLinks(const struct media_v2_topology &topology); > >>> void fixupEntityFlags(struct media_v2_entity *entity); > >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > >>> index 353f34a81..6d58f09eb 100644 > >>> --- a/src/libcamera/media_device.cpp > >>> +++ b/src/libcamera/media_device.cpp > >>> @@ -257,12 +257,13 @@ int MediaDevice::populate() > >> > >> You need to update the function documemntation. > > > > Ack > > > >>> } > >>> /* Populate entities, pads and links. */ > >>> - if (populateEntities(topology) && > >>> - populatePads(topology) && > >>> - populateLinks(topology)) > >>> - valid_ = true; > >>> + if (!(ret = populateEntities(topology))) { > >>> + if (populatePads(topology) && populateLinks(topology)) > >>> + valid_ = true; > >>> + else > >>> + ret = -EINVAL; > >>> + } > >>> - ret = 0; > >> > >> This looks convoluted. Let's improve the function first: > >> > >> - Turn the ents, interfaces, links and pads arrays into std::vector > >> instances. That will avoid having to delete the array manually in the > >> exit path of the function. > >> > >> - Use utils::scope_exit to perform the cleanup, calling > >> > >> close(); > >> if (!valie) > >> clear(); > >> > >> That will allow you to get rid of the gotos. > >> > >> The above code can then be simplified and made clearer. > > > > Oooh; TIL of utils::scope_exit - thanks! I'll take a look. > > > >>> done: > >>> close(); > >>> @@ -271,10 +272,8 @@ done: > >>> delete[] pads; > >>> delete[] links; > >>> - if (!valid_) { > >>> + if (!valid_) > >>> clear(); > >>> - return -EINVAL; > >>> - } > >>> return ret; > >>> } > >>> @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo > >>> * For each entity in the media graph create a MediaEntity and store a > >>> * reference in the media device objects map and entities list. > >>> */ > >>> -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>> +int MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>> { > >>> struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> > >>> (topology.ptr_entities); > >>> @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>> */ > >>> struct media_v2_interface *iface = > >>> findInterface(topology, ent->id); > >>> + if (!iface) { > >>> + LOG(MediaDevice, Debug) > >>> + << "Entity " << ent->name << " has no interface yet"; > >>> + return -EAGAIN; > >>> + } > >>> + > >>> MediaEntity *entity = new MediaEntity(this, ent, iface); > >>> if (!addObject(entity)) { > >>> delete entity; > >>> - return false; > >>> + return -EINVAL; > >>> } > >>> entities_.push_back(entity); > >>> } > >>> - return true; > >>> + return 0; > >>> } > >>> bool MediaDevice::populatePads(const struct media_v2_topology &topology)
Hi Laurent On 21/08/2025 12:09, Laurent Pinchart wrote: > On Thu, Aug 21, 2025 at 11:52:29AM +0100, Daniel Scally wrote: >> On 20/08/2025 22:33, Dan Scally wrote: >>> On 20/08/2025 20:39, Laurent Pinchart wrote: >>>> On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: >>>>> At the moment populateEntities() just returns true or false to indicate >>>>> success or failure. Although the populate() function assumes that the >>>>> topology is settled once the version number stops incrementing, that's >>>>> not guaranteed to be the case. One indicator that it's likely to increase >>>>> again in the future is the absence of interfaces for an entity, which >>>> >>>> Not all entities necessarily expose an interface to userspace, the code >>>> has to cope with that. >>> >>> Ah, good point...can I just check for MEDIA_ENTITY_TYPE_V4L2_SUBDEV? >>> That ought to be enough I think right? > > That would probably cover the vast majority of cases in practice, but > there's nothing in the spec that forces subdevs to have an interface. > >> Having investigated this a bit, it turns out that the type field >> that's returned from MEDIA_IOC_ENUM_ENTITIES for an entity isn't >> actually a value from MEDIA_ENTITY_TYPE_* but instead is the function, >> which I don't think map specifically to subdevices? Or if they do we'd >> likely have to have a large list of possible entity functions that we >> wanted to treat as qualifying here which seems suboptimal. >> >> I'm not sure how to proceed with identifying entities that are >> embedded within a subdevice without that type field...we could update >> struct media_entity_desc so that one of the reserved fields returns >> the obj_type to give us those MEDIA_ENTITY_TYPE_V4L2_SUBDEV / >> TYPE_VIDEO_DEVICE values but that won't help in the short term - do >> you have any thoughts? > > As subdevs are not required to have an entity, I don't think that would > be enough. > > One option would be to create the interface at the same time the entity > is registered. We won't know the major/minor at that point, and they > could stay 0 until the userspace-facing device is registered. This would > allow userspace to know if an interface is eventually expected to > appear. I haven't investigated how difficult this would be on the kernel > side. So create the interface with the entity, but leave major and minor as 0 and within libcamera defer any MediaDevice that has an entity with an interface but who's major and minor numbers are currently 0; is that right? I think that would be feasible though I haven't looked in detail at the kernel side changes. > > Another thing that would likely help is adding support for events in the > MC API, to report updates to the topology. That way libcamera wouldn't > need to reject an incomplete media device, it could instead update it. Yes; reliance on the topology_version becoming stable is not too reliable I think; an events system for MC is needed in the long term. > >>>>> would only be created for V4L2 subdevices once the v4l2-async framework >>>>> had run to completion. >>>> >>>> How is that the case ? If no subdevide is created, there should be no >>>> entity in the media graph. >>> >>> Indeed; the subdevice's entity exists, it's the interface that doesn't seem to. >>> >>>>> Update populateEntities() to return -EAGAIN in the event an entity has >>>>> no associated interface yet and pass that error out from populate to >>>>> indicate to callers that they should re-try the population at a later >>>>> time. >>>>> >>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>>> --- >>>>> include/libcamera/internal/media_device.h | 2 +- >>>>> src/libcamera/media_device.cpp | 27 ++++++++++++++--------- >>>>> 2 files changed, 17 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h >>>>> index b3a48b98d..d3c5eaa02 100644 >>>>> --- a/include/libcamera/internal/media_device.h >>>>> +++ b/include/libcamera/internal/media_device.h >>>>> @@ -70,7 +70,7 @@ private: >>>>> struct media_v2_interface *findInterface(const struct media_v2_topology &topology, >>>>> unsigned int entityId); >>>>> - bool populateEntities(const struct media_v2_topology &topology); >>>>> + int populateEntities(const struct media_v2_topology &topology); >>>>> bool populatePads(const struct media_v2_topology &topology); >>>>> bool populateLinks(const struct media_v2_topology &topology); >>>>> void fixupEntityFlags(struct media_v2_entity *entity); >>>>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >>>>> index 353f34a81..6d58f09eb 100644 >>>>> --- a/src/libcamera/media_device.cpp >>>>> +++ b/src/libcamera/media_device.cpp >>>>> @@ -257,12 +257,13 @@ int MediaDevice::populate() >>>> >>>> You need to update the function documemntation. >>> >>> Ack >>> >>>>> } >>>>> /* Populate entities, pads and links. */ >>>>> - if (populateEntities(topology) && >>>>> - populatePads(topology) && >>>>> - populateLinks(topology)) >>>>> - valid_ = true; >>>>> + if (!(ret = populateEntities(topology))) { >>>>> + if (populatePads(topology) && populateLinks(topology)) >>>>> + valid_ = true; >>>>> + else >>>>> + ret = -EINVAL; >>>>> + } >>>>> - ret = 0; >>>> >>>> This looks convoluted. Let's improve the function first: >>>> >>>> - Turn the ents, interfaces, links and pads arrays into std::vector >>>> instances. That will avoid having to delete the array manually in the >>>> exit path of the function. >>>> >>>> - Use utils::scope_exit to perform the cleanup, calling >>>> >>>> close(); >>>> if (!valie) >>>> clear(); >>>> >>>> That will allow you to get rid of the gotos. >>>> >>>> The above code can then be simplified and made clearer. >>> >>> Oooh; TIL of utils::scope_exit - thanks! I'll take a look. >>> >>>>> done: >>>>> close(); >>>>> @@ -271,10 +272,8 @@ done: >>>>> delete[] pads; >>>>> delete[] links; >>>>> - if (!valid_) { >>>>> + if (!valid_) >>>>> clear(); >>>>> - return -EINVAL; >>>>> - } >>>>> return ret; >>>>> } >>>>> @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo >>>>> * For each entity in the media graph create a MediaEntity and store a >>>>> * reference in the media device objects map and entities list. >>>>> */ >>>>> -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >>>>> +int MediaDevice::populateEntities(const struct media_v2_topology &topology) >>>>> { >>>>> struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> >>>>> (topology.ptr_entities); >>>>> @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) >>>>> */ >>>>> struct media_v2_interface *iface = >>>>> findInterface(topology, ent->id); >>>>> + if (!iface) { >>>>> + LOG(MediaDevice, Debug) >>>>> + << "Entity " << ent->name << " has no interface yet"; >>>>> + return -EAGAIN; >>>>> + } >>>>> + >>>>> MediaEntity *entity = new MediaEntity(this, ent, iface); >>>>> if (!addObject(entity)) { >>>>> delete entity; >>>>> - return false; >>>>> + return -EINVAL; >>>>> } >>>>> entities_.push_back(entity); >>>>> } >>>>> - return true; >>>>> + return 0; >>>>> } >>>>> bool MediaDevice::populatePads(const struct media_v2_topology &topology) >
On Thu, Aug 21, 2025 at 12:59:56PM +0100, Daniel Scally wrote: > On 21/08/2025 12:09, Laurent Pinchart wrote: > > On Thu, Aug 21, 2025 at 11:52:29AM +0100, Daniel Scally wrote: > >> On 20/08/2025 22:33, Dan Scally wrote: > >>> On 20/08/2025 20:39, Laurent Pinchart wrote: > >>>> On Wed, Aug 20, 2025 at 02:23:14PM +0100, Daniel Scally wrote: > >>>>> At the moment populateEntities() just returns true or false to indicate > >>>>> success or failure. Although the populate() function assumes that the > >>>>> topology is settled once the version number stops incrementing, that's > >>>>> not guaranteed to be the case. One indicator that it's likely to increase > >>>>> again in the future is the absence of interfaces for an entity, which > >>>> > >>>> Not all entities necessarily expose an interface to userspace, the code > >>>> has to cope with that. > >>> > >>> Ah, good point...can I just check for MEDIA_ENTITY_TYPE_V4L2_SUBDEV? > >>> That ought to be enough I think right? > > > > That would probably cover the vast majority of cases in practice, but > > there's nothing in the spec that forces subdevs to have an interface. > > > >> Having investigated this a bit, it turns out that the type field > >> that's returned from MEDIA_IOC_ENUM_ENTITIES for an entity isn't > >> actually a value from MEDIA_ENTITY_TYPE_* but instead is the function, > >> which I don't think map specifically to subdevices? Or if they do we'd > >> likely have to have a large list of possible entity functions that we > >> wanted to treat as qualifying here which seems suboptimal. > >> > >> I'm not sure how to proceed with identifying entities that are > >> embedded within a subdevice without that type field...we could update > >> struct media_entity_desc so that one of the reserved fields returns > >> the obj_type to give us those MEDIA_ENTITY_TYPE_V4L2_SUBDEV / > >> TYPE_VIDEO_DEVICE values but that won't help in the short term - do > >> you have any thoughts? > > > > As subdevs are not required to have an entity, I don't think that would > > be enough. > > > > One option would be to create the interface at the same time the entity > > is registered. We won't know the major/minor at that point, and they > > could stay 0 until the userspace-facing device is registered. This would > > allow userspace to know if an interface is eventually expected to > > appear. I haven't investigated how difficult this would be on the kernel > > side. > > So create the interface with the entity, but leave major and minor as > 0 and within libcamera defer any MediaDevice that has an entity with > an interface but who's major and minor numbers are currently 0; is > that right? Yes, that's the idea. For subdevs you will likely need to do so in __v4l2_device_register_subdev(). At that point the subdev HAS_DEVNODE flag should be available, but you won't know if the driver will eventually call v4l2_device_register_subdev_nodes(). We may need to store a flag in the v4l2_device that drivers initialize early to provide that information. Another issue is that the subdev devnode exposed to userspace is implemented in the kernel as a video_device. The initial media controller framework had a different design, but we were asked to use a video_device during review. The video_device for the subdev interface is allocated in __v4l2_device_register_subdev_nodes(), and the interface itself is allocated in __video_register_device(). All this will likely need heavy and clever refactoring. > I think that would be feasible though I haven't looked in detail at the kernel > side changes. Let's give it a try and see where it leads. > > Another thing that would likely help is adding support for events in the > > MC API, to report updates to the topology. That way libcamera wouldn't > > need to reject an incomplete media device, it could instead update it. > > Yes; reliance on the topology_version becoming stable is not too > reliable I think; an events system for MC is needed in the long term. We never relied on the topology version not changing between two calls as a sign that the media graph is complete. The check is there only to ensure we read a full topology, not a partial topology. Userspace has to allocate the memory that will store the topology information, and that memory has to be sized using the counts returned by a previous call to G_TOPOLOGY. We iterate until the version remains stable to ensure the memory is properly sized. > >>>>> would only be created for V4L2 subdevices once the v4l2-async framework > >>>>> had run to completion. > >>>> > >>>> How is that the case ? If no subdevide is created, there should be no > >>>> entity in the media graph. > >>> > >>> Indeed; the subdevice's entity exists, it's the interface that doesn't seem to. > >>> > >>>>> Update populateEntities() to return -EAGAIN in the event an entity has > >>>>> no associated interface yet and pass that error out from populate to > >>>>> indicate to callers that they should re-try the population at a later > >>>>> time. > >>>>> > >>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >>>>> --- > >>>>> include/libcamera/internal/media_device.h | 2 +- > >>>>> src/libcamera/media_device.cpp | 27 ++++++++++++++--------- > >>>>> 2 files changed, 17 insertions(+), 12 deletions(-) > >>>>> > >>>>> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h > >>>>> index b3a48b98d..d3c5eaa02 100644 > >>>>> --- a/include/libcamera/internal/media_device.h > >>>>> +++ b/include/libcamera/internal/media_device.h > >>>>> @@ -70,7 +70,7 @@ private: > >>>>> struct media_v2_interface *findInterface(const struct media_v2_topology &topology, > >>>>> unsigned int entityId); > >>>>> - bool populateEntities(const struct media_v2_topology &topology); > >>>>> + int populateEntities(const struct media_v2_topology &topology); > >>>>> bool populatePads(const struct media_v2_topology &topology); > >>>>> bool populateLinks(const struct media_v2_topology &topology); > >>>>> void fixupEntityFlags(struct media_v2_entity *entity); > >>>>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > >>>>> index 353f34a81..6d58f09eb 100644 > >>>>> --- a/src/libcamera/media_device.cpp > >>>>> +++ b/src/libcamera/media_device.cpp > >>>>> @@ -257,12 +257,13 @@ int MediaDevice::populate() > >>>> > >>>> You need to update the function documemntation. > >>> > >>> Ack > >>> > >>>>> } > >>>>> /* Populate entities, pads and links. */ > >>>>> - if (populateEntities(topology) && > >>>>> - populatePads(topology) && > >>>>> - populateLinks(topology)) > >>>>> - valid_ = true; > >>>>> + if (!(ret = populateEntities(topology))) { > >>>>> + if (populatePads(topology) && populateLinks(topology)) > >>>>> + valid_ = true; > >>>>> + else > >>>>> + ret = -EINVAL; > >>>>> + } > >>>>> - ret = 0; > >>>> > >>>> This looks convoluted. Let's improve the function first: > >>>> > >>>> - Turn the ents, interfaces, links and pads arrays into std::vector > >>>> instances. That will avoid having to delete the array manually in the > >>>> exit path of the function. > >>>> > >>>> - Use utils::scope_exit to perform the cleanup, calling > >>>> > >>>> close(); > >>>> if (!valie) > >>>> clear(); > >>>> > >>>> That will allow you to get rid of the gotos. > >>>> > >>>> The above code can then be simplified and made clearer. > >>> > >>> Oooh; TIL of utils::scope_exit - thanks! I'll take a look. > >>> > >>>>> done: > >>>>> close(); > >>>>> @@ -271,10 +272,8 @@ done: > >>>>> delete[] pads; > >>>>> delete[] links; > >>>>> - if (!valid_) { > >>>>> + if (!valid_) > >>>>> clear(); > >>>>> - return -EINVAL; > >>>>> - } > >>>>> return ret; > >>>>> } > >>>>> @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo > >>>>> * For each entity in the media graph create a MediaEntity and store a > >>>>> * reference in the media device objects map and entities list. > >>>>> */ > >>>>> -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>>>> +int MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>>>> { > >>>>> struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> > >>>>> (topology.ptr_entities); > >>>>> @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > >>>>> */ > >>>>> struct media_v2_interface *iface = > >>>>> findInterface(topology, ent->id); > >>>>> + if (!iface) { > >>>>> + LOG(MediaDevice, Debug) > >>>>> + << "Entity " << ent->name << " has no interface yet"; > >>>>> + return -EAGAIN; > >>>>> + } > >>>>> + > >>>>> MediaEntity *entity = new MediaEntity(this, ent, iface); > >>>>> if (!addObject(entity)) { > >>>>> delete entity; > >>>>> - return false; > >>>>> + return -EINVAL; > >>>>> } > >>>>> entities_.push_back(entity); > >>>>> } > >>>>> - return true; > >>>>> + return 0; > >>>>> } > >>>>> bool MediaDevice::populatePads(const struct media_v2_topology &topology)
diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h index b3a48b98d..d3c5eaa02 100644 --- a/include/libcamera/internal/media_device.h +++ b/include/libcamera/internal/media_device.h @@ -70,7 +70,7 @@ private: struct media_v2_interface *findInterface(const struct media_v2_topology &topology, unsigned int entityId); - bool populateEntities(const struct media_v2_topology &topology); + int populateEntities(const struct media_v2_topology &topology); bool populatePads(const struct media_v2_topology &topology); bool populateLinks(const struct media_v2_topology &topology); void fixupEntityFlags(struct media_v2_entity *entity); diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index 353f34a81..6d58f09eb 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -257,12 +257,13 @@ int MediaDevice::populate() } /* Populate entities, pads and links. */ - if (populateEntities(topology) && - populatePads(topology) && - populateLinks(topology)) - valid_ = true; + if (!(ret = populateEntities(topology))) { + if (populatePads(topology) && populateLinks(topology)) + valid_ = true; + else + ret = -EINVAL; + } - ret = 0; done: close(); @@ -271,10 +272,8 @@ done: delete[] pads; delete[] links; - if (!valid_) { + if (!valid_) clear(); - return -EINVAL; - } return ret; } @@ -618,7 +617,7 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo * For each entity in the media graph create a MediaEntity and store a * reference in the media device objects map and entities list. */ -bool MediaDevice::populateEntities(const struct media_v2_topology &topology) +int MediaDevice::populateEntities(const struct media_v2_topology &topology) { struct media_v2_entity *mediaEntities = reinterpret_cast<struct media_v2_entity *> (topology.ptr_entities); @@ -639,17 +638,23 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) */ struct media_v2_interface *iface = findInterface(topology, ent->id); + if (!iface) { + LOG(MediaDevice, Debug) + << "Entity " << ent->name << " has no interface yet"; + return -EAGAIN; + } + MediaEntity *entity = new MediaEntity(this, ent, iface); if (!addObject(entity)) { delete entity; - return false; + return -EINVAL; } entities_.push_back(entity); } - return true; + return 0; } bool MediaDevice::populatePads(const struct media_v2_topology &topology)
At the moment populateEntities() just returns true or false to indicate success or failure. Although the populate() function assumes that the topology is settled once the version number stops incrementing, that's not guaranteed to be the case. One indicator that it's likely to increase again in the future is the absence of interfaces for an entity, which would only be created for V4L2 subdevices once the v4l2-async framework had run to completion. Update populateEntities() to return -EAGAIN in the event an entity has no associated interface yet and pass that error out from populate to indicate to callers that they should re-try the population at a later time. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- include/libcamera/internal/media_device.h | 2 +- src/libcamera/media_device.cpp | 27 ++++++++++++++--------- 2 files changed, 17 insertions(+), 12 deletions(-)