Discussion:
[PATCH v2 0/6] media token resource framework
Shuah Khan
2014-10-14 14:58:36 UTC
Permalink
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.

This patch series consists of media token resource framework
and changes to use it in dvb-core, v4l2-core, au0828 driver,
and snd-usb-audio driver.

With these changes dvb and v4l2 can share the tuner without
disrupting each other. Used tvtime, xawtv, kaffeine, and vlc,
vlc audio capture option, arecord/aplay during development to
identify v4l2 vb2 and vb1 ioctls and file operations that
disrupt the digital stream and would require changes to check
tuner ownership prior to changing the tuner configuration.
vb2 changes are made in the v4l2-core and vb1 changes are made
in the au0828 driver to encourage porting drivers to vb2 to
advantage of the new media token resource framework with changes
in the core.

In this patch v2 series, fixed problems identified in the
patch v1 series. Important ones are changing snd-usb-audio
to use media tokens, holding tuner lock in VIDIOC_ENUMINPUT,
and VIDIOC_QUERYSTD.

Shuah Khan (6):
media: add media token device resource framework
media: v4l2-core changes to use media token api
media: au0828-video changes to use media token api
media: dvb-core changes to use media token api
sound/usb: pcm changes to use media token api
media: au0828-core changes to create and destroy media

MAINTAINERS | 2 +
drivers/media/dvb-core/dvb_frontend.c | 14 +-
drivers/media/usb/au0828/au0828-core.c | 23 +++
drivers/media/usb/au0828/au0828-video.c | 42 +++++-
drivers/media/v4l2-core/v4l2-fh.c | 7 +
drivers/media/v4l2-core/v4l2-ioctl.c | 61 ++++++++
include/linux/media_tknres.h | 50 +++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 +++++++++++++++++++++++++++++++
sound/usb/pcm.c | 9 ++
10 files changed, 445 insertions(+), 2 deletions(-)
create mode 100644 include/linux/media_tknres.h
create mode 100644 lib/media_tknres.c
--
1.7.10.4
Shuah Khan
2014-10-14 14:58:39 UTC
Permalink
au0828-video driver uses vb1 api and needs changes to vb1
v4l2 interfaces that change the tuner status. In addition
to that this driver initializes the tuner from a some ioctls
that are query (read) tuner status. These ioctls are changed
to hold the tuner and audio tokens to avoid disrupting digital
stream if active. Further more, release v4l2_file_operations
powers down the tuner. The following changes are made:

read, poll v4l2_file_operations:
- hold tuner and audio tokens
- return leaving tuner and audio tokens locked

vb1 streamon:
- hold tuner and audio tokens
- return leaving tuner and audio tokens locked

release v4l2_file_operations:
- hold tuner and audio tokens before power down
Don't call s_power when tuner is busy

Note that media_get_tuner_tkn() will do a get on audio token
and return with both tuner and audio tokens locked. When tuner
token released using media_put_tuner_tkn() , audio token is
released. Initialize dev_parent field struct video_device to
enable media tuner token lookup from v4l2-core.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
drivers/media/usb/au0828/au0828-video.c | 42 ++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
index 5f337b1..931e736 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -34,6 +34,7 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/device.h>
+#include <linux/media_tknres.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-event.h>
@@ -1085,10 +1086,21 @@ static int au0828_v4l2_close(struct file *filp)

au0828_uninit_isoc(dev);

+ ret = media_get_tuner_tkn(&dev->usbdev->dev);
+ if (ret) {
+ dev_info(&dev->usbdev->dev,
+ "%s: Tuner is busy\n", __func__);
+ /* don't touch tuner - avoid putting to sleep step */
+ goto skip_s_power;
+ }
+ dev_info(&dev->usbdev->dev, "%s: Putting tuner to sleep\n",
+ __func__);
/* Save some power by putting tuner to sleep */
v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
- dev->std_set_in_tuner_core = 0;
+ media_put_tuner_tkn(&dev->usbdev->dev);

