| Message ID | 1416586480-19982-2-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacek, Thank you for the updated patchset. On Fri, Nov 21, 2014 at 05:14:30PM +0100, Jacek Anaszewski wrote: > Add struct v4l2_subdev as a representation of the v4l2 sub-device > related to a media entity. Add sd property, the pointer to > the newly introduced structure, to the struct media_entity > and move fd property to it. > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > utils/media-ctl/libmediactl.c | 30 +++++++++++++++++++++++++----- > utils/media-ctl/libv4l2subdev.c | 34 +++++++++++++++++----------------- > utils/media-ctl/mediactl-priv.h | 5 +++++ > utils/media-ctl/mediactl.h | 22 ++++++++++++++++++++++ > 4 files changed, 69 insertions(+), 22 deletions(-) > > diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > index ec360bd..53921f5 100644 > --- a/utils/media-ctl/libmediactl.c > +++ b/utils/media-ctl/libmediactl.c > @@ -511,7 +511,6 @@ static int media_enum_entities(struct media_device *media) > > entity = &media->entities[media->entities_count]; > memset(entity, 0, sizeof(*entity)); > - entity->fd = -1; I think I'd definitely leave the fd to the media_entity itself. Not all the entities are sub-devices, even right now. > entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; > entity->media = media; > > @@ -529,11 +528,13 @@ static int media_enum_entities(struct media_device *media) > > entity->pads = malloc(entity->info.pads * sizeof(*entity->pads)); > entity->links = malloc(entity->max_links * sizeof(*entity->links)); > - if (entity->pads == NULL || entity->links == NULL) { > + entity->sd = calloc(1, sizeof(*entity->sd)); > + if (entity->pads == NULL || entity->links == NULL || entity->sd == NULL) { > ret = -ENOMEM; > break; > } > > + entity->sd->fd = -1; > media->entities_count++; > > if (entity->info.flags & MEDIA_ENT_FL_DEFAULT) { > @@ -704,8 +705,9 @@ void media_device_unref(struct media_device *media) > > free(entity->pads); > free(entity->links); > - if (entity->fd != -1) > - close(entity->fd); > + if (entity->sd->fd != -1) > + close(entity->sd->fd); > + free(entity->sd); > } > > free(media->entities); > @@ -726,13 +728,17 @@ int media_device_add_entity(struct media_device *media, > if (entity == NULL) > return -ENOMEM; > > + entity->sd = calloc(1, sizeof(*entity->sd)); > + if (entity->sd == NULL) > + return -ENOMEM; > + > media->entities = entity; > media->entities_count++; > > entity = &media->entities[media->entities_count - 1]; > memset(entity, 0, sizeof *entity); > > - entity->fd = -1; > + entity->sd->fd = -1; > entity->media = media; > strncpy(entity->devname, devnode, sizeof entity->devname); > entity->devname[sizeof entity->devname - 1] = '\0'; > @@ -955,3 +961,17 @@ int media_parse_setup_links(struct media_device *media, const char *p) > > return *end ? -EINVAL : 0; > } > + > +/* ----------------------------------------------------------------------------- > + * Media entity access > + */ > + > +int media_entity_get_fd(struct media_entity *entity) > +{ > + return entity->sd->fd; > +} > + > +void media_entity_set_fd(struct media_entity *entity, int fd) > +{ > + entity->sd->fd = fd; > +} You access the fd directly now inside the library. I don't think there should be a need to set it. > diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c > index 8015330..09e0081 100644 > --- a/utils/media-ctl/libv4l2subdev.c > +++ b/utils/media-ctl/libv4l2subdev.c > @@ -41,11 +41,11 @@ > > int v4l2_subdev_open(struct media_entity *entity) > { > - if (entity->fd != -1) > + if (entity->sd->fd != -1) > return 0; > > - entity->fd = open(entity->devname, O_RDWR); > - if (entity->fd == -1) { > + entity->sd->fd = open(entity->devname, O_RDWR); > + if (entity->sd->fd == -1) { > int ret = -errno; > media_dbg(entity->media, > "%s: Failed to open subdev device node %s\n", __func__, > @@ -58,8 +58,8 @@ int v4l2_subdev_open(struct media_entity *entity) > > void v4l2_subdev_close(struct media_entity *entity) > { > - close(entity->fd); > - entity->fd = -1; > + close(entity->sd->fd); > + entity->sd->fd = -1; > } > > int v4l2_subdev_get_format(struct media_entity *entity, > @@ -77,7 +77,7 @@ int v4l2_subdev_get_format(struct media_entity *entity, > fmt.pad = pad; > fmt.which = which; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FMT, &fmt); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FMT, &fmt); > if (ret < 0) > return -errno; > > @@ -101,7 +101,7 @@ int v4l2_subdev_set_format(struct media_entity *entity, > fmt.which = which; > fmt.format = *format; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FMT, &fmt); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FMT, &fmt); > if (ret < 0) > return -errno; > > @@ -128,7 +128,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, > u.sel.target = target; > u.sel.which = which; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); > if (ret >= 0) { > *rect = u.sel.r; > return 0; > @@ -140,7 +140,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, > u.crop.pad = pad; > u.crop.which = which; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); > if (ret < 0) > return -errno; > > @@ -168,7 +168,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, > u.sel.which = which; > u.sel.r = *rect; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); > if (ret >= 0) { > *rect = u.sel.r; > return 0; > @@ -181,7 +181,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, > u.crop.which = which; > u.crop.rect = *rect; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); > if (ret < 0) > return -errno; > > @@ -202,7 +202,7 @@ int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity, > memset(caps, 0, sizeof(*caps)); > caps->pad = pad; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); > if (ret < 0) > return -errno; > > @@ -220,7 +220,7 @@ int v4l2_subdev_query_dv_timings(struct media_entity *entity, > > memset(timings, 0, sizeof(*timings)); > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); > if (ret < 0) > return -errno; > > @@ -238,7 +238,7 @@ int v4l2_subdev_get_dv_timings(struct media_entity *entity, > > memset(timings, 0, sizeof(*timings)); > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); > if (ret < 0) > return -errno; > > @@ -254,7 +254,7 @@ int v4l2_subdev_set_dv_timings(struct media_entity *entity, > if (ret < 0) > return ret; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); > if (ret < 0) > return -errno; > > @@ -273,7 +273,7 @@ int v4l2_subdev_get_frame_interval(struct media_entity *entity, > > memset(&ival, 0, sizeof(ival)); > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); > if (ret < 0) > return -errno; > > @@ -294,7 +294,7 @@ int v4l2_subdev_set_frame_interval(struct media_entity *entity, > memset(&ival, 0, sizeof(ival)); > ival.interval = *interval; > > - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); > + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); > if (ret < 0) > return -errno; > > diff --git a/utils/media-ctl/mediactl-priv.h b/utils/media-ctl/mediactl-priv.h > index a0d3a55..4bcb1e0 100644 > --- a/utils/media-ctl/mediactl-priv.h > +++ b/utils/media-ctl/mediactl-priv.h > @@ -34,7 +34,12 @@ struct media_entity { > unsigned int max_links; > unsigned int num_links; > > + struct v4l2_subdev *sd; > + > char devname[32]; > +}; > + > +struct v4l2_subdev { > int fd; struct v4l2_subdev should be defined in v4l2subdev.h. > }; > > diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h > index 77ac182..b8cefe8 100644 > --- a/utils/media-ctl/mediactl.h > +++ b/utils/media-ctl/mediactl.h > @@ -420,4 +420,26 @@ int media_parse_setup_link(struct media_device *media, > */ > int media_parse_setup_links(struct media_device *media, const char *p); > > +/** > + * @brief Get file descriptor of the entity sub-device > + * @param entity - media entity > + * > + * This function gets the file descriptor of the opened > + * sub-device node related to the entity. > + * > + * @return file descriptor of the opened sub-device, > + or -1 if the sub-device is closed > + */ > +int media_entity_get_fd(struct media_entity *entity); > + > +/** > + * @brief Set file descriptor of the entity sub-device > + * @param entity - media entity > + * @param fd - entity sub-device file descriptor > + * > + * This function sets the file descriptor of the opened > + * sub-device node related to the entity. > + */ > +void media_entity_set_fd(struct media_entity *entity, int fd); > + > #endif
Hi Sakari, On 11/25/2014 12:36 PM, Sakari Ailus wrote: > Hi Jacek, > > Thank you for the updated patchset. > > On Fri, Nov 21, 2014 at 05:14:30PM +0100, Jacek Anaszewski wrote: >> Add struct v4l2_subdev as a representation of the v4l2 sub-device >> related to a media entity. Add sd property, the pointer to >> the newly introduced structure, to the struct media_entity >> and move fd property to it. >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> utils/media-ctl/libmediactl.c | 30 +++++++++++++++++++++++++----- >> utils/media-ctl/libv4l2subdev.c | 34 +++++++++++++++++----------------- >> utils/media-ctl/mediactl-priv.h | 5 +++++ >> utils/media-ctl/mediactl.h | 22 ++++++++++++++++++++++ >> 4 files changed, 69 insertions(+), 22 deletions(-) >> >> diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c >> index ec360bd..53921f5 100644 >> --- a/utils/media-ctl/libmediactl.c >> +++ b/utils/media-ctl/libmediactl.c >> @@ -511,7 +511,6 @@ static int media_enum_entities(struct media_device *media) >> >> entity = &media->entities[media->entities_count]; >> memset(entity, 0, sizeof(*entity)); >> - entity->fd = -1; > > I think I'd definitely leave the fd to the media_entity itself. Not all the > entities are sub-devices, even right now. I am aware of it, I even came across this issue while implementing the function v4l2_subdev_apply_pipeline_fmt. I added suitable comment explaining why the entity not being a sub-device has its representation. I moved the fd out of media_entity by following Laurent's message [1], where he mentioned this, however I think that it would be indeed best if it remained intact. >> entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; >> entity->media = media; >> >> @@ -529,11 +528,13 @@ static int media_enum_entities(struct media_device *media) >> >> entity->pads = malloc(entity->info.pads * sizeof(*entity->pads)); >> entity->links = malloc(entity->max_links * sizeof(*entity->links)); >> - if (entity->pads == NULL || entity->links == NULL) { >> + entity->sd = calloc(1, sizeof(*entity->sd)); >> + if (entity->pads == NULL || entity->links == NULL || entity->sd == NULL) { >> ret = -ENOMEM; >> break; >> } >> >> + entity->sd->fd = -1; >> media->entities_count++; >> >> if (entity->info.flags & MEDIA_ENT_FL_DEFAULT) { >> @@ -704,8 +705,9 @@ void media_device_unref(struct media_device *media) >> >> free(entity->pads); >> free(entity->links); >> - if (entity->fd != -1) >> - close(entity->fd); >> + if (entity->sd->fd != -1) >> + close(entity->sd->fd); >> + free(entity->sd); >> } >> >> free(media->entities); >> @@ -726,13 +728,17 @@ int media_device_add_entity(struct media_device *media, >> if (entity == NULL) >> return -ENOMEM; >> >> + entity->sd = calloc(1, sizeof(*entity->sd)); >> + if (entity->sd == NULL) >> + return -ENOMEM; >> + >> media->entities = entity; >> media->entities_count++; >> >> entity = &media->entities[media->entities_count - 1]; >> memset(entity, 0, sizeof *entity); >> >> - entity->fd = -1; >> + entity->sd->fd = -1; >> entity->media = media; >> strncpy(entity->devname, devnode, sizeof entity->devname); >> entity->devname[sizeof entity->devname - 1] = '\0'; >> @@ -955,3 +961,17 @@ int media_parse_setup_links(struct media_device *media, const char *p) >> >> return *end ? -EINVAL : 0; >> } >> + >> +/* ----------------------------------------------------------------------------- >> + * Media entity access >> + */ >> + >> +int media_entity_get_fd(struct media_entity *entity) >> +{ >> + return entity->sd->fd; >> +} >> + >> +void media_entity_set_fd(struct media_entity *entity, int fd) >> +{ >> + entity->sd->fd = fd; >> +} > > You access the fd directly now inside the library. I don't think there > should be a need to set it. struct media_entity is defined in mediactl-priv.h, whose name implies that it shouldn't be made public. Thats way I implemented the setter. I use it in the libv4l-exynos4-camera.c. >> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c >> index 8015330..09e0081 100644 >> --- a/utils/media-ctl/libv4l2subdev.c >> +++ b/utils/media-ctl/libv4l2subdev.c >> @@ -41,11 +41,11 @@ >> >> int v4l2_subdev_open(struct media_entity *entity) >> { >> - if (entity->fd != -1) >> + if (entity->sd->fd != -1) >> return 0; >> >> - entity->fd = open(entity->devname, O_RDWR); >> - if (entity->fd == -1) { >> + entity->sd->fd = open(entity->devname, O_RDWR); >> + if (entity->sd->fd == -1) { >> int ret = -errno; >> media_dbg(entity->media, >> "%s: Failed to open subdev device node %s\n", __func__, >> @@ -58,8 +58,8 @@ int v4l2_subdev_open(struct media_entity *entity) >> >> void v4l2_subdev_close(struct media_entity *entity) >> { >> - close(entity->fd); >> - entity->fd = -1; >> + close(entity->sd->fd); >> + entity->sd->fd = -1; >> } >> >> int v4l2_subdev_get_format(struct media_entity *entity, >> @@ -77,7 +77,7 @@ int v4l2_subdev_get_format(struct media_entity *entity, >> fmt.pad = pad; >> fmt.which = which; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FMT, &fmt); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FMT, &fmt); >> if (ret < 0) >> return -errno; >> >> @@ -101,7 +101,7 @@ int v4l2_subdev_set_format(struct media_entity *entity, >> fmt.which = which; >> fmt.format = *format; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FMT, &fmt); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FMT, &fmt); >> if (ret < 0) >> return -errno; >> >> @@ -128,7 +128,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, >> u.sel.target = target; >> u.sel.which = which; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); >> if (ret >= 0) { >> *rect = u.sel.r; >> return 0; >> @@ -140,7 +140,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, >> u.crop.pad = pad; >> u.crop.which = which; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); >> if (ret < 0) >> return -errno; >> >> @@ -168,7 +168,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, >> u.sel.which = which; >> u.sel.r = *rect; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); >> if (ret >= 0) { >> *rect = u.sel.r; >> return 0; >> @@ -181,7 +181,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, >> u.crop.which = which; >> u.crop.rect = *rect; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); >> if (ret < 0) >> return -errno; >> >> @@ -202,7 +202,7 @@ int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity, >> memset(caps, 0, sizeof(*caps)); >> caps->pad = pad; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); >> if (ret < 0) >> return -errno; >> >> @@ -220,7 +220,7 @@ int v4l2_subdev_query_dv_timings(struct media_entity *entity, >> >> memset(timings, 0, sizeof(*timings)); >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); >> if (ret < 0) >> return -errno; >> >> @@ -238,7 +238,7 @@ int v4l2_subdev_get_dv_timings(struct media_entity *entity, >> >> memset(timings, 0, sizeof(*timings)); >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); >> if (ret < 0) >> return -errno; >> >> @@ -254,7 +254,7 @@ int v4l2_subdev_set_dv_timings(struct media_entity *entity, >> if (ret < 0) >> return ret; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); >> if (ret < 0) >> return -errno; >> >> @@ -273,7 +273,7 @@ int v4l2_subdev_get_frame_interval(struct media_entity *entity, >> >> memset(&ival, 0, sizeof(ival)); >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); >> if (ret < 0) >> return -errno; >> >> @@ -294,7 +294,7 @@ int v4l2_subdev_set_frame_interval(struct media_entity *entity, >> memset(&ival, 0, sizeof(ival)); >> ival.interval = *interval; >> >> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); >> + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); >> if (ret < 0) >> return -errno; >> >> diff --git a/utils/media-ctl/mediactl-priv.h b/utils/media-ctl/mediactl-priv.h >> index a0d3a55..4bcb1e0 100644 >> --- a/utils/media-ctl/mediactl-priv.h >> +++ b/utils/media-ctl/mediactl-priv.h >> @@ -34,7 +34,12 @@ struct media_entity { >> unsigned int max_links; >> unsigned int num_links; >> >> + struct v4l2_subdev *sd; >> + >> char devname[32]; >> +}; >> + >> +struct v4l2_subdev { >> int fd; > > struct v4l2_subdev should be defined in v4l2subdev.h. > >> }; Right. >> diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h >> index 77ac182..b8cefe8 100644 >> --- a/utils/media-ctl/mediactl.h >> +++ b/utils/media-ctl/mediactl.h >> @@ -420,4 +420,26 @@ int media_parse_setup_link(struct media_device *media, >> */ >> int media_parse_setup_links(struct media_device *media, const char *p); >> >> +/** >> + * @brief Get file descriptor of the entity sub-device >> + * @param entity - media entity >> + * >> + * This function gets the file descriptor of the opened >> + * sub-device node related to the entity. >> + * >> + * @return file descriptor of the opened sub-device, >> + or -1 if the sub-device is closed >> + */ >> +int media_entity_get_fd(struct media_entity *entity); >> + >> +/** >> + * @brief Set file descriptor of the entity sub-device >> + * @param entity - media entity >> + * @param fd - entity sub-device file descriptor >> + * >> + * This function sets the file descriptor of the opened >> + * sub-device node related to the entity. >> + */ >> +void media_entity_set_fd(struct media_entity *entity, int fd); >> + >> #endif > Best Regards, Jacek Anaszewski [1] http://www.spinics.net/lists/linux-media/msg82219.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Tue, Nov 25, 2014 at 01:22:50PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > On 11/25/2014 12:36 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thank you for the updated patchset. > > > >On Fri, Nov 21, 2014 at 05:14:30PM +0100, Jacek Anaszewski wrote: > >>Add struct v4l2_subdev as a representation of the v4l2 sub-device > >>related to a media entity. Add sd property, the pointer to > >>the newly introduced structure, to the struct media_entity > >>and move fd property to it. > >> > >>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > >>Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > >>--- > >> utils/media-ctl/libmediactl.c | 30 +++++++++++++++++++++++++----- > >> utils/media-ctl/libv4l2subdev.c | 34 +++++++++++++++++----------------- > >> utils/media-ctl/mediactl-priv.h | 5 +++++ > >> utils/media-ctl/mediactl.h | 22 ++++++++++++++++++++++ > >> 4 files changed, 69 insertions(+), 22 deletions(-) > >> > >>diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c > >>index ec360bd..53921f5 100644 > >>--- a/utils/media-ctl/libmediactl.c > >>+++ b/utils/media-ctl/libmediactl.c > >>@@ -511,7 +511,6 @@ static int media_enum_entities(struct media_device *media) > >> > >> entity = &media->entities[media->entities_count]; > >> memset(entity, 0, sizeof(*entity)); > >>- entity->fd = -1; > > > >I think I'd definitely leave the fd to the media_entity itself. Not all the > >entities are sub-devices, even right now. > > I am aware of it, I even came across this issue while implementing the > function v4l2_subdev_apply_pipeline_fmt. I added suitable comment > explaining why the entity not being a sub-device has its representation. > > I moved the fd out of media_entity by following Laurent's message [1], > where he mentioned this, however I think that it would be indeed > best if it remained intact. I read Laurent's reply again, and I can see why he suggested that. I wouldn't mind, but then we should avoid touching it from libmediactl, and only access it from libv4l2subdev. > >> entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; > >> entity->media = media; > >> > >>@@ -529,11 +528,13 @@ static int media_enum_entities(struct media_device *media) > >> > >> entity->pads = malloc(entity->info.pads * sizeof(*entity->pads)); > >> entity->links = malloc(entity->max_links * sizeof(*entity->links)); > >>- if (entity->pads == NULL || entity->links == NULL) { > >>+ entity->sd = calloc(1, sizeof(*entity->sd)); > >>+ if (entity->pads == NULL || entity->links == NULL || entity->sd == NULL) { > >> ret = -ENOMEM; > >> break; > >> } > >> > >>+ entity->sd->fd = -1; > >> media->entities_count++; > >> > >> if (entity->info.flags & MEDIA_ENT_FL_DEFAULT) { > >>@@ -704,8 +705,9 @@ void media_device_unref(struct media_device *media) > >> > >> free(entity->pads); > >> free(entity->links); > >>- if (entity->fd != -1) > >>- close(entity->fd); > >>+ if (entity->sd->fd != -1) > >>+ close(entity->sd->fd); > >>+ free(entity->sd); > >> } > >> > >> free(media->entities); > >>@@ -726,13 +728,17 @@ int media_device_add_entity(struct media_device *media, > >> if (entity == NULL) > >> return -ENOMEM; > >> > >>+ entity->sd = calloc(1, sizeof(*entity->sd)); > >>+ if (entity->sd == NULL) > >>+ return -ENOMEM; > >>+ > >> media->entities = entity; > >> media->entities_count++; > >> > >> entity = &media->entities[media->entities_count - 1]; > >> memset(entity, 0, sizeof *entity); > >> > >>- entity->fd = -1; > >>+ entity->sd->fd = -1; > >> entity->media = media; > >> strncpy(entity->devname, devnode, sizeof entity->devname); > >> entity->devname[sizeof entity->devname - 1] = '\0'; > >>@@ -955,3 +961,17 @@ int media_parse_setup_links(struct media_device *media, const char *p) > >> > >> return *end ? -EINVAL : 0; > >> } > >>+ > >>+/* ----------------------------------------------------------------------------- > >>+ * Media entity access > >>+ */ > >>+ > >>+int media_entity_get_fd(struct media_entity *entity) > >>+{ > >>+ return entity->sd->fd; > >>+} > >>+ > >>+void media_entity_set_fd(struct media_entity *entity, int fd) > >>+{ > >>+ entity->sd->fd = fd; > >>+} > > > >You access the fd directly now inside the library. I don't think there > >should be a need to set it. > > struct media_entity is defined in mediactl-priv.h, whose name implies > that it shouldn't be made public. Thats way I implemented the setter. > I use it in the libv4l-exynos4-camera.c. Ah, I now understand why you wnat to do this. You should also close the file handle --- this is used internally by the library, and simply setting the value will lead the loss of the existing handle.
diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c index ec360bd..53921f5 100644 --- a/utils/media-ctl/libmediactl.c +++ b/utils/media-ctl/libmediactl.c @@ -511,7 +511,6 @@ static int media_enum_entities(struct media_device *media) entity = &media->entities[media->entities_count]; memset(entity, 0, sizeof(*entity)); - entity->fd = -1; entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT; entity->media = media; @@ -529,11 +528,13 @@ static int media_enum_entities(struct media_device *media) entity->pads = malloc(entity->info.pads * sizeof(*entity->pads)); entity->links = malloc(entity->max_links * sizeof(*entity->links)); - if (entity->pads == NULL || entity->links == NULL) { + entity->sd = calloc(1, sizeof(*entity->sd)); + if (entity->pads == NULL || entity->links == NULL || entity->sd == NULL) { ret = -ENOMEM; break; } + entity->sd->fd = -1; media->entities_count++; if (entity->info.flags & MEDIA_ENT_FL_DEFAULT) { @@ -704,8 +705,9 @@ void media_device_unref(struct media_device *media) free(entity->pads); free(entity->links); - if (entity->fd != -1) - close(entity->fd); + if (entity->sd->fd != -1) + close(entity->sd->fd); + free(entity->sd); } free(media->entities); @@ -726,13 +728,17 @@ int media_device_add_entity(struct media_device *media, if (entity == NULL) return -ENOMEM; + entity->sd = calloc(1, sizeof(*entity->sd)); + if (entity->sd == NULL) + return -ENOMEM; + media->entities = entity; media->entities_count++; entity = &media->entities[media->entities_count - 1]; memset(entity, 0, sizeof *entity); - entity->fd = -1; + entity->sd->fd = -1; entity->media = media; strncpy(entity->devname, devnode, sizeof entity->devname); entity->devname[sizeof entity->devname - 1] = '\0'; @@ -955,3 +961,17 @@ int media_parse_setup_links(struct media_device *media, const char *p) return *end ? -EINVAL : 0; } + +/* ----------------------------------------------------------------------------- + * Media entity access + */ + +int media_entity_get_fd(struct media_entity *entity) +{ + return entity->sd->fd; +} + +void media_entity_set_fd(struct media_entity *entity, int fd) +{ + entity->sd->fd = fd; +} diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c index 8015330..09e0081 100644 --- a/utils/media-ctl/libv4l2subdev.c +++ b/utils/media-ctl/libv4l2subdev.c @@ -41,11 +41,11 @@ int v4l2_subdev_open(struct media_entity *entity) { - if (entity->fd != -1) + if (entity->sd->fd != -1) return 0; - entity->fd = open(entity->devname, O_RDWR); - if (entity->fd == -1) { + entity->sd->fd = open(entity->devname, O_RDWR); + if (entity->sd->fd == -1) { int ret = -errno; media_dbg(entity->media, "%s: Failed to open subdev device node %s\n", __func__, @@ -58,8 +58,8 @@ int v4l2_subdev_open(struct media_entity *entity) void v4l2_subdev_close(struct media_entity *entity) { - close(entity->fd); - entity->fd = -1; + close(entity->sd->fd); + entity->sd->fd = -1; } int v4l2_subdev_get_format(struct media_entity *entity, @@ -77,7 +77,7 @@ int v4l2_subdev_get_format(struct media_entity *entity, fmt.pad = pad; fmt.which = which; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FMT, &fmt); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FMT, &fmt); if (ret < 0) return -errno; @@ -101,7 +101,7 @@ int v4l2_subdev_set_format(struct media_entity *entity, fmt.which = which; fmt.format = *format; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FMT, &fmt); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FMT, &fmt); if (ret < 0) return -errno; @@ -128,7 +128,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, u.sel.target = target; u.sel.which = which; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel); if (ret >= 0) { *rect = u.sel.r; return 0; @@ -140,7 +140,7 @@ int v4l2_subdev_get_selection(struct media_entity *entity, u.crop.pad = pad; u.crop.which = which; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_CROP, &u.crop); if (ret < 0) return -errno; @@ -168,7 +168,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, u.sel.which = which; u.sel.r = *rect; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel); if (ret >= 0) { *rect = u.sel.r; return 0; @@ -181,7 +181,7 @@ int v4l2_subdev_set_selection(struct media_entity *entity, u.crop.which = which; u.crop.rect = *rect; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_CROP, &u.crop); if (ret < 0) return -errno; @@ -202,7 +202,7 @@ int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity, memset(caps, 0, sizeof(*caps)); caps->pad = pad; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps); if (ret < 0) return -errno; @@ -220,7 +220,7 @@ int v4l2_subdev_query_dv_timings(struct media_entity *entity, memset(timings, 0, sizeof(*timings)); - ret = ioctl(entity->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings); if (ret < 0) return -errno; @@ -238,7 +238,7 @@ int v4l2_subdev_get_dv_timings(struct media_entity *entity, memset(timings, 0, sizeof(*timings)); - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings); if (ret < 0) return -errno; @@ -254,7 +254,7 @@ int v4l2_subdev_set_dv_timings(struct media_entity *entity, if (ret < 0) return ret; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings); if (ret < 0) return -errno; @@ -273,7 +273,7 @@ int v4l2_subdev_get_frame_interval(struct media_entity *entity, memset(&ival, 0, sizeof(ival)); - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_G_FRAME_INTERVAL, &ival); if (ret < 0) return -errno; @@ -294,7 +294,7 @@ int v4l2_subdev_set_frame_interval(struct media_entity *entity, memset(&ival, 0, sizeof(ival)); ival.interval = *interval; - ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); + ret = ioctl(entity->sd->fd, VIDIOC_SUBDEV_S_FRAME_INTERVAL, &ival); if (ret < 0) return -errno; diff --git a/utils/media-ctl/mediactl-priv.h b/utils/media-ctl/mediactl-priv.h index a0d3a55..4bcb1e0 100644 --- a/utils/media-ctl/mediactl-priv.h +++ b/utils/media-ctl/mediactl-priv.h @@ -34,7 +34,12 @@ struct media_entity { unsigned int max_links; unsigned int num_links; + struct v4l2_subdev *sd; + char devname[32]; +}; + +struct v4l2_subdev { int fd; }; diff --git a/utils/media-ctl/mediactl.h b/utils/media-ctl/mediactl.h index 77ac182..b8cefe8 100644 --- a/utils/media-ctl/mediactl.h +++ b/utils/media-ctl/mediactl.h @@ -420,4 +420,26 @@ int media_parse_setup_link(struct media_device *media, */ int media_parse_setup_links(struct media_device *media, const char *p); +/** + * @brief Get file descriptor of the entity sub-device + * @param entity - media entity + * + * This function gets the file descriptor of the opened + * sub-device node related to the entity. + * + * @return file descriptor of the opened sub-device, + or -1 if the sub-device is closed + */ +int media_entity_get_fd(struct media_entity *entity); + +/** + * @brief Set file descriptor of the entity sub-device + * @param entity - media entity + * @param fd - entity sub-device file descriptor + * + * This function sets the file descriptor of the opened + * sub-device node related to the entity. + */ +void media_entity_set_fd(struct media_entity *entity, int fd); + #endif