[RFC] v4l2-dev: split video_register_device into v4l2_vdev_init & _register
Commit Message
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@xs4all.nl>
---
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
---
Comments
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@xs4all.nl>
> ---
> 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
> *
>
@@ -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);
@@ -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
*