+skip_s_power:
+ dev->std_set_in_tuner_core = 0;
/* When close the device, set the usb intf0 into alt0 to free
USB bandwidth */
ret = usb_set_interface(dev->usbdev, 0, 0);
@@ -1136,6 +1148,12 @@ static ssize_t au0828_v4l2_read(struct file *filp, char __user *buf,
if (rc < 0)
return rc;

+ /* don't put the tuner token - this case is same as STREAMON */
+ rc = media_get_tuner_tkn(&dev->usbdev->dev);
+ if (rc) {
+ dev_info(&dev->usbdev->dev, "%s: Tuner is busy\n", __func__);
+ return -EBUSY;
+ }
if (mutex_lock_interruptible(&dev->lock))
return -ERESTARTSYS;
au0828_init_tuner(dev);
@@ -1177,6 +1195,12 @@ static unsigned int au0828_v4l2_poll(struct file *filp, poll_table *wait)
if (check_dev(dev) < 0)
return POLLERR;

+ /* don't put the tuner token - this case is same as STREAMON */
+ res = media_get_tuner_tkn(&dev->usbdev->dev);
+ if (res) {
+ dev_info(&dev->usbdev->dev, "%s: Tuner is busy\n", __func__);
+ return -EBUSY;
+ }
res = v4l2_ctrl_poll(filp, wait);
if (!(req_events & (POLLIN | POLLRDNORM)))
return res;
@@ -1548,10 +1572,17 @@ static int vidioc_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t)
{
struct au0828_fh *fh = priv;
struct au0828_dev *dev = fh->dev;
+ int ret;

if (t->index != 0)
return -EINVAL;

+ ret = media_get_tuner_tkn(&dev->usbdev->dev);
+ if (ret) {
+ dev_info(&dev->usbdev->dev, "%s: Tuner is busy\n", __func__);
+ return -EBUSY;
+ }
+
strcpy(t->name, "Auvitek tuner");

au0828_init_tuner(dev);
@@ -1682,6 +1713,12 @@ static int vidioc_streamon(struct file *file, void *priv,
dprintk(1, "vidioc_streamon fh=%p t=%d fh->res=%d dev->res=%d\n",
fh, type, fh->resources, dev->resources);

+ rc = media_get_tuner_tkn(&dev->usbdev->dev);
+ if (rc) {
+ dev_info(&dev->usbdev->dev, "%s: Tuner is busy\n", __func__);
+ return -EBUSY;
+ }
+
if (unlikely(!res_get(fh, get_ressource(fh))))
return -EBUSY;

@@ -2083,12 +2120,15 @@ int au0828_analog_register(struct au0828_dev *dev,
dev->vdev->v4l2_dev = &dev->v4l2_dev;
dev->vdev->lock = &dev->lock;
strcpy(dev->vdev->name, "au0828a video");
+ /* there is no way to deduce parent from v4l2_dev */
+ dev->vdev->dev_parent = &dev->usbdev->dev;

/* Setup the VBI device */
*dev->vbi_dev = au0828_video_template;
dev->vbi_dev->v4l2_dev = &dev->v4l2_dev;
dev->vbi_dev->lock = &dev->lock;
strcpy(dev->vbi_dev->name, "au0828a vbi");
+ dev->vbi_dev->dev_parent = &dev->usbdev->dev;

/* Register the v4l2 device */
video_set_drvdata(dev->vdev, dev);
--
1.7.10.4
Shuah Khan
2014-10-14 14:58:37 UTC
Permalink
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.

A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 291 insertions(+)
create mode 100644 include/linux/media_tknres.h
create mode 100644 lib/media_tknres.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e80a275..9216179 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-*
F: include/uapi/linux/meye.h
F: include/uapi/linux/ivtv*
F: include/uapi/linux/uvcvideo.h
+F: include/linux/media_tknres.h
+F: lib/media_tknres.c

MEDIAVISION PRO MOVIE STUDIO DRIVER
M: Hans Verkuil <***@xs4all.nl>
diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
new file mode 100644
index 0000000..6d37327
--- /dev/null
+++ b/include/linux/media_tknres.h
@@ -0,0 +1,50 @@
+/*
+ * media_tknres.h - managed media token resource
+ *
+ * Copyright (c) 2014 Shuah Khan <***@osg.samsung.com>
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef __LINUX_MEDIA_TOKEN_H
+#define __LINUX_MEDIA_TOKEN_H
+
+struct device;
+
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
+#else
+static inline int media_tknres_create(struct device *dev)
+{
+ return 0;
+}
+static inline int media_tknres_destroy(struct device *dev)
+{
+ return 0;
+}
+static inline int media_get_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static inline int media_put_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_get_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_put_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+#endif /* __LINUX_MEDIA_TOKEN_H */
diff --git a/lib/Makefile b/lib/Makefile
index d6b4bc4..6f21695 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o

obj-$(CONFIG_GLOB) += glob.o

+obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
+
obj-$(CONFIG_MPILIB) += mpi/
obj-$(CONFIG_SIGNATURE) += digsig.o

diff --git a/lib/media_tknres.c b/lib/media_tknres.c
new file mode 100644
index 0000000..e0a36cb
--- /dev/null
+++ b/lib/media_tknres.c
@@ -0,0 +1,237 @@
+/*
+ * media_tknres.c - managed media token resource
+ *
+ * Copyright (c) 2014 Shuah Khan <***@osg.samsung.com>
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+/*
+ * Media devices often have hardware resources that are shared
+ * across several functions. For instance, TV tuner cards often
+ * have MUXes, converters, radios, tuners, etc. that are shared
+ * across various functions. However, v4l2, alsa, DVB, usbfs, and
+ * all other drivers have no knowledge of what resources are
+ * shared. For example, users can't access DVB and alsa at the same
+ * time, or the DVB and V4L analog API at the same time, since many
+ * only have one converter that can be in either analog or digital
+ * mode. Accessing and/or changing mode of a converter while it is
+ * in use by another function results in video stream error.
+ *
+ * A shared media tokens resource is created using devres framework
+ * for drivers to find and lock/unlock. Creating a shared devres
+ * helps avoid creating data structure dependencies between drivers.
+ * This media token resource contains media token for tuner, and
+ * audio. When tuner token is requested, audio token is issued.
+ * Subsequent token (for tuner and audio) gets from the same task
+ * and task in the same tgid succeed. This allows applications that
+ * make multiple v4l2 ioctls to work with the first call acquiring
+ * the token and applications that create separate threads to handle
+ * video and audio functions.
+ *
+ * API
+ * int media_tknres_create(struct device *dev);
+ * int media_tknres_destroy(struct device *dev);
+ *
+ * int media_get_tuner_tkn(struct device *dev);
+ * int media_put_tuner_tkn(struct device *dev);
+ *
+ * int media_get_audio_tkn(struct device *dev);
+ * int media_put_audio_tkn(struct device *dev);
+*/
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/sched.h>
+#include <linux/media_tknres.h>
+
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
+
+#define TUNER "Tuner"
+#define AUDIO "Audio"
+
+static void media_tknres_release(struct device *dev, void *res)
+{
+ dev_info(dev, "%s: Media Token Resource released\n", __func__);
+}
+
+int media_tknres_create(struct device *dev)
+{
+ struct media_tknres *tkn;
+
+ tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres),
+ GFP_KERNEL);
+ if (!tkn)
+ return -ENOMEM;
+
+ spin_lock_init(&tkn->tuner.lock);
+ tkn->tuner.owner = 0;
+ tkn->tuner.tgid = 0;
+ tkn->tuner.task = NULL;
+
+ spin_lock_init(&tkn->audio.lock);
+ tkn->audio.owner = 0;
+ tkn->audio.tgid = 0;
+ tkn->audio.task = NULL;
+
+ devres_add(dev, tkn);
+
+ dev_info(dev, "%s: Media Token Resource created\n", __func__);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_create);
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
+ rc = -EBUSY;
+ } else {
+ tkn->owner = tpid;
+ tkn->tgid = tgid;
+ tkn->task = current;
+ }
+ pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
+ if (tkn->task == NULL ||
+ ((tkn->task != current) && (tkn->tgid != tgid))) {
+ rc = -EINVAL;
+ } else {
+ tkn->owner = 0;
+ tkn->tgid = 0;
+ tkn->task = NULL;
+ }
+ pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+/*
+ * When media tknres doesn't exist, get and put interfaces
+ * return 0 to let the callers take legacy code paths. This
+ * will also cover the drivers that don't create media tknres.
+ * Returning -ENODEV will require additional checks by callers.
+ * Instead handle the media tknres not present case as a driver
+ * not supporting media tknres and return 0.
+*/
+int media_get_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+ int ret = 0;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ ret = __media_get_tkn(&tkn_ptr->tuner, TUNER);
+ if (ret)
+ return ret;
+
+ /* get audio token */
+ ret = __media_get_tkn(&tkn_ptr->audio, AUDIO);
+ if (ret)
+ __media_put_tkn(&tkn_ptr->tuner, TUNER);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(media_get_tuner_tkn);
+
+int media_put_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ /* put audio token and then tuner token */
+ __media_put_tkn(&tkn_ptr->audio, AUDIO);
+
+ return __media_put_tkn(&tkn_ptr->tuner, TUNER);
+}
+EXPORT_SYMBOL_GPL(media_put_tuner_tkn);
+
+int media_get_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_get_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_get_audio_tkn);
+
+int media_put_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_put_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_put_audio_tkn);
+
+int media_tknres_destroy(struct device *dev)
+{
+ int rc;
+
+ rc = devres_release(dev, media_tknres_release, NULL, NULL);
+ WARN_ON(rc);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_destroy);
--
1.7.10.4
Takashi Iwai
2014-10-15 17:05:29 UTC
Permalink
At Tue, 14 Oct 2014 08:58:37 -0600,
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 291 insertions(+)
create mode 100644 include/linux/media_tknres.h
create mode 100644 lib/media_tknres.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e80a275..9216179 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-*
F: include/uapi/linux/meye.h
F: include/uapi/linux/ivtv*
F: include/uapi/linux/uvcvideo.h
+F: include/linux/media_tknres.h
+F: lib/media_tknres.c
MEDIAVISION PRO MOVIE STUDIO DRIVER
diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
new file mode 100644
index 0000000..6d37327
--- /dev/null
+++ b/include/linux/media_tknres.h
@@ -0,0 +1,50 @@
+/*
+ * media_tknres.h - managed media token resource
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef __LINUX_MEDIA_TOKEN_H
+#define __LINUX_MEDIA_TOKEN_H
+
+struct device;
+
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me. IMO, it should be "token".
Post by Shuah Khan
+#else
+static inline int media_tknres_create(struct device *dev)
+{
+ return 0;
+}
+static inline int media_tknres_destroy(struct device *dev)
+{
+ return 0;
+}
+static inline int media_get_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static inline int media_put_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_get_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_put_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+#endif /* __LINUX_MEDIA_TOKEN_H */
diff --git a/lib/Makefile b/lib/Makefile
index d6b4bc4..6f21695 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o
obj-$(CONFIG_GLOB) += glob.o
+obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
+
obj-$(CONFIG_MPILIB) += mpi/
obj-$(CONFIG_SIGNATURE) += digsig.o
diff --git a/lib/media_tknres.c b/lib/media_tknres.c
new file mode 100644
index 0000000..e0a36cb
--- /dev/null
+++ b/lib/media_tknres.c
@@ -0,0 +1,237 @@
+/*
+ * media_tknres.c - managed media token resource
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+/*
+ * Media devices often have hardware resources that are shared
+ * across several functions. For instance, TV tuner cards often
+ * have MUXes, converters, radios, tuners, etc. that are shared
+ * across various functions. However, v4l2, alsa, DVB, usbfs, and
+ * all other drivers have no knowledge of what resources are
+ * shared. For example, users can't access DVB and alsa at the same
+ * time, or the DVB and V4L analog API at the same time, since many
+ * only have one converter that can be in either analog or digital
+ * mode. Accessing and/or changing mode of a converter while it is
+ * in use by another function results in video stream error.
+ *
+ * A shared media tokens resource is created using devres framework
+ * for drivers to find and lock/unlock. Creating a shared devres
+ * helps avoid creating data structure dependencies between drivers.
+ * This media token resource contains media token for tuner, and
+ * audio. When tuner token is requested, audio token is issued.
+ * Subsequent token (for tuner and audio) gets from the same task
+ * and task in the same tgid succeed. This allows applications that
+ * make multiple v4l2 ioctls to work with the first call acquiring
+ * the token and applications that create separate threads to handle
+ * video and audio functions.
+ *
+ * API
+ * int media_tknres_create(struct device *dev);
+ * int media_tknres_destroy(struct device *dev);
+ *
+ * int media_get_tuner_tkn(struct device *dev);
+ * int media_put_tuner_tkn(struct device *dev);
+ *
+ * int media_get_audio_tkn(struct device *dev);
+ * int media_put_audio_tkn(struct device *dev);
+*/
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/sched.h>
+#include <linux/media_tknres.h>
+
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
Why do you need to have both tuner and audio tokens? If I understand
correctly, no matter whether it's tuner or audio, if it's being used,
the open must fail, right?
Post by Shuah Khan
+
+#define TUNER "Tuner"
+#define AUDIO "Audio"
+
+static void media_tknres_release(struct device *dev, void *res)
+{
+ dev_info(dev, "%s: Media Token Resource released\n", __func__);
+}
+
+int media_tknres_create(struct device *dev)
+{
+ struct media_tknres *tkn;
+
+ tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres),
+ GFP_KERNEL);
+ if (!tkn)
+ return -ENOMEM;
+
+ spin_lock_init(&tkn->tuner.lock);
+ tkn->tuner.owner = 0;
+ tkn->tuner.tgid = 0;
+ tkn->tuner.task = NULL;
+
+ spin_lock_init(&tkn->audio.lock);
+ tkn->audio.owner = 0;
+ tkn->audio.tgid = 0;
+ tkn->audio.task = NULL;
+
+ devres_add(dev, tkn);
+
+ dev_info(dev, "%s: Media Token Resource created\n", __func__);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_create);
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
You should use spin_lock_irqsave() here and in all other places.
The trigger callback in usb-audio, for example, may be called in irq
context.
Post by Shuah Khan
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
IMO, it's not "release" but "steal"... But what happens if the stolen
owner calls put? Then it'll be released although the stealing owner
still thinks it's being held.
Post by Shuah Khan
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
Shouldn't the second "&&" be "||" instead?
And too many parentheses there.
Post by Shuah Khan
+ rc = -EBUSY;
Wrong indentation.

I have a feeling that the whole thing can be a bit more simplified in
the end...


thanks,

Takashi
Post by Shuah Khan
+ } else {
+ tkn->owner = tpid;
+ tkn->tgid = tgid;
+ tkn->task = current;
+ }
+ pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
+ if (tkn->task == NULL ||
+ ((tkn->task != current) && (tkn->tgid != tgid))) {
+ rc = -EINVAL;
+ } else {
+ tkn->owner = 0;
+ tkn->tgid = 0;
+ tkn->task = NULL;
+ }
+ pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+/*
+ * When media tknres doesn't exist, get and put interfaces
+ * return 0 to let the callers take legacy code paths. This
+ * will also cover the drivers that don't create media tknres.
+ * Returning -ENODEV will require additional checks by callers.
+ * Instead handle the media tknres not present case as a driver
+ * not supporting media tknres and return 0.
+*/
+int media_get_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+ int ret = 0;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ ret = __media_get_tkn(&tkn_ptr->tuner, TUNER);
+ if (ret)
+ return ret;
+
+ /* get audio token */
+ ret = __media_get_tkn(&tkn_ptr->audio, AUDIO);
+ if (ret)
+ __media_put_tkn(&tkn_ptr->tuner, TUNER);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(media_get_tuner_tkn);
+
+int media_put_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ /* put audio token and then tuner token */
+ __media_put_tkn(&tkn_ptr->audio, AUDIO);
+
+ return __media_put_tkn(&tkn_ptr->tuner, TUNER);
+}
+EXPORT_SYMBOL_GPL(media_put_tuner_tkn);
+
+int media_get_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_get_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_get_audio_tkn);
+
+int media_put_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_put_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_put_audio_tkn);
+
+int media_tknres_destroy(struct device *dev)
+{
+ int rc;
+
+ rc = devres_release(dev, media_tknres_release, NULL, NULL);
+ WARN_ON(rc);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_destroy);
--
1.7.10.4
Shuah Khan
2014-10-16 00:53:28 UTC
Permalink
Post by Takashi Iwai
Post by Shuah Khan
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me. IMO, it should be "token".
No problem. I can change that to media_token_create/destroy()
and expand token in other functions.
Post by Takashi Iwai
Post by Shuah Khan
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
Why do you need to have both tuner and audio tokens? If I understand
correctly, no matter whether it's tuner or audio, if it's being used,
the open must fail, right?
As it evolved during development, it turns out at the moment I don't
have any use-cases that require holding audio and tuner separately.
It probably could be collapsed into just a media token. I have to
think about this some.
Post by Takashi Iwai
Post by Shuah Khan
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
You should use spin_lock_irqsave() here and in all other places.
The trigger callback in usb-audio, for example, may be called in irq
context.
ok. Good point, will change that.
Post by Takashi Iwai
Post by Shuah Khan
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
IMO, it's not "release" but "steal"... But what happens if the stolen
owner calls put? Then it'll be released although the stealing owner
still thinks it's being held.
Yeah it could be called a steal. :) Essentially tgid happens to be
the real owner. I am overwriting the pid with current pid when
tgid is same.

