| Message ID | 20211203224230.38700-3-djrscally@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, Thank you for the patch. On Fri, Dec 03, 2021 at 10:42:25PM +0000, Daniel Scally wrote: > The populateLinks() function can't currently handle ancillary links > which causes an error to be thrown in process() when the MediaObject > cannot be cast to a MediaPad. > > Add explicit handling for the different link types, creating either > pad-2-pad links or else storing the pointer to the ancillary device > MediaEntity in the ancillaryEntities_ member of the primary. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Storing pointer to the sink entity in the new ancillaryEntities_ > vector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK > rather than creating a MediaLink (Laurent) > > include/linux/media.h | 1 + > src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++----------- > 2 files changed, 32 insertions(+), 15 deletions(-) > > diff --git a/include/linux/media.h b/include/linux/media.h > index 17432318..e3123d1a 100644 > --- a/include/linux/media.h > +++ b/include/linux/media.h > @@ -222,6 +222,7 @@ struct media_pad_desc { > #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28) > # define MEDIA_LNK_FL_DATA_LINK (0 << 28) > # define MEDIA_LNK_FL_INTERFACE_LINK (1 << 28) > +# define MEDIA_LNK_FL_ANCILLARY_LINK (2 << 28) This should be split to a separate patch. You can run git log on include/linux/media.h to see how we usually handle kernel header updates. > struct media_link_desc { > struct media_pad_desc source; > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index aa93da75..6eddb5a0 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) > { > struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *> > (topology.ptr_links); > + unsigned int link_type; libcamera uses camelCase, not snake_case. > + MediaLink *link; > > for (unsigned int i = 0; i < topology.num_links; ++i) { > /* > * Skip links between entities and interfaces: we only care > - * about pad-2-pad links here. > + * about pad-2-pad and entity-2-entity links here. > */ > if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) == > MEDIA_LNK_FL_INTERFACE_LINK) > continue; Should this be moved to the switch-case below to avoid a special case ? > > - /* Store references to source and sink pads in the link. */ > + /* Store references to source and sink objects in the link. */ /* Look up the source and sink objects. */ as you don't necessarily store them in a link anymore. > unsigned int source_id = mediaLinks[i].source_id; > - MediaPad *source = dynamic_cast<MediaPad *> > - (object(source_id)); > + MediaObject *source = object(source_id); > if (!source) { > LOG(MediaDevice, Error) > - << "Failed to find pad with id: " > + << "Failed to find MediaObject with id: " > << source_id; > return false; > } > > unsigned int sink_id = mediaLinks[i].sink_id; > - MediaPad *sink = dynamic_cast<MediaPad *> > - (object(sink_id)); > + MediaObject *sink = object(sink_id); > if (!sink) { > LOG(MediaDevice, Error) > - << "Failed to find pad with id: " > + << "Failed to find MediaObject with id: " > << sink_id; > return false; > } > > - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); > - if (!addObject(link)) { > - delete link; > - return false; > - } > + link_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE; > + > + switch (link_type) { switch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) { > + case MEDIA_LNK_FL_DATA_LINK: > + link = new MediaLink(&mediaLinks[i], > + dynamic_cast<MediaPad *>(source), > + dynamic_cast<MediaPad *>(sink)); > + if (!addObject(link)) { > + delete link; > + return false; > + } > + > + link->source()->addLink(link); > + link->sink()->addLink(link); > > - source->addLink(link); > - sink->addLink(link); > + break; Missing blank line. > + case MEDIA_LNK_FL_ANCILLARY_LINK: > + MediaEntity *primary = dynamic_cast<MediaEntity *>(source); > + MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink); > + > + primary->addAncillaryEntity(ancillary); > + > + break; Should we add a default case and log a warning ? > + } > } > > return true;
Hi Laurent On 04/12/2021 00:12, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Fri, Dec 03, 2021 at 10:42:25PM +0000, Daniel Scally wrote: >> The populateLinks() function can't currently handle ancillary links >> which causes an error to be thrown in process() when the MediaObject >> cannot be cast to a MediaPad. >> >> Add explicit handling for the different link types, creating either >> pad-2-pad links or else storing the pointer to the ancillary device >> MediaEntity in the ancillaryEntities_ member of the primary. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> >> - Storing pointer to the sink entity in the new ancillaryEntities_ >> vector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK >> rather than creating a MediaLink (Laurent) >> >> include/linux/media.h | 1 + >> src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++----------- >> 2 files changed, 32 insertions(+), 15 deletions(-) >> >> diff --git a/include/linux/media.h b/include/linux/media.h >> index 17432318..e3123d1a 100644 >> --- a/include/linux/media.h >> +++ b/include/linux/media.h >> @@ -222,6 +222,7 @@ struct media_pad_desc { >> #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28) >> # define MEDIA_LNK_FL_DATA_LINK (0 << 28) >> # define MEDIA_LNK_FL_INTERFACE_LINK (1 << 28) >> +# define MEDIA_LNK_FL_ANCILLARY_LINK (2 << 28) > > This should be split to a separate patch.You can run git log on > include/linux/media.h to see how we usually handle kernel header > updates. No problem. I did wonder if this was something that would have to wait until it was merged kernelside, but it does look like a few manual additions have been made (and kept) when the headers were upgraded to the newest kernel version. > >> struct media_link_desc { >> struct media_pad_desc source; >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >> index aa93da75..6eddb5a0 100644 >> --- a/src/libcamera/media_device.cpp >> +++ b/src/libcamera/media_device.cpp >> @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) >> { >> struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *> >> (topology.ptr_links); >> + unsigned int link_type; > > libcamera uses camelCase, not snake_case. Ack > >> + MediaLink *link; >> >> for (unsigned int i = 0; i < topology.num_links; ++i) { >> /* >> * Skip links between entities and interfaces: we only care >> - * about pad-2-pad links here. >> + * about pad-2-pad and entity-2-entity links here. >> */ >> if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) == >> MEDIA_LNK_FL_INTERFACE_LINK) >> continue; > > Should this be moved to the switch-case below to avoid a special case ? Yup, not sure why I didn't see that... Ack, to the rest of the comments here. > >> >> - /* Store references to source and sink pads in the link. */ >> + /* Store references to source and sink objects in the link. */ > > /* Look up the source and sink objects. */ > > as you don't necessarily store them in a link anymore. > >> unsigned int source_id = mediaLinks[i].source_id; >> - MediaPad *source = dynamic_cast<MediaPad *> >> - (object(source_id)); >> + MediaObject *source = object(source_id); >> if (!source) { >> LOG(MediaDevice, Error) >> - << "Failed to find pad with id: " >> + << "Failed to find MediaObject with id: " >> << source_id; >> return false; >> } >> >> unsigned int sink_id = mediaLinks[i].sink_id; >> - MediaPad *sink = dynamic_cast<MediaPad *> >> - (object(sink_id)); >> + MediaObject *sink = object(sink_id); >> if (!sink) { >> LOG(MediaDevice, Error) >> - << "Failed to find pad with id: " >> + << "Failed to find MediaObject with id: " >> << sink_id; >> return false; >> } >> >> - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); >> - if (!addObject(link)) { >> - delete link; >> - return false; >> - } >> + link_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE; >> + >> + switch (link_type) { > > switch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) { > >> + case MEDIA_LNK_FL_DATA_LINK: >> + link = new MediaLink(&mediaLinks[i], >> + dynamic_cast<MediaPad *>(source), >> + dynamic_cast<MediaPad *>(sink)); >> + if (!addObject(link)) { >> + delete link; >> + return false; >> + } >> + >> + link->source()->addLink(link); >> + link->sink()->addLink(link); >> >> - source->addLink(link); >> - sink->addLink(link); >> + break; > > Missing blank line. > >> + case MEDIA_LNK_FL_ANCILLARY_LINK: >> + MediaEntity *primary = dynamic_cast<MediaEntity *>(source); >> + MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink); >> + >> + primary->addAncillaryEntity(ancillary); >> + >> + break; > > Should we add a default case and log a warning ? > >> + } >> } >> >> return true; >
diff --git a/include/linux/media.h b/include/linux/media.h index 17432318..e3123d1a 100644 --- a/include/linux/media.h +++ b/include/linux/media.h @@ -222,6 +222,7 @@ struct media_pad_desc { #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28) # define MEDIA_LNK_FL_DATA_LINK (0 << 28) # define MEDIA_LNK_FL_INTERFACE_LINK (1 << 28) +# define MEDIA_LNK_FL_ANCILLARY_LINK (2 << 28) struct media_link_desc { struct media_pad_desc source; diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index aa93da75..6eddb5a0 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -699,45 +699,61 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) { struct media_v2_link *mediaLinks = reinterpret_cast<struct media_v2_link *> (topology.ptr_links); + unsigned int link_type; + MediaLink *link; for (unsigned int i = 0; i < topology.num_links; ++i) { /* * Skip links between entities and interfaces: we only care - * about pad-2-pad links here. + * about pad-2-pad and entity-2-entity links here. */ if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) == MEDIA_LNK_FL_INTERFACE_LINK) continue; - /* Store references to source and sink pads in the link. */ + /* Store references to source and sink objects in the link. */ unsigned int source_id = mediaLinks[i].source_id; - MediaPad *source = dynamic_cast<MediaPad *> - (object(source_id)); + MediaObject *source = object(source_id); if (!source) { LOG(MediaDevice, Error) - << "Failed to find pad with id: " + << "Failed to find MediaObject with id: " << source_id; return false; } unsigned int sink_id = mediaLinks[i].sink_id; - MediaPad *sink = dynamic_cast<MediaPad *> - (object(sink_id)); + MediaObject *sink = object(sink_id); if (!sink) { LOG(MediaDevice, Error) - << "Failed to find pad with id: " + << "Failed to find MediaObject with id: " << sink_id; return false; } - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); - if (!addObject(link)) { - delete link; - return false; - } + link_type = mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE; + + switch (link_type) { + case MEDIA_LNK_FL_DATA_LINK: + link = new MediaLink(&mediaLinks[i], + dynamic_cast<MediaPad *>(source), + dynamic_cast<MediaPad *>(sink)); + if (!addObject(link)) { + delete link; + return false; + } + + link->source()->addLink(link); + link->sink()->addLink(link); - source->addLink(link); - sink->addLink(link); + break; + case MEDIA_LNK_FL_ANCILLARY_LINK: + MediaEntity *primary = dynamic_cast<MediaEntity *>(source); + MediaEntity *ancillary = dynamic_cast<MediaEntity *>(sink); + + primary->addAncillaryEntity(ancillary); + + break; + } } return true;
The populateLinks() function can't currently handle ancillary links which causes an error to be thrown in process() when the MediaObject cannot be cast to a MediaPad. Add explicit handling for the different link types, creating either pad-2-pad links or else storing the pointer to the ancillary device MediaEntity in the ancillaryEntities_ member of the primary. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Storing pointer to the sink entity in the new ancillaryEntities_ vector of the source entity where in case MEDIA_LNK_FL_ANCILLARY_LINK rather than creating a MediaLink (Laurent) include/linux/media.h | 1 + src/libcamera/media_device.cpp | 46 +++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 15 deletions(-)