Re: [RFC PATCH] v4l2-dev: split video_register_device into v4l2_vdev_init & _register

Sowjanya Komatineni <skomatineni@xxxxxxxxxx> · Tue, 7 Jul 2020 11:37:20 -0700

On 7/7/20 8:23 AM, Hans Verkuil wrote:
Especially for more complex devices you often want to postpone creating
device nodes until everything has been initialized.

However, video_register_device both initializes and registers the video
device, and it is not possible to do this in two stages.

This patch creates thee new functions: v4l2_vdev_init(), v4l2_vdev_uninit()
and v4l2_vdev_register().

The existing __video_register_device() function now just calls those two
new functions.

Drivers can replace video_register_device() by this two-stage call sequence,
and so postpone the actual device node registration until the very end.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
---
This is just a proof of concept, in part because of problems that Sowjanya
experiences with the tegra-video driver. I'm not at all sure about the
naming convention, I just know that I would like to move to a proper
v4l2_<foo>_init/uninit/register/unregister naming, as is done everywhere else.

See: https://lore.kernel.org/patchwork/patch/1257372/ for the tegra discussion.

Sowjanya, can you test this patch?

Regards,

	Hans

Hi Hans,

With this patch, updated on Tegra video driver side to do 
v4l2_vdev_init() and then create media links along with ctrl handler 
setup by retrieving subdev through media remote pads and in the last did 
v4l2_vdev_register(). This works fine.

Should I wait for this split patch to be merged before I use these new 
API's?

Thanks

Sowjanya

---

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index a593ea0598b5..a81ea5b01f12 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -873,10 +873,9 @@ static int video_register_media_controller(struct video_device *vdev)
  	return 0;
  }