media dvb and v4l apps start two or more threads that all share the
tgid and subsequent token gets should be allowed based on the tgid.

Scenario 1:

Please note that there are 3 device files in question and media
token resource is the lock for all. Hence the changes to v4l-core,
dvb-core, and snd-usb-audio to hold the token for exclusive access
when the task or tgid don't match.

program starts:
- first thread opens device file in R/W mode - open gets the token
or thread makes ioctls calls that clearly indicates intent to
change tuner settings
- creates one thread for audio
- creates another for video or continues video function
- video thread does a close and re-opens the device file

In this case when thread tries to close, token put fails unless
tasks with same tgid are allowed to release.
( I ran into this one of the media applications and it is a valid
case to handle, thread can close the file and should be able to
open again without running into token busy case )

- or continue to just use the device file
- audio thread does snd_pcm_capture ops - trigger start

program exits:
- video thread closes the device file
- audio thread does snd_pcm_capture ops - trigger stop

This got me thinking about the scenario when an application
does a fork() as opposed to create a thread. I have to think
about this and see how I can address that.
Post by Takashi Iwai
Post by Shuah Khan
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
Shouldn't the second "&&" be "||" instead?
And too many parentheses there.
Right - this is my bad. The comment right above this conditional
is a give away that, at some point I did a copy and paste from
_put to _get and only changed the first task null check.
I am yelling at myself at the moment.
Post by Takashi Iwai
Post by Shuah Khan
+ rc = -EBUSY;
Wrong indentation.
Yes. Will fix that.
Post by Takashi Iwai
I have a feeling that the whole thing can be a bit more simplified in
the end...
Any ideas to simplify are welcome.

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Takashi Iwai
2014-10-16 14:00:03 UTC
Permalink
At Wed, 15 Oct 2014 18:53:28 -0600,
Post by Shuah Khan
Post by Takashi Iwai
Post by Shuah Khan
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me. IMO, it should be "token".
No problem. I can change that to media_token_create/destroy()
and expand token in other functions.
Post by Takashi Iwai
Post by Shuah Khan
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
Why do you need to have both tuner and audio tokens? If I understand
correctly, no matter whether it's tuner or audio, if it's being used,
the open must fail, right?
As it evolved during development, it turns out at the moment I don't
have any use-cases that require holding audio and tuner separately.
It probably could be collapsed into just a media token. I have to
think about this some.
Post by Takashi Iwai
Post by Shuah Khan
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
You should use spin_lock_irqsave() here and in all other places.
The trigger callback in usb-audio, for example, may be called in irq
context.
ok. Good point, will change that.
Post by Takashi Iwai
Post by Shuah Khan
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
IMO, it's not "release" but "steal"... But what happens if the stolen
owner calls put? Then it'll be released although the stealing owner
still thinks it's being held.
Yeah it could be called a steal. :) Essentially tgid happens to be
the real owner. I am overwriting the pid with current pid when
tgid is same.
media dvb and v4l apps start two or more threads that all share the
tgid and subsequent token gets should be allowed based on the tgid.
Please note that there are 3 device files in question and media
token resource is the lock for all. Hence the changes to v4l-core,
dvb-core, and snd-usb-audio to hold the token for exclusive access
when the task or tgid don't match.
- first thread opens device file in R/W mode - open gets the token
or thread makes ioctls calls that clearly indicates intent to
change tuner settings
- creates one thread for audio
- creates another for video or continues video function
- video thread does a close and re-opens the device file
In this case when thread tries to close, token put fails unless
tasks with same tgid are allowed to release.
( I ran into this one of the media applications and it is a valid
case to handle, thread can close the file and should be able to
open again without running into token busy case )
- or continue to just use the device file
- audio thread does snd_pcm_capture ops - trigger start
- video thread closes the device file
- audio thread does snd_pcm_capture ops - trigger stop
This got me thinking about the scenario when an application
does a fork() as opposed to create a thread. I have to think
about this and see how I can address that.
Post by Takashi Iwai
Post by Shuah Khan
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
Shouldn't the second "&&" be "||" instead?
And too many parentheses there.
Right - this is my bad. The comment right above this conditional
is a give away that, at some point I did a copy and paste from
_put to _get and only changed the first task null check.
I am yelling at myself at the moment.
Post by Takashi Iwai
Post by Shuah Khan
+ rc = -EBUSY;
Wrong indentation.
Yes. Will fix that.
Post by Takashi Iwai
I have a feeling that the whole thing can be a bit more simplified in
the end...
Any ideas to simplify are welcome.
There are a few points to make things more consistent and simplified,
IMO. First of all, the whole concept can be more generalized. It's
not necessarily only for media and audio. Rather we can make it a
generic dev_token. Then it'll become more convincing to land into
lib/*.

The media-specific handling is only about the pid and gid checks.
This can be implemented as a callback that is passed at creating a
token, for example. This will reduce the core code in lib/*.

Also, get and put should handle a proper refcount. The point I
mentioned in my previous post is the possible unbalance, and you'll
see unexpected unlock in the scenario. In addition, with use of kref,
the lock can be removed.

So, essentially the lib code would be just a devres_alloc() for an
object with kref, and dev_token_get() will take a kref with the
exclusion check.


Takashi
Shuah Khan
2014-10-16 14:39:26 UTC
Permalink
Post by Takashi Iwai
At Wed, 15 Oct 2014 18:53:28 -0600,
Post by Shuah Khan
Post by Takashi Iwai
Post by Shuah Khan
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me. IMO, it should be "token".
No problem. I can change that to media_token_create/destroy()
and expand token in other functions.
Post by Takashi Iwai
Post by Shuah Khan
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
Why do you need to have both tuner and audio tokens? If I understand
correctly, no matter whether it's tuner or audio, if it's being used,
the open must fail, right?
As it evolved during development, it turns out at the moment I don't
have any use-cases that require holding audio and tuner separately.
It probably could be collapsed into just a media token. I have to
think about this some.
Post by Takashi Iwai
Post by Shuah Khan
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
You should use spin_lock_irqsave() here and in all other places.
The trigger callback in usb-audio, for example, may be called in irq
context.
ok. Good point, will change that.
Post by Takashi Iwai
Post by Shuah Khan
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
IMO, it's not "release" but "steal"... But what happens if the stolen
owner calls put? Then it'll be released although the stealing owner
still thinks it's being held.
Yeah it could be called a steal. :) Essentially tgid happens to be
the real owner. I am overwriting the pid with current pid when
tgid is same.
media dvb and v4l apps start two or more threads that all share the
tgid and subsequent token gets should be allowed based on the tgid.
Please note that there are 3 device files in question and media
token resource is the lock for all. Hence the changes to v4l-core,
dvb-core, and snd-usb-audio to hold the token for exclusive access
when the task or tgid don't match.
- first thread opens device file in R/W mode - open gets the token
or thread makes ioctls calls that clearly indicates intent to
change tuner settings
- creates one thread for audio
- creates another for video or continues video function
- video thread does a close and re-opens the device file
In this case when thread tries to close, token put fails unless
tasks with same tgid are allowed to release.
( I ran into this one of the media applications and it is a valid
case to handle, thread can close the file and should be able to
open again without running into token busy case )
- or continue to just use the device file
- audio thread does snd_pcm_capture ops - trigger start
- video thread closes the device file
- audio thread does snd_pcm_capture ops - trigger stop
This got me thinking about the scenario when an application
does a fork() as opposed to create a thread. I have to think
about this and see how I can address that.
Post by Takashi Iwai
Post by Shuah Khan
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
Shouldn't the second "&&" be "||" instead?
And too many parentheses there.
Right - this is my bad. The comment right above this conditional
is a give away that, at some point I did a copy and paste from
_put to _get and only changed the first task null check.
I am yelling at myself at the moment.
Post by Takashi Iwai
Post by Shuah Khan
+ rc = -EBUSY;
Wrong indentation.
Yes. Will fix that.
Post by Takashi Iwai
I have a feeling that the whole thing can be a bit more simplified in
the end...
Any ideas to simplify are welcome.
There are a few points to make things more consistent and simplified,
IMO. First of all, the whole concept can be more generalized. It's
not necessarily only for media and audio. Rather we can make it a
generic dev_token. Then it'll become more convincing to land into
lib/*.
Right. At the moment this is very media specific.
Post by Takashi Iwai
The media-specific handling is only about the pid and gid checks.
This can be implemented as a callback that is passed at creating a
token, for example. This will reduce the core code in lib/*.
Also, get and put should handle a proper refcount. The point I
mentioned in my previous post is the possible unbalance, and you'll
see unexpected unlock in the scenario. In addition, with use of kref,
the lock can be removed.
Yes. kref would eliminate the need for locks and potential for
unbalanced scenario.
Post by Takashi Iwai
So, essentially the lib code would be just a devres_alloc() for an
object with kref, and dev_token_get() will take a kref with the
exclusion check.
I considered callback for media specific handling to make this token
construct generic. Talked myself out of it with the idea to get this
working for media use-case first and then extend it to be generic.

With this patch set covering media cases including non-media driver,
I think I am confident that the approach itself works to address the
issues and I don't mind pursuing a generic approach you are suggesting.
The current work isn't that far off and I can get the generic working
without many changes.

Thanks again for talking through the problem and ideas.

-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Hans Verkuil
2014-10-21 10:46:03 UTC
Permalink
Hi Shuah,

As promised, here is my review for this patch series.
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?

I also have the same question as Takashi: why do we have an audio
token in the first place? While you are streaming audio over alsa
the underlying tuner must be marked as being in use. It's all about
the tuner, since that's the resource that is being shared, not about
audio as such.

For the remainder of my review I will ignore the audio-related code
and concentrate only on the tuner part.
Post by Shuah Khan
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
I am still not convinced myself that this should be a generic API.
The only reason we need it today is for sharing tuners. Which is almost
a purely media thing with USB audio as the single non-media driver that
will be affected. Today I see no use case outside of tuners. I would
probably want to keep this inside drivers/media.

If this is going to be expanded it can always be moved to lib later.
Post by Shuah Khan
4 files changed, 291 insertions(+)
create mode 100644 include/linux/media_tknres.h
create mode 100644 lib/media_tknres.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e80a275..9216179 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-*
F: include/uapi/linux/meye.h
F: include/uapi/linux/ivtv*
F: include/uapi/linux/uvcvideo.h
+F: include/linux/media_tknres.h
+F: lib/media_tknres.c
MEDIAVISION PRO MOVIE STUDIO DRIVER
diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
new file mode 100644
index 0000000..6d37327
--- /dev/null
+++ b/include/linux/media_tknres.h
@@ -0,0 +1,50 @@
+/*
+ * media_tknres.h - managed media token resource
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef __LINUX_MEDIA_TOKEN_H
+#define __LINUX_MEDIA_TOKEN_H
+
+struct device;
+
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
+#else
+static inline int media_tknres_create(struct device *dev)
+{
+ return 0;
+}
+static inline int media_tknres_destroy(struct device *dev)
+{
+ return 0;
+}
+static inline int media_get_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static inline int media_put_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_get_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_put_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+#endif /* __LINUX_MEDIA_TOKEN_H */
diff --git a/lib/Makefile b/lib/Makefile
index d6b4bc4..6f21695 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o
obj-$(CONFIG_GLOB) += glob.o
+obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
+
obj-$(CONFIG_MPILIB) += mpi/
obj-$(CONFIG_SIGNATURE) += digsig.o
diff --git a/lib/media_tknres.c b/lib/media_tknres.c
new file mode 100644
index 0000000..e0a36cb
--- /dev/null
+++ b/lib/media_tknres.c
@@ -0,0 +1,237 @@
+/*
+ * media_tknres.c - managed media token resource
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+/*
+ * Media devices often have hardware resources that are shared
+ * across several functions. For instance, TV tuner cards often
+ * have MUXes, converters, radios, tuners, etc. that are shared
+ * across various functions. However, v4l2, alsa, DVB, usbfs, and
+ * all other drivers have no knowledge of what resources are
+ * shared. For example, users can't access DVB and alsa at the same
+ * time, or the DVB and V4L analog API at the same time, since many
+ * only have one converter that can be in either analog or digital
+ * mode. Accessing and/or changing mode of a converter while it is
+ * in use by another function results in video stream error.
+ *
+ * A shared media tokens resource is created using devres framework
+ * for drivers to find and lock/unlock. Creating a shared devres
+ * helps avoid creating data structure dependencies between drivers.
+ * This media token resource contains media token for tuner, and
+ * audio. When tuner token is requested, audio token is issued.
+ * Subsequent token (for tuner and audio) gets from the same task
+ * and task in the same tgid succeed. This allows applications that
+ * make multiple v4l2 ioctls to work with the first call acquiring
+ * the token and applications that create separate threads to handle
+ * video and audio functions.
+ *
+ * API
+ * int media_tknres_create(struct device *dev);
+ * int media_tknres_destroy(struct device *dev);
+ *
+ * int media_get_tuner_tkn(struct device *dev);
+ * int media_put_tuner_tkn(struct device *dev);
+ *
+ * int media_get_audio_tkn(struct device *dev);
+ * int media_put_audio_tkn(struct device *dev);
+*/
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/sched.h>
+#include <linux/media_tknres.h>
+
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
+
+#define TUNER "Tuner"
+#define AUDIO "Audio"
+
+static void media_tknres_release(struct device *dev, void *res)
+{
+ dev_info(dev, "%s: Media Token Resource released\n", __func__);
dev_dbg would be more appropriate.
Post by Shuah Khan
+}
+
+int media_tknres_create(struct device *dev)
+{
+ struct media_tknres *tkn;
+
+ tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres),
+ GFP_KERNEL);
+ if (!tkn)
+ return -ENOMEM;
+
+ spin_lock_init(&tkn->tuner.lock);
+ tkn->tuner.owner = 0;
+ tkn->tuner.tgid = 0;
+ tkn->tuner.task = NULL;
+
+ spin_lock_init(&tkn->audio.lock);
+ tkn->audio.owner = 0;
+ tkn->audio.tgid = 0;
+ tkn->audio.task = NULL;
+
+ devres_add(dev, tkn);
+
+ dev_info(dev, "%s: Media Token Resource created\n", __func__);
Ditto.
Post by Shuah Khan
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_create);
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
+ rc = -EBUSY;
As I understand it this makes no sense: if tkn->task == current,
then tkn->tgid == tgid.

