| Message ID | 2fi0-eM0Z4VDp7ia9hJVx96VwN0n3oGU3Jv05Af9xKBJ5mGjXNJ5KV7SRbExyTImQblR8jBiGAtJnPhtuMVzDqrLBkYd_KQ2q77ScInUiSM=@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Dan, Welcome to libcamera, and thanks for contributing! On 24/08/2020 21:01, Dan Scally wrote: Our normal style for patches is to have short prefixes on the commit title to describe the component being modified, so I think we can simplify the $SUBJECT: "libcamera: media_device: Account for existing flags with setupLink" > The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag > to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast > to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. > This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in > media-ctl. Oh, interesting catch, what platform did you hit this on? > ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 > > Signed-off-by: Daniel Scally <djrscally@protonmail.com> > --- > src/libcamera/media_device.cpp | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index de18d57..cd44902 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -780,11 +780,20 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity) > * > * \return 0 on success or a negative error code otherwise > */ > -int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) > +int MediaDevice::setupLink(const MediaLink *nlink, unsigned int flags) > { > struct media_link_desc linkDesc = {}; > - MediaPad *source = link->source(); > - MediaPad *sink = link->sink(); > + MediaPad *source = nlink->source(); > + MediaPad *sink = nlink->sink(); > + MediaLink *elink; // existing link > + > + elink = link(source, sink); Isn't this the same link as the passed in (link,nlink) ? I don't think we need to refetch it do we? (Perhaps I missed something reading the code?) > + > + if (elink == NULL) { > + LOG(MediaDevice, Error) > + << __func__ << ": Link not found\n"; > + return -ENOENT; > + } > > > linkDesc.source.entity = source->entity()->id(); > linkDesc.source.index = source->index(); > @@ -794,7 +803,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) > linkDesc.sink.index = sink->index(); > linkDesc.sink.flags = MEDIA_PAD_FL_SINK; > > > - linkDesc.flags = flags; > + linkDesc.flags = flags | (elink->flags() & MEDIA_LNK_FL_IMMUTABLE); If we can re-use the const MediaLink *link, we could then simply take the link->flags() ... but I'd then be wondering if we need to update the flags when they've been set ... but I don't think you need to worry about that for this patch. Could you test to see if setting this as simply : linkDesc.flags = flags | (link->flags() & MEDIA_LNK_FL_IMMUTABLE); without changing the rest of the function solves your issue please? -- Regards Kieran > > > int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); > if (ret) { > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index de18d57..cd44902 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -780,11 +780,20 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity) * * \return 0 on success or a negative error code otherwise */ -int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) +int MediaDevice::setupLink(const MediaLink *nlink, unsigned int flags) { struct media_link_desc linkDesc = {}; - MediaPad *source = link->source(); - MediaPad *sink = link->sink(); + MediaPad *source = nlink->source(); + MediaPad *sink = nlink->sink(); + MediaLink *elink; // existing link + + elink = link(source, sink); + + if (elink == NULL) { + LOG(MediaDevice, Error) + << __func__ << ": Link not found\n"; + return -ENOENT; + } linkDesc.source.entity = source->entity()->id();
The setupLink function fails (ioctl returns EINVAL) when it passes the MEDIA_LNK_FL_ENABLE flag to a link that is already flagged with MEDIA_LNK_FL_ENABLE and MEDIA_LNK_FL_IMMUTABLE. Contrast to media-ctl's equivalent media_setup_link() which ORs the new flags with the existing values. This patch modifies the behaviour of setupLink() to behave the same as media_setup_link() in media-ctl. ref https://git.linuxtv.org/v4l-utils.git/tree/utils/media-ctl/libmediactl.c#n210 Signed-off-by: Daniel Scally <djrscally@protonmail.com> --- src/libcamera/media_device.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) linkDesc.source.index = source->index(); @@ -794,7 +803,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags) linkDesc.sink.index = sink->index(); linkDesc.sink.flags = MEDIA_PAD_FL_SINK; - linkDesc.flags = flags; + linkDesc.flags = flags | (elink->flags() & MEDIA_LNK_FL_IMMUTABLE); int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc); if (ret) {