-int __video_register_device(struct video_device *vdev,
-			    enum vfl_devnode_type type,
-			    int nr, int warn_if_nr_in_use,
-			    struct module *owner)
+int v4l2_vdev_init(struct video_device *vdev,
+		   enum vfl_devnode_type type,
+		   int nr, int warn_if_nr_in_use)
  {
  	int i = 0;
  	int ret;
@@ -1009,7 +1008,68 @@ int __video_register_device(struct video_device *vdev,
  	if (vdev->ioctl_ops)
  		determine_valid_ioctls(vdev);

-	/* Part 3: Initialize the character device */
+	/* Part 3: initialize vdev->dev */
+	vdev->dev.class = &video_class;
+	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
+	vdev->dev.parent = vdev->dev_parent;
+	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
+	/* Register the release callback that will be called when the last
+	   reference to the device goes away. */
+	vdev->dev.release = v4l2_device_release;
+
+	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
+		pr_warn("%s: requested %s%d, got %s\n", __func__,
+			name_base, nr, video_device_node_name(vdev));
+
+	/* Part 4: Register the entity. */
+	ret = video_register_media_controller(vdev);
+	if (ret)
+		goto cleanup;
+
+	/* Part 5: mark as initialized. */
+	set_bit(V4L2_FL_INITIALIZED, &vdev->flags);
+
+	return 0;
+
+cleanup:
+	mutex_lock(&videodev_lock);
+	video_devices[vdev->minor] = NULL;
+	devnode_clear(vdev);
+	mutex_unlock(&videodev_lock);
+	/* Mark this video device as never having been registered. */
+	vdev->minor = -1;
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_vdev_init);
+
+void v4l2_vdev_uninit(struct video_device *vdev)
+{
+	mutex_lock(&videodev_lock);
+	clear_bit(V4L2_FL_INITIALIZED, &vdev->flags);
+	video_devices[vdev->minor] = NULL;
+	devnode_clear(vdev);
+	mutex_unlock(&videodev_lock);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (vdev->v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
+		/* Remove interfaces and interface links */
+		media_devnode_remove(vdev->intf_devnode);
+		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
+			media_device_unregister_entity(&vdev->entity);
+	}
+#endif
+	/* Mark this video device as never having been registered. */
+	vdev->minor = -1;
+}
+EXPORT_SYMBOL(v4l2_vdev_uninit);
+
+int v4l2_vdev_register(struct video_device *vdev, struct module *owner)
+{
+	int ret;
+
+	if (WARN_ON(!test_bit(V4L2_FL_INITIALIZED, &vdev->flags)))
+		return -EINVAL;
+
+	/* Part 1: Initialize the character device */
  	vdev->cdev = cdev_alloc();
  	if (vdev->cdev == NULL) {
  		ret = -ENOMEM;
@@ -1025,11 +1085,7 @@ int __video_register_device(struct video_device *vdev,
  		goto cleanup;
  	}

-	/* Part 4: register the device with sysfs */
-	vdev->dev.class = &video_class;
-	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
-	vdev->dev.parent = vdev->dev_parent;
-	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
+	/* Part 2: register the device with sysfs */
  	ret = device_register(&vdev->dev);
  	if (ret < 0) {
  		pr_err("%s: device_register failed\n", __func__);
@@ -1039,17 +1095,10 @@ int __video_register_device(struct video_device *vdev,
  	   reference to the device goes away. */
  	vdev->dev.release = v4l2_device_release;

-	if (nr != -1 && nr != vdev->num && warn_if_nr_in_use)
-		pr_warn("%s: requested %s%d, got %s\n", __func__,
-			name_base, nr, video_device_node_name(vdev));
-
  	/* Increase v4l2_device refcount */
  	v4l2_device_get(vdev->v4l2_dev);

-	/* Part 5: Register the entity. */
-	ret = video_register_media_controller(vdev);
-
-	/* Part 6: Activate this minor. The char device can now be used. */
+	/* Part 3: Activate this minor. The char device can now be used. */
  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);

  	return 0;
@@ -1058,11 +1107,24 @@ int __video_register_device(struct video_device *vdev,
  	mutex_lock(&videodev_lock);
  	if (vdev->cdev)
  		cdev_del(vdev->cdev);
-	video_devices[vdev->minor] = NULL;
-	devnode_clear(vdev);
  	mutex_unlock(&videodev_lock);
-	/* Mark this video device as never having been registered. */
-	vdev->minor = -1;
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_vdev_register);
+
+int __video_register_device(struct video_device *vdev,
+			    enum vfl_devnode_type type,
+			    int nr, int warn_if_nr_in_use,
+			    struct module *owner)
+{
+	int ret;
+
+	ret = v4l2_vdev_init(vdev, type, nr, warn_if_nr_in_use);
+	if (ret)
+		return ret;
+	ret = v4l2_vdev_register(vdev, owner);
+	if (ret)
+		v4l2_vdev_uninit(vdev);
  	return ret;
  }
  EXPORT_SYMBOL(__video_register_device);
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index ad2d41952442..0a8e22e92e6a 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -66,6 +66,9 @@ struct v4l2_ctrl_handler;
  /**
   * enum v4l2_video_device_flags - Flags used by &struct video_device
   *
+ * @V4L2_FL_INITIALIZED:
+ *	indicates that a &struct video_device is initialized.
+ *	It is cleared by video_unregister_device.
   * @V4L2_FL_REGISTERED:
   *	indicates that a &struct video_device is registered.
   *	Drivers can clear this flag if they want to block all future
@@ -90,10 +93,11 @@ struct v4l2_ctrl_handler;
   *	handler to restrict access to some ioctl calls.
   */
  enum v4l2_video_device_flags {
-	V4L2_FL_REGISTERED		= 0,
-	V4L2_FL_USES_V4L2_FH		= 1,
-	V4L2_FL_QUIRK_INVERTED_CROP	= 2,
-	V4L2_FL_SUBDEV_RO_DEVNODE	= 3,
+	V4L2_FL_INITIALIZED,
+	V4L2_FL_REGISTERED,
+	V4L2_FL_USES_V4L2_FH,
+	V4L2_FL_QUIRK_INVERTED_CROP,
+	V4L2_FL_SUBDEV_RO_DEVNODE,
  };

  /* Priority helper functions */
@@ -326,6 +330,13 @@ struct video_device
   */
  #define to_video_device(cd) container_of(cd, struct video_device, dev)

+int v4l2_vdev_init(struct video_device *vdev,
+		   enum vfl_devnode_type type,
+		   int nr, int warn_if_nr_in_use);
+void v4l2_vdev_uninit(struct video_device *vdev);
+
+int v4l2_vdev_register(struct video_device *vdev, struct module *owner);
+
  /**
   * __video_register_device - register video4linux devices
   *