Why would you allow different task in the same group id to release anyway?
Post by Shuah Khan
+ } else {
+ tkn->owner = tpid;
+ tkn->tgid = tgid;
+ tkn->task = current;
+ }
+ pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
+ if (tkn->task == NULL ||
+ ((tkn->task != current) && (tkn->tgid != tgid))) {
+ rc = -EINVAL;
+ } else {
+ tkn->owner = 0;
+ tkn->tgid = 0;
+ tkn->task = NULL;
+ }
+ pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n",
+ __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc);
+
+ spin_unlock(&tkn->lock);
+ return rc;
+}
+
+/*
+ * When media tknres doesn't exist, get and put interfaces
+ * return 0 to let the callers take legacy code paths. This
+ * will also cover the drivers that don't create media tknres.
+ * Returning -ENODEV will require additional checks by callers.
+ * Instead handle the media tknres not present case as a driver
+ * not supporting media tknres and return 0.
+*/
+int media_get_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+ int ret = 0;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ ret = __media_get_tkn(&tkn_ptr->tuner, TUNER);
OK, this makes no sense. I am completely missing the tuner mode here.
Speaking for V4L2 (I am less sure about DVB) any application can access
the tuner even if it is owned by another application as long as you don't
switch mode.

The way it works now it would block any other application from accessing
the tuner, or even other threads in the same process group. That's pretty
much guaranteed to break existing code.

So these tokens should be refcounted (certainly for the analog tuner mode,
but probably for all modes). In the V4L2 wrapper function you just check
if the token has been obtained already by setting a flag in struct v4l2_fh.
If not, then you get the token.

When the file handle is released and if it has the token, then you release
that token.

It is my understanding (correct me if I am wrong) that DVB and ALSA allow
only one user of the interface at a time, so you can still use refcounting
there, but the refcount will never go above 1 anyway.

I just hacked tknres support into vivid to test this and it really disables
this important feature.

I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION
and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this
module to the GPL devres_* functions. It took me some time to figure that
out.
Post by Shuah Khan
+ if (ret)
+ return ret;
+
+ /* get audio token */
+ ret = __media_get_tkn(&tkn_ptr->audio, AUDIO);
+ if (ret)
+ __media_put_tkn(&tkn_ptr->tuner, TUNER);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(media_get_tuner_tkn);
+
+int media_put_tuner_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ /* put audio token and then tuner token */
+ __media_put_tkn(&tkn_ptr->audio, AUDIO);
+
+ return __media_put_tkn(&tkn_ptr->tuner, TUNER);
+}
+EXPORT_SYMBOL_GPL(media_put_tuner_tkn);
+
+int media_get_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_get_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_get_audio_tkn);
+
+int media_put_audio_tkn(struct device *dev)
+{
+ struct media_tknres *tkn_ptr;
+
+ tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL);
+ if (tkn_ptr == NULL) {
+ dev_dbg(dev, "%s: Media Token Resource not found\n",
+ __func__);
+ return 0;
+ }
+
+ return __media_put_tkn(&tkn_ptr->audio, AUDIO);
+}
+EXPORT_SYMBOL_GPL(media_put_audio_tkn);
+
+int media_tknres_destroy(struct device *dev)
+{
+ int rc;
+
+ rc = devres_release(dev, media_tknres_release, NULL, NULL);
+ WARN_ON(rc);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_tknres_destroy);
I'm really sorry but I have to nack this patch series. It simply breaks
existing V4L functionality that I know people are using.

I'm not going to review the other patches, they seemed mostly OK to me
although I would expect to see changes to videobuf2-core.c as well which
I didn't.

You might want to think about adding media token support to vivid to test
this. The vivid driver supports both radio and TV tuners. Currently those
are emulated as independent tuners, but you could try to use media token
to emulate it as a shared resource. That way you can test this with a vb2
driver as well.

Anyway:

Nacked-by: Hans Verkuil <***@cisco.com>

Regards,

Hans
Takashi Iwai
2014-10-21 11:51:52 UTC
Permalink
At Tue, 21 Oct 2014 12:46:03 +0200,
Post by Hans Verkuil
Hi Shuah,
As promised, here is my review for this patch series.
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?
I also have the same question as Takashi: why do we have an audio
token in the first place? While you are streaming audio over alsa
the underlying tuner must be marked as being in use. It's all about
the tuner, since that's the resource that is being shared, not about
audio as such.
For the remainder of my review I will ignore the audio-related code
and concentrate only on the tuner part.
Post by Shuah Khan
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
I am still not convinced myself that this should be a generic API.
The only reason we need it today is for sharing tuners. Which is almost
a purely media thing with USB audio as the single non-media driver that
will be affected. Today I see no use case outside of tuners. I would
probably want to keep this inside drivers/media.
If this is going to be expanded it can always be moved to lib later.
Well, my argument is that it should be more generic if it were
intended to be put in lib. It'd be fine to put into drivers/media,
but this code snippet must be a separate module. Otherwise usb-audio
would grab the whole media stuff even if not needed at all.

(snip)
Post by Hans Verkuil
I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION
and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this
module to the GPL devres_* functions. It took me some time to figure that
out.
It was a code in lib, so it cannot be a module at all :)


