Re: [PATCH 01/15] mediactl: Introduce v4l2_subdev structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Sakari,

Thanks for the review.

On 02/12/2016 01:42 PM, Sakari Ailus wrote:

Hi Jacek,

Thanks for continuing this work! And my apologies for reviewing only
now... please see the comments below.

Jacek Anaszewski wrote:

Add struct v4l2_subdev - a representation of the v4l2 sub-device,
related to the media entity. Add field 'sd', the pointer to
the newly introduced structure, to the struct media_entity
and move 'fd' property from struct media entity to struct v4l2_subdev.
Avoid accessing sub-device file descriptor from libmediactl and
make the v4l2_subdev_open capable of creating the v4l2_subdev
if the 'sd' pointer is uninitialized.

Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
  utils/media-ctl/libmediactl.c   |    4 --
  utils/media-ctl/libv4l2subdev.c |   82 +++++++++++++++++++++++++++++++--------
  utils/media-ctl/mediactl-priv.h |    5 ++-
  utils/media-ctl/v4l2subdev.h    |   38 ++++++++++++++++++
  4 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c
index 4a82d24..7e98440 100644
--- a/utils/media-ctl/libmediactl.c
+++ b/utils/media-ctl/libmediactl.c
@@ -525,7 +525,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;

@@ -719,8 +718,6 @@ void media_device_unref(struct media_device *media)

  		free(entity->pads);
  		free(entity->links);
-		if (entity->fd != -1)
-			close(entity->fd);
  	}

  	free(media->entities);
@@ -747,7 +744,6 @@ int media_device_add_entity(struct media_device *media,
  	entity = &media->entities[media->entities_count - 1];
  	memset(entity, 0, sizeof *entity);

-	entity->fd = -1;
  	entity->media = media;
  	strncpy(entity->devname, devnode, sizeof entity->devname);
  	entity->devname[sizeof entity->devname - 1] = '\0';
diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 33c1ee6..3977ce5 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -39,13 +39,61 @@
  #include "tools.h"
  #include "v4l2subdev.h"

+int v4l2_subdev_create(struct media_entity *entity)
+{
+	if (entity->sd)
+		return 0;
+
+	entity->sd = calloc(1, sizeof(*entity->sd));
+	if (entity->sd == NULL)
+		return -ENOMEM;
+
+	entity->sd->fd = -1;
+
+	return 0;
+}
+
+int v4l2_subdev_create_with_fd(struct media_entity *entity, int fd)
+{
+	int ret;
+
+	if (entity->sd)
+		return -EEXIST;
+
+	ret = v4l2_subdev_create(entity);
+	if (ret < 0)
+		return ret;
+
+	entity->sd->fd = fd;
+
+	return 0;
+}
+
+void v4l2_subdev_release(struct media_entity *entity, bool close_fd)
+{
+	if (entity->sd == NULL)
+		return;
+
+	if (close_fd)
+		v4l2_subdev_close(entity);
+
+	free(entity->sd->v4l2_control_redir);
+	free(entity->sd);
+}
+
  int v4l2_subdev_open(struct media_entity *entity)
  {
-	if (entity->fd != -1)
+	int ret;
+
+	ret = v4l2_subdev_create(entity);


The current users of v4l2_subdev_open() in libv4l2subdev do not
explicitly close the sub-devices they open; thus calling
v4l2_subdev_create() here creates a memory leak.


Currently in my use cases there is no memory leak since I assumed
that the one who instantiates struct media_device should take
care of releasing it properly. I added v4l2_subdev_open_pipeline()
and v4l2_subdev_release_pipeline() API that is called on plugin
init and close respectively.

Probably it would be good to remove v4l2_subdev_open from
v4l2_subdev_* prefixed API and return error if sd property
of passed struct media_entity is not initialized.


I wonder if it'd do harm to open all the associated devices in
media_device_open() and close them in media_device_close().


Opening all sub-devices within a media device would be waste
of resources since we only need the sub-devices that belong
to the pipeline connected to the opened video device. It would
also incur unnecessary additional power consumption as you
mentioned below.


The sub-device objects could exist for the entire lifespan of the media
device object (in user space), and they could be used to store whatever
information is needed.

One would no longer need to call v4l2_subdev_open() directly either.

I'd like to have Laurent's opinion on this, too.

The power management currently is based on open file handles and this is
a bit of a problem, but we have to have a better solution on this (based
on latencies and perhaps PM QoS framework).


+	if (ret < 0)
+		return ret;
+
+	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 +106,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 +125,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 +149,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 +176,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 +188,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 +216,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 +229,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 +250,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 +268,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 +286,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 +302,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 +321,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 +342,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..f531c52 100644
--- a/utils/media-ctl/mediactl-priv.h
+++ b/utils/media-ctl/mediactl-priv.h
@@ -26,6 +26,8 @@

  #include "mediactl.h"

+struct v4l2_subdev;
+
  struct media_entity {
  	struct media_device *media;
  	struct media_entity_desc info;
@@ -34,8 +36,9 @@ struct media_entity {
  	unsigned int max_links;
  	unsigned int num_links;

+	struct v4l2_subdev *sd;
+
  	char devname[32];
-	int fd;
  };

  struct media_device {
diff --git a/utils/media-ctl/v4l2subdev.h b/utils/media-ctl/v4l2subdev.h
index 104e420..ba9b8c4 100644
--- a/utils/media-ctl/v4l2subdev.h
+++ b/utils/media-ctl/v4l2subdev.h
@@ -27,6 +27,44 @@
  struct media_device;
  struct media_entity;

+struct v4l2_subdev {
+	int fd;
+};
+
+/**
+ * @brief Create a v4l2-subdev
+ * @param entity - sub-device media entity.
+ *
+ * Create the representation of the entity sub-device.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_create(struct media_entity *entity);
+
+/**
+ * @brief Create a representation of the already opened v4l2-subdev
+ * @param entity - sub-device media entity.
+ * @param fd - sub-device file descriptor.
+ *
+ * Create the representation of the sub-device that had been opened
+ * before the parent media device was created, and associate it
+ * with the media entity.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_create_with_fd(struct media_entity *entity, int fd);
+
+/**
+ * @brief Release a v4l2-subdev
+ * @param entity - sub-device media entity.
+ * @param close_fd - indicates whether subdev fd should be closed.
+ *
+ * Release the representation of the entity sub-device.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+void v4l2_subdev_release(struct media_entity *entity, bool close_fd);
+


Is there a need to call these outside the library itself? Should they be
static instead?


  /**
   * @brief Open a sub-device.
   * @param entity - sub-device media entity.






--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux