| Message ID | 1476282922-11544-4-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacek, On Wed, Oct 12, 2016 at 04:35:18PM +0200, Jacek Anaszewski wrote: > Add a new graph helper useful for discovering video pipeline. > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > utils/media-ctl/libmediactl.c | 21 +++++++++++++++++++++ > utils/media-ctl/mediactl.h | 15 +++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > index 91ed003..155b65f 100644 > --- a/utils/media-ctl/libmediactl.c > +++ b/utils/media-ctl/libmediactl.c > @@ -36,6 +36,7 @@ > #include <unistd.h> > > #include <linux/media.h> > +#include <linux/kdev_t.h> Is there something that needs this one in the patch? > #include <linux/videodev2.h> > > #include "mediactl.h" > @@ -172,6 +173,26 @@ const struct media_entity_desc *media_entity_get_info(struct media_entity *entit > return &entity->info; > } > > +int media_entity_get_backlinks(struct media_entity *entity, > + struct media_link **backlinks, > + unsigned int *num_backlinks) > +{ > + unsigned int num_bklinks = 0; > + int i; > + > + if (entity == NULL || backlinks == NULL || num_backlinks == NULL) > + return -EINVAL; > + If you have an interface that accesses a memory buffer of unknown size, you need to verify that the user has provided a buffer large enough. How about using the num_backlinks argument to provide the maximum size to the function, and passing the actual number to the user, the latter of which you already do? Alternatively, an iterator style API could be nice as well. Up to you. I wonder what Laurent thinks. > + for (i = 0; i < entity->num_links; ++i) > + if ((entity->links[i].flags & MEDIA_LNK_FL_ENABLED) && > + (entity->links[i].sink->entity == entity)) > + backlinks[num_bklinks++] = &entity->links[i]; > + > + *num_backlinks = num_bklinks; > + > + return 0; > +} > + > /* ----------------------------------------------------------------------------- > * Open/close > */ > diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h > index 336cbf9..b1f33cd 100644 > --- a/utils/media-ctl/mediactl.h > +++ b/utils/media-ctl/mediactl.h > @@ -434,6 +434,20 @@ int media_parse_setup_link(struct media_device *media, > int media_parse_setup_links(struct media_device *media, const char *p); > > /** > + * @brief Get entity's enabled backlinks > + * @param entity - media entity. > + * @param backlinks - array of pointers to matching backlinks. > + * @param num_backlinks - number of matching backlinks. > + * > + * Get links that are connected to the entity sink pads. > + * > + * @return 0 on success, or a negative error code on failure. > + */ > +int media_entity_get_backlinks(struct media_entity *entity, > + struct media_link **backlinks, > + unsigned int *num_backlinks); > + > +/** > * @brief Get v4l2_subdev for the entity > * @param entity - media entity > * > @@ -443,4 +457,5 @@ int media_parse_setup_links(struct media_device *media, const char *p); > */ > struct v4l2_subdev *media_entity_get_v4l2_subdev(struct media_entity *entity); > > + Unrelated change. > #endif
Hi Sakari, Thanks for the review. On 11/24/2016 01:40 PM, Sakari Ailus wrote: > Hi Jacek, > > On Wed, Oct 12, 2016 at 04:35:18PM +0200, Jacek Anaszewski wrote: >> Add a new graph helper useful for discovering video pipeline. >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> utils/media-ctl/libmediactl.c | 21 +++++++++++++++++++++ >> utils/media-ctl/mediactl.h | 15 +++++++++++++++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c >> index 91ed003..155b65f 100644 >> --- a/utils/media-ctl/libmediactl.c >> +++ b/utils/media-ctl/libmediactl.c >> @@ -36,6 +36,7 @@ >> #include <unistd.h> >> >> #include <linux/media.h> >> +#include <linux/kdev_t.h> > > Is there something that needs this one in the patch? MAJOR and MINOR macros. > >> #include <linux/videodev2.h> >> >> #include "mediactl.h" >> @@ -172,6 +173,26 @@ const struct media_entity_desc *media_entity_get_info(struct media_entity *entit >> return &entity->info; >> } >> >> +int media_entity_get_backlinks(struct media_entity *entity, >> + struct media_link **backlinks, >> + unsigned int *num_backlinks) >> +{ >> + unsigned int num_bklinks = 0; >> + int i; >> + >> + if (entity == NULL || backlinks == NULL || num_backlinks == NULL) >> + return -EINVAL; >> + > > If you have an interface that accesses a memory buffer of unknown size, you > need to verify that the user has provided a buffer large enough. > > How about using the num_backlinks argument to provide the maximum size to > the function, and passing the actual number to the user, the latter of which > you already do? Sounds reasonable. > Alternatively, an iterator style API could be nice as well. Up to you. It would probably need an addition of some generic infrastructure. I suppose that there is no such a feature in v4l-utils? > > I wonder what Laurent thinks. > >> + for (i = 0; i < entity->num_links; ++i) >> + if ((entity->links[i].flags & MEDIA_LNK_FL_ENABLED) && >> + (entity->links[i].sink->entity == entity)) >> + backlinks[num_bklinks++] = &entity->links[i]; >> + >> + *num_backlinks = num_bklinks; >> + >> + return 0; >> +} >> + >> /* ----------------------------------------------------------------------------- >> * Open/close >> */ >> diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h >> index 336cbf9..b1f33cd 100644 >> --- a/utils/media-ctl/mediactl.h >> +++ b/utils/media-ctl/mediactl.h >> @@ -434,6 +434,20 @@ int media_parse_setup_link(struct media_device *media, >> int media_parse_setup_links(struct media_device *media, const char *p); >> >> /** >> + * @brief Get entity's enabled backlinks >> + * @param entity - media entity. >> + * @param backlinks - array of pointers to matching backlinks. >> + * @param num_backlinks - number of matching backlinks. >> + * >> + * Get links that are connected to the entity sink pads. >> + * >> + * @return 0 on success, or a negative error code on failure. >> + */ >> +int media_entity_get_backlinks(struct media_entity *entity, >> + struct media_link **backlinks, >> + unsigned int *num_backlinks); >> + >> +/** >> * @brief Get v4l2_subdev for the entity >> * @param entity - media entity >> * >> @@ -443,4 +457,5 @@ int media_parse_setup_links(struct media_device *media, const char *p); >> */ >> struct v4l2_subdev *media_entity_get_v4l2_subdev(struct media_entity *entity); >> >> + > > Unrelated change. > >> #endif >
Hi Jacek, On Thu, Nov 24, 2016 at 03:00:46PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. It's taken way too long. :-( My apologies for that. > On 11/24/2016 01:40 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Wed, Oct 12, 2016 at 04:35:18PM +0200, Jacek Anaszewski wrote: > >>Add a new graph helper useful for discovering video pipeline. > >> > >>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > >>Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > >>--- > >> utils/media-ctl/libmediactl.c | 21 +++++++++++++++++++++ > >> utils/media-ctl/mediactl.h | 15 +++++++++++++++ > >> 2 files changed, 36 insertions(+) > >> > >>diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > >>index 91ed003..155b65f 100644 > >>--- a/utils/media-ctl/libmediactl.c > >>+++ b/utils/media-ctl/libmediactl.c > >>@@ -36,6 +36,7 @@ > >> #include <unistd.h> > >> > >> #include <linux/media.h> > >>+#include <linux/kdev_t.h> > > > >Is there something that needs this one in the patch? > > MAJOR and MINOR macros. Ok. > > > > >> #include <linux/videodev2.h> > >> > >> #include "mediactl.h" > >>@@ -172,6 +173,26 @@ const struct media_entity_desc *media_entity_get_info(struct media_entity *entit > >> return &entity->info; > >> } > >> > >>+int media_entity_get_backlinks(struct media_entity *entity, > >>+ struct media_link **backlinks, > >>+ unsigned int *num_backlinks) > >>+{ > >>+ unsigned int num_bklinks = 0; > >>+ int i; > >>+ > >>+ if (entity == NULL || backlinks == NULL || num_backlinks == NULL) > >>+ return -EINVAL; > >>+ > > > >If you have an interface that accesses a memory buffer of unknown size, you > >need to verify that the user has provided a buffer large enough. > > > >How about using the num_backlinks argument to provide the maximum size to > >the function, and passing the actual number to the user, the latter of which > >you already do? > > Sounds reasonable. > > >Alternatively, an iterator style API could be nice as well. Up to you. > > It would probably need an addition of some generic infrastructure. > I suppose that there is no such a feature in v4l-utils? You basically need to store a value for the framework to tell which entity is being worked on. So nothing too fancy. I guess Laurent would prefer the current interface but I let him answer that.
diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c index 91ed003..155b65f 100644 --- a/utils/media-ctl/libmediactl.c +++ b/utils/media-ctl/libmediactl.c @@ -36,6 +36,7 @@ #include <unistd.h> #include <linux/media.h> +#include <linux/kdev_t.h> #include <linux/videodev2.h> #include "mediactl.h" @@ -172,6 +173,26 @@ const struct media_entity_desc *media_entity_get_info(struct media_entity *entit return &entity->info; } +int media_entity_get_backlinks(struct media_entity *entity, + struct media_link **backlinks, + unsigned int *num_backlinks) +{ + unsigned int num_bklinks = 0; + int i; + + if (entity == NULL || backlinks == NULL || num_backlinks == NULL) + return -EINVAL; + + for (i = 0; i < entity->num_links; ++i) + if ((entity->links[i].flags & MEDIA_LNK_FL_ENABLED) && + (entity->links[i].sink->entity == entity)) + backlinks[num_bklinks++] = &entity->links[i]; + + *num_backlinks = num_bklinks; + + return 0; +} + /* ----------------------------------------------------------------------------- * Open/close */ diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h index 336cbf9..b1f33cd 100644 --- a/utils/media-ctl/mediactl.h +++ b/utils/media-ctl/mediactl.h @@ -434,6 +434,20 @@ int media_parse_setup_link(struct media_device *media, int media_parse_setup_links(struct media_device *media, const char *p); /** + * @brief Get entity's enabled backlinks + * @param entity - media entity. + * @param backlinks - array of pointers to matching backlinks. + * @param num_backlinks - number of matching backlinks. + * + * Get links that are connected to the entity sink pads. + * + * @return 0 on success, or a negative error code on failure. + */ +int media_entity_get_backlinks(struct media_entity *entity, + struct media_link **backlinks, + unsigned int *num_backlinks); + +/** * @brief Get v4l2_subdev for the entity * @param entity - media entity * @@ -443,4 +457,5 @@ int media_parse_setup_links(struct media_device *media, const char *p); */ struct v4l2_subdev *media_entity_get_v4l2_subdev(struct media_entity *entity); + #endif