Takashi
Hans Verkuil
2014-10-21 11:58:59 UTC
Permalink
Post by Takashi Iwai
At Tue, 21 Oct 2014 12:46:03 +0200,
Post by Hans Verkuil
Hi Shuah,
As promised, here is my review for this patch series.
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?
I also have the same question as Takashi: why do we have an audio
token in the first place? While you are streaming audio over alsa
the underlying tuner must be marked as being in use. It's all about
the tuner, since that's the resource that is being shared, not about
audio as such.
For the remainder of my review I will ignore the audio-related code
and concentrate only on the tuner part.
Post by Shuah Khan
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
I am still not convinced myself that this should be a generic API.
The only reason we need it today is for sharing tuners. Which is almost
a purely media thing with USB audio as the single non-media driver that
will be affected. Today I see no use case outside of tuners. I would
probably want to keep this inside drivers/media.
If this is going to be expanded it can always be moved to lib later.
Well, my argument is that it should be more generic if it were
intended to be put in lib. It'd be fine to put into drivers/media,
but this code snippet must be a separate module. Otherwise usb-audio
would grab the whole media stuff even if not needed at all.
Certainly.
Post by Takashi Iwai
(snip)
Post by Hans Verkuil
I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION
and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this
module to the GPL devres_* functions. It took me some time to figure that
out.
It was a code in lib, so it cannot be a module at all :)
Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it
compiles as a module :-)

Regards,

Hans
Takashi Iwai
2014-10-21 13:07:29 UTC
Permalink
At Tue, 21 Oct 2014 13:58:59 +0200,
Post by Hans Verkuil
Post by Takashi Iwai
At Tue, 21 Oct 2014 12:46:03 +0200,
Post by Hans Verkuil
Hi Shuah,
As promised, here is my review for this patch series.
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?
I also have the same question as Takashi: why do we have an audio
token in the first place? While you are streaming audio over alsa
the underlying tuner must be marked as being in use. It's all about
the tuner, since that's the resource that is being shared, not about
audio as such.
For the remainder of my review I will ignore the audio-related code
and concentrate only on the tuner part.
Post by Shuah Khan
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
---
MAINTAINERS | 2 +
include/linux/media_tknres.h | 50 +++++++++
lib/Makefile | 2 +
lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
I am still not convinced myself that this should be a generic API.
The only reason we need it today is for sharing tuners. Which is almost
a purely media thing with USB audio as the single non-media driver that
will be affected. Today I see no use case outside of tuners. I would
probably want to keep this inside drivers/media.
If this is going to be expanded it can always be moved to lib later.
Well, my argument is that it should be more generic if it were
intended to be put in lib. It'd be fine to put into drivers/media,
but this code snippet must be a separate module. Otherwise usb-audio
would grab the whole media stuff even if not needed at all.
Certainly.
Post by Takashi Iwai
(snip)
Post by Hans Verkuil
I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION
and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this
module to the GPL devres_* functions. It took me some time to figure that
out.
It was a code in lib, so it cannot be a module at all :)
Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it
compiles as a module :-)
Ah, I thought it was a boolean, but yes, this can be a module indeed.
Then I don't think it's worth to put in lib.


Takashi
Shuah Khan
2014-10-14 14:58:40 UTC
Permalink
Change dvb_frontend_open() to hold tuner and audio tokens
when frontend is opened in R/W mode. Tuner and audio tokens
are released when frontend is released in frontend exit state.
This change allows main dvb application process to hold the
tokens for all threads it creates and be able to handle channel
change requests without releasing the tokens thereby risking
loosing tokens to another application.

Note that media_get_tuner_tkn() will do a get on audio token
and return with both tuner and audio tokens locked. When tuner
token released using media_put_tuner_tkn() , audio token is
released. Initialize dev_parent field struct video_device to
enable media tuner token lookup from v4l2-core.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
drivers/media/dvb-core/dvb_frontend.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index b8579ee..fcf5f08 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -41,6 +41,7 @@
#include <linux/jiffies.h>
#include <linux/kthread.h>
#include <asm/processor.h>
+#include <linux/media_tknres.h>

#include "dvb_frontend.h"
#include "dvbdev.h"
@@ -2499,9 +2500,15 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
fepriv->tone = -1;
fepriv->voltage = -1;

+ /* get tuner and audio tokens - device is opened in R/W */
+ ret = media_get_tuner_tkn(fe->dvb->device);
+ if (ret == -EBUSY) {
+ dev_info(fe->dvb->device, "dvb: Tuner is busy\n");
+ goto err2;
+ }
ret = dvb_frontend_start (fe);
if (ret)
- goto err2;
+ goto start_err;

/* empty event queue */
fepriv->events.eventr = fepriv->events.eventw = 0;
@@ -2511,6 +2518,8 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
mutex_unlock (&adapter->mfe_lock);
return ret;

+start_err:
+ media_put_tuner_tkn(fe->dvb->device);
err2:
dvb_generic_release(inode, file);
err1:
@@ -2542,6 +2551,9 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
wake_up(&fepriv->wait_queue);
if (fe->exit != DVB_FE_NO_EXIT)
wake_up(&dvbdev->wait_queue);
+ /* release token if fe is in exit state */
+ else
+ media_put_tuner_tkn(fe->dvb->device);
if (fe->ops.ts_bus_ctrl)
fe->ops.ts_bus_ctrl(fe, 0);
}
--
1.7.10.4
Shuah Khan
2014-10-14 14:58:41 UTC
Permalink
Change snd_usb_capture_ops trigger to hold audio token prior
starting endpoints for SNDRV_PCM_TRIGGER_START request and
release after stopping endpoints for SNDRV_PCM_TRIGGER_STOP
request. Audio token is released from snd_usb_capture_ops
close interface to cover the case where an application exits
without stopping capture. Audio token get request will succeed
if it is free or when a media application with the same task
pid or task gid makes the request. This covers the cases when
a media application first hold the tuner nd audio token and
then requests SNDRV_PCM_TRIGGER_START either from the same
thread or a another thread in the same process group.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
sound/usb/pcm.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c62a165..d23abeb 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -21,6 +21,7 @@
#include <linux/usb.h>
#include <linux/usb/audio.h>
#include <linux/usb/audio-v2.h>
+#include <linux/media_tknres.h>

#include <sound/core.h>
#include <sound/pcm.h>
@@ -1220,6 +1221,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)

subs->pcm_substream = NULL;
snd_usb_autosuspend(subs->stream->chip);
+ media_put_audio_tkn(&subs->dev->dev);

return 0;
}
@@ -1573,6 +1575,12 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream

switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
+ return err;
+ }
err = start_endpoints(subs, false);
if (err < 0)
return err;
@@ -1583,6 +1591,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream
case SNDRV_PCM_TRIGGER_STOP:
stop_endpoints(subs, false);
subs->running = 0;
+ media_put_audio_tkn(&subs->dev->dev);
return 0;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
subs->data_endpoint->retire_data_urb = NULL;
--
1.7.10.4
Lars-Peter Clausen
2014-10-16 12:00:52 UTC
Permalink
On 10/14/2014 04:58 PM, Shuah Khan wrote:
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Post by Shuah Khan
+ return err;
+ }
err = start_endpoints(subs, false);
if (err < 0)
return err;
Shuah Khan
2014-10-16 13:10:37 UTC
Permalink
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Takashi Iwai
2014-10-16 14:01:16 UTC
Permalink
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?

Also, is this token restriction needed only for PCM? No mixer or
other controls?


Takashi
Shuah Khan
2014-10-16 14:10:52 UTC
Permalink
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Takashi Iwai
2014-10-16 14:16:59 UTC
Permalink
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?


Takashi
Shuah Khan
2014-10-16 14:39:14 UTC
Permalink
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.

-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Takashi Iwai
2014-10-16 14:48:03 UTC
Permalink
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.

But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.

How applications (e.g. PA) should behave if the token get fails?
Shouldn't it retry or totally give up?


Takashi
Shuah Khan
2014-10-16 14:59:29 UTC
Permalink
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.
But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.
ah. ok your recommendation is to go with open and close.
Mauro has some reservations with holding at open when I discussed
my observations with pulseaudio when I was holding token in open
instead of trigger start. Maybe he can chime with his concerns.
I think his concern was breaking applications if token is held in
open().

Based on what you are seeing trigger could be worse.
Post by Takashi Iwai
How applications (e.g. PA) should behave if the token get fails?
Shouldn't it retry or totally give up?
It would be up to the application I would think. I see that arecord
quits right away when it finds the device busy. pluseaudio on the other
hand appears to retry. I downloaded pulseaudio sources to understand
what it is doing, however I didn't get too far. The way it does audio
handling is complex for me to follow without spending a lot of time.

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Mauro Carvalho Chehab
2014-10-18 18:49:58 UTC
Permalink
Em Thu, 16 Oct 2014 08:59:29 -0600
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.
But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.
ah. ok your recommendation is to go with open and close.
Mauro has some reservations with holding at open when I discussed
my observations with pulseaudio when I was holding token in open
instead of trigger start. Maybe he can chime with his concerns.
I think his concern was breaking applications if token is held in
open().
Yes. My concern is that PA has weird behaviors, and it tries to open and
keep opened all audio devices. Is there a way for avoiding it to keep doing
it for V4L devices?
Post by Shuah Khan
Based on what you are seeing trigger could be worse.
Post by Takashi Iwai
How applications (e.g. PA) should behave if the token get fails?
Shouldn't it retry or totally give up?
It would be up to the application I would think. I see that arecord
quits right away when it finds the device busy. pluseaudio on the other
hand appears to retry. I downloaded pulseaudio sources to understand
what it is doing, however I didn't get too far. The way it does audio
handling is complex for me to follow without spending a lot of time.
thanks,
-- Shuah
--
Cheers,
Mauro
Takashi Iwai
2014-10-19 09:00:11 UTC
Permalink
At Sat, 18 Oct 2014 20:49:58 +0200,
Post by Mauro Carvalho Chehab
Em Thu, 16 Oct 2014 08:59:29 -0600
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.
But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.
ah. ok your recommendation is to go with open and close.
Mauro has some reservations with holding at open when I discussed
my observations with pulseaudio when I was holding token in open
instead of trigger start. Maybe he can chime with his concerns.
I think his concern was breaking applications if token is held in
open().
Yes. My concern is that PA has weird behaviors, and it tries to open and
keep opened all audio devices.
PA usually closes the PCM devices when unused. If it doesn't, it must
be a bug.


Takashi
Hans Verkuil
2014-10-21 15:42:51 UTC
Permalink
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.
But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.
How applications (e.g. PA) should behave if the token get fails?
Shouldn't it retry or totally give up?
Quite often media apps open the alsa device at the start and then switch
between TV, radio or DVB mode. If the alsa device would claim the tuner
just by being opened (as opposed to actually using the tuner, which happens
when you start streaming), then that would make it impossible for the
application to switch tuner mode. In general you want to avoid that open()
will start configuring hardware since that can quite often be slow. Tuner
configuration in particular can be slow since several common tuners need
to load firmware over i2c. You only want to do that when it is really needed,
and not when some application (udev!) opens the device just to examine what
sort of device it is.

