Re: [PATCH v10 1/4] media: Media Device Allocator API
From: shuah
Date: Fri Jan 25 2019 - 19:27:29 EST
Hi Sakari,
On 1/25/19 8:38 AM, Sakari Ailus wrote:
Hi Shuah,
On Thu, Jan 24, 2019 at 01:32:38PM -0700, Shuah Khan wrote:
Media Device Allocator API to allows multiple drivers share a media device.
This API solves a very common use-case for media devices where one physical
device (an USB stick) provides both audio and video. When such media device
exposes a standard USB Audio class, a proprietary Video class, two or more
independent drivers will share a single physical USB bridge. In such cases,
it is necessary to coordinate access to the shared resource.
Using this API, drivers can allocate a media device with the shared struct
device as the key. Once the media device is allocated by a driver, other
drivers can get a reference to it. The media device is released when all
the references are released.
Are there real, non-USB devices that could use the same API?
There might be. I don't have any to test. This patch is restricted
to USB at the moment.
Signed-off-by: Shuah Khan <shuah@xxxxxxxxxx>
---
Documentation/media/kapi/mc-core.rst | 41 ++++++++
drivers/media/Makefile | 4 +
drivers/media/media-dev-allocator.c | 144 +++++++++++++++++++++++++++
include/media/media-dev-allocator.h | 53 ++++++++++
4 files changed, 242 insertions(+)
create mode 100644 drivers/media/media-dev-allocator.c
create mode 100644 include/media/media-dev-allocator.h
diff --git a/Documentation/media/kapi/mc-core.rst b/Documentation/media/kapi/mc-core.rst
index 0bcfeadbc52d..07f2a6a90af2 100644
--- a/Documentation/media/kapi/mc-core.rst
+++ b/Documentation/media/kapi/mc-core.rst
@@ -259,6 +259,45 @@ Subsystems should facilitate link validation by providing subsystem specific
helper functions to provide easy access for commonly needed information, and
in the end provide a way to use driver-specific callbacks.
+Media Controller Device Allocator API
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When the media device belongs to more than one driver, the shared media
+device is allocated with the shared struct device as the key for look ups.
+
+The shared media device should stay in registered state until the last
+driver unregisters it. In addition, the media device should be released when
+all the references are released. Each driver gets a reference to the media
+device during probe, when it allocates the media device. If media device is
+already allocated, the allocate API bumps up the refcount and returns the
+existing media device. The driver puts the reference back in its disconnect
+routine when it calls :c:func:`media_device_delete()`.
+
+The media device is unregistered and cleaned up from the kref put handler to
+ensure that the media device stays in registered state until the last driver
+unregisters the media device.
+
+**Driver Usage**
+
+Drivers should use the appropriate media-core routines to manage the shared
+media device life-time handling the two states:
+1. allocate -> register -> delete
+2. get reference to already registered device -> delete
+
+call :c:func:`media_device_delete()` routine to make sure the shared media
+device delete is handled correctly.
+
+**driver probe:**
+Call :c:func:`media_device_usb_allocate()` to allocate or get a reference
+Call :c:func:`media_device_register()`, if media devnode isn't registered
+
+**driver disconnect:**
+Call :c:func:`media_device_delete()` to free the media_device. Freeing is
+handled by the kref put handler.
+
+API Definitions
+^^^^^^^^^^^^^^^
+
.. kernel-doc:: include/media/media-device.h
.. kernel-doc:: include/media/media-devnode.h
@@ -266,3 +305,5 @@ in the end provide a way to use driver-specific callbacks.
.. kernel-doc:: include/media/media-entity.h
.. kernel-doc:: include/media/media-request.h
+
+.. kernel-doc:: include/media/media-dev-allocator.h
diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 985d35ec6b29..1d7653318af6 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -6,6 +6,10 @@
media-objs := media-device.o media-devnode.o media-entity.o \
media-request.o
+ifeq ($(CONFIG_USB),y)
+ media-objs += media-dev-allocator.o
+endif
+
#
# I2C drivers should come before other drivers, otherwise they'll fail
# when compiled as builtin drivers
diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
new file mode 100644
index 000000000000..4606456c1e86
--- /dev/null
+++ b/drivers/media/media-dev-allocator.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
GPL-2.0+
I would like to address this in a follow-on patch instead of
re-doing the series again. Hope that is okay.
+/*
+ * media-dev-allocator.c - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2018 Shuah Khan <shuah@xxxxxxxxxx>
+ *
+ * Credits: Suggested by Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
+ */
+
+/*
+ * This file adds a global refcounted Media Controller Device Instance API.
+ * A system wide global media device list is managed and each media device
+ * includes a kref count. The last put on the media device releases the media
+ * device instance.
+ *
+ */
+
+#include <linux/kref.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+#include <media/media-device.h>
+
+static LIST_HEAD(media_device_list);
+static DEFINE_MUTEX(media_device_lock);
+
+struct media_device_instance {
+ struct media_device mdev;
+ struct module *owner;
+ struct list_head list;
+ struct kref refcount;
+};
+
+static inline struct media_device_instance *
+to_media_device_instance(struct media_device *mdev)
+{
+ return container_of(mdev, struct media_device_instance, mdev);
+}
+
+static void media_device_instance_release(struct kref *kref)
+{
+ struct media_device_instance *mdi =
+ container_of(kref, struct media_device_instance, refcount);
+
+ dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
+
+ mutex_lock(&media_device_lock);
+
+ media_device_unregister(&mdi->mdev);
+ media_device_cleanup(&mdi->mdev);
This is a problem, albeit not really more of a problem than it is in a
driver.
Okay good to know that it can be addressed in your media device
refcounting series.
The refcounting changes can be made here instead. I'll take this
into account in the media device refcounting series I'm planning to start
working on again; would you be perhaps able to help testing with this
device once I have patches in that shape? I have no access to the hardware.
Absolutely. I will be happy to help you with testing on the same
hardware, I used for this series.
+
+ list_del(&mdi->list);
+ mutex_unlock(&media_device_lock);
+
+ kfree(mdi);
+}
+
+/* Callers should hold media_device_lock when calling this function */
+static struct media_device *__media_device_get(struct device *dev,
+ const char *module_name,
+ struct module *modp)
+{
+ struct media_device_instance *mdi;
+
+ list_for_each_entry(mdi, &media_device_list, list) {
+
+ if (mdi->mdev.dev != dev)
+ continue;
+
+ kref_get(&mdi->refcount);
+
+ /* get module reference for the media_device owner */
+ if (modp != mdi->owner && !try_module_get(mdi->owner))
+ dev_err(dev, "%s: try_module_get() error\n", __func__);
+ dev_dbg(dev, "%s: get mdev=%p module_name %s\n",
+ __func__, &mdi->mdev, module_name);
+ return &mdi->mdev;
+ }
+
+ mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
+ if (!mdi)
+ return NULL;
+
+ mdi->owner = modp;
+ kref_init(&mdi->refcount);
+ list_add_tail(&mdi->list, &media_device_list);
+
+ dev_dbg(dev, "%s: alloc mdev=%p module_name %s\n", __func__,
+ &mdi->mdev, module_name);
+ return &mdi->mdev;
+}
+
+#if IS_ENABLED(CONFIG_USB)
You already compile the file only if CONFIG_USB is enabled. I think you
could remove this.
+struct media_device *media_device_usb_allocate(struct usb_device *udev,
+ const char *module_name)
I'd like to suggest working based on usb_interface instead of usb_device
here: that object already exists and you can find out the device based on
it. It seems all callers of this function already have the usb_interface
around.
Is there an advantage to using interface instead of the parent device?
Is there a problem doing it this way?
+{
+ struct media_device *mdev;
+ struct module *modptr;
+
+ mutex_lock(&module_mutex);
+ modptr = find_module(module_name);
+ mutex_unlock(&module_mutex);
+
+ mutex_lock(&media_device_lock);
+ mdev = __media_device_get(&udev->dev, module_name, modptr);
+ if (!mdev) {
+ mutex_unlock(&media_device_lock);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* check if media device is already initialized */
+ if (!mdev->dev)
+ __media_device_usb_init(mdev, udev, udev->product,
+ module_name);
+ mutex_unlock(&media_device_lock);
+ return mdev;
+}
+EXPORT_SYMBOL_GPL(media_device_usb_allocate);
+#endif
+
+void media_device_delete(struct media_device *mdev, const char *module_name)
Same here. The use of the module name seems a bit hackish to me, albeit I
suppose it'd work, too.
What is hackish about it? I found it very useful to use it in debug and
error messages that are user-friendly.
thanks,
-- Shuah