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.anaszew...@samsung.com> Acked-by: Kyungmin Park <kyungmin.p...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