So claiming the tuner in the trigger seems to be the right place. If
returning EBUSY is a poor error code for alsa, then we can use something else
for that. EACCES perhaps?

Regards,

Hans
Takashi Iwai
2014-10-21 16:05:41 UTC
Permalink
At Tue, 21 Oct 2014 17:42:51 +0200,
Post by Hans Verkuil
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:39:14 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 08:10:52 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Thu, 16 Oct 2014 07:10:37 -0600,
Post by Shuah Khan
Post by Lars-Peter Clausen
[...]
Post by Shuah Khan
switch (cmd) {
+ err = media_get_audio_tkn(&subs->dev->dev);
+ if (err == -EBUSY) {
+ dev_info(&subs->dev->dev, "%s device is busy\n",
+ __func__);
In my opinion this should not dev_info() as this is out of band error
signaling and also as the potential to spam the log. The userspace
application is already properly notified by the return code.
Yes it has the potential to flood the dmesg especially with alsa,
I will remove the dev_info().
Yes. And, I think doing this in the trigger isn't the best.
Why not in open & close?
My first cut of this change was in open and close. I saw pulseaudio
application go into this loop trying to open the device. To avoid
such problems, I went with trigger stat and stop. That made all the
pulseaudio continues attempts to open problems go away.
But now starting the stream gives the error, and PA would loop it
again, no?
Post by Shuah Khan
Post by Takashi Iwai
Also, is this token restriction needed only for PCM? No mixer or
other controls?
snd_pcm_ops are the only ones media drivers implement and use. So
I don't think mixer is needed. If it is needed, it is not to hard
to add for that case.
Well, then I wonder what resource does actually conflict with
usb-audio and media drivers at all...?
audio for dvb/v4l apps gets disrupted when audio app starts. For
example, dvb or v4l app tuned to a channel, and when an audio app
starts. audio path needs protected to avoid conflicts between
digital and analog applications as well.
OK, then concentrating on only PCM is fine.
But, I'm still not convinced about doing the token management in the
trigger. The reason -EBUSY doesn't work is that it's the very same
error code when a PCM device is blocked by other processes. And
-EAGAIN is interpreted by PCM core to -EBUSY for historical reasons.
How applications (e.g. PA) should behave if the token get fails?
Shouldn't it retry or totally give up?
Quite often media apps open the alsa device at the start and then switch
between TV, radio or DVB mode. If the alsa device would claim the tuner
just by being opened (as opposed to actually using the tuner, which happens
when you start streaming),
What about parameter changes? The sound devices have to be configured
before using. Don't they influence on others at all, i.e. you can
change the PCM sample rate etc during TV, radio or DVB is running?
Post by Hans Verkuil
then that would make it impossible for the
application to switch tuner mode. In general you want to avoid that open()
will start configuring hardware since that can quite often be slow. Tuner
configuration in particular can be slow since several common tuners need
to load firmware over i2c. You only want to do that when it is really needed,
and not when some application (udev!) opens the device just to examine what
sort of device it is.
But most apps close the device soon after that, no?
Which programs keep the PCM device (not the control) opened without
actually using?
Post by Hans Verkuil
So claiming the tuner in the trigger seems to be the right place. If
returning EBUSY is a poor error code for alsa, then we can use something else
for that. EACCES perhaps?
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.


Takashi
Devin Heitmueller
2014-10-21 16:08:59 UTC
Permalink
Post by Takashi Iwai
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.
I can say that I've definitely seen cases where if you configure a
device as the "default" capture device in PulseAudio, then pulse will
continue to capture from it even if you're not actively capturing the
audio from pulse. I only spotted this because I had a USB analyzer on
the device and was dumbfounded when the ISOC packets kept arriving
even after I had closed VLC.

Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Takashi Iwai
2014-10-21 16:14:41 UTC
Permalink
At Tue, 21 Oct 2014 12:08:59 -0400,
Post by Devin Heitmueller
Post by Takashi Iwai
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.
I can say that I've definitely seen cases where if you configure a
device as the "default" capture device in PulseAudio, then pulse will
continue to capture from it even if you're not actively capturing the
audio from pulse. I only spotted this because I had a USB analyzer on
the device and was dumbfounded when the ISOC packets kept arriving
even after I had closed VLC.
You might have had an input monitor active in some PA apps?
PA shouldn't do it as default. If it does unintentionally, you should
report it to PA guys.


Takashi
Pierre-Louis Bossart
2014-10-22 19:26:41 UTC
Permalink
Post by Devin Heitmueller
Post by Takashi Iwai
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.
I can say that I've definitely seen cases where if you configure a
device as the "default" capture device in PulseAudio, then pulse will
continue to capture from it even if you're not actively capturing the
audio from pulse. I only spotted this because I had a USB analyzer on
the device and was dumbfounded when the ISOC packets kept arriving
even after I had closed VLC.
this seems like a feature, not a bug. PulseAudio starts streaming before
clients push any data and likewise keeps sources active even after for
some time after clients stop recording. Closing VLC in your example
doesn't immediately close the ALSA device. look for
module-suspend-on-idle in your default.pa config file.
I also agree that the open/close of the alsa device is the only way to
control exclusion.
-Pierre
Devin Heitmueller
2014-10-22 19:45:42 UTC
Permalink
Post by Pierre-Louis Bossart
this seems like a feature, not a bug. PulseAudio starts streaming before
clients push any data and likewise keeps sources active even after for some
time after clients stop recording. Closing VLC in your example doesn't
immediately close the ALSA device. look for module-suspend-on-idle in your
default.pa config file.
The ALSA userland emulation in PulseAudio is supposed to faithfully emulate
the behavior of the ALSA kernel ABI... except when it doesn't, then it's not
a bug but rather a feature. :-)
Post by Pierre-Louis Bossart
I also agree that the open/close of the alsa device is the only way to
control exclusion.
I was also a proponent that we should have fairly coarse locking done
at open/close for the various device nodes (ALSA/V4L/DVB). The challenge here
is that we have a large installed based of existing applications that
rely on kernel
behavior that isn't formally specified in any specification. Hence
we're forced to try
to come up with a solution that minimizes the risk of ABI breakage.

If we were doing this from scratch then we could lay down some hard/fast rules
about things apps aren't supposed to do and how apps are supposed to respond
to those exception cases. Unfortunately we don't have that luxury here.

Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
Shuah Khan
2014-10-21 17:32:10 UTC
Permalink
Post by Takashi Iwai
At Tue, 21 Oct 2014 17:42:51 +0200,
Post by Hans Verkuil
Quite often media apps open the alsa device at the start and then switch
between TV, radio or DVB mode. If the alsa device would claim the tuner
just by being opened (as opposed to actually using the tuner, which happens
when you start streaming),
What about parameter changes? The sound devices have to be configured
before using. Don't they influence on others at all, i.e. you can
change the PCM sample rate etc during TV, radio or DVB is running?
Yes. kaffeine uses snd_usb_capture_ops ioctl -> snd_pcm_lib_ioctl

Other v4l and vlc (dvb) uses open/close as well as trigger start and
stop. trigger start/stop is done by a special audio thread in some
cases. open/close happens from the main thread.
Post by Takashi Iwai
Post by Hans Verkuil
then that would make it impossible for the
application to switch tuner mode. In general you want to avoid that open()
will start configuring hardware since that can quite often be slow. Tuner
configuration in particular can be slow since several common tuners need
to load firmware over i2c. You only want to do that when it is really needed,
and not when some application (udev!) opens the device just to examine what
sort of device it is.
But most apps close the device soon after that, no?
Which programs keep the PCM device (not the control) opened without
actually using?
Post by Hans Verkuil
So claiming the tuner in the trigger seems to be the right place. If
returning EBUSY is a poor error code for alsa, then we can use something else
for that. EACCES perhaps?
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.
Let me share my test matrix for this patch series. Hans pointed out
one test case I didn't know about as a result missed testing. Please
see if any of the tests miss use-cases or break them: you can scroll
down to the proposal at the end, if this is too much detail :)

Digital active and analog starting testing:
kaffeine running
- v4l2-ctl --all - works
- Changing channels works with the same token hold, even when
frequency changes. Tested changing channels that force freq
change.
- vlc resource is busy with no disruption to kaffeine
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings - snd_usb_pcm_open detects device is busy
( pcm open called from the same thread, trigger gets called
from another thread )
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
( pcm open called from the same thread, trigger gets called
from another thread )
- vlc - audio capture on WinTV HVR-950 - device is busy
start vlc with no channels for this test
- arecord to capture on WinTV HVR-950 - device busy

vlc running
vlc -v channels.xspf
- v4l2-ctl --all - works
- Changing channels works with the same token hold, even when
frequency changes. Tested changing channels that force freq
change.
- kaffeine resource is busy with no disruption to vlc
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings - snd_usb_pcm_open detects device is busy
( pcm open called from the same thread, trigger gets called
from another thread )
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
( pcm open called from the same thread, trigger gets called
from another thread )
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy

Analog active and start digital testing:
xawtv -noalsa -c /dev/video1
- v4l2-ctl --all - works
- start kaffeine - fails with device busy and no disruption
- start vlc - fails with device busy and no disruption
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy

tvtime
- v4l2-ctl --all - works
- start kaffeine - fails with device busy and no disruption
- start vlc - fails with device busy and no disruption
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy

The following audio/video start/stop combination tests:
( used arecord as well to test these cases, arecord )

- tvtime start/vlc start/vlc stop/tvtime stop
no disruption to tvtime
- tvtime start/vlc start/tvtie stop/vlc stop
One tvtime stops, could trigger capture manually
- vlc start/tvtime start/tvtime stop/vlc stop
vlc audio capture continues, tvtime detect tuner busy
- vlc start/tvtime start/vlc stop/tvtime start
when vlc stops, tvtime could open the tuner device

Repeated the above with kaffeine and vlc and arecord audio
capture combinations.

Hans pointed out I am missing:

v4l2-ctl -f 180 --sleep=10
While it is sleeping you must still be able to set the frequency from
another console.

And it doesn't matter which of the two v4l2-ctl processes ends first,
as long as one has a reference to the tuner you should be blocked
from switching the tuner to a different mode (radio or dvb)

I think this above fails because tuner token ownership doesn't
span processes. Token should act as a lock between dvb/audio/v4l,
and v4l itself has mechanisms to handle the multiple ownership token
shouldn't interfere with.

Provided the only thing that is breaking is the v4l case, I think
I can get it to work with the bit field approach.

Here is what I propose for patch v3:
- make it a module under media
- collapse tuner and audio tokens into media token
- change names (get rid of abbreviated tkn stuff)
- Make other changes Takashi/Lars pointed out in pcm
- hold token in pcm open/close
- add a bitfield to struct v4l2_fh to handle
the v4l specific multiple ownership cases.
- v4l-core and au0828-videp patches will see changes.
- dvb patch should still be good (crossing fingers)

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Hans Verkuil
2014-10-22 15:24:13 UTC
Permalink
Hi Shuah,

Some notes below...
Post by Shuah Khan
Post by Takashi Iwai
At Tue, 21 Oct 2014 17:42:51 +0200,
Post by Hans Verkuil
Quite often media apps open the alsa device at the start and then switch
between TV, radio or DVB mode. If the alsa device would claim the tuner
just by being opened (as opposed to actually using the tuner, which happens
when you start streaming),
What about parameter changes? The sound devices have to be configured
before using. Don't they influence on others at all, i.e. you can
change the PCM sample rate etc during TV, radio or DVB is running?
Yes. kaffeine uses snd_usb_capture_ops ioctl -> snd_pcm_lib_ioctl
Other v4l and vlc (dvb) uses open/close as well as trigger start and
stop. trigger start/stop is done by a special audio thread in some
cases. open/close happens from the main thread.
Post by Takashi Iwai
Post by Hans Verkuil
then that would make it impossible for the
application to switch tuner mode. In general you want to avoid that open()
will start configuring hardware since that can quite often be slow. Tuner
configuration in particular can be slow since several common tuners need
to load firmware over i2c. You only want to do that when it is really needed,
and not when some application (udev!) opens the device just to examine what
sort of device it is.
But most apps close the device soon after that, no?
Which programs keep the PCM device (not the control) opened without
actually using?
Post by Hans Verkuil
So claiming the tuner in the trigger seems to be the right place. If
returning EBUSY is a poor error code for alsa, then we can use something else
for that. EACCES perhaps?
Sorry, I'm not convinced by that. If the device has to be controlled
exclusively, the right position is the open/close. Otherwise, the
program cannot know when it becomes inaccessible out of sudden during
its operation.
Let me share my test matrix for this patch series. Hans pointed out
one test case I didn't know about as a result missed testing. Please
see if any of the tests miss use-cases or break them: you can scroll
down to the proposal at the end, if this is too much detail :)
kaffeine running
- v4l2-ctl --all - works
I would recommend using v4l2-ctl --all --list-inputs, this enumerates
inputs as well (which includes reading the tuner status). This would
have failed with all these tests with this patch series.
Post by Shuah Khan
- Changing channels works with the same token hold, even when
frequency changes. Tested changing channels that force freq
change.
- vlc resource is busy with no disruption to kaffeine
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings - snd_usb_pcm_open detects device is busy
( pcm open called from the same thread, trigger gets called
from another thread )
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
( pcm open called from the same thread, trigger gets called
from another thread )
- vlc - audio capture on WinTV HVR-950 - device is busy
start vlc with no channels for this test
- arecord to capture on WinTV HVR-950 - device busy
vlc running
vlc -v channels.xspf
- v4l2-ctl --all - works
- Changing channels works with the same token hold, even when
frequency changes. Tested changing channels that force freq
change.
- kaffeine resource is busy with no disruption to vlc
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings - snd_usb_pcm_open detects device is busy
( pcm open called from the same thread, trigger gets called
from another thread )
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
( pcm open called from the same thread, trigger gets called
from another thread )
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy
xawtv -noalsa -c /dev/video1
- v4l2-ctl --all - works
- start kaffeine - fails with device busy and no disruption
- start vlc - fails with device busy and no disruption
- tvtime - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy
tvtime
- v4l2-ctl --all - works
- start kaffeine - fails with device busy and no disruption
- start vlc - fails with device busy and no disruption
- xawtv - tuner busy when it tries to do ioctls that change
tuner settings with no disruption to kaffeine
- vlc - audio capture on WinTV HVR-950 - device is busy
- arecord to capture on WinTV HVR-950 - device busy
( used arecord as well to test these cases, arecord )
- tvtime start/vlc start/vlc stop/tvtime stop
no disruption to tvtime
- tvtime start/vlc start/tvtie stop/vlc stop
One tvtime stops, could trigger capture manually
- vlc start/tvtime start/tvtime stop/vlc stop
vlc audio capture continues, tvtime detect tuner busy
- vlc start/tvtime start/vlc stop/tvtime start
when vlc stops, tvtime could open the tuner device
Repeated the above with kaffeine and vlc and arecord audio
capture combinations.
v4l2-ctl -f 180 --sleep=10
While it is sleeping you must still be able to set the frequency from
another console.
And it doesn't matter which of the two v4l2-ctl processes ends first,
as long as one has a reference to the tuner you should be blocked
from switching the tuner to a different mode (radio or dvb)
I think this above fails because tuner token ownership doesn't
span processes. Token should act as a lock between dvb/audio/v4l,
and v4l itself has mechanisms to handle the multiple ownership token
shouldn't interfere with.
Provided the only thing that is breaking is the v4l case, I think
I can get it to work with the bit field approach.
- make it a module under media
- collapse tuner and audio tokens into media token
- change names (get rid of abbreviated tkn stuff)
- Make other changes Takashi/Lars pointed out in pcm
- hold token in pcm open/close
I remain skeptical about this. I'm simply not sure what, if anything,
might break after this change. You need to test with applications that
can switch between V4L and DVB and that can handle an alsa device for
audio capture. MythTV is probably one. None of the applications you
are testing can switch between V4L and DVB devices dynamically to my
knowledge.

Today it is no problem for an application to open the alsa device,
start streaming analog video + audio, stop streaming video + audio,
switch to a digital input and start streaming the transport stream
from the dvb input.

This is all valid. But after this change you will no longer be able
to switch back and forth because the alsa open() locks the tuner mode.

I'm afraid that we will break some applications, either open source or
not, by making this change.

By locking the tuner mode only while streaming audio apps would be prevented
from breaking. The only time the trigger function will return an error
is when some other application has the tuner in a different (e.g. DVB)
mode and starting to stream would forcefully switch the tuner to
a different mode. And we *know* that will cause all sorts of unpredictable
behavior which is why we are doing all this effort to prevent that from
happening. Returning an error in that case makes perfect sense to me and
it is something that should not happen during normal use.
Post by Shuah Khan
- add a bitfield to struct v4l2_fh to handle
the v4l specific multiple ownership cases.
- v4l-core and au0828-videp patches will see changes.
- dvb patch should still be good (crossing fingers)
The remainder of the list looks OK to me.

I would also recommend making more use of the various command line tools:
arecord, v4l2-ctl, dvbv5-zap. By combining these you can test scenarios
that are hard to test in applications such as this:

- make a small utility that just opens the alsa node without streaming
and that keeps it open (i.e. sleep(100000) or something). Let's call it alsaopen.
- run alsaopen.
- call arecord to start streaming audio (most likely using
the analog TV tuner).
- stream with v4l2-ctl to see if that works (it should)
- stop v4l2-ctl
- now use dvbv5-zap to select a DVB channel: I would expect that that
fails because you are still streaming audio.
- stop arecord.
- try dvbv5-zap again: I think this should work, even though the alsaopen
utility still has alsa open, but with the current proposal this will fail.
- stop alsaopen.
- now dvbv5-zap should work.

Regards,

Hans
Shuah Khan
2014-10-14 14:58:42 UTC
Permalink
Changed au0828-core to create media token resource in its
usb_probe() and destroy it from usb_disconnect() interfaces.
It creates the resource on the main struct device which is
the parent device for the interface usb device. This is the
main struct device that is common for all the drivers that
control the media device, including the non-media sound
drivers.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
drivers/media/usb/au0828/au0828-core.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bc06480..189e435 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -26,6 +26,7 @@
#include <linux/videodev2.h>
#include <media/v4l2-common.h>
#include <linux/mutex.h>
+#include <linux/media_tknres.h>

/*
* 1 = General debug messages
@@ -127,6 +128,17 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
return status;
}

+/* interfaces to create and destroy media tknres */
+static int au0828_create_media_tknres(struct au0828_dev *dev)
+{
+ return media_tknres_create(&dev->usbdev->dev);
+}
+
+static int au0828_destroy_media_tknres(struct au0828_dev *dev)
+{
+ return media_tknres_destroy(&dev->usbdev->dev);
+}
+
static void au0828_usb_release(struct au0828_dev *dev)
{
/* I2C */
@@ -157,6 +169,8 @@ static void au0828_usb_disconnect(struct usb_interface *interface)
/* Digital TV */
au0828_dvb_unregister(dev);

+ au0828_destroy_media_tknres(dev);
+
usb_set_intfdata(interface, NULL);
mutex_lock(&dev->mutex);
dev->usbdev = NULL;
@@ -215,6 +229,13 @@ static int au0828_usb_probe(struct usb_interface *interface,
dev->usbdev = usbdev;
dev->boardnr = id->driver_info;

+ /* create media token resource */
+ if (au0828_create_media_tknres(dev)) {
+ mutex_unlock(&dev->lock);
+ kfree(dev);
+ return -ENOMEM;
+ }
+
#ifdef CONFIG_VIDEO_AU0828_V4L2
dev->v4l2_dev.release = au0828_usb_v4l2_release;

@@ -223,6 +244,7 @@ static int au0828_usb_probe(struct usb_interface *interface,
if (retval) {
pr_err("%s() v4l2_device_register failed\n",
__func__);
+ au0828_destroy_media_tknres(dev);
mutex_unlock(&dev->lock);
kfree(dev);
return retval;
@@ -232,6 +254,7 @@ static int au0828_usb_probe(struct usb_interface *interface,
if (retval) {
pr_err("%s() v4l2_ctrl_handler_init failed\n",
__func__);
+ au0828_destroy_media_tknres(dev);
mutex_unlock(&dev->lock);
kfree(dev);
return retval;
--
1.7.10.4
Shuah Khan
2014-10-14 14:58:38 UTC
Permalink
Changes to v4l2-core to hold tuner and audio tokens in v4l2
ioctl that change the tuner modes, and release the token from
fh exit. The changes are limited to vb2 calls that disrupt
digital stream. vb1 changes are made in the driver. The
following ioctls are changed:

S_INPUT, S_FMT, S_TUNER, S_FREQUENCY, S_STD, S_HW_FREQ_SEEK,
VIDIOC_ENUMINPUT, and VIDIOC_QUERYSTD:

- hold tuner and audio tokens before calling appropriate
ops->vidioc_s_*
- return tuner and audio tokens locked.

v4l2_fh_exit:

- releases tuner and audio tokens.

Note that media_get_tuner_tkn() will do a get on audio token
and return with both tuner and audio tokens locked. When tuner
token released using media_put_tuner_tkn() , audio token is
released.

Signed-off-by: Shuah Khan <***@osg.samsung.com>
---
drivers/media/v4l2-core/v4l2-fh.c | 7 ++++
drivers/media/v4l2-core/v4l2-ioctl.c | 61 ++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index c97067a..717e03d 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -25,7 +25,10 @@
#include <linux/bitops.h>
#include <linux/slab.h>
#include <linux/export.h>
+#include <linux/device.h>
+#include <linux/media_tknres.h>
#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
#include <media/v4l2-fh.h>
#include <media/v4l2-event.h>
#include <media/v4l2-ioctl.h>
@@ -92,6 +95,10 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
{
if (fh->vdev == NULL)
return;
+
+ if (fh->vdev->dev_parent)
+ media_put_tuner_tkn(fh->vdev->dev_parent);
+
v4l2_event_unsubscribe_all(fh);
fh->vdev = NULL;
}
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 9ccb19a..686f428 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -17,6 +17,7 @@
#include <linux/types.h>
#include <linux/kernel.h>
#include <linux/version.h>
+#include <linux/media_tknres.h>

#include <linux/videodev2.h>

@@ -1003,6 +1004,15 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
sizeof(fmt->fmt.pix) - offset);
}

+static int v4l_get_tuner_tkn(struct video_device *vfd)
+{
+ int rc = 0;
+
+ if (vfd->dev_parent)
+ rc = media_get_tuner_tkn(vfd->dev_parent);
+ return rc;
+}
+
static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -1022,6 +1032,14 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
+ int ret = 0;
+
+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
}

@@ -1063,7 +1081,13 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
{
struct video_device *vfd = video_devdata(file);
struct v4l2_input *p = arg;
+ int ret;

+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
/*
* We set the flags for CAP_DV_TIMINGS &
* CAP_STD here based on ioctl handler provided by the
@@ -1236,6 +1260,12 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
int ret;

+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
+
v4l_sanitize_format(p);

switch (p->type) {
@@ -1415,9 +1445,15 @@ static int v4l_s_tuner(const struct v4l2_ioctl_ops *ops,
{
struct video_device *vfd = video_devdata(file);
struct v4l2_tuner *p = arg;
+ int ret;

p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
return ops->vidioc_s_tuner(file, fh, p);
}

@@ -1453,6 +1489,7 @@ static int v4l_s_frequency(const struct v4l2_ioctl_ops *ops,
struct video_device *vfd = video_devdata(file);
const struct v4l2_frequency *p = arg;
enum v4l2_tuner_type type;
+ int ret;

if (vfd->vfl_type == VFL_TYPE_SDR) {
if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
@@ -1462,6 +1499,11 @@ static int v4l_s_frequency(const struct v4l2_ioctl_ops *ops,
V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
if (type != p->type)
return -EINVAL;
+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
}
return ops->vidioc_s_frequency(file, fh, p);
}
@@ -1508,11 +1550,18 @@ static int v4l_s_std(const struct v4l2_ioctl_ops *ops,
{
struct video_device *vfd = video_devdata(file);
v4l2_std_id id = *(v4l2_std_id *)arg, norm;
+ int ret = 0;

norm = id & vfd->tvnorms;
if (vfd->tvnorms && !norm) /* Check if std is supported */
return -EINVAL;

+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
+
/* Calls the specific handler */
return ops->vidioc_s_std(file, fh, norm);
}
@@ -1522,7 +1571,13 @@ static int v4l_querystd(const struct v4l2_ioctl_ops *ops,
{
struct video_device *vfd = video_devdata(file);
v4l2_std_id *p = arg;
+ int ret = 0;

+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
/*
* If no signal is detected, then the driver should return
* V4L2_STD_UNKNOWN. Otherwise it should return tvnorms with
@@ -1541,6 +1596,7 @@ static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
struct video_device *vfd = video_devdata(file);
struct v4l2_hw_freq_seek *p = arg;
enum v4l2_tuner_type type;
+ int ret;

/* s_hw_freq_seek is not supported for SDR for now */
if (vfd->vfl_type == VFL_TYPE_SDR)
@@ -1550,6 +1606,11 @@ static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
if (p->type != type)
return -EINVAL;
+ ret = v4l_get_tuner_tkn(vfd);
+ if (ret) {
+ pr_info("%s: Tuner is busy\n", __func__);
+ return ret;
+ }
return ops->vidioc_s_hw_freq_seek(file, fh, p);
}
--
1.7.10.4
Takashi Iwai
2014-10-15 16:48:08 UTC
Permalink
At Tue, 14 Oct 2014 08:58:36 -0600,
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
This patch series consists of media token resource framework
and changes to use it in dvb-core, v4l2-core, au0828 driver,
and snd-usb-audio driver.
With these changes dvb and v4l2 can share the tuner without
disrupting each other. Used tvtime, xawtv, kaffeine, and vlc,
vlc audio capture option, arecord/aplay during development to
identify v4l2 vb2 and vb1 ioctls and file operations that
disrupt the digital stream and would require changes to check
tuner ownership prior to changing the tuner configuration.
vb2 changes are made in the v4l2-core and vb1 changes are made
in the au0828 driver to encourage porting drivers to vb2 to
advantage of the new media token resource framework with changes
in the core.
In this patch v2 series, fixed problems identified in the
patch v1 series. Important ones are changing snd-usb-audio
to use media tokens, holding tuner lock in VIDIOC_ENUMINPUT,
and VIDIOC_QUERYSTD.
Just took a quick glance over the patches, and my first concern is why
this has to be lib/*. This means it's always built-in as long as this
config is enabled (and will be so on distro kernel) even if it's not
used at all.


thanks,

Takashi
Shuah Khan
2014-10-15 20:21:34 UTC
Permalink
Post by Takashi Iwai
At Tue, 14 Oct 2014 08:58:36 -0600,
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
This patch series consists of media token resource framework
and changes to use it in dvb-core, v4l2-core, au0828 driver,
and snd-usb-audio driver.
With these changes dvb and v4l2 can share the tuner without
disrupting each other. Used tvtime, xawtv, kaffeine, and vlc,
vlc audio capture option, arecord/aplay during development to
identify v4l2 vb2 and vb1 ioctls and file operations that
disrupt the digital stream and would require changes to check
tuner ownership prior to changing the tuner configuration.
vb2 changes are made in the v4l2-core and vb1 changes are made
in the au0828 driver to encourage porting drivers to vb2 to
advantage of the new media token resource framework with changes
in the core.
In this patch v2 series, fixed problems identified in the
patch v1 series. Important ones are changing snd-usb-audio
to use media tokens, holding tuner lock in VIDIOC_ENUMINPUT,
and VIDIOC_QUERYSTD.
Just took a quick glance over the patches, and my first concern is why
this has to be lib/*. This means it's always built-in as long as this
config is enabled (and will be so on distro kernel) even if it's not
used at all.
Right this module gets built when CONFIG_MEDIA_SUPPORT is enabled
and stubs are in place when it is not enabled. The intent is for
this feature to be enabled by default when media support is enabled.
When a driver doesn't create the resource, it will simply not find it
and for drivers like snd-usb-audio that aren't tried to media support,
the stubs are in place and feature is essentially disabled.

I picked lib so this module can be included in non-media drivers
e.g: snd-usb-audio.

Does this help explain the design? I didn't want to introduce a new
config for this feature. If lib isn't right place, could you recommend
another one that makes this modules available to non-media drivers?
moving isn't a problem.

thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
***@osg.samsung.com | (970) 217-8978
Takashi Iwai
2014-10-16 14:02:49 UTC
Permalink
At Wed, 15 Oct 2014 14:21:34 -0600,
Post by Shuah Khan
Post by Takashi Iwai
At Tue, 14 Oct 2014 08:58:36 -0600,
Post by Shuah Khan
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
This patch series consists of media token resource framework
and changes to use it in dvb-core, v4l2-core, au0828 driver,
and snd-usb-audio driver.
With these changes dvb and v4l2 can share the tuner without
disrupting each other. Used tvtime, xawtv, kaffeine, and vlc,
vlc audio capture option, arecord/aplay during development to
identify v4l2 vb2 and vb1 ioctls and file operations that
disrupt the digital stream and would require changes to check
tuner ownership prior to changing the tuner configuration.
vb2 changes are made in the v4l2-core and vb1 changes are made
in the au0828 driver to encourage porting drivers to vb2 to
advantage of the new media token resource framework with changes
in the core.
In this patch v2 series, fixed problems identified in the
patch v1 series. Important ones are changing snd-usb-audio
to use media tokens, holding tuner lock in VIDIOC_ENUMINPUT,
and VIDIOC_QUERYSTD.
Just took a quick glance over the patches, and my first concern is why
this has to be lib/*. This means it's always built-in as long as this
config is enabled (and will be so on distro kernel) even if it's not
used at all.
Right this module gets built when CONFIG_MEDIA_SUPPORT is enabled
and stubs are in place when it is not enabled. The intent is for
this feature to be enabled by default when media support is enabled.
When a driver doesn't create the resource, it will simply not find it
and for drivers like snd-usb-audio that aren't tried to media support,
the stubs are in place and feature is essentially disabled.
I picked lib so this module can be included in non-media drivers
e.g: snd-usb-audio.
Does this help explain the design? I didn't want to introduce a new
config for this feature. If lib isn't right place, could you recommend
another one that makes this modules available to non-media drivers?
moving isn't a problem.
We can create a small module depending on CONFIG_MEDIA. But it'll be
rather a question of the size. If it's reasonably small and generic
enough, it's worth to put into lib/*, I think.


Takashi
Loading...