[PATCH v2 0/6] media token resource framework

[PATCH v2 0/6] media token resource framework

[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.

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

[PATCH v2 1/6] media: add media token device resource framework

[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.

Signed-off-by: Shuah Khan <shuahkh@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 <hverkuil@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 <shuahkh@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 <shuahkh@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

Re: [PATCH v2 1/6] media: add media token device resource framework

[Takashi Iwai]

At Tue, 14 Oct 2014 08:58:37 -0600,
Shuah Khan wrote:
> 
> 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 <shuahkh@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 <hverkuil@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 <shuahkh@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);

The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me.  IMO, it should be "token".

> +#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 <shuahkh@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;
> +};

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?

> +
> +#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.

> +
> +	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.

> +	if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {

Shouldn't the second "&&" be "||" instead?
And too many parentheses there.

> +			rc = -EBUSY;

Wrong indentation.

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


thanks,

Takashi

> +	} 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
> 

Re: [PATCH v2 1/6] media: add media token device resource framework

[Shuah Khan]

On 10/15/2014 11:05 AM, Takashi Iwai wrote:

>> +#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.

>> +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.

>> +
>> +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.

> 
>> +
>> +	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.

> 
>> +	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.

> 
>> +			rc = -EBUSY;
> 
> Wrong indentation.

Yes. Will fix that.

> 
> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH v2 1/6] media: add media token device resource framework

[Takashi Iwai]

At Wed, 15 Oct 2014 18:53:28 -0600,
Shuah Khan wrote:
> 
> On 10/15/2014 11:05 AM, Takashi Iwai wrote:
> 
> >> +#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.
> 
> >> +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.
> 
> >> +
> >> +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.
> 
> > 
> >> +
> >> +	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.
> 
> > 
> >> +	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.
> 
> > 
> >> +			rc = -EBUSY;
> > 
> > Wrong indentation.
> 
> Yes. Will fix that.
> 
> > 
> > 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

Re: [PATCH v2 1/6] media: add media token device resource framework

[Shuah Khan]

On 10/16/2014 08:00 AM, Takashi Iwai wrote:
> At Wed, 15 Oct 2014 18:53:28 -0600,
> Shuah Khan wrote:
>>
>> On 10/15/2014 11:05 AM, Takashi Iwai wrote:
>>
>>>> +#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.
>>
>>>> +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.
>>
>>>> +
>>>> +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.
>>
>>>
>>>> +
>>>> +	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.
>>
>>>
>>>> +	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.
>>
>>>
>>>> +			rc = -EBUSY;
>>>
>>> Wrong indentation.
>>
>> Yes. Will fix that.
>>
>>>
>>> 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.

> 
> 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.

> 
> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH v2 1/6] media: add media token device resource framework

[Hans Verkuil]

Hi Shuah,

As promised, here is my review for this patch series.

On 10/14/2014 04:58 PM, Shuah Khan wrote:
> 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.

> 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 <shuahkh@osg.samsung.com>
> ---
>   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.

>   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 <hverkuil@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 <shuahkh@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 <shuahkh@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__);

dev_dbg would be more appropriate.

> +}
> +
> +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.

> +	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?

> +	} 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.

> +	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 <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [PATCH v2 1/6] media: add media token device resource framework

[Takashi Iwai]

At Tue, 21 Oct 2014 12:46:03 +0200,
Hans Verkuil wrote:
> 
> Hi Shuah,
> 
> As promised, here is my review for this patch series.
> 
> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> > 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.
> 
> > 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 <shuahkh@osg.samsung.com>
> > ---
> >   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)
> 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

Re: [PATCH v2 1/6] media: add media token device resource framework

[Hans Verkuil]

On 10/21/2014 01:51 PM, Takashi Iwai wrote:
> At Tue, 21 Oct 2014 12:46:03 +0200,
> Hans Verkuil wrote:
>>
>> Hi Shuah,
>>
>> As promised, here is my review for this patch series.
>>
>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>> 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.
>>
>>> 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 <shuahkh@osg.samsung.com>
>>> ---
>>>    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.

>
> (snip)
>> 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

Re: [PATCH v2 1/6] media: add media token device resource framework

[Takashi Iwai]

At Tue, 21 Oct 2014 13:58:59 +0200,
Hans Verkuil wrote:
> 
> 
> 
> On 10/21/2014 01:51 PM, Takashi Iwai wrote:
> > At Tue, 21 Oct 2014 12:46:03 +0200,
> > Hans Verkuil wrote:
> >>
> >> Hi Shuah,
> >>
> >> As promised, here is my review for this patch series.
> >>
> >> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> >>> 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.
> >>
> >>> 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 <shuahkh@osg.samsung.com>
> >>> ---
> >>>    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.
> 
> >
> > (snip)
> >> 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

Re: [PATCH v2 1/6] media: add media token device resource framework

[Sakari Ailus]

Hi Shuah,

A few comments below.

On Tue, Oct 14, 2014 at 08:58:37AM -0600, Shuah Khan wrote:
> 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 <shuahkh@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 <hverkuil@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 <shuahkh@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

I'd add a new Kconfig option for this, as it's only needed by a very
specific kind of drivers. It could be automatically selected by those that
need it.

> +
>  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 <shuahkh@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>

Alphabetical order, please.

> +
> +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;

Indentation.

I'd like to someone else comment on this approach in general, but I'm not
aware of better ways to do this either as there's no common device node (so
file handles can't be used either).

> +	} 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;

Indentation.

> +	} 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);

No need to print things while holding the 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);

You're calling media_get_tuner_tkn() from a number of V4L2 framework
functions that handle common and often performance critical IOCTLs.
devres_find() will loop over the entire list of resources associated with
that device. I think this would be just fine if it was done just once in a
non-performance critical time, but not every time everywhere.

> +
> +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);

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v2 2/6] media: v4l2-core changes to use media token api

[Shuah Khan]

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 <shuahkh@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

[PATCH v2 3/6] media: au0828-video changes to use media token api

[Shuah Khan]

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 <shuahkh@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

[PATCH v2 4/6] media: dvb-core changes to use media token api

[Shuah Khan]

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 <shuahkh@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

[PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

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 <shuahkh@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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Lars-Peter Clausen]

On 10/14/2014 04:58 PM, Shuah Khan wrote:
[...]
>   	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__);

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.

> +			return err;
> +		}
>   		err = start_endpoints(subs, false);
>   		if (err < 0)
>   			return err;

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> [...]
>>       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__);
> 
> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Thu, 16 Oct 2014 07:10:37 -0600,
Shuah Khan wrote:
> 
> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> > On 10/14/2014 04:58 PM, Shuah Khan wrote:
> > [...]
> >>       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__);
> > 
> > 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> At Thu, 16 Oct 2014 07:10:37 -0600,
> Shuah Khan wrote:
>>
>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>> [...]
>>>>       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__);
>>>
>>> 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.

> 
> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Thu, 16 Oct 2014 08:10:52 -0600,
Shuah Khan wrote:
> 
> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> > At Thu, 16 Oct 2014 07:10:37 -0600,
> > Shuah Khan wrote:
> >>
> >> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> >>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> >>> [...]
> >>>>       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__);
> >>>
> >>> 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?


> > 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/16/2014 08:16 AM, Takashi Iwai wrote:
> At Thu, 16 Oct 2014 08:10:52 -0600,
> Shuah Khan wrote:
>>
>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
>>> At Thu, 16 Oct 2014 07:10:37 -0600,
>>> Shuah Khan wrote:
>>>>
>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>>>> [...]
>>>>>>       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__);
>>>>>
>>>>> 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?
> 
> 
>>> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Thu, 16 Oct 2014 08:39:14 -0600,
Shuah Khan wrote:
> 
> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
> > At Thu, 16 Oct 2014 08:10:52 -0600,
> > Shuah Khan wrote:
> >>
> >> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> >>> At Thu, 16 Oct 2014 07:10:37 -0600,
> >>> Shuah Khan wrote:
> >>>>
> >>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> >>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> >>>>> [...]
> >>>>>>       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__);
> >>>>>
> >>>>> 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?
> > 
> > 
> >>> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/16/2014 08:48 AM, Takashi Iwai wrote:
> At Thu, 16 Oct 2014 08:39:14 -0600,
> Shuah Khan wrote:
>>
>> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
>>> At Thu, 16 Oct 2014 08:10:52 -0600,
>>> Shuah Khan wrote:
>>>>
>>>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
>>>>> At Thu, 16 Oct 2014 07:10:37 -0600,
>>>>> Shuah Khan wrote:
>>>>>>
>>>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
>>>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>>>>>> [...]
>>>>>>>>       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__);
>>>>>>>
>>>>>>> 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?
>>>
>>>
>>>>> 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.

> 
> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Thu, 16 Oct 2014 08:59:29 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 10/16/2014 08:48 AM, Takashi Iwai wrote:
> > At Thu, 16 Oct 2014 08:39:14 -0600,
> > Shuah Khan wrote:
> >>
> >> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
> >>> At Thu, 16 Oct 2014 08:10:52 -0600,
> >>> Shuah Khan wrote:
> >>>>
> >>>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> >>>>> At Thu, 16 Oct 2014 07:10:37 -0600,
> >>>>> Shuah Khan wrote:
> >>>>>>
> >>>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> >>>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> >>>>>>> [...]
> >>>>>>>>       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__);
> >>>>>>>
> >>>>>>> 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?
> >>>
> >>>
> >>>>> 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?

> 
> Based on what you are seeing trigger could be worse.
> 
> > 
> > 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Sat, 18 Oct 2014 20:49:58 +0200,
Mauro Carvalho Chehab wrote:
> 
> Em Thu, 16 Oct 2014 08:59:29 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
> > On 10/16/2014 08:48 AM, Takashi Iwai wrote:
> > > At Thu, 16 Oct 2014 08:39:14 -0600,
> > > Shuah Khan wrote:
> > >>
> > >> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
> > >>> At Thu, 16 Oct 2014 08:10:52 -0600,
> > >>> Shuah Khan wrote:
> > >>>>
> > >>>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> > >>>>> At Thu, 16 Oct 2014 07:10:37 -0600,
> > >>>>> Shuah Khan wrote:
> > >>>>>>
> > >>>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> > >>>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> > >>>>>>> [...]
> > >>>>>>>>       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__);
> > >>>>>>>
> > >>>>>>> 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?
> > >>>
> > >>>
> > >>>>> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Hans Verkuil]

On 10/16/2014 04:48 PM, Takashi Iwai wrote:
> At Thu, 16 Oct 2014 08:39:14 -0600,
> Shuah Khan wrote:
>>
>> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
>>> At Thu, 16 Oct 2014 08:10:52 -0600,
>>> Shuah Khan wrote:
>>>>
>>>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
>>>>> At Thu, 16 Oct 2014 07:10:37 -0600,
>>>>> Shuah Khan wrote:
>>>>>>
>>>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
>>>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
>>>>>>> [...]
>>>>>>>>        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__);
>>>>>>>
>>>>>>> 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?
>>>
>>>
>>>>> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Tue, 21 Oct 2014 17:42:51 +0200,
Hans Verkuil wrote:
> 
> 
> 
> On 10/16/2014 04:48 PM, Takashi Iwai wrote:
> > At Thu, 16 Oct 2014 08:39:14 -0600,
> > Shuah Khan wrote:
> >>
> >> On 10/16/2014 08:16 AM, Takashi Iwai wrote:
> >>> At Thu, 16 Oct 2014 08:10:52 -0600,
> >>> Shuah Khan wrote:
> >>>>
> >>>> On 10/16/2014 08:01 AM, Takashi Iwai wrote:
> >>>>> At Thu, 16 Oct 2014 07:10:37 -0600,
> >>>>> Shuah Khan wrote:
> >>>>>>
> >>>>>> On 10/16/2014 06:00 AM, Lars-Peter Clausen wrote:
> >>>>>>> On 10/14/2014 04:58 PM, Shuah Khan wrote:
> >>>>>>> [...]
> >>>>>>>>        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__);
> >>>>>>>
> >>>>>>> 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?
> >>>
> >>>
> >>>>> 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?

> 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?

> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Devin Heitmueller]

> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Tue, 21 Oct 2014 12:08:59 -0400,
Devin Heitmueller wrote:
> 
> > 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Pierre-Louis Bossart]

On 10/21/14, 11:08 AM, Devin Heitmueller wrote:
>> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Devin Heitmueller]

> 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.  :-)

> 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/22/2014 01:45 PM, Devin Heitmueller wrote:
>> 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.  :-)
> 
>> 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.
> 

Sounds like we don't have a clear direction on open/close or capture
start/stop. What I am hearing is open/close isn't acceptable for
media maintainers and capture trigger start/stop isn't acceptable
to sound maintainers. :) Fork in the road, which way do we go?

Implementation wise, supporting capture trigger start/stop approach
will be harder to maintain in longterm. It adds more variables to
the mix. Applications open sounds device from the main thread and
then create a new thread to handle streams. I can see that based
on the token hold requests that come in. So the token hold logic
will have to take that into account, leading into potential
unbalanced lock/unlock scenarios. It is not impossible to solve,
that's what I did in this patch series, but it does get complex.

What I am looking for is some consensus on let's go with an approach
and try. Doesn't matter which way we go, and how much testing I do,
I am bound to miss something and this work needs to soak for a bit in
the media experimental branch.

thanks,
-- Shuah



-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Wed, 22 Oct 2014 14:26:41 -0500
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> escreveu:

> On 10/21/14, 11:08 AM, Devin Heitmueller wrote:
> >> 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.

This could be a feature for an audio capture device that is standalone,
but for sure it isn't a feature for an audio capture device where the
audio is only available if video is also being streamed.

A V4L device with ALSA capture is a different beast than a standalone
capture port. In a simplified way, it will basically follow the following
state machine:

     +---------------+
     |     start     |
     +---------------+
       |
       |
       v
     +--------------------------------+
     |              idle              | <+
     +--------------------------------+  |
       |                ^           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
     | configuration |  |           |    |
     +---------------+  |           |    |
       |                |           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
  +> |   streaming   | -+           |    |
  |  +---------------+              |    |
  |    |                            |    |
  |    |                            |    |
  |    v                            v    |
  |  +---------------+-----------+----+  |
  +- |       1       | suspended |  2 | -+
     +---------------+-----------+----+


1) start state

This is when the V4L2 device gots probed. It checks if the hardware is
present and initializes some vars/registers, turning off everything
that can be powered down.

The tuner on put in sleep mode, analog audio/video decoders and the
dvb frontend and demux are also turned off.

2) idle state

As the device is powered off, audio won't produce anything. 

Depending on the device, reading for audio may return a sequence of 
zeros, or may even fail, as the DMA engine is not ready yet for
streaming.

Also, the audio mixer is muted, but the audio input switch is on a
random state.

2) configuration state

When V4L2 node device is opened and configured, the audio mixer will
be switched to input audio from the same source of the video stream.
The corresponding audio input is also unmuted. Almost all devices have 
at least two audio/video inputs: TV TUNER and COMPOSITE. Other devices
may also have S-VIDEO, COMPOSITE 2, RADIO TUNER, etc.

If the device is set on TUNER mode, on modern devices, a tuner firmware
will be loaded. That may require a long time. Typically, most devices
take 1/2 seconds to load a firmware, but some devices may take up to 30
seconds. The firmware may depend on the TV standard that will be used,
so this can't be loaded at driver warm up state. 

Also, the power consumption of the tuners is high (it can be ~100-200 mW
or more when powered, and ~16mW when just I2C is powered). We don't want
to keep it powered when the device is not used, as this spends battery.
Also, the life of the device reduces a lot if we keep it always powered.

During this stage, if an ALSA call is issued, it may interfere at the
device settings and/or firmware load, with can cause the audio to fail. 
On such cases, applications might need to close the V4L2 node and re-open
again.

3) streaming state

The change to this staging requires a V4L2 ioctl.

Please notice, however, that some apps will open the audio device before
the V4L2 node, while others will open it after that.

In any case, audio will only start to produce data after the V4L2 ioctl
at V4L2 that starts the DMA engine there.

After that ioctl:
 - Audio PCM capture will work;
 - The mixers will be in a good state: unmuted, and switched to the
   corresponding input as the video stream.

If the user wants to do something unusual, like mixing the composite audio
input with the tuner audio input, it can use the ALSA mixer for doing
that. Otherwise, the only part of the ALSA device that will be used is
the PCM engine.

4) streaming->stop transition

When the V4L2 device is closed (or an ioctl for stream stop is issued),
the driver will release the resources. What will be released vary from
driver to driver.

Some drivers will stop both audio and video DMA engine (as they actually
have just one DMA engine for both). Other devices will still be streaming
audio until the alsa trigger stops it.

In any case, the driver will put the tuner into sleep mode. Some drivers
do it immediately. Others will wait for a few seconds before doing that.

Ether way, from the PCM capture audio PoV, the device is now useless: it
will either not return anything, or it will just return a stream with
all zeros.

5) suspend/resume transitions

When the machine suspends to disk or memory, the DMA engine will stop, and
the devices will be powered off.

The logic of returning from suspend will depend on the previous state.

If the device were in stop pode, at resume time, it should return to 
idle state.

If the device is streaming, it should return to streaming state.
In such case, no ALSA ioctls should happen while V4L2 is suspending 
or resuming, otherwise the device will be on an unknown state, and
will only return to work after replugging the device.


> I also agree that the open/close of the alsa device is the only way to 
> control exclusion.

This will break apps, as they can open the alsa device before configuring
the stream with V4L2. They can also close alsa either before or after
stopping the V4L2 stream.

> -Pierre

-

As a reference, the state machine above came from this dot config, using
graph-easy to produce an ascii output:

digraph media {
  node [shape=record]
  suspended[label = "<streaming> 1 | <suspended> suspended | <idle> 2"]
  start -> idle
  idle -> configuration
  configuration -> streaming
  streaming -> idle
  idle -> "suspended":idle
  "suspended":idle -> idle
  streaming -> "suspended":streaming
  "suspended":streaming -> streaming
}

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

(re-sending from my other e-mail - somehow, the email I sent via
 m.chehab@samsung.com doesn't seem to be delivered at vger.kernel.org)

Em Wed, 22 Oct 2014 14:26:41 -0500
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> escreveu:

> On 10/21/14, 11:08 AM, Devin Heitmueller wrote:
> >> 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.

This could be a feature for an audio capture device that is standalone,
but for sure it isn't a feature for an audio capture device where the
audio is only available if video is also being streamed.

A V4L device with ALSA capture is a different beast than a standalone
capture port. In a simplified way, it will basically follow the following
state machine:

     +---------------+
     |     start     |
     +---------------+
       |
       |
       v
     +--------------------------------+
     |              idle              | <+
     +--------------------------------+  |
       |                ^           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
     | configuration |  |           |    |
     +---------------+  |           |    |
       |                |           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
  +> |   streaming   | -+           |    |
  |  +---------------+              |    |
  |    |                            |    |
  |    |                            |    |
  |    v                            v    |
  |  +---------------+-----------+----+  |
  +- |       1       | suspended |  2 | -+
     +---------------+-----------+----+


1) start state

This is when the V4L2 device gots probed. It checks if the hardware is
present and initializes some vars/registers, turning off everything
that can be powered down.

The tuner on put in sleep mode, analog audio/video decoders and the
dvb frontend and demux are also turned off.

2) idle state

As the device is powered off, audio won't produce anything. 

Depending on the device, reading for audio may return a sequence of 
zeros, or may even fail, as the DMA engine is not ready yet for
streaming.

Also, the audio mixer is muted, but the audio input switch is on a
random state.

2) configuration state

When V4L2 node device is opened and configured, the audio mixer will
be switched to input audio from the same source of the video stream.
The corresponding audio input is also unmuted. Almost all devices have 
at least two audio/video inputs: TV TUNER and COMPOSITE. Other devices
may also have S-VIDEO, COMPOSITE 2, RADIO TUNER, etc.

If the device is set on TUNER mode, on modern devices, a tuner firmware
will be loaded. That may require a long time. Typically, most devices
take 1/2 seconds to load a firmware, but some devices may take up to 30
seconds. The firmware may depend on the TV standard that will be used,
so this can't be loaded at driver warm up state. 

Also, the power consumption of the tuners is high (it can be ~100-200 mW
or more when powered, and ~16mW when just I2C is powered). We don't want
to keep it powered when the device is not used, as this spends battery.
Also, the life of the device reduces a lot if we keep it always powered.

During this stage, if an ALSA call is issued, it may interfere at the
device settings and/or firmware load, with can cause the audio to fail. 
On such cases, applications might need to close the V4L2 node and re-open
again.

3) streaming state

The change to this staging requires a V4L2 ioctl.

Please notice, however, that some apps will open the audio device before
the V4L2 node, while others will open it after that.

In any case, audio will only start to produce data after the V4L2 ioctl
at V4L2 that starts the DMA engine there.

After that ioctl:
 - Audio PCM capture will work;
 - The mixers will be in a good state: unmuted, and switched to the
   corresponding input as the video stream.

If the user wants to do something unusual, like mixing the composite audio
input with the tuner audio input, it can use the ALSA mixer for doing
that. Otherwise, the only part of the ALSA device that will be used is
the PCM engine.

4) streaming->stop transition

When the V4L2 device is closed (or an ioctl for stream stop is issued),
the driver will release the resources. What will be released vary from
driver to driver.

Some drivers will stop both audio and video DMA engine (as they actually
have just one DMA engine for both). Other devices will still be streaming
audio until the alsa trigger stops it.

In any case, the driver will put the tuner into sleep mode. Some drivers
do it immediately. Others will wait for a few seconds before doing that.

Ether way, from the PCM capture audio PoV, the device is now useless: it
will either not return anything, or it will just return a stream with
all zeros.

5) suspend/resume transitions

When the machine suspends to disk or memory, the DMA engine will stop, and
the devices will be powered off.

The logic of returning from suspend will depend on the previous state.

If the device were in stop pode, at resume time, it should return to 
idle state.

If the device is streaming, it should return to streaming state.
In such case, no ALSA ioctls should happen while V4L2 is suspending 
or resuming, otherwise the device will be on an unknown state, and
will only return to work after replugging the device.


> I also agree that the open/close of the alsa device is the only way to 
> control exclusion.

This will break apps, as they can open the alsa device before configuring
the stream with V4L2. They can also close alsa either before or after
stopping the V4L2 stream.

> -Pierre

-

As a reference, the state machine above came from this dot config, using
graph-easy to produce an ascii output:

digraph media {
  node [shape=record]
  suspended[label = "<streaming> 1 | <suspended> suspended | <idle> 2"]
  start -> idle
  idle -> configuration
  configuration -> streaming
  streaming -> idle
  idle -> "suspended":idle
  "suspended":idle -> idle
  streaming -> "suspended":streaming
  "suspended":streaming -> streaming
}

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

(re-sending from my third e-mail - somehow, the two emails I have at
 Samsung didn't seem to be delivering to vger.kernel.org today)

Em Wed, 22 Oct 2014 14:26:41 -0500
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> escreveu:

> On 10/21/14, 11:08 AM, Devin Heitmueller wrote:
> >> 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.

This could be a feature for an audio capture device that is standalone,
but for sure it isn't a feature for an audio capture device where the
audio is only available if video is also being streamed.

A V4L device with ALSA capture is a different beast than a standalone
capture port. In a simplified way, it will basically follow the following
state machine:

     +---------------+
     |     start     |
     +---------------+
       |
       |
       v
     +--------------------------------+
     |              idle              | <+
     +--------------------------------+  |
       |                ^           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
     | configuration |  |           |    |
     +---------------+  |           |    |
       |                |           |    |
       |                |           |    |
       v                |           |    |
     +---------------+  |           |    |
  +> |   streaming   | -+           |    |
  |  +---------------+              |    |
  |    |                            |    |
  |    |                            |    |
  |    v                            v    |
  |  +---------------+-----------+----+  |
  +- |       1       | suspended |  2 | -+
     +---------------+-----------+----+


1) start state

This is when the V4L2 device gots probed. It checks if the hardware is
present and initializes some vars/registers, turning off everything
that can be powered down.

The tuner on put in sleep mode, analog audio/video decoders and the
dvb frontend and demux are also turned off.

2) idle state

As the device is powered off, audio won't produce anything. 

Depending on the device, reading for audio may return a sequence of 
zeros, or may even fail, as the DMA engine is not ready yet for
streaming.

Also, the audio mixer is muted, but the audio input switch is on a
random state.

2) configuration state

When V4L2 node device is opened and configured, the audio mixer will
be switched to input audio from the same source of the video stream.
The corresponding audio input is also unmuted. Almost all devices have 
at least two audio/video inputs: TV TUNER and COMPOSITE. Other devices
may also have S-VIDEO, COMPOSITE 2, RADIO TUNER, etc.

If the device is set on TUNER mode, on modern devices, a tuner firmware
will be loaded. That may require a long time. Typically, most devices
take 1/2 seconds to load a firmware, but some devices may take up to 30
seconds. The firmware may depend on the TV standard that will be used,
so this can't be loaded at driver warm up state. 

Also, the power consumption of the tuners is high (it can be ~100-200 mW
or more when powered, and ~16mW when just I2C is powered). We don't want
to keep it powered when the device is not used, as this spends battery.
Also, the life of the device reduces a lot if we keep it always powered.

During this stage, if an ALSA call is issued, it may interfere at the
device settings and/or firmware load, with can cause the audio to fail. 
On such cases, applications might need to close the V4L2 node and re-open
again.

3) streaming state

The change to this staging requires a V4L2 ioctl.

Please notice, however, that some apps will open the audio device before
the V4L2 node, while others will open it after that.

In any case, audio will only start to produce data after the V4L2 ioctl
at V4L2 that starts the DMA engine there.

After that ioctl:
 - Audio PCM capture will work;
 - The mixers will be in a good state: unmuted, and switched to the
   corresponding input as the video stream.

If the user wants to do something unusual, like mixing the composite audio
input with the tuner audio input, it can use the ALSA mixer for doing
that. Otherwise, the only part of the ALSA device that will be used is
the PCM engine.

4) streaming->stop transition

When the V4L2 device is closed (or an ioctl for stream stop is issued),
the driver will release the resources. What will be released vary from
driver to driver.

Some drivers will stop both audio and video DMA engine (as they actually
have just one DMA engine for both). Other devices will still be streaming
audio until the alsa trigger stops it.

In any case, the driver will put the tuner into sleep mode. Some drivers
do it immediately. Others will wait for a few seconds before doing that.

Ether way, from the PCM capture audio PoV, the device is now useless: it
will either not return anything, or it will just return a stream with
all zeros.

5) suspend/resume transitions

When the machine suspends to disk or memory, the DMA engine will stop, and
the devices will be powered off.

The logic of returning from suspend will depend on the previous state.

If the device were in stop pode, at resume time, it should return to 
idle state.

If the device is streaming, it should return to streaming state.
In such case, no ALSA ioctls should happen while V4L2 is suspending 
or resuming, otherwise the device will be on an unknown state, and
will only return to work after replugging the device.


> I also agree that the open/close of the alsa device is the only way to 
> control exclusion.

This will break apps, as they can open the alsa device before configuring
the stream with V4L2. They can also close alsa either before or after
stopping the V4L2 stream.

> -Pierre

-

As a reference, the state machine above came from this dot config, using
graph-easy to produce an ascii output:

digraph media {
  node [shape=record]
  suspended[label = "<streaming> 1 | <suspended> suspended | <idle> 2"]
  start -> idle
  idle -> configuration
  configuration -> streaming
  streaming -> idle
  idle -> "suspended":idle
  "suspended":idle -> idle
  streaming -> "suspended":streaming
  "suspended":streaming -> streaming
}

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Takashi Iwai]

At Sat, 25 Oct 2014 11:41:15 -0200,
Mauro Carvalho Chehab wrote:
> 
> (re-sending from my third e-mail - somehow, the two emails I have at
>  Samsung didn't seem to be delivering to vger.kernel.org today)
> 
> Em Wed, 22 Oct 2014 14:26:41 -0500
> Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> escreveu:
> 
> > On 10/21/14, 11:08 AM, Devin Heitmueller wrote:
> > >> 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.
> 
> This could be a feature for an audio capture device that is standalone,
> but for sure it isn't a feature for an audio capture device where the
> audio is only available if video is also being streamed.
> 
> A V4L device with ALSA capture is a different beast than a standalone
> capture port. In a simplified way, it will basically follow the following
> state machine:
> 
>      +---------------+
>      |     start     |
>      +---------------+
>        |
>        |
>        v
>      +--------------------------------+
>      |              idle              | <+
>      +--------------------------------+  |
>        |                ^           |    |
>        |                |           |    |
>        v                |           |    |
>      +---------------+  |           |    |
>      | configuration |  |           |    |
>      +---------------+  |           |    |
>        |                |           |    |
>        |                |           |    |
>        v                |           |    |
>      +---------------+  |           |    |
>   +> |   streaming   | -+           |    |
>   |  +---------------+              |    |
>   |    |                            |    |
>   |    |                            |    |
>   |    v                            v    |
>   |  +---------------+-----------+----+  |
>   +- |       1       | suspended |  2 | -+
>      +---------------+-----------+----+
> 
> 
> 1) start state
> 
> This is when the V4L2 device gots probed. It checks if the hardware is
> present and initializes some vars/registers, turning off everything
> that can be powered down.
> 
> The tuner on put in sleep mode, analog audio/video decoders and the
> dvb frontend and demux are also turned off.
> 
> 2) idle state
> 
> As the device is powered off, audio won't produce anything. 
> 
> Depending on the device, reading for audio may return a sequence of 
> zeros, or may even fail, as the DMA engine is not ready yet for
> streaming.
> 
> Also, the audio mixer is muted, but the audio input switch is on a
> random state.
> 
> 2) configuration state
> 
> When V4L2 node device is opened and configured, the audio mixer will
> be switched to input audio from the same source of the video stream.
> The corresponding audio input is also unmuted. Almost all devices have 
> at least two audio/video inputs: TV TUNER and COMPOSITE. Other devices
> may also have S-VIDEO, COMPOSITE 2, RADIO TUNER, etc.
> 
> If the device is set on TUNER mode, on modern devices, a tuner firmware
> will be loaded. That may require a long time. Typically, most devices
> take 1/2 seconds to load a firmware, but some devices may take up to 30
> seconds. The firmware may depend on the TV standard that will be used,
> so this can't be loaded at driver warm up state. 
> 
> Also, the power consumption of the tuners is high (it can be ~100-200 mW
> or more when powered, and ~16mW when just I2C is powered). We don't want
> to keep it powered when the device is not used, as this spends battery.
> Also, the life of the device reduces a lot if we keep it always powered.
> 
> During this stage, if an ALSA call is issued, it may interfere at the
> device settings and/or firmware load, with can cause the audio to fail. 
> On such cases, applications might need to close the V4L2 node and re-open
> again.
> 
> 3) streaming state
> 
> The change to this staging requires a V4L2 ioctl.
> 
> Please notice, however, that some apps will open the audio device before
> the V4L2 node, while others will open it after that.
> 
> In any case, audio will only start to produce data after the V4L2 ioctl
> at V4L2 that starts the DMA engine there.
> 
> After that ioctl:
>  - Audio PCM capture will work;
>  - The mixers will be in a good state: unmuted, and switched to the
>    corresponding input as the video stream.
> 
> If the user wants to do something unusual, like mixing the composite audio
> input with the tuner audio input, it can use the ALSA mixer for doing
> that. Otherwise, the only part of the ALSA device that will be used is
> the PCM engine.
> 
> 4) streaming->stop transition
> 
> When the V4L2 device is closed (or an ioctl for stream stop is issued),
> the driver will release the resources. What will be released vary from
> driver to driver.
> 
> Some drivers will stop both audio and video DMA engine (as they actually
> have just one DMA engine for both). Other devices will still be streaming
> audio until the alsa trigger stops it.
> 
> In any case, the driver will put the tuner into sleep mode. Some drivers
> do it immediately. Others will wait for a few seconds before doing that.
> 
> Ether way, from the PCM capture audio PoV, the device is now useless: it
> will either not return anything, or it will just return a stream with
> all zeros.
> 
> 5) suspend/resume transitions
> 
> When the machine suspends to disk or memory, the DMA engine will stop, and
> the devices will be powered off.
> 
> The logic of returning from suspend will depend on the previous state.
> 
> If the device were in stop pode, at resume time, it should return to 
> idle state.
> 
> If the device is streaming, it should return to streaming state.
> In such case, no ALSA ioctls should happen while V4L2 is suspending 
> or resuming, otherwise the device will be on an unknown state, and
> will only return to work after replugging the device.
> 
> 
> > I also agree that the open/close of the alsa device is the only way to 
> > control exclusion.
> 
> This will break apps, as they can open the alsa device before configuring
> the stream with V4L2. They can also close alsa either before or after
> stopping the V4L2 stream.
> 
> > -Pierre
> 
> -
> 
> As a reference, the state machine above came from this dot config, using
> graph-easy to produce an ascii output:
> 
> digraph media {
>   node [shape=record]
>   suspended[label = "<streaming> 1 | <suspended> suspended | <idle> 2"]
>   start -> idle
>   idle -> configuration
>   configuration -> streaming
>   streaming -> idle
>   idle -> "suspended":idle
>   "suspended":idle -> idle
>   streaming -> "suspended":streaming
>   "suspended":streaming -> streaming
> }

So, judging from your description, the problem isn't about the
exclusive lock, but rather implementation a kind of master/slave
devices?  Then the proposed patch doesn't look like a correct
implementation to me.

What I (and supposedly Pierre) opposed is the implementation of
exclusive lock control in spontaneous callbacks.  Especially the
trigger callback is a bad place since it's a callback that is supposed
to just trigger atomically.  In general, the only good place for
allowing user-space to *control* the exclusive lock is open/close,
unless the finer lock control is exposed.

But, reading through the argument from you guys, the intention of the
patch seems like just to raise the conflict from the hardware level to
the software level.  If so, I doubt whether such an exclusive lock is
the best way.   For example, audio stream can simply receive an error
at any time if something is wrong and reacts accordingly instead of
keeping the lock while streaming.  Then the master side (video) can
set the error flag, let the audio stream stop in the driver (if
running), and sync with it.

That said, we should go back and start discussing the design goal at
first.


thanks,

Takashi

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Sun, 26 Oct 2014 09:27:40 +0100
Takashi Iwai <tiwai@suse.de> escreveu:

> At Sat, 25 Oct 2014 11:41:15 -0200,
> Mauro Carvalho Chehab wrote:

> >      +---------------+
> >      |     start     |
> >      +---------------+
> >        |
> >        |
> >        v
> >      +--------------------------------+
> >      |              idle              | <+
> >      +--------------------------------+  |
> >        |                ^           |    |
> >        |                |           |    |
> >        v                |           |    |
> >      +---------------+  |           |    |
> >      | configuration |  |           |    |
> >      +---------------+  |           |    |
> >        |                |           |    |
> >        |                |           |    |
> >        v                |           |    |
> >      +---------------+  |           |    |
> >   +> |   streaming   | -+           |    |
> >   |  +---------------+              |    |
> >   |    |                            |    |
> >   |    |                            |    |
> >   |    v                            v    |
> >   |  +---------------+-----------+----+  |
> >   +- |       1       | suspended |  2 | -+
> >      +---------------+-----------+----+


The above diagram is actually simplified. There's an extra state
there that should be mentioned:

idle <-> DVB stream

(actually, DVB stream is a copy of the above, except that ALSA
won't have any business to do while the device is on DVB mode)

> So, judging from your description, the problem isn't about the
> exclusive lock, but rather implementation a kind of master/slave
> devices?  Then the proposed patch doesn't look like a correct
> implementation to me.

You might see it as a master/slave, except that it is not that simple.
It is really a hardware lock.

There are actually 3 (sub)drivers that may need to set the hardware:
	- ALSA driver;
	- V4L2 driver;
	- DVB driver.

The goal of the lock is to prevent that more than one driver would try 
to use a common piece of the hardware that it is already in usage
by another driver.

So, for example, the ALSA driver should not reprogram the hardware while 
the V4L2 driver is doing that.

I agree that, for V4L2/ALSA driver's interaction PoV, it could resemble
a sort of master/slave control, in the sense that ALSA capture start only
makes sense while V4L2 is at streaming state. Yet, just opening the device
or (even start capture on ALSA, for the hardware with multiple DMA engines,
like the ones that use snd-usb-audio) won't cause any harm if the V4L2 driver
is not reconfiguring the audio registers at the same time.

Also, ALSA open/close should be supported any time, as otherwise it will 
break existing applications. 

However, when the device is streaming on DVB mode, it is not possible
to stream on V4L2 mode, as there's just one DMA for both and just one
tuner. Also, ALSA capture doesn't make much sense on such case. Still,
locking on open/close may eventually break existing applications. Also,
it doesn't really make any sense to block the device to move from analog
to digital mode just because the ALSA devnodes are opened.

> What I (and supposedly Pierre) opposed is the implementation of
> exclusive lock control in spontaneous callbacks.  Especially the
> trigger callback is a bad place since it's a callback that is supposed
> to just trigger atomically.  In general, the only good place for
> allowing user-space to *control* the exclusive lock is open/close,
> unless the finer lock control is exposed.

I see your point. Then perhaps we should expose callbacks from other
parts of the ALSA core, perhaps at the logic that calls the trigger
callback at read and poll syscalls, plus the corresponding logic that is
used when the device is using the mmap syscall.

> But, reading through the argument from you guys, the intention of the
> patch seems like just to raise the conflict from the hardware level to
> the software level. 

Yes.

> If so, I doubt whether such an exclusive lock is
> the best way.   For example, audio stream can simply receive an error
> at any time if something is wrong and reacts accordingly instead of
> keeping the lock while streaming. Then the master side (video) can
> set the error flag, let the audio stream stop in the driver (if
> running), and sync with it.

Hmm... this is actually more complex than that. V4L2 driver doesn't
know if ALSA is streaming or not, or even if ALSA device node is opened
while he is touching at the hardware configuration or changing the
state. I mean: it is not an error to set the hardware. The error only
happens if ALSA and V4L2 tries to do it at the same time on an incompatible
way.

Also, this won't work for DVB, as on DVB this is really an exclusive
lock that would prevent both ALSA and V4L2 drivers to stream while in
DVB mode.

Implementing it with a lock seems to be the best approach, at least on
my eyes.

> That said, we should go back and start discussing the design goal at
> first.

Surely.

> 
> 
> thanks,
> 
> Takashi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/27/2014 06:52 AM, Mauro Carvalho Chehab wrote:
> Em Sun, 26 Oct 2014 09:27:40 +0100
> Takashi Iwai <tiwai@suse.de> escreveu:
> 

> 
> Hmm... this is actually more complex than that. V4L2 driver doesn't
> know if ALSA is streaming or not, or even if ALSA device node is opened
> while he is touching at the hardware configuration or changing the
> state. I mean: it is not an error to set the hardware. The error only
> happens if ALSA and V4L2 tries to do it at the same time on an incompatible
> way.
> 
> Also, this won't work for DVB, as on DVB this is really an exclusive
> lock that would prevent both ALSA and V4L2 drivers to stream while in
> DVB mode.
> 
> Implementing it with a lock seems to be the best approach, at least on
> my eyes.
> 
>> That said, we should go back and start discussing the design goal at
>> first.
> 
> Surely.

This is long, however, hoping it will describe the problem and
solution that is being pursued in detail:

At a higher level the problem description is:

There are 3 different device files that get created to control
tuner and audio functions on a media device. 3 drivers (dvb,
v4l2, alsa), and 3 core apis (dvb-core, v4l-core, audio) that
control the tuner and audio hardware and provide user api to
these 3 device files.

User applications, drivers and the core have no knowledge of each
other. The only thing that is common across all these drivers is
the parent device for the main usb device which is controlled by
the main usb driver.

The premise for the main design idea in this series is creating
a common construct at the parent device structure that is visible
to all drivers to act as a master access control (lock). Let's call
this media token object with two sub-tokens one for tuner and another
for audio.

Each of the apis evolved separately, hence have their own backwards
compatibility to maintain. Starting with v4l2:

v4l2 case:
Multiple v4l2 applications are allowed to open /dev/video0 in
read/write mode with no restrictions as long as the tuner is in
analog mode. v4l2 core handles conflicting requests between v4l2
applications. It doesn't have the knowledge that the tuner is in
use by a dvb and/or audio is in use. As soon as a v4l2 application
starts, digital stream glitches and audio glitches.

dvb case:
Multiple dvb applications can open the dvb device in read only mode.
As soon an application open the device read/write mode a separate
kthread is kicked off to handle the request. Only one application
can open the device in read/write mode. Similar to v4l2 case,
dvb-core doesn't have any knowledge that the tuner is in use by
v4l2 and/or audio is in use. As soon as a dvb application starts v4l2
video glitches and audio glitches.

audio case:
Same scenario is applicable to audio application. When a v4l2 or dvb
application starts, audio application gets impacted.

Problems to address:

dvb owns tuner and audio: another dvb, v4l2 app and audio app should
                          detect tuner/audio busy right away and exit.

v4l2 owns tuner and audio: another dvb and audio app should detect
                           tuner/audio busy right away and exit.
                           v4l2 app can continue to use it until it
                           tries to change the tuner/audio state.


audio owns audio: dvb and v4l2 apps should detect audio busy and exit.

Special cases:

dvb apps. access tuner and audio in exclusive mode. i.e only one dvb app.
at a time is allowed to open the device read/write mode. As dvb apps.
create threads to handle audio and video, all threads in that group
should be allowed by the higher level construct to access the tuner and
audio. dvb application will have to hold tuner and audio tokens so v4l2
and audio apps. know they are in use.

audio apps. access audio in exclusive mode. i.e only one audio app. at
a time is allowed to open the device in read/write mode. Audio apps.
create threads and thread closes and re-opens the audio device. Threads
can do this and hence something that higher level construct has to allow.
audio app. has to hold audio token so dvb and v4l2 know that it is in use.
(Note: I am not sure if I have the audio scenario right)

v4l2 apps. access tuner and audio in shared v4l2 mode. i.e several v4l2
processes and threads could use tuner and audio at the same time. The
higher level construct has to allow multiple v4l2 apps. to access and
disallow dvb and audio apps. access when they are in use by v4l2.

Adding to this, both dvb and v4l2 open audio device and make snd pcm
capture callbacks. There is no way to tell if dvb or v4l2 or audio
app is the one that is making this request. dvb app would like audio
in exclusive mode allowing only one process and its threads to access
it. v4l2 on the other hand would like audio in shared state accessible
to all v4l2 processes. If dvb-core and v4l2-core get tuner and audio
tokens at the same time, the window for having tuner token and not
getting audio token go down.

In dvb case when dvb device is opened in read/write mode, and v4l2
case when an app. tries to change the status. Audio callbacks have to
detect if audio is busy, if not which mode to request the token in.
For dvb and audio app. cases, the audio token should be requested in
exclusive mode and in v4l2 case shared mode. The logic for requesting
audio token will have to be try to get in exclusive mode, if fails,
try to get in shared mode, and if that fails give up.

Current status:
Combining patch v1 and patch v2 designs by allowing shared mode token
hold for v4l2, and deciding on where to hold audio token from
alsa driver will solve the above conflict scenarios. That said, the
question is "is this the right approach?" or are there other ways to
solve the problem. One thing is clear, we need some common higher level
construct for all the device drivers and dvb, v4l2, and audio ioctls,
callbacks etc, to detect the hardware is in use.

I do think lock/token approach has the best potential to solve the
problems. We are at this point very close to addressing conflicts.
At least the ones I am able to test.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[RFCv1] Media Token API needs - Was: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Hi Shuah,

I'm understanding that you're collecting comments to write a RFC with the
needs by the media token, right?

I'm sending you my contributions to such text. See enclosed.

I suggest to change the subject and submit this on a separate thread, after
we finish the review of such document. Anyway, I'm changing the subject
of this Thread to reflect that.

Regards,
Mauro

Em Tue, 28 Oct 2014 15:15:43 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 10/27/2014 06:52 AM, Mauro Carvalho Chehab wrote:
> > Em Sun, 26 Oct 2014 09:27:40 +0100
> > Takashi Iwai <tiwai@suse.de> escreveu:
> > 
> 
> > 
> > Hmm... this is actually more complex than that. V4L2 driver doesn't
> > know if ALSA is streaming or not, or even if ALSA device node is opened
> > while he is touching at the hardware configuration or changing the
> > state. I mean: it is not an error to set the hardware. The error only
> > happens if ALSA and V4L2 tries to do it at the same time on an incompatible
> > way.
> > 
> > Also, this won't work for DVB, as on DVB this is really an exclusive
> > lock that would prevent both ALSA and V4L2 drivers to stream while in
> > DVB mode.
> > 
> > Implementing it with a lock seems to be the best approach, at least on
> > my eyes.
> > 
> >> That said, we should go back and start discussing the design goal at
> >> first.
> > 
> > Surely.
> 
> This is long, however, hoping it will describe the problem and
> solution that is being pursued in detail:

Before starting with the description, this is the simplified diagram of
a media device (without IR, eeprom and other beasts):

  +-----------------------------------------------------------------------------------------+
  |                                                                                         |
  |                   +----------------+     +------------------+-------+----------------+  |
  |                   |  demod_video   | <-- |      analog      | tuner |     digital    |  |
  |                   +----------------+     +------------------+-------+----------------+  |
  |                     |                      |                           |                |
  |                     |                      |                           |                |
  v                     v                      v                           v                |
+--------------+-----+-----------------+     +------------------+        +---------------+  |
|     dvb      | DMA |      analog     |     |   demod_audio    |        | digital_demux | -+
+--------------+-----+-----------------+     +------------------+        +---------------+
  |                     |                      |
  |                     |                      |
  v                     v                      v
+--------------+      +----------------+     +------------------+
| devnode dvr0 |      | devnode video0 |     |    audio DMA     |
+--------------+      +----------------+     +------------------+
                                               |
                                               |
                                               v
                                             +------------------+
                                             | devnode pcmC1D0c |
                                             +------------------+

There are two components that are shared there between analog and digital:
the tuner (where the signal is captured) and the DMA engine used to stream
analog and Digital TV (dvb).

PS.: the diagram is over-simplified, as the tuner is just one of the possible
inputs for the analog part of the device. Other possible inputs are S-Video,
composite, HDMI, etc.

Sometimes, the audio DMA is also shared, e. g. just one stream comes from
the hardware. It is up to the driver to split audio and video and send
them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.

Those shared components can be used either at analog or digital mode,
but not at the same time.

Also, programming the V4L2 analog and audio DMA and demods should be done
via V4L2 API, as this API allows the selection of the proper audio/video
input (almost all devices have multiple analog inputs).

Please notice that, if the tuner is on digital mode, the entire analog
path is disabled, including ALSA output.

If the tuner is on analog mode, both ALSA and V4L2 can work at the
same time. However, during the period where the tuner firmware is
loaded, and during the DMA configuration and input selection time,
neither ALSA or V4L2 can stream. Such configuration/firmware load
is commanded via V4L2 API, as ALSA knows nothing about tuner or
input selection.

> 
> At a higher level the problem description is:
> 
> There are 3 different device files that get created to control
> tuner and audio functions on a media device. 3 drivers (dvb,
> v4l2, alsa), and 3 core apis (dvb-core, v4l-core, audio) that
> control the tuner and audio hardware and provide user api to
> these 3 device files.


There's actually a 4th component for some drivers: the mceusb driver,
that handles remote controllers. The mceusb handles the Microsoft 
Media Center Remote Control protocol. It supports standalone remote
controller devices, but it also supports a few USB devices that use
a separate interface for IR.

There are currently some issues on cx231xx and mceusb, as both drivers 
can be used at the same time, but, when cx231xx sends certain commands, 
the mceusb IR polls fail. This is out of the scope of the audio lock,
but it also needs to be addressed some day.

> User applications, drivers and the core have no knowledge of each
> other. The only thing that is common across all these drivers is
> the parent device for the main usb device which is controlled by
> the main usb driver.

I would add that there are user applications that can handle all
3 APIs like MythTV. But, at least MythTV doesn't know how to associate
ALSA, V4L2 and DVB devnodes that belong to the same device.

I mean: if MythTV finds, let's say, 3 V4L2 nodes, 3 ALSA nodes, 
and 1 DVB node, it doesn't know what device is associated with the
DVB node.

Almost all applications that are aware of V4L2 API are also aware of
ALSA API and may associate audio and video, as there is a way to 
associate it using sysfs. However, several apps don't use it.

> The premise for the main design idea in this series is creating
> a common construct at the parent device structure that is visible
> to all drivers to act as a master access control (lock). Let's call
> this media token object with two sub-tokens one for tuner and another
> for audio.
> 
> Each of the apis evolved separately, hence have their own backwards
> compatibility to maintain. Starting with v4l2:
> 
> v4l2 case:
> Multiple v4l2 applications are allowed to open /dev/video0 in
> read/write mode with no restrictions as long as the tuner is in
> analog mode. v4l2 core handles conflicting requests between v4l2
> applications. 

> It doesn't have the knowledge that the tuner is in

To be clear: "It" here refers to v4l2 core. The drivers may have this
knowledge as, except for one case (bttv driver), they share some data.

> use by a dvb and/or audio is in use. As soon as a v4l2 application
> starts, digital stream glitches and audio glitches.
> 
> dvb case:
> Multiple dvb applications can open the dvb device in read only mode.

There's no issue with ALSA on R/O mode, as the application is not
allowed to modify anything at the stream. This is used only to monitor
an already opened device in R/W mode.

> As soon an application open the device read/write mode a separate
> kthread is kicked off to handle the request. Only one application
> can open the device in read/write mode. 

> Similar to v4l2 case,

s/v4l2/v4l2 core/

> dvb-core doesn't have any knowledge that the tuner is in use by
> v4l2 and/or audio is in use. As soon as a dvb application starts v4l2
> video glitches and audio glitches.
> 
> audio case:
> Same scenario is applicable to audio application. When a v4l2 or dvb
> application starts, audio application gets impacted.
> 
> Problems to address:
> 
> dvb owns tuner and audio: another dvb, v4l2 app and audio app should
>                           detect tuner/audio busy right away and exit.
> 
> v4l2 owns tuner and audio: another dvb and audio app should detect
>                            tuner/audio busy right away and exit.

Actually, no: audio should not exit. The V4L2 should only hold the
token for the required time to initialize the device and/or load the
firmware. ALSA applications should wait for V4L2 to finish
programming at audio, and should keep working after that.

>                            v4l2 app can continue to use it until it
>                            tries to change the tuner/audio state.
> 
> 
> audio owns audio: dvb and v4l2 apps should detect audio busy and exit.

Actually no. It is, instead:

audio owns audio: dvb apps should detect audio busy and exit.
V4L2 apps should work. However, when certain V4L2 ioctls are issued, 
the audio device driver should not send any command to the hardware.
After such commands, the audio mixers may change.

We need two separate tokens because of that: the behavior is different.

This is basically why we need two separate tokens, and because we cannot
implement locking at ALSA open/close.

> 
> Special cases:
> 
> dvb apps. access tuner and audio in exclusive mode. i.e only one dvb app.
> at a time is allowed to open the device read/write mode.

To be clearer: dvb apps won't use the audio node, but audio should be blocked,
as the devices can't use audio while in DVB mode.

> As dvb apps.
> create threads to handle audio and video, 

No. DVB apps don't handle audio/video. It receives data as MPEG-TS,
using a separate device node. Yet, the same DMA engine that provides
video (and, sometimes audio) is used by the DVB devnode.

> all threads in that group
> should be allowed by the higher level construct to access the tuner and
> audio. dvb application will have to hold tuner and audio tokens so v4l2
> and audio apps. know they are in use.
> 
> audio apps. access audio in exclusive mode. i.e only one audio app. at
> a time is allowed to open the device in read/write mode. Audio apps.
> create threads and thread closes and re-opens the audio device. Threads
> can do this and hence something that higher level construct has to allow.
> audio app. has to hold audio token so dvb and v4l2 know that it is in use.
> (Note: I am not sure if I have the audio scenario right)
> 
> v4l2 apps. access tuner and audio in shared v4l2 mode. i.e several v4l2
> processes and threads could use tuner and audio at the same time. The
> higher level construct has to allow multiple v4l2 apps. to access and
> disallow dvb and audio apps. access when they are in use by v4l2.

Actually, V4L2 core handles concurrency. There's just one file handler
with full control to start/stop stream at V4L2 side.

> 
> Adding to this, both dvb and v4l2 open audio device and make snd pcm
> capture callbacks.

Huh? DVB won't need to touch at PCM capture callbacks. It should just
avoid audio PCM capture to stream while in DVB mode.

> There is no way to tell if dvb or v4l2 or audio
> app is the one that is making this request.

> dvb app would like audio
> in exclusive mode allowing only one process and its threads to access
> it.

No. It just wants to disable the part of the hardware that can now
be powered off.

> v4l2 on the other hand would like audio in shared state accessible
> to all v4l2 processes. 

> If dvb-core and v4l2-core get tuner and audio
> tokens at the same time, the window for having tuner token and not
> getting audio token go down.

No. It should not be allowed that both dvb-core and v4l2-core to get
the tokens at the same time. This is an exclusive lock.

> In dvb case when dvb device is opened in read/write mode, and v4l2
> case when an app. tries to change the status. Audio callbacks have to
> detect if audio is busy, if not which mode to request the token in.

Huh?

> For dvb and audio app. cases, the audio token should be requested in
> exclusive mode and in v4l2 case shared mode. The logic for requesting
> audio token will have to be try to get in exclusive mode, if fails,
> try to get in shared mode, and if that fails give up.

Huh?

> 
> Current status:
> Combining patch v1 and patch v2 designs by allowing shared mode token
> hold for v4l2, and deciding on where to hold audio token from
> alsa driver will solve the above conflict scenarios. That said, the
> question is "is this the right approach?" or are there other ways to
> solve the problem. One thing is clear, we need some common higher level
> construct for all the device drivers and dvb, v4l2, and audio ioctls,
> callbacks etc, to detect the hardware is in use.

I think that the current status is that we need to finish the spec
first. Then check if the patches are doing what's above.

It seems that we agree to not agree at the requirements so far ;)

> 
> I do think lock/token approach has the best potential to solve the
> problems. We are at this point very close to addressing conflicts.
> At least the ones I am able to test.
> 
> thanks,
> -- Shuah

Regards,
Mauro

Re: [RFCv1] Media Token API needs - Was: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Tue, 28 Oct 2014 21:42:50 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Before starting with the description, this is the simplified diagram of
> a media device (without IR, eeprom and other beasts):

As reference, a more complete diagram would be:



                                                                +------------------+
                                                                |        IR        |
                                                                +------------------+
                                                                  |
                                                                  |
                                                                  v
                                                                +------------------+
                                                                |  IR POLL or DMA  |
                                                                +------------------+
                                                                  |
                                                                  |
                                                                  v
                                                                +------------------+
                                                                |  devnode input8  |
                                                                +------------------+

                             +------------------------------------+
                             v                                    |
                           +----------------+          +-------+-------------------+-----------+
                           |  demod_video   |  +-----> | tuner |     A/V switch    | composite |
                           +----------------+  |       +-------+-------------------+-----------+
                             |                 |                  |
                             |                 |                  |
                             v                 |                  v
     +--------------+-----+-----------------+  |                +------------------+
  +- |     dvb      | DMA |      analog     |  |                |   demod_audio    |
  |  +--------------+-----+-----------------+  |                +------------------+
  |    ^                     |                 |                  |
  |    +---------------+     |                 |                  |
  |                    |     v                 |                  v
  |  +--------------+  |   +----------------+  |                +------------------+
  +> | devnode dvr0 |  |   | devnode video0 |  |                |    audio DMA     |
     +--------------+  |   +----------------+  |                +------------------+
                       |                       |                  |
                       |                       |                  |
                       |                       |                  v
                       |                       |                +------------------+
                       |                       |                | devnode pcmC1D0c |
                       |                       |                +------------------+
                       |                       |
                       |                       |
                       |                       |
                       |                     +--------+--------+-------------------+
                       |                     | analog |  tuner |      digital      |
                       |                     +--------+--------+-------------------+
                       |                                          |
                       |                                          |
                       |                                          v
                       |                                        +------------------+
                       +--------------------------------------- |  digital_demux   |
                                                                +------------------+

Regards,
Mauro

-

Dot file for the above diagram:

digraph media_hardware {
  node [shape=record]
  DMA1[label = "<dvb> dvb | <dma> DMA | <video> analog"]
  DMA2[label = "audio DMA"]
  DMA3[label = "IR POLL or DMA"]
  tuner[label = "<analog> analog | <tuner> tuner | <digital> digital"]
  input[label = "<tuner> tuner | <switch> A/V switch | <composite> composite"]

  DMA1:video -> "devnode video0"
  DMA1:dvb -> "devnode dvr0"

  "digital_demux" -> DMA1:dvb
  demod_video -> DMA1:video

  tuner:digital -> "digital_demux"
  tuner:analog -> input:tuner

  input:switch -> demod_video
  input:switch -> demod_audio

  demod_audio -> DMA2
  DMA2 -> "devnode pcmC1D0c"

  IR -> DMA3
  DMA3 -> "devnode input8"
}

Re: [RFCv1] Media Token API needs - Was: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Tue, 28 Oct 2014 22:00:51 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Tue, 28 Oct 2014 21:42:50 -0200
> Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:
> 
> > Before starting with the description, this is the simplified diagram of
> > a media device (without IR, eeprom and other beasts):
> 
> As reference, a more complete diagram would be:

An even more complete (but still simplified) diagram is shown at:
	http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png

The dot lines represent the parts of the graph that are switched by
the tuner, DMA or input select.

Please notice that the DMA engines, together with the stuff needed to
control A/V switches is at one single chip. Changing the registers there
can affect the other streams, specially on most sophisticated devices like
cx231xx, where it even has a power management IP block that validades if
a device to be turned on/off won't exceed the maximum drain current of
500mA. That's basically why we need to do a temporary lock alsa, dvb, v4l
and IR drivers when doing certain changes.

Also, please notice that I2C buses that can be as slow as 10kbps 
are used to control for several devices, like:
	- the tuner
	- the Digital TV (DTV) demod
	- Analog and/or Video demod (sometimes embedded at the main chip)
	- DTV demux (sometimes embedded at the main chip)
	- The remote controller (sometimes embedded at the main chip)

For some devices, after powered on, or when certain parameters change, a
new firmware (and sometimes a hardware reset) is required. The firmware
size can be about 64KB or even bigger.

Also, the A/V switch it is actually two independent switches (or one
switch for video and one audio mux for audio) that needs to be changed 
together when the source changes.

Regards,
Mauro

For those curious enough or that are using mutt/pine this is the graph,
in text mode, manually adjusted to fit into 80 cols, and with a link
added by hand, as graph-easy failed to represent everything on this
graph:

                                              +------------------+
                                              |        IR        |
                                              +------------------+
                                                |
                                                |
                                                v
                                              +------------------+
                                              |  IR POLL or DMA  |
                                              +------------------+
                                                |
                                                |
                                                v
                                              +------------------+
                                              |  devnode input8  |
                                              +------------------+

                           ..................................................
                           :                                                :
                           :              +-------------+-----------+---------+
                           :              |  digital    |   tuner   | analog  |
                           :              +-------------+-----------+---------+
                           :                :
                           :                : DTV IF
                           :                : off on A/V
                           :                v
                           :              +------------------+
                           :              |    DTV demod     |
                           :              +------------------+
                           :                |
                           :                | MPEG-TS
                           :                | off on A/V
                           :                v
                           :              +------------------+
                           :              |      demux       |
                           :              +------------------+
                           :                |
                           :                | MPEG-TS
                           :                | off on A/V
                           :                v
                           :  +---------------+---------+-------------+
                           :  |    dvb        |   DMA   |    analog   | <----+
                           :  +---------------+---------+-------------+      |
                           :    :                          :                 |
                           :    : MPEG-TS     Video stream :    Video stream | 
                           :    : off on A/V    off on DTV :      off on DTV |
                           :    v                          v                 |
                           :  +------------------+       +----------------+  |
                ATV IF     :  |                  |       |                |  |
                off on DTV :  |   devnode dvr0   |       | devnode video0 |  |
          off on Composite :  |                  |       |                |  |
                           :  +------------------+       +----------------+  |
                           :                                                 |
                      +----+-------------------------------------------------+
                      |    v
+-------------+       |  +---------+------------+-----------+     +-----------+
| video demod | ------+  |  tuner  | A/V switch | composite | <-- | comp. src |
+-------------+          +---------+------------+-----------+     +-----------+
        ^                               :      :
        :                               :      : Audio IF
        .................................      : off on DTV
               Video IF                        v
               off on DTV                    +------------------+
                                             |   audio demod    |
                                             +------------------+
                                                |
                                                | Audio stream
                                                | off on DTV
                                                v
                                              +------------------+
                                              |    audio DMA     |
                                              +------------------+
                                                |
                                                | Audio stream
                                                | off on DTV
                                                v
                                              +------------------+
                                              | devnode pcmC1D0c |
                                              +------------------+

And this is the dot file to produce it:

digraph media_hardware {
  node [shape=record]
  DMA1[label = "<dvb> dvb | <dma> DMA | <video> analog"]
  DMA2[label = "audio DMA"]
  DMA3[label = "IR POLL or DMA"]
  tuner[label = "<digital> digital | <tuner> tuner | <analog> analog"]
  input[label = "<tuner> tuner | <switch> A/V switch | <composite> composite"]

  "composite src" -> input:composite

  DMA1:video -> "devnode video0" [style=dotted label="Video stream\noff on DTV"]
  DMA1:dvb -> "devnode dvr0" [style=dotted label="MPEG-TS\noff on A/V"]

  "demux" -> DMA1:dvb [label="MPEG-TS\noff on A/V"]
  "video demod" -> DMA1:video [label="Video stream\noff on DTV"]

  tuner:digital -> "DTV demod" [style=dotted label="DTV IF\noff on A/V"]
  "DTV demod" -> "demux" [label="MPEG-TS\noff on A/V"]
  tuner:analog -> input:tuner [style=dotted label="ATV IF\noff on DTV\noff on Composite"]

  input:switch -> "video demod" [style=dotted label="Video IF\noff on DTV"]
  input:switch -> "audio demod" [style=dotted label="Audio IF\noff on DTV"]

  "audio demod" -> DMA2  [label="Audio stream\noff on DTV"];
  DMA2 -> "devnode pcmC1D0c" [label="Audio stream\noff on DTV"];

  IR -> DMA3
  DMA3 -> "devnode input8"
}

Re: [RFCv1] Media Token API needs - Was: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/28/2014 05:42 PM, Mauro Carvalho Chehab wrote:
> Hi Shuah,
> 
> I'm understanding that you're collecting comments to write a RFC with the
> needs by the media token, right?
> 
> I'm sending you my contributions to such text. See enclosed.
> 
> I suggest to change the subject and submit this on a separate thread, after
> we finish the review of such document. Anyway, I'm changing the subject
> of this Thread to reflect that.
> 
> Regards,
> Mauro
> 
> Em Tue, 28 Oct 2014 15:15:43 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> On 10/27/2014 06:52 AM, Mauro Carvalho Chehab wrote:
>>> Em Sun, 26 Oct 2014 09:27:40 +0100
>>> Takashi Iwai <tiwai@suse.de> escreveu:
>>>
>>
>>>
>>> Hmm... this is actually more complex than that. V4L2 driver doesn't
>>> know if ALSA is streaming or not, or even if ALSA device node is opened
>>> while he is touching at the hardware configuration or changing the
>>> state. I mean: it is not an error to set the hardware. The error only
>>> happens if ALSA and V4L2 tries to do it at the same time on an incompatible
>>> way.
>>>
>>> Also, this won't work for DVB, as on DVB this is really an exclusive
>>> lock that would prevent both ALSA and V4L2 drivers to stream while in
>>> DVB mode.
>>>
>>> Implementing it with a lock seems to be the best approach, at least on
>>> my eyes.
>>>
>>>> That said, we should go back and start discussing the design goal at
>>>> first.
>>>
>>> Surely.
>>
>> This is long, however, hoping it will describe the problem and
>> solution that is being pursued in detail:
> 
> Before starting with the description, this is the simplified diagram of
> a media device (without IR, eeprom and other beasts):
> 
>   +-----------------------------------------------------------------------------------------+
>   |                                                                                         |
>   |                   +----------------+     +------------------+-------+----------------+  |
>   |                   |  demod_video   | <-- |      analog      | tuner |     digital    |  |
>   |                   +----------------+     +------------------+-------+----------------+  |
>   |                     |                      |                           |                |
>   |                     |                      |                           |                |
>   v                     v                      v                           v                |
> +--------------+-----+-----------------+     +------------------+        +---------------+  |
> |     dvb      | DMA |      analog     |     |   demod_audio    |        | digital_demux | -+
> +--------------+-----+-----------------+     +------------------+        +---------------+
>   |                     |                      |
>   |                     |                      |
>   v                     v                      v
> +--------------+      +----------------+     +------------------+
> | devnode dvr0 |      | devnode video0 |     |    audio DMA     |
> +--------------+      +----------------+     +------------------+
>                                                |
>                                                |
>                                                v
>                                              +------------------+
>                                              | devnode pcmC1D0c |
>                                              +------------------+
> 
> There are two components that are shared there between analog and digital:
> the tuner (where the signal is captured) and the DMA engine used to stream
> analog and Digital TV (dvb).
> 
> PS.: the diagram is over-simplified, as the tuner is just one of the possible
> inputs for the analog part of the device. Other possible inputs are S-Video,
> composite, HDMI, etc.
> 
> Sometimes, the audio DMA is also shared, e. g. just one stream comes from
> the hardware. It is up to the driver to split audio and video and send
> them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.
> 
> Those shared components can be used either at analog or digital mode,
> but not at the same time.
> 
> Also, programming the V4L2 analog and audio DMA and demods should be done
> via V4L2 API, as this API allows the selection of the proper audio/video
> input (almost all devices have multiple analog inputs).
> 
> Please notice that, if the tuner is on digital mode, the entire analog
> path is disabled, including ALSA output.
> 
> If the tuner is on analog mode, both ALSA and V4L2 can work at the
> same time. However, during the period where the tuner firmware is
> loaded, and during the DMA configuration and input selection time,
> neither ALSA or V4L2 can stream. Such configuration/firmware load
> is commanded via V4L2 API, as ALSA knows nothing about tuner or
> input selection.
> 
>>
>> At a higher level the problem description is:
>>
>> There are 3 different device files that get created to control
>> tuner and audio functions on a media device. 3 drivers (dvb,
>> v4l2, alsa), and 3 core apis (dvb-core, v4l-core, audio) that
>> control the tuner and audio hardware and provide user api to
>> these 3 device files.
> 
> 
> There's actually a 4th component for some drivers: the mceusb driver,
> that handles remote controllers. The mceusb handles the Microsoft 
> Media Center Remote Control protocol. It supports standalone remote
> controller devices, but it also supports a few USB devices that use
> a separate interface for IR.
> 
> There are currently some issues on cx231xx and mceusb, as both drivers 
> can be used at the same time, but, when cx231xx sends certain commands, 
> the mceusb IR polls fail. This is out of the scope of the audio lock,
> but it also needs to be addressed some day.
> 
>> User applications, drivers and the core have no knowledge of each
>> other. The only thing that is common across all these drivers is
>> the parent device for the main usb device which is controlled by
>> the main usb driver.
> 
> I would add that there are user applications that can handle all
> 3 APIs like MythTV. But, at least MythTV doesn't know how to associate
> ALSA, V4L2 and DVB devnodes that belong to the same device.
> 
> I mean: if MythTV finds, let's say, 3 V4L2 nodes, 3 ALSA nodes, 
> and 1 DVB node, it doesn't know what device is associated with the
> DVB node.
> 
> Almost all applications that are aware of V4L2 API are also aware of
> ALSA API and may associate audio and video, as there is a way to 
> associate it using sysfs. However, several apps don't use it.
> 
>> The premise for the main design idea in this series is creating
>> a common construct at the parent device structure that is visible
>> to all drivers to act as a master access control (lock). Let's call
>> this media token object with two sub-tokens one for tuner and another
>> for audio.
>>
>> Each of the apis evolved separately, hence have their own backwards
>> compatibility to maintain. Starting with v4l2:
>>
>> v4l2 case:
>> Multiple v4l2 applications are allowed to open /dev/video0 in
>> read/write mode with no restrictions as long as the tuner is in
>> analog mode. v4l2 core handles conflicting requests between v4l2
>> applications. 
> 
>> It doesn't have the knowledge that the tuner is in
> 
> To be clear: "It" here refers to v4l2 core. The drivers may have this
> knowledge as, except for one case (bttv driver), they share some data.
> 
>> use by a dvb and/or audio is in use. As soon as a v4l2 application
>> starts, digital stream glitches and audio glitches.
>>
>> dvb case:
>> Multiple dvb applications can open the dvb device in read only mode.
> 
> There's no issue with ALSA on R/O mode, as the application is not
> allowed to modify anything at the stream. This is used only to monitor
> an already opened device in R/W mode.
> 
>> As soon an application open the device read/write mode a separate
>> kthread is kicked off to handle the request. Only one application
>> can open the device in read/write mode. 
> 
>> Similar to v4l2 case,
> 
> s/v4l2/v4l2 core/
> 
>> dvb-core doesn't have any knowledge that the tuner is in use by
>> v4l2 and/or audio is in use. As soon as a dvb application starts v4l2
>> video glitches and audio glitches.
>>
>> audio case:
>> Same scenario is applicable to audio application. When a v4l2 or dvb
>> application starts, audio application gets impacted.
>>
>> Problems to address:
>>
>> dvb owns tuner and audio: another dvb, v4l2 app and audio app should
>>                           detect tuner/audio busy right away and exit.
>>
>> v4l2 owns tuner and audio: another dvb and audio app should detect
>>                            tuner/audio busy right away and exit.
> 
> Actually, no: audio should not exit. The V4L2 should only hold the
> token for the required time to initialize the device and/or load the
> firmware. ALSA applications should wait for V4L2 to finish
> programming at audio, and should keep working after that.
> 
>>                            v4l2 app can continue to use it until it
>>                            tries to change the tuner/audio state.
>>
>>
>> audio owns audio: dvb and v4l2 apps should detect audio busy and exit.
> 
> Actually no. It is, instead:
> 
> audio owns audio: dvb apps should detect audio busy and exit.
> V4L2 apps should work. However, when certain V4L2 ioctls are issued, 
> the audio device driver should not send any command to the hardware.
> After such commands, the audio mixers may change.
> 
> We need two separate tokens because of that: the behavior is different.
> 
> This is basically why we need two separate tokens, and because we cannot
> implement locking at ALSA open/close.
> 
>>
>> Special cases:
>>
>> dvb apps. access tuner and audio in exclusive mode. i.e only one dvb app.
>> at a time is allowed to open the device read/write mode.
> 
> To be clearer: dvb apps won't use the audio node, but audio should be blocked,
> as the devices can't use audio while in DVB mode.
> 
>> As dvb apps.
>> create threads to handle audio and video, 
> 
> No. DVB apps don't handle audio/video. It receives data as MPEG-TS,
> using a separate device node. Yet, the same DMA engine that provides
> video (and, sometimes audio) is used by the DVB devnode.
> 
>> all threads in that group
>> should be allowed by the higher level construct to access the tuner and
>> audio. dvb application will have to hold tuner and audio tokens so v4l2
>> and audio apps. know they are in use.
>>
>> audio apps. access audio in exclusive mode. i.e only one audio app. at
>> a time is allowed to open the device in read/write mode. Audio apps.
>> create threads and thread closes and re-opens the audio device. Threads
>> can do this and hence something that higher level construct has to allow.
>> audio app. has to hold audio token so dvb and v4l2 know that it is in use.
>> (Note: I am not sure if I have the audio scenario right)
>>
>> v4l2 apps. access tuner and audio in shared v4l2 mode. i.e several v4l2
>> processes and threads could use tuner and audio at the same time. The
>> higher level construct has to allow multiple v4l2 apps. to access and
>> disallow dvb and audio apps. access when they are in use by v4l2.
> 
> Actually, V4L2 core handles concurrency. There's just one file handler
> with full control to start/stop stream at V4L2 side.
> 
>>
>> Adding to this, both dvb and v4l2 open audio device and make snd pcm
>> capture callbacks.
> 
> Huh? DVB won't need to touch at PCM capture callbacks. It should just
> avoid audio PCM capture to stream while in DVB mode.
> 
>> There is no way to tell if dvb or v4l2 or audio
>> app is the one that is making this request.
> 
>> dvb app would like audio
>> in exclusive mode allowing only one process and its threads to access
>> it.
> 
> No. It just wants to disable the part of the hardware that can now
> be powered off.

Hmm. I am seeing some snd_pcm_lib_ioctls coming from dvb application.
>From what you are saying, these could be for poweroff.

> 
>> v4l2 on the other hand would like audio in shared state accessible
>> to all v4l2 processes. 
> 
>> If dvb-core and v4l2-core get tuner and audio
>> tokens at the same time, the window for having tuner token and not
>> getting audio token go down.
> 
> No. It should not be allowed that both dvb-core and v4l2-core to get
> the tokens at the same time. This is an exclusive lock.

I should have explained this better. What I meant was that dvb-core when
it gets the tuner, it should also obtain audio right away. v4l2-core
when it gets the tuner, it should get the audio at the same time. When
dvb-core has the tuner, v4l2 shouldn't get it and vice versa.

> 
>> In dvb case when dvb device is opened in read/write mode, and v4l2
>> case when an app. tries to change the status. Audio callbacks have to
>> detect if audio is busy, if not which mode to request the token in.
> 
> Huh?

ok again bad explaining on my part. Let me try again. When dvb-core has
the audio locked, audio application should detect the condition and take
appropriate action. When v4l2-core has audio locked, audio application
should detect the condition and take appropriate action.

> 
>> For dvb and audio app. cases, the audio token should be requested in
>> exclusive mode and in v4l2 case shared mode. The logic for requesting
>> audio token will have to be try to get in exclusive mode, if fails,
>> try to get in shared mode, and if that fails give up.
> 
> Huh?

Consider this flow:

step 1: dvb-core locks tuner and audio.
step 2: audio ioctl is initiated (from the application)
step 3: If dvb-core has the audio locked, how does alsa know if it can
        proceed with the ioctl request or not?

> 
>>
>> Current status:
>> Combining patch v1 and patch v2 designs by allowing shared mode token
>> hold for v4l2, and deciding on where to hold audio token from
>> alsa driver will solve the above conflict scenarios. That said, the
>> question is "is this the right approach?" or are there other ways to
>> solve the problem. One thing is clear, we need some common higher level
>> construct for all the device drivers and dvb, v4l2, and audio ioctls,
>> callbacks etc, to detect the hardware is in use.
> 
> I think that the current status is that we need to finish the spec
> first. Then check if the patches are doing what's above.
> 
> It seems that we agree to not agree at the requirements so far ;)

I would say it is more of not having the same understanding of
the requirements as opposed to wanting to agree to disagree.

Right I agree that developing a clear RFC spec will help us all be
on the same page while I continue to find solution for this problem,
and continuing work towards patch v3.

I will compile what we discussed so far in RFC and send it out for
review in a day or two. I will include your diagrams and the scenarios
I put together with your corrections in it.

-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [RFCv1] Media Token API needs - Was: Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Mauro Carvalho Chehab]

Em Wed, 29 Oct 2014 10:06:51 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 10/28/2014 05:42 PM, Mauro Carvalho Chehab wrote:
> > Hi Shuah,
> > 
> > I'm understanding that you're collecting comments to write a RFC with the
> > needs by the media token, right?
> > 
> > I'm sending you my contributions to such text. See enclosed.
> > 
> > I suggest to change the subject and submit this on a separate thread, after
> > we finish the review of such document. Anyway, I'm changing the subject
> > of this Thread to reflect that.
> > 
> > Regards,
> > Mauro
> > 
> > Em Tue, 28 Oct 2014 15:15:43 -0600
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> > 
> >> On 10/27/2014 06:52 AM, Mauro Carvalho Chehab wrote:
> >>> Em Sun, 26 Oct 2014 09:27:40 +0100
> >>> Takashi Iwai <tiwai@suse.de> escreveu:
> >>>
> >>
> >>>
> >>> Hmm... this is actually more complex than that. V4L2 driver doesn't
> >>> know if ALSA is streaming or not, or even if ALSA device node is opened
> >>> while he is touching at the hardware configuration or changing the
> >>> state. I mean: it is not an error to set the hardware. The error only
> >>> happens if ALSA and V4L2 tries to do it at the same time on an incompatible
> >>> way.
> >>>
> >>> Also, this won't work for DVB, as on DVB this is really an exclusive
> >>> lock that would prevent both ALSA and V4L2 drivers to stream while in
> >>> DVB mode.
> >>>
> >>> Implementing it with a lock seems to be the best approach, at least on
> >>> my eyes.
> >>>
> >>>> That said, we should go back and start discussing the design goal at
> >>>> first.
> >>>
> >>> Surely.
> >>
> >> This is long, however, hoping it will describe the problem and
> >> solution that is being pursued in detail:
> > 
> > Before starting with the description, this is the simplified diagram of
> > a media device (without IR, eeprom and other beasts):
> > 
> >   +-----------------------------------------------------------------------------------------+
> >   |                                                                                         |
> >   |                   +----------------+     +------------------+-------+----------------+  |
> >   |                   |  demod_video   | <-- |      analog      | tuner |     digital    |  |
> >   |                   +----------------+     +------------------+-------+----------------+  |
> >   |                     |                      |                           |                |
> >   |                     |                      |                           |                |
> >   v                     v                      v                           v                |
> > +--------------+-----+-----------------+     +------------------+        +---------------+  |
> > |     dvb      | DMA |      analog     |     |   demod_audio    |        | digital_demux | -+
> > +--------------+-----+-----------------+     +------------------+        +---------------+
> >   |                     |                      |
> >   |                     |                      |
> >   v                     v                      v
> > +--------------+      +----------------+     +------------------+
> > | devnode dvr0 |      | devnode video0 |     |    audio DMA     |
> > +--------------+      +----------------+     +------------------+
> >                                                |
> >                                                |
> >                                                v
> >                                              +------------------+
> >                                              | devnode pcmC1D0c |
> >                                              +------------------+
> > 
> > There are two components that are shared there between analog and digital:
> > the tuner (where the signal is captured) and the DMA engine used to stream
> > analog and Digital TV (dvb).
> > 
> > PS.: the diagram is over-simplified, as the tuner is just one of the possible
> > inputs for the analog part of the device. Other possible inputs are S-Video,
> > composite, HDMI, etc.
> > 
> > Sometimes, the audio DMA is also shared, e. g. just one stream comes from
> > the hardware. It is up to the driver to split audio and video and send
> > them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.
> > 
> > Those shared components can be used either at analog or digital mode,
> > but not at the same time.
> > 
> > Also, programming the V4L2 analog and audio DMA and demods should be done
> > via V4L2 API, as this API allows the selection of the proper audio/video
> > input (almost all devices have multiple analog inputs).
> > 
> > Please notice that, if the tuner is on digital mode, the entire analog
> > path is disabled, including ALSA output.
> > 
> > If the tuner is on analog mode, both ALSA and V4L2 can work at the
> > same time. However, during the period where the tuner firmware is
> > loaded, and during the DMA configuration and input selection time,
> > neither ALSA or V4L2 can stream. Such configuration/firmware load
> > is commanded via V4L2 API, as ALSA knows nothing about tuner or
> > input selection.
> > 
> >>
> >> At a higher level the problem description is:
> >>
> >> There are 3 different device files that get created to control
> >> tuner and audio functions on a media device. 3 drivers (dvb,
> >> v4l2, alsa), and 3 core apis (dvb-core, v4l-core, audio) that
> >> control the tuner and audio hardware and provide user api to
> >> these 3 device files.
> > 
> > 
> > There's actually a 4th component for some drivers: the mceusb driver,
> > that handles remote controllers. The mceusb handles the Microsoft 
> > Media Center Remote Control protocol. It supports standalone remote
> > controller devices, but it also supports a few USB devices that use
> > a separate interface for IR.
> > 
> > There are currently some issues on cx231xx and mceusb, as both drivers 
> > can be used at the same time, but, when cx231xx sends certain commands, 
> > the mceusb IR polls fail. This is out of the scope of the audio lock,
> > but it also needs to be addressed some day.
> > 
> >> User applications, drivers and the core have no knowledge of each
> >> other. The only thing that is common across all these drivers is
> >> the parent device for the main usb device which is controlled by
> >> the main usb driver.
> > 
> > I would add that there are user applications that can handle all
> > 3 APIs like MythTV. But, at least MythTV doesn't know how to associate
> > ALSA, V4L2 and DVB devnodes that belong to the same device.
> > 
> > I mean: if MythTV finds, let's say, 3 V4L2 nodes, 3 ALSA nodes, 
> > and 1 DVB node, it doesn't know what device is associated with the
> > DVB node.
> > 
> > Almost all applications that are aware of V4L2 API are also aware of
> > ALSA API and may associate audio and video, as there is a way to 
> > associate it using sysfs. However, several apps don't use it.
> > 
> >> The premise for the main design idea in this series is creating
> >> a common construct at the parent device structure that is visible
> >> to all drivers to act as a master access control (lock). Let's call
> >> this media token object with two sub-tokens one for tuner and another
> >> for audio.
> >>
> >> Each of the apis evolved separately, hence have their own backwards
> >> compatibility to maintain. Starting with v4l2:
> >>
> >> v4l2 case:
> >> Multiple v4l2 applications are allowed to open /dev/video0 in
> >> read/write mode with no restrictions as long as the tuner is in
> >> analog mode. v4l2 core handles conflicting requests between v4l2
> >> applications. 
> > 
> >> It doesn't have the knowledge that the tuner is in
> > 
> > To be clear: "It" here refers to v4l2 core. The drivers may have this
> > knowledge as, except for one case (bttv driver), they share some data.
> > 
> >> use by a dvb and/or audio is in use. As soon as a v4l2 application
> >> starts, digital stream glitches and audio glitches.
> >>
> >> dvb case:
> >> Multiple dvb applications can open the dvb device in read only mode.
> > 
> > There's no issue with ALSA on R/O mode, as the application is not
> > allowed to modify anything at the stream. This is used only to monitor
> > an already opened device in R/W mode.
> > 
> >> As soon an application open the device read/write mode a separate
> >> kthread is kicked off to handle the request. Only one application
> >> can open the device in read/write mode. 
> > 
> >> Similar to v4l2 case,
> > 
> > s/v4l2/v4l2 core/
> > 
> >> dvb-core doesn't have any knowledge that the tuner is in use by
> >> v4l2 and/or audio is in use. As soon as a dvb application starts v4l2
> >> video glitches and audio glitches.
> >>
> >> audio case:
> >> Same scenario is applicable to audio application. When a v4l2 or dvb
> >> application starts, audio application gets impacted.
> >>
> >> Problems to address:
> >>
> >> dvb owns tuner and audio: another dvb, v4l2 app and audio app should
> >>                           detect tuner/audio busy right away and exit.
> >>
> >> v4l2 owns tuner and audio: another dvb and audio app should detect
> >>                            tuner/audio busy right away and exit.
> > 
> > Actually, no: audio should not exit. The V4L2 should only hold the
> > token for the required time to initialize the device and/or load the
> > firmware. ALSA applications should wait for V4L2 to finish
> > programming at audio, and should keep working after that.
> > 
> >>                            v4l2 app can continue to use it until it
> >>                            tries to change the tuner/audio state.
> >>
> >>
> >> audio owns audio: dvb and v4l2 apps should detect audio busy and exit.
> > 
> > Actually no. It is, instead:
> > 
> > audio owns audio: dvb apps should detect audio busy and exit.
> > V4L2 apps should work. However, when certain V4L2 ioctls are issued, 
> > the audio device driver should not send any command to the hardware.
> > After such commands, the audio mixers may change.
> > 
> > We need two separate tokens because of that: the behavior is different.
> > 
> > This is basically why we need two separate tokens, and because we cannot
> > implement locking at ALSA open/close.
> > 
> >>
> >> Special cases:
> >>
> >> dvb apps. access tuner and audio in exclusive mode. i.e only one dvb app.
> >> at a time is allowed to open the device read/write mode.
> > 
> > To be clearer: dvb apps won't use the audio node, but audio should be blocked,
> > as the devices can't use audio while in DVB mode.
> > 
> >> As dvb apps.
> >> create threads to handle audio and video, 
> > 
> > No. DVB apps don't handle audio/video. It receives data as MPEG-TS,
> > using a separate device node. Yet, the same DMA engine that provides
> > video (and, sometimes audio) is used by the DVB devnode.
> > 
> >> all threads in that group
> >> should be allowed by the higher level construct to access the tuner and
> >> audio. dvb application will have to hold tuner and audio tokens so v4l2
> >> and audio apps. know they are in use.
> >>
> >> audio apps. access audio in exclusive mode. i.e only one audio app. at
> >> a time is allowed to open the device in read/write mode. Audio apps.
> >> create threads and thread closes and re-opens the audio device. Threads
> >> can do this and hence something that higher level construct has to allow.
> >> audio app. has to hold audio token so dvb and v4l2 know that it is in use.
> >> (Note: I am not sure if I have the audio scenario right)
> >>
> >> v4l2 apps. access tuner and audio in shared v4l2 mode. i.e several v4l2
> >> processes and threads could use tuner and audio at the same time. The
> >> higher level construct has to allow multiple v4l2 apps. to access and
> >> disallow dvb and audio apps. access when they are in use by v4l2.
> > 
> > Actually, V4L2 core handles concurrency. There's just one file handler
> > with full control to start/stop stream at V4L2 side.
> > 
> >>
> >> Adding to this, both dvb and v4l2 open audio device and make snd pcm
> >> capture callbacks.
> > 
> > Huh? DVB won't need to touch at PCM capture callbacks. It should just
> > avoid audio PCM capture to stream while in DVB mode.
> > 
> >> There is no way to tell if dvb or v4l2 or audio
> >> app is the one that is making this request.
> > 
> >> dvb app would like audio
> >> in exclusive mode allowing only one process and its threads to access
> >> it.
> > 
> > No. It just wants to disable the part of the hardware that can now
> > be powered off.
> 
> Hmm. I am seeing some snd_pcm_lib_ioctls coming from dvb application.

Well, probably it is related to the audio output and not audio
capture (or the application you're looking into is an hybrid one).

> >From what you are saying, these could be for poweroff.
> 
> > 
> >> v4l2 on the other hand would like audio in shared state accessible
> >> to all v4l2 processes. 
> > 
> >> If dvb-core and v4l2-core get tuner and audio
> >> tokens at the same time, the window for having tuner token and not
> >> getting audio token go down.
> > 
> > No. It should not be allowed that both dvb-core and v4l2-core to get
> > the tokens at the same time. This is an exclusive lock.
> 
> I should have explained this better. What I meant was that dvb-core when
> it gets the tuner, it should also obtain audio right away. v4l2-core
> when it gets the tuner, it should get the audio at the same time. When
> dvb-core has the tuner, v4l2 shouldn't get it and vice versa.

Yep.

> > 
> >> In dvb case when dvb device is opened in read/write mode, and v4l2
> >> case when an app. tries to change the status. Audio callbacks have to
> >> detect if audio is busy, if not which mode to request the token in.
> > 
> > Huh?
> 
> ok again bad explaining on my part. Let me try again. When dvb-core has
> the audio locked, audio application should detect the condition and take
> appropriate action. When v4l2-core has audio locked, audio application
> should detect the condition and take appropriate action.
> 
> > 
> >> For dvb and audio app. cases, the audio token should be requested in
> >> exclusive mode and in v4l2 case shared mode. The logic for requesting
> >> audio token will have to be try to get in exclusive mode, if fails,
> >> try to get in shared mode, and if that fails give up.
> > 
> > Huh?
> 
> Consider this flow:
> 
> step 1: dvb-core locks tuner and audio.
> step 2: audio ioctl is initiated (from the application)
> step 3: If dvb-core has the audio locked, how does alsa know if it can
>         proceed with the ioctl request or not?

That's simple: it should stop streaming, as part of the hardware can
be powered off. It can only return opening the device after DVB
releases.

> >> Current status:
> >> Combining patch v1 and patch v2 designs by allowing shared mode token
> >> hold for v4l2, and deciding on where to hold audio token from
> >> alsa driver will solve the above conflict scenarios. That said, the
> >> question is "is this the right approach?" or are there other ways to
> >> solve the problem. One thing is clear, we need some common higher level
> >> construct for all the device drivers and dvb, v4l2, and audio ioctls,
> >> callbacks etc, to detect the hardware is in use.
> > 
> > I think that the current status is that we need to finish the spec
> > first. Then check if the patches are doing what's above.
> > 
> > It seems that we agree to not agree at the requirements so far ;)
> 
> I would say it is more of not having the same understanding of
> the requirements as opposed to wanting to agree to disagree.

True.

> Right I agree that developing a clear RFC spec will help us all be
> on the same page while I continue to find solution for this problem,
> and continuing work towards patch v3.
> 
> I will compile what we discussed so far in RFC and send it out for
> review in a day or two. I will include your diagrams and the scenarios
> I put together with your corrections in it.

Ok. After that, we can proceed with the token patch v3, or to see if
it is possible to use the media controller here, as Sakari suggested.

Regards,
Mauro

[RFCv2] Media Token API Spec.

[Shuah Khan]

Hi Mauro,

Here is the RFC as promised. I also included the Media controller
as a an alternative and captured the discussion in the thread on
that topic. Please review.

-- Shuah

-----------------------------------------------------------------
RFC Media Token API Specification

Let's start with a diagram of a media device (without IR, eeprom
and others):

http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png

The dot lines represent the parts of the graph that are switched by
the tuner, DMA or input select.

Please notice that the DMA engines, together with the stuff needed to
control A/V switches is at one single chip. Changing the registers there
can affect the other streams, specially on most sophisticated devices
like cx231xx, where it even has a power management IP block that
validates if a device to be turned on/off won't exceed the maximum
drain current of 500mA. That's basically why we need to do a temporary
lock alsa, dvb, v4l and IR drivers when doing certain changes.

Also, please notice that I2C buses that can be as slow as 10kbps
are used to control for several devices, like:
        - the tuner
        - the Digital TV (DTV) demod
        - Analog and/or Video demod (sometimes embedded at the main
          chip)
        - DTV demux (sometimes embedded at the main chip)
        - The remote controller (sometimes embedded at the main chip)

For some devices, after powered on, or when certain parameters change, a
new firmware (and sometimes a hardware reset) is required. The firmware
size can be about 64KB or even bigger.

Also, the A/V switch it is actually two independent switches (or one
switch for video and one audio mux for audio) that needs to be changed
together when the source changes.

There are two components that are shared there between analog and
digital: the tuner (where the signal is captured) and the DMA engine
used to stream analog and Digital TV (dvb).

PS.: the diagram is over-simplified, as the tuner is just one of the
possible inputs for the analog part of the device. Other possible
inputs are S-Video, composite, HDMI, etc.

Sometimes, the audio DMA is also shared, e. g. just one stream comes
from the hardware. It is up to the driver to split audio and video and
send them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.

Those shared components can be used either at analog or digital mode,
but not at the same time.

Also, programming the V4L2 analog and audio DMA and demods should be
done via V4L2 API, as this API allows the selection of the proper
audio/video input (almost all devices have multiple analog inputs).

Please notice that, if the tuner is on digital mode, the entire analog
path is disabled, including ALSA output.

If the tuner is on analog mode, both ALSA and V4L2 can work at the
same time. However, during the period where the tuner firmware is
loaded, and during the DMA configuration and input selection time,
neither ALSA or V4L2 can stream. Such configuration/firmware load
is commanded via V4L2 API, as ALSA knows nothing about tuner or
input selection.

At a higher level the problem description is:

There are 3 different device files that get created to control
tuner and audio functions on a media device. 4 drivers (dvb,
v4l2, alsa, and the main usb driver for the usb device), and
3 core APIs (dvb-core, v4l-core, audio) that control the tuner
and audio hardware and provide user API to these 3 device files.

The above driver model is simplified, there's 4th component for
some drivers: the mceusb driver, that handles remote controllers.
The mceusb handles the Microsoft Media Center Remote Control
protocol. It supports stand alone remote controller devices, but
it also supports a few USB devices that use a separate interface
for IR.

There are currently some issues on cx231xx and mceusb, as both drivers
can be used at the same time, but, when cx231xx sends certain commands,
the mceusb IR polls fail. This is out of the scope of the audio lock,
but it also needs to be addressed some day.

Most media user applications, drivers and the core have no knowledge
of each other. The only thing that is common across all these drivers
is the parent device for the main usb device which is controlled by
the usb driver.

Some media user applications like MythTV can handle all 3 APIs,
however, MythTV doesn't know how to associate ALSA, V4L2 and DVB
devnodes that belong to the same device. If MythTV finds, 3 V4L2
nodes, 3 ALSA nodes, and 1 DVB node, it doesn't know which device
is associated with the DVB node.

Almost all applications that are aware of V4L2 API are also aware of
ALSA API and may associate audio and video, as there is a way to
associate it using sysfs. However, several applications don't use it.

The premise for the main design idea in this series is creating
a common construct at the parent device structure that is visible
to all drivers to act as a master access control (lock). Let's call
this media token object with two sub-tokens one for tuner and another
for audio.

Each of the APIS evolved separately, hence have their own backwards
compatibility to maintain. Starting with v4l2:

V4L2 case:
Multiple v4l2 applications are allowed to open /dev/video0 in
read/write mode with no restrictions as long as the tuner is in
analog mode. V4L2-core handles conflicting requests between v4l2
applications. V4L2-core doesn't have the knowledge that the tuner
is in use by a dvb and/or audio is in use. Individual drivers
may have this knowledge as, except for one case (bttv driver),
they share some data.

As soon as a V4L2 application starts, digital stream glitches and
audio glitches.

DVB case:
Multiple dvb applications can open the dvb device in read only mode.
As soon an application open the device read/write mode a separate
kthread is kicked off to handle the request. Only one application
can open the device in read/write mode. There's no issue with ALSA
in R/O mode, as the application is not allowed to modify anything
with the stream. This is used only to monitor an already opened
device in R/W mode.

Similar to V4L2-core case, dvb-core doesn't have any knowledge that
the tuner is in use by v4l2 and/or audio is in use. As soon as a
dvb application starts v4l2 video glitches and audio glitches.

Audio case:
Same scenario is applicable to audio application. When a v4l2 or dvb
application starts, audio application gets impacted.

Problems to address:

Dvb owns tuner and audio: another dvb, v4l2 application and
                          ALSA application should detect
                          tuner/audio busy right away and exit.
                          Dvb applications don't use audio node,
                          however, devices can't use audio hardware
                          while in DVB mode.

V4l2 owns tuner and audio: dvb should detect tuner/audio busy
                           right away and exit.
                           The V4L2-core should only hold the token for
                           the required time to initialize the device
                           and/or load the firmware.
                           ALSA applications should wait for v4L2-core
			   to finish programming at audio, and should
                           keep working after that.

Audio owns audio: dvb applications should detect audio busy and
exit. V4L2 applications should work. However, when certain V4L2
ioctls are issued, the audio device driver should not send any
command to the hardware. After such commands, the audio mixers
may change. This is why two tokens are necessary, one for tuner
and another for audio.

Because of the above mentioned difference in behavior between dvb
and v4l applications when audio is busy, two tokens (one for tuner
and another for audio) are necessary and audio token lock should not
be held at ALSA open/close.

Special cases:

Dvb applications access tuner in exclusive mode. i.e only one dvb
application at a time is allowed to open the device read/write mode.
Dvb applications don't use audio node, however, devices can't use
audio hardware while in DVB mode. Dvb applications receive data as
MPEG-TS, using a separate device node. The same DMA engine that
provides video (and, sometimes audio) is used by the DVB device
node, making it inaccessible to audio applications while tuner
is in DVB mode. Hence, the need to prevent audio applications from
accessing audio node when tuner is in DVB mode. As a result, dvb-core
will have to hold tuner and audio tokens so v4l2-core and ALSA know
that audio is not available. Dvb disables audio hardware so it could
be powered-off in some cases.

Audio applications access audio in exclusive mode. i.e only one audio
application at a time is allowed to open the device in read/write mode.
Audio applications create threads and thread closes and re-opens the
audio device. Threads can do this and hence something that higher level
construct has to allow. Audio application has to hold audio token so
dvb and v4l2 know that it is in use.

V4l2 applications access tuner and audio in shared v4l2 mode.
i.e several v4l2 processes and threads could use tuner and audio
at the same time. V4L2 core handles concurrency. There's just
one file handler with full control to start/stop stream at V4L2
side. The higher level construct should not break the ability of
multiple v4l2 applications to access tuner and audio in shared
mode, and disallow dvb and audio applications access when they
are in use by the V4L2-core.

Dvb-core when it gets the tuner, it should also obtain audio right
away. v4l2-core when it gets the tuner, it should get the audio at
the same time. When dvb-core has the tuner, v4l2 shouldn't get it
and vice versa.

When dvb-core has the audio locked, audio application should detect
condition and stop streaming, as part of the hardware can be powered
off. It can only return opening the device after DVB releases audio
token.

When v4l2-core has audio locked, audio application should detect the
condition and stop sending commands to audio hardware. It can only
resume audio access after V4L2 releases audio token.

Open issues:
During testing, snd_pcm_lib_ioctls are coming from dvb application.
It is likely that these are related to the audio output and not audio
capture or the application in question is an hybrid one. This issue
needs further investigation.

Alternatives: (proposed by Sakari Ailus)
Can Media controller be used to solve the problem?

The usage of the media controller for this specific usage is that
we should not force userspace applications to be aware of the
media controller just because of hardware locking.

Currently, media entities may only be entities bound to a given
subsystem, but likely need to change media controller for complex
embedded DVB device support ...

In case of the Media controller, mutual exclusion of different users
is currently performed by adding the entities to a pipeline and
incrementing the streaming count once streaming is enabled --- on
different interfaces streaming may mean a different thing.

However, we'll still need to find a way for ALSA to prevent it to use
the audio demod and DMA engine that will be powered off when DVB is
streaming.

The Media controller interface does not handle serializing potential
users that may wish to configure the device. Handling serializing is
necessary if Media controller is extended instead of pursuing Media
Token API to solve the problem.

Reconfiguring the DMA engine and some other registers via V4L2 API
should be blocked. The same applies to firmware load, if the device
is using tuner input for analog TV.

If we use the media controller, we'll need to add a state to it,
to indicate that a block at the pipeline is being reconfigured.

It is dependent on Media Controller adoption on ALSA as well.

Next steps:

1. Review RFC
2. Evaluate the media controller alternative.
3. Proceed with development once the decision is made.

----------------------------------------------------------------------

-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [RFCv2] Media Token API Spec.

[Hans Verkuil]

Hi Shuah,

This looks good. I have a few remarks, see below...

On 11/05/2014 12:08 AM, Shuah Khan wrote:
> Hi Mauro,
> 
> Here is the RFC as promised. I also included the Media controller
> as a an alternative and captured the discussion in the thread on
> that topic. Please review.
> 
> -- Shuah
> 
> -----------------------------------------------------------------
> RFC Media Token API Specification
> 
> Let's start with a diagram of a media device (without IR, eeprom
> and others):
> 
> http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png
> 
> The dot lines represent the parts of the graph that are switched by
> the tuner, DMA or input select.
> 
> Please notice that the DMA engines, together with the stuff needed to
> control A/V switches is at one single chip. Changing the registers there
> can affect the other streams, specially on most sophisticated devices
> like cx231xx, where it even has a power management IP block that
> validates if a device to be turned on/off won't exceed the maximum
> drain current of 500mA. That's basically why we need to do a temporary
> lock alsa, dvb, v4l and IR drivers when doing certain changes.
> 
> Also, please notice that I2C buses that can be as slow as 10kbps
> are used to control for several devices, like:
>         - the tuner
>         - the Digital TV (DTV) demod
>         - Analog and/or Video demod (sometimes embedded at the main
>           chip)
>         - DTV demux (sometimes embedded at the main chip)
>         - The remote controller (sometimes embedded at the main chip)
> 
> For some devices, after powered on, or when certain parameters change, a
> new firmware (and sometimes a hardware reset) is required. The firmware
> size can be about 64KB or even bigger.
> 
> Also, the A/V switch it is actually two independent switches (or one
> switch for video and one audio mux for audio) that needs to be changed
> together when the source changes.
> 
> There are two components that are shared there between analog and
> digital: the tuner (where the signal is captured) and the DMA engine
> used to stream analog and Digital TV (dvb).
> 
> PS.: the diagram is over-simplified, as the tuner is just one of the
> possible inputs for the analog part of the device. Other possible
> inputs are S-Video, composite, HDMI, etc.
> 
> Sometimes, the audio DMA is also shared, e. g. just one stream comes
> from the hardware. It is up to the driver to split audio and video and
> send them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.
> 
> Those shared components can be used either at analog or digital mode,
> but not at the same time.

At least in theory there may be independent DMA engines for analog and
digital video, which means that you can have DVB capture and Composite
analog capture (or any other analog input as long as it isn't from the
tuner). I actually suspect that one or more of the conexant devices
can do that. But I doubt we support that in the driver and I'm not sure
we want that anyway.

> 
> Also, programming the V4L2 analog and audio DMA and demods should be
> done via V4L2 API, as this API allows the selection of the proper
> audio/video input (almost all devices have multiple analog inputs).
> 
> Please notice that, if the tuner is on digital mode, the entire analog
> path is disabled, including ALSA output.
> 
> If the tuner is on analog mode, both ALSA and V4L2 can work at the
> same time. However, during the period where the tuner firmware is
> loaded, and during the DMA configuration and input selection time,
> neither ALSA or V4L2 can stream. Such configuration/firmware load
> is commanded via V4L2 API, as ALSA knows nothing about tuner or
> input selection.
> 
> At a higher level the problem description is:
> 
> There are 3 different device files that get created to control
> tuner and audio functions on a media device. 4 drivers (dvb,
> v4l2, alsa, and the main usb driver for the usb device), and
> 3 core APIs (dvb-core, v4l-core, audio) that control the tuner
> and audio hardware and provide user API to these 3 device files.
> 
> The above driver model is simplified, there's 4th component for
> some drivers: the mceusb driver, that handles remote controllers.
> The mceusb handles the Microsoft Media Center Remote Control
> protocol. It supports stand alone remote controller devices, but
> it also supports a few USB devices that use a separate interface
> for IR.
> 
> There are currently some issues on cx231xx and mceusb, as both drivers
> can be used at the same time, but, when cx231xx sends certain commands,
> the mceusb IR polls fail. This is out of the scope of the audio lock,
> but it also needs to be addressed some day.
> 
> Most media user applications, drivers and the core have no knowledge
> of each other. The only thing that is common across all these drivers
> is the parent device for the main usb device which is controlled by
> the usb driver.
> 
> Some media user applications like MythTV can handle all 3 APIs,
> however, MythTV doesn't know how to associate ALSA, V4L2 and DVB
> devnodes that belong to the same device. If MythTV finds, 3 V4L2
> nodes, 3 ALSA nodes, and 1 DVB node, it doesn't know which device
> is associated with the DVB node.
> 
> Almost all applications that are aware of V4L2 API are also aware of
> ALSA API and may associate audio and video, as there is a way to
> associate it using sysfs. However, several applications don't use it.
> 
> The premise for the main design idea in this series is creating
> a common construct at the parent device structure that is visible
> to all drivers to act as a master access control (lock). Let's call
> this media token object with two sub-tokens one for tuner and another
> for audio.
> 
> Each of the APIS evolved separately, hence have their own backwards
> compatibility to maintain. Starting with v4l2:
> 
> V4L2 case:
> Multiple v4l2 applications are allowed to open /dev/video0 in
> read/write mode with no restrictions as long as the tuner is in
> analog mode. V4L2-core handles conflicting requests between v4l2
> applications. V4L2-core doesn't have the knowledge that the tuner
> is in use by a dvb and/or audio is in use. Individual drivers
> may have this knowledge as, except for one case (bttv driver),
> they share some data.
> 
> As soon as a V4L2 application starts, digital stream glitches and
> audio glitches.
> 
> DVB case:
> Multiple dvb applications can open the dvb device in read only mode.
> As soon an application open the device read/write mode a separate
> kthread is kicked off to handle the request. Only one application
> can open the device in read/write mode. There's no issue with ALSA
> in R/O mode, as the application is not allowed to modify anything
> with the stream. This is used only to monitor an already opened
> device in R/W mode.
> 
> Similar to V4L2-core case, dvb-core doesn't have any knowledge that
> the tuner is in use by v4l2 and/or audio is in use. As soon as a
> dvb application starts v4l2 video glitches and audio glitches.
> 
> Audio case:
> Same scenario is applicable to audio application. When a v4l2 or dvb
> application starts, audio application gets impacted.
> 
> Problems to address:
> 
> Dvb owns tuner and audio: another dvb, v4l2 application and
>                           ALSA application should detect
>                           tuner/audio busy right away and exit.
>                           Dvb applications don't use audio node,
>                           however, devices can't use audio hardware
>                           while in DVB mode.
> 
> V4l2 owns tuner and audio: dvb should detect tuner/audio busy
>                            right away and exit.
>                            The V4L2-core should only hold the token for
>                            the required time to initialize the device
>                            and/or load the firmware.
>                            ALSA applications should wait for v4L2-core
> 			   to finish programming at audio, and should
>                            keep working after that.
> 
> Audio owns audio: dvb applications should detect audio busy and
> exit. V4L2 applications should work. However, when certain V4L2
> ioctls are issued, the audio device driver should not send any
> command to the hardware. After such commands, the audio mixers
> may change. This is why two tokens are necessary, one for tuner
> and another for audio.
> 
> Because of the above mentioned difference in behavior between dvb
> and v4l applications when audio is busy, two tokens (one for tuner
> and another for audio) are necessary and audio token lock should not
> be held at ALSA open/close.
> 
> Special cases:
> 
> Dvb applications access tuner in exclusive mode. i.e only one dvb
> application at a time is allowed to open the device read/write mode.
> Dvb applications don't use audio node, however, devices can't use
> audio hardware while in DVB mode. Dvb applications receive data as
> MPEG-TS, using a separate device node. The same DMA engine that
> provides video (and, sometimes audio) is used by the DVB device
> node, making it inaccessible to audio applications while tuner
> is in DVB mode. Hence, the need to prevent audio applications from
> accessing audio node when tuner is in DVB mode. As a result, dvb-core
> will have to hold tuner and audio tokens so v4l2-core and ALSA know
> that audio is not available. Dvb disables audio hardware so it could
> be powered-off in some cases.
> 
> Audio applications access audio in exclusive mode. i.e only one audio
> application at a time is allowed to open the device in read/write mode.
> Audio applications create threads and thread closes and re-opens the
> audio device. Threads can do this and hence something that higher level
> construct has to allow. Audio application has to hold audio token so
> dvb and v4l2 know that it is in use.
> 
> V4l2 applications access tuner and audio in shared v4l2 mode.
> i.e several v4l2 processes and threads could use tuner and audio
> at the same time. V4L2 core handles concurrency. There's just
> one file handler with full control to start/stop stream at V4L2
> side. The higher level construct should not break the ability of
> multiple v4l2 applications to access tuner and audio in shared
> mode, and disallow dvb and audio applications access when they
> are in use by the V4L2-core.
> 
> Dvb-core when it gets the tuner, it should also obtain audio right
> away. v4l2-core when it gets the tuner, it should get the audio at
> the same time. When dvb-core has the tuner, v4l2 shouldn't get it
> and vice versa.
> 
> When dvb-core has the audio locked, audio application should detect
> condition and stop streaming, as part of the hardware can be powered
> off. It can only return opening the device after DVB releases audio
> token.
> 
> When v4l2-core has audio locked, audio application should detect the
> condition and stop sending commands to audio hardware. It can only
> resume audio access after V4L2 releases audio token.
> 
> Open issues:
> During testing, snd_pcm_lib_ioctls are coming from dvb application.
> It is likely that these are related to the audio output and not audio
> capture or the application in question is an hybrid one. This issue
> needs further investigation.
> 
> Alternatives: (proposed by Sakari Ailus)
> Can Media controller be used to solve the problem?

I don't think the MC has anything to do with this. The problem is with
existing non-MC drivers and existing applications. Adding MC support to
drivers will not help this in any way that I can see.

In the end this is simply a resource ownership problem: who owns the
tuner? The only thing special about it is that it crosses subsystems.

> 
> The usage of the media controller for this specific usage is that
> we should not force userspace applications to be aware of the
> media controller just because of hardware locking.
> 
> Currently, media entities may only be entities bound to a given
> subsystem, but likely need to change media controller for complex
> embedded DVB device support ...
> 
> In case of the Media controller, mutual exclusion of different users
> is currently performed by adding the entities to a pipeline and
> incrementing the streaming count once streaming is enabled --- on
> different interfaces streaming may mean a different thing.
> 
> However, we'll still need to find a way for ALSA to prevent it to use
> the audio demod and DMA engine that will be powered off when DVB is
> streaming.
> 
> The Media controller interface does not handle serializing potential
> users that may wish to configure the device. Handling serializing is
> necessary if Media controller is extended instead of pursuing Media
> Token API to solve the problem.
> 
> Reconfiguring the DMA engine and some other registers via V4L2 API
> should be blocked. The same applies to firmware load, if the device
> is using tuner input for analog TV.
> 
> If we use the media controller, we'll need to add a state to it,
> to indicate that a block at the pipeline is being reconfigured.
> 
> It is dependent on Media Controller adoption on ALSA as well.
> 
> Next steps:
> 
> 1. Review RFC
> 2. Evaluate the media controller alternative.
> 3. Proceed with development once the decision is made.
> 
> ----------------------------------------------------------------------
> 

Regards,

	Hans

Re: [RFCv2] Media Token API Spec.

[Sakari Ailus]

Hi Shuah,

Many thanks for the RFC, for and being so patient with my terrible review
schedule. My comments below.

On Tue, Nov 04, 2014 at 04:08:50PM -0700, Shuah Khan wrote:
> Hi Mauro,
> 
> Here is the RFC as promised. I also included the Media controller
> as a an alternative and captured the discussion in the thread on
> that topic. Please review.
> 
> -- Shuah
> 
> -----------------------------------------------------------------
> RFC Media Token API Specification
> 
> Let's start with a diagram of a media device (without IR, eeprom
> and others):
> 
> http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png
> 
> The dot lines represent the parts of the graph that are switched by
> the tuner, DMA or input select.
> 
> Please notice that the DMA engines, together with the stuff needed to
> control A/V switches is at one single chip. Changing the registers there
> can affect the other streams, specially on most sophisticated devices
> like cx231xx, where it even has a power management IP block that
> validates if a device to be turned on/off won't exceed the maximum
> drain current of 500mA. That's basically why we need to do a temporary
> lock alsa, dvb, v4l and IR drivers when doing certain changes.
> 
> Also, please notice that I2C buses that can be as slow as 10kbps
> are used to control for several devices, like:
>         - the tuner
>         - the Digital TV (DTV) demod
>         - Analog and/or Video demod (sometimes embedded at the main
>           chip)
>         - DTV demux (sometimes embedded at the main chip)
>         - The remote controller (sometimes embedded at the main chip)
> 
> For some devices, after powered on, or when certain parameters change, a
> new firmware (and sometimes a hardware reset) is required. The firmware
> size can be about 64KB or even bigger.
> 
> Also, the A/V switch it is actually two independent switches (or one
> switch for video and one audio mux for audio) that needs to be changed
> together when the source changes.
> 
> There are two components that are shared there between analog and
> digital: the tuner (where the signal is captured) and the DMA engine
> used to stream analog and Digital TV (dvb).
> 
> PS.: the diagram is over-simplified, as the tuner is just one of the
> possible inputs for the analog part of the device. Other possible
> inputs are S-Video, composite, HDMI, etc.
> 
> Sometimes, the audio DMA is also shared, e. g. just one stream comes
> from the hardware. It is up to the driver to split audio and video and
> send them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.
> 
> Those shared components can be used either at analog or digital mode,
> but not at the same time.
> 
> Also, programming the V4L2 analog and audio DMA and demods should be
> done via V4L2 API, as this API allows the selection of the proper
> audio/video input (almost all devices have multiple analog inputs).
> 
> Please notice that, if the tuner is on digital mode, the entire analog
> path is disabled, including ALSA output.
> 
> If the tuner is on analog mode, both ALSA and V4L2 can work at the
> same time. However, during the period where the tuner firmware is
> loaded, and during the DMA configuration and input selection time,
> neither ALSA or V4L2 can stream. Such configuration/firmware load
> is commanded via V4L2 API, as ALSA knows nothing about tuner or
> input selection.
> 
> At a higher level the problem description is:
> 
> There are 3 different device files that get created to control
> tuner and audio functions on a media device. 4 drivers (dvb,
> v4l2, alsa, and the main usb driver for the usb device), and
> 3 core APIs (dvb-core, v4l-core, audio) that control the tuner
> and audio hardware and provide user API to these 3 device files.
> 
> The above driver model is simplified, there's 4th component for
> some drivers: the mceusb driver, that handles remote controllers.
> The mceusb handles the Microsoft Media Center Remote Control
> protocol. It supports stand alone remote controller devices, but
> it also supports a few USB devices that use a separate interface
> for IR.
> 
> There are currently some issues on cx231xx and mceusb, as both drivers
> can be used at the same time, but, when cx231xx sends certain commands,
> the mceusb IR polls fail. This is out of the scope of the audio lock,
> but it also needs to be addressed some day.
> 
> Most media user applications, drivers and the core have no knowledge
> of each other. The only thing that is common across all these drivers
> is the parent device for the main usb device which is controlled by
> the usb driver.
> 
> Some media user applications like MythTV can handle all 3 APIs,
> however, MythTV doesn't know how to associate ALSA, V4L2 and DVB
> devnodes that belong to the same device. If MythTV finds, 3 V4L2
> nodes, 3 ALSA nodes, and 1 DVB node, it doesn't know which device
> is associated with the DVB node.
> 
> Almost all applications that are aware of V4L2 API are also aware of
> ALSA API and may associate audio and video, as there is a way to
> associate it using sysfs. However, several applications don't use it.
> 
> The premise for the main design idea in this series is creating
> a common construct at the parent device structure that is visible
> to all drivers to act as a master access control (lock). Let's call
> this media token object with two sub-tokens one for tuner and another
> for audio.
> 
> Each of the APIS evolved separately, hence have their own backwards
> compatibility to maintain. Starting with v4l2:
> 
> V4L2 case:
> Multiple v4l2 applications are allowed to open /dev/video0 in
> read/write mode with no restrictions as long as the tuner is in
> analog mode. V4L2-core handles conflicting requests between v4l2
> applications. V4L2-core doesn't have the knowledge that the tuner
> is in use by a dvb and/or audio is in use. Individual drivers
> may have this knowledge as, except for one case (bttv driver),
> they share some data.
> 
> As soon as a V4L2 application starts, digital stream glitches and
> audio glitches.
> 
> DVB case:
> Multiple dvb applications can open the dvb device in read only mode.
> As soon an application open the device read/write mode a separate
> kthread is kicked off to handle the request. Only one application
> can open the device in read/write mode. There's no issue with ALSA
> in R/O mode, as the application is not allowed to modify anything
> with the stream. This is used only to monitor an already opened
> device in R/W mode.
> 
> Similar to V4L2-core case, dvb-core doesn't have any knowledge that
> the tuner is in use by v4l2 and/or audio is in use. As soon as a
> dvb application starts v4l2 video glitches and audio glitches.
> 
> Audio case:
> Same scenario is applicable to audio application. When a v4l2 or dvb
> application starts, audio application gets impacted.
> 
> Problems to address:
> 
> Dvb owns tuner and audio: another dvb, v4l2 application and
>                           ALSA application should detect
>                           tuner/audio busy right away and exit.
>                           Dvb applications don't use audio node,
>                           however, devices can't use audio hardware
>                           while in DVB mode.
> 
> V4l2 owns tuner and audio: dvb should detect tuner/audio busy
>                            right away and exit.
>                            The V4L2-core should only hold the token for
>                            the required time to initialize the device
>                            and/or load the firmware.
>                            ALSA applications should wait for v4L2-core
> 			   to finish programming at audio, and should
>                            keep working after that.
> 
> Audio owns audio: dvb applications should detect audio busy and
> exit. V4L2 applications should work. However, when certain V4L2
> ioctls are issued, the audio device driver should not send any
> command to the hardware. After such commands, the audio mixers
> may change. This is why two tokens are necessary, one for tuner
> and another for audio.
> 
> Because of the above mentioned difference in behavior between dvb
> and v4l applications when audio is busy, two tokens (one for tuner
> and another for audio) are necessary and audio token lock should not
> be held at ALSA open/close.
> 
> Special cases:
> 
> Dvb applications access tuner in exclusive mode. i.e only one dvb
> application at a time is allowed to open the device read/write mode.
> Dvb applications don't use audio node, however, devices can't use
> audio hardware while in DVB mode. Dvb applications receive data as
> MPEG-TS, using a separate device node. The same DMA engine that
> provides video (and, sometimes audio) is used by the DVB device
> node, making it inaccessible to audio applications while tuner
> is in DVB mode. Hence, the need to prevent audio applications from
> accessing audio node when tuner is in DVB mode. As a result, dvb-core
> will have to hold tuner and audio tokens so v4l2-core and ALSA know
> that audio is not available. Dvb disables audio hardware so it could
> be powered-off in some cases.
> 
> Audio applications access audio in exclusive mode. i.e only one audio
> application at a time is allowed to open the device in read/write mode.
> Audio applications create threads and thread closes and re-opens the
> audio device. Threads can do this and hence something that higher level
> construct has to allow. Audio application has to hold audio token so
> dvb and v4l2 know that it is in use.
> 
> V4l2 applications access tuner and audio in shared v4l2 mode.
> i.e several v4l2 processes and threads could use tuner and audio
> at the same time. V4L2 core handles concurrency. There's just
> one file handler with full control to start/stop stream at V4L2
> side. The higher level construct should not break the ability of
> multiple v4l2 applications to access tuner and audio in shared
> mode, and disallow dvb and audio applications access when they
> are in use by the V4L2-core.
> 
> Dvb-core when it gets the tuner, it should also obtain audio right
> away. v4l2-core when it gets the tuner, it should get the audio at
> the same time. When dvb-core has the tuner, v4l2 shouldn't get it
> and vice versa.
> 
> When dvb-core has the audio locked, audio application should detect
> condition and stop streaming, as part of the hardware can be powered
> off. It can only return opening the device after DVB releases audio
> token.
> 
> When v4l2-core has audio locked, audio application should detect the
> condition and stop sending commands to audio hardware. It can only
> resume audio access after V4L2 releases audio token.
> 
> Open issues:
> During testing, snd_pcm_lib_ioctls are coming from dvb application.
> It is likely that these are related to the audio output and not audio
> capture or the application in question is an hybrid one. This issue
> needs further investigation.
> 
> Alternatives: (proposed by Sakari Ailus)
> Can Media controller be used to solve the problem?
> 
> The usage of the media controller for this specific usage is that
> we should not force userspace applications to be aware of the
> media controller just because of hardware locking.
> 
> Currently, media entities may only be entities bound to a given
> subsystem, but likely need to change media controller for complex
> embedded DVB device support ...
> 
> In case of the Media controller, mutual exclusion of different users
> is currently performed by adding the entities to a pipeline and
> incrementing the streaming count once streaming is enabled --- on
> different interfaces streaming may mean a different thing.
> 
> However, we'll still need to find a way for ALSA to prevent it to use
> the audio demod and DMA engine that will be powered off when DVB is
> streaming.
> 
> The Media controller interface does not handle serializing potential
> users that may wish to configure the device. Handling serializing is
> necessary if Media controller is extended instead of pursuing Media
> Token API to solve the problem.
> 
> Reconfiguring the DMA engine and some other registers via V4L2 API
> should be blocked. The same applies to firmware load, if the device
> is using tuner input for analog TV.
> 
> If we use the media controller, we'll need to add a state to it,
> to indicate that a block at the pipeline is being reconfigured.
> 
> It is dependent on Media Controller adoption on ALSA as well.

Thank you for the detailed description of the problem domain.

Using Media controller for this at this point isn't straightforward, and
especially for existing drivers for which the current APIs serve the
existing devices well enough, perhaps not the best solution even when the
missing pieces were implemented.

More complex devices, though, still need MC in order to control them in a
meaningful way. If a tuner is connected into one, the media token framework
wouldn't help there. In other words, we'll need something for such devices
as well. That'd be proper MC support for DVB and ALSA, but it'll be a
separate discussion.

One big upside in this patchset is that it does not change the user space
interface.

One concern I have is how generic this framework really is. Do you see
potential use cases outside the current one?

I'd move this under drivers/media, and possibly think of the naming of the
framework a little bit.

I'll review the rest of the v2.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFCv2] Media Token API Spec.

[Shuah Khan]

On 11/18/2014 02:15 PM, Sakari Ailus wrote:
> Hi Shuah,
> 
> Many thanks for the RFC, for and being so patient with my terrible review
> schedule. My comments below.
> 
> On Tue, Nov 04, 2014 at 04:08:50PM -0700, Shuah Khan wrote:
>> Hi Mauro,
>>
>> Here is the RFC as promised. I also included the Media controller
>> as a an alternative and captured the discussion in the thread on
>> that topic. Please review.
>>
>> -- Shuah
>>
>> -----------------------------------------------------------------
>> RFC Media Token API Specification
>>
>> Let's start with a diagram of a media device (without IR, eeprom
>> and others):
>>
>> http://linuxtv.org/downloads/presentations/typical_hybrid_hardware.png
>>
>> The dot lines represent the parts of the graph that are switched by
>> the tuner, DMA or input select.
>>
>> Please notice that the DMA engines, together with the stuff needed to
>> control A/V switches is at one single chip. Changing the registers there
>> can affect the other streams, specially on most sophisticated devices
>> like cx231xx, where it even has a power management IP block that
>> validates if a device to be turned on/off won't exceed the maximum
>> drain current of 500mA. That's basically why we need to do a temporary
>> lock alsa, dvb, v4l and IR drivers when doing certain changes.
>>
>> Also, please notice that I2C buses that can be as slow as 10kbps
>> are used to control for several devices, like:
>>         - the tuner
>>         - the Digital TV (DTV) demod
>>         - Analog and/or Video demod (sometimes embedded at the main
>>           chip)
>>         - DTV demux (sometimes embedded at the main chip)
>>         - The remote controller (sometimes embedded at the main chip)
>>
>> For some devices, after powered on, or when certain parameters change, a
>> new firmware (and sometimes a hardware reset) is required. The firmware
>> size can be about 64KB or even bigger.
>>
>> Also, the A/V switch it is actually two independent switches (or one
>> switch for video and one audio mux for audio) that needs to be changed
>> together when the source changes.
>>
>> There are two components that are shared there between analog and
>> digital: the tuner (where the signal is captured) and the DMA engine
>> used to stream analog and Digital TV (dvb).
>>
>> PS.: the diagram is over-simplified, as the tuner is just one of the
>> possible inputs for the analog part of the device. Other possible
>> inputs are S-Video, composite, HDMI, etc.
>>
>> Sometimes, the audio DMA is also shared, e. g. just one stream comes
>> from the hardware. It is up to the driver to split audio and video and
>> send them to the V4L2 and ALSA APIs. This is the case of tm6000 driver.
>>
>> Those shared components can be used either at analog or digital mode,
>> but not at the same time.
>>
>> Also, programming the V4L2 analog and audio DMA and demods should be
>> done via V4L2 API, as this API allows the selection of the proper
>> audio/video input (almost all devices have multiple analog inputs).
>>
>> Please notice that, if the tuner is on digital mode, the entire analog
>> path is disabled, including ALSA output.
>>
>> If the tuner is on analog mode, both ALSA and V4L2 can work at the
>> same time. However, during the period where the tuner firmware is
>> loaded, and during the DMA configuration and input selection time,
>> neither ALSA or V4L2 can stream. Such configuration/firmware load
>> is commanded via V4L2 API, as ALSA knows nothing about tuner or
>> input selection.
>>
>> At a higher level the problem description is:
>>
>> There are 3 different device files that get created to control
>> tuner and audio functions on a media device. 4 drivers (dvb,
>> v4l2, alsa, and the main usb driver for the usb device), and
>> 3 core APIs (dvb-core, v4l-core, audio) that control the tuner
>> and audio hardware and provide user API to these 3 device files.
>>
>> The above driver model is simplified, there's 4th component for
>> some drivers: the mceusb driver, that handles remote controllers.
>> The mceusb handles the Microsoft Media Center Remote Control
>> protocol. It supports stand alone remote controller devices, but
>> it also supports a few USB devices that use a separate interface
>> for IR.
>>
>> There are currently some issues on cx231xx and mceusb, as both drivers
>> can be used at the same time, but, when cx231xx sends certain commands,
>> the mceusb IR polls fail. This is out of the scope of the audio lock,
>> but it also needs to be addressed some day.
>>
>> Most media user applications, drivers and the core have no knowledge
>> of each other. The only thing that is common across all these drivers
>> is the parent device for the main usb device which is controlled by
>> the usb driver.
>>
>> Some media user applications like MythTV can handle all 3 APIs,
>> however, MythTV doesn't know how to associate ALSA, V4L2 and DVB
>> devnodes that belong to the same device. If MythTV finds, 3 V4L2
>> nodes, 3 ALSA nodes, and 1 DVB node, it doesn't know which device
>> is associated with the DVB node.
>>
>> Almost all applications that are aware of V4L2 API are also aware of
>> ALSA API and may associate audio and video, as there is a way to
>> associate it using sysfs. However, several applications don't use it.
>>
>> The premise for the main design idea in this series is creating
>> a common construct at the parent device structure that is visible
>> to all drivers to act as a master access control (lock). Let's call
>> this media token object with two sub-tokens one for tuner and another
>> for audio.
>>
>> Each of the APIS evolved separately, hence have their own backwards
>> compatibility to maintain. Starting with v4l2:
>>
>> V4L2 case:
>> Multiple v4l2 applications are allowed to open /dev/video0 in
>> read/write mode with no restrictions as long as the tuner is in
>> analog mode. V4L2-core handles conflicting requests between v4l2
>> applications. V4L2-core doesn't have the knowledge that the tuner
>> is in use by a dvb and/or audio is in use. Individual drivers
>> may have this knowledge as, except for one case (bttv driver),
>> they share some data.
>>
>> As soon as a V4L2 application starts, digital stream glitches and
>> audio glitches.
>>
>> DVB case:
>> Multiple dvb applications can open the dvb device in read only mode.
>> As soon an application open the device read/write mode a separate
>> kthread is kicked off to handle the request. Only one application
>> can open the device in read/write mode. There's no issue with ALSA
>> in R/O mode, as the application is not allowed to modify anything
>> with the stream. This is used only to monitor an already opened
>> device in R/W mode.
>>
>> Similar to V4L2-core case, dvb-core doesn't have any knowledge that
>> the tuner is in use by v4l2 and/or audio is in use. As soon as a
>> dvb application starts v4l2 video glitches and audio glitches.
>>
>> Audio case:
>> Same scenario is applicable to audio application. When a v4l2 or dvb
>> application starts, audio application gets impacted.
>>
>> Problems to address:
>>
>> Dvb owns tuner and audio: another dvb, v4l2 application and
>>                           ALSA application should detect
>>                           tuner/audio busy right away and exit.
>>                           Dvb applications don't use audio node,
>>                           however, devices can't use audio hardware
>>                           while in DVB mode.
>>
>> V4l2 owns tuner and audio: dvb should detect tuner/audio busy
>>                            right away and exit.
>>                            The V4L2-core should only hold the token for
>>                            the required time to initialize the device
>>                            and/or load the firmware.
>>                            ALSA applications should wait for v4L2-core
>> 			   to finish programming at audio, and should
>>                            keep working after that.
>>
>> Audio owns audio: dvb applications should detect audio busy and
>> exit. V4L2 applications should work. However, when certain V4L2
>> ioctls are issued, the audio device driver should not send any
>> command to the hardware. After such commands, the audio mixers
>> may change. This is why two tokens are necessary, one for tuner
>> and another for audio.
>>
>> Because of the above mentioned difference in behavior between dvb
>> and v4l applications when audio is busy, two tokens (one for tuner
>> and another for audio) are necessary and audio token lock should not
>> be held at ALSA open/close.
>>
>> Special cases:
>>
>> Dvb applications access tuner in exclusive mode. i.e only one dvb
>> application at a time is allowed to open the device read/write mode.
>> Dvb applications don't use audio node, however, devices can't use
>> audio hardware while in DVB mode. Dvb applications receive data as
>> MPEG-TS, using a separate device node. The same DMA engine that
>> provides video (and, sometimes audio) is used by the DVB device
>> node, making it inaccessible to audio applications while tuner
>> is in DVB mode. Hence, the need to prevent audio applications from
>> accessing audio node when tuner is in DVB mode. As a result, dvb-core
>> will have to hold tuner and audio tokens so v4l2-core and ALSA know
>> that audio is not available. Dvb disables audio hardware so it could
>> be powered-off in some cases.
>>
>> Audio applications access audio in exclusive mode. i.e only one audio
>> application at a time is allowed to open the device in read/write mode.
>> Audio applications create threads and thread closes and re-opens the
>> audio device. Threads can do this and hence something that higher level
>> construct has to allow. Audio application has to hold audio token so
>> dvb and v4l2 know that it is in use.
>>
>> V4l2 applications access tuner and audio in shared v4l2 mode.
>> i.e several v4l2 processes and threads could use tuner and audio
>> at the same time. V4L2 core handles concurrency. There's just
>> one file handler with full control to start/stop stream at V4L2
>> side. The higher level construct should not break the ability of
>> multiple v4l2 applications to access tuner and audio in shared
>> mode, and disallow dvb and audio applications access when they
>> are in use by the V4L2-core.
>>
>> Dvb-core when it gets the tuner, it should also obtain audio right
>> away. v4l2-core when it gets the tuner, it should get the audio at
>> the same time. When dvb-core has the tuner, v4l2 shouldn't get it
>> and vice versa.
>>
>> When dvb-core has the audio locked, audio application should detect
>> condition and stop streaming, as part of the hardware can be powered
>> off. It can only return opening the device after DVB releases audio
>> token.
>>
>> When v4l2-core has audio locked, audio application should detect the
>> condition and stop sending commands to audio hardware. It can only
>> resume audio access after V4L2 releases audio token.
>>
>> Open issues:
>> During testing, snd_pcm_lib_ioctls are coming from dvb application.
>> It is likely that these are related to the audio output and not audio
>> capture or the application in question is an hybrid one. This issue
>> needs further investigation.
>>
>> Alternatives: (proposed by Sakari Ailus)
>> Can Media controller be used to solve the problem?
>>
>> The usage of the media controller for this specific usage is that
>> we should not force userspace applications to be aware of the
>> media controller just because of hardware locking.
>>
>> Currently, media entities may only be entities bound to a given
>> subsystem, but likely need to change media controller for complex
>> embedded DVB device support ...
>>
>> In case of the Media controller, mutual exclusion of different users
>> is currently performed by adding the entities to a pipeline and
>> incrementing the streaming count once streaming is enabled --- on
>> different interfaces streaming may mean a different thing.
>>
>> However, we'll still need to find a way for ALSA to prevent it to use
>> the audio demod and DMA engine that will be powered off when DVB is
>> streaming.
>>
>> The Media controller interface does not handle serializing potential
>> users that may wish to configure the device. Handling serializing is
>> necessary if Media controller is extended instead of pursuing Media
>> Token API to solve the problem.
>>
>> Reconfiguring the DMA engine and some other registers via V4L2 API
>> should be blocked. The same applies to firmware load, if the device
>> is using tuner input for analog TV.
>>
>> If we use the media controller, we'll need to add a state to it,
>> to indicate that a block at the pipeline is being reconfigured.
>>
>> It is dependent on Media Controller adoption on ALSA as well.
> 
> Thank you for the detailed description of the problem domain.
> 
> Using Media controller for this at this point isn't straightforward, and
> especially for existing drivers for which the current APIs serve the
> existing devices well enough, perhaps not the best solution even when the
> missing pieces were implemented.
> 
> More complex devices, though, still need MC in order to control them in a
> meaningful way. If a tuner is connected into one, the media token framework
> wouldn't help there. In other words, we'll need something for such devices
> as well. That'd be proper MC support for DVB and ALSA, but it'll be a
> separate discussion.
> 
> One big upside in this patchset is that it does not change the user space
> interface.

Right that is the goal.

> 
> One concern I have is how generic this framework really is. Do you see
> potential use cases outside the current one?
> 
> I'd move this under drivers/media, and possibly think of the naming of the
> framework a little bit.

Others gave the same feedback on both of your points on generic and
location. I moved it under drivers/media for the patch v3 pre-work
I did before we started the RFC discussion. So we are good there.

> 
> I'll review the rest of the v2.
> 

Great. Please do and give me feedback. I will wait for that before
I get started with patch v3 work.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/21/2014 10:05 AM, Takashi Iwai wrote:
> At Tue, 21 Oct 2014 17:42:51 +0200,
> Hans Verkuil wrote:

>>
>> 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.

> 
>> 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?
> 
>> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Hans Verkuil]

Hi Shuah,

Some notes below...

On 10/21/2014 07:32 PM, Shuah Khan wrote:
> On 10/21/2014 10:05 AM, Takashi Iwai wrote:
>> At Tue, 21 Oct 2014 17:42:51 +0200,
>> Hans Verkuil wrote:
> 
>>>
>>> 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.
> 
>>
>>> 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?
>>
>>> 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

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.

> - 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

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.

> - 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

Re: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Shuah Khan]

On 10/25/2014 04:57 AM, Mauro Carvalho Chehab wrote:
> Hi,
> 
> Sorry to enter into this thread so late. Last week was a full week, due to the
> 4 conferences I paticiapated, and last week I needed to fill lots of trip
> reports. Also, I have another trip to give two speeches.
> 
> Em Tue, 21 Oct 2014 11:32:10 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Here is what I propose for patch v3:
>> - make it a module under media
>> - collapse tuner and audio tokens into media token
> 
> I'm a little skeptical about this. Merging tuner and audio tokens seems
> weird on my eyes, as there are actually two different hardware resources
> we need to lock, and we may be locking them on different places.

I think the suggestion for collapsing came about because, in this
patch series, the dvb and v4l use-cases are such that tuner and
audio need to be held together. i.e when tuner is held, audio is
held as well. With an intent to simplify the usage, I decided to
have get_tuner interface to return audio token. It does simplify
the logic for callers - all the paces that get tuner will also
need to make a call to get audio and handle errors when audio is
not available. That made it even more confusing perhaps and raised
the question that "why can't we collapse".

Having two tokens will allow the ability hold them independently.
Maybe the right approach for patch v3 is don't collapse, but
change dvb and v4l to get tuner followed by audio and handle the
errors paths themselves.

> 
>> - change names (get rid of abbreviated tkn stuff)
>> - Make other changes Takashi/Lars pointed out in pcm
>> - hold token in pcm open/close
> 

-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[PATCH v2 6/6] media: au0828-core changes to create and destroy media

[Shuah Khan]

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 <shuahkh@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

Re: [PATCH v2 0/6] media token resource framework

[Takashi Iwai]

At Tue, 14 Oct 2014 08:58:36 -0600,
Shuah Khan wrote:
> 
> 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

Re: [PATCH v2 0/6] media token resource framework

[Shuah Khan]

On 10/15/2014 10:48 AM, Takashi Iwai wrote:
> At Tue, 14 Oct 2014 08:58:36 -0600,
> Shuah Khan wrote:
>>
>> 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)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH v2 0/6] media token resource framework

[Takashi Iwai]

At Wed, 15 Oct 2014 14:21:34 -0600,
Shuah Khan wrote:
> 
> On 10/15/2014 10:48 AM, Takashi Iwai wrote:
> > At Tue, 14 Oct 2014 08:58:36 -0600,
> > Shuah Khan wrote:
> >>
> >> 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

Re: [PATCH v2 0/6] media token resource framework

[Sakari Ailus]

Hi Shuah and others,

Shuah Khan wrote:
> 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.

I know this comes quite late after the first patch series has been sent, 
but I'd like to ask if you have you considered a different approach: 
rather than implementing something entirely new, the Media controller 
can almost do this already. It models the physical layout of the device, 
instead of creating special use case specific Media entity like 
constructs for tuner and audio. Also the Media token framework does not 
appear to be as a perfect match for the Media controller framework which 
is also planned to be used by DVB already:

<URL:http://linuxtv.org/news.php?entry=2014-10-21.mchehab>; look for "3) 
DVB API improvements". There have been ALSA MC patches as well but I'm 
not aware of the status of those at the moment.

The tokens appear much like media entities of specific kind to me.

Currently, media entities may only be entities bound to a given 
subsystem, but I don't think it has to (or perhaps even may) stay that way.

In case of the Media controller, mutual exclusion of different users is 
currently performed by adding the entities to a pipeline and 
incrementing the streaming count once streaming is enabled --- on 
different interfaces streaming may mean a different thing.

The Media controller interface does not handle serialising potential 
users that may wish to configure the device. If that's needed then we'll 
need to think how to add it.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 0/6] media token resource framework

[Mauro Carvalho Chehab]

Em Wed, 29 Oct 2014 11:17:44 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Shuah and others,
> 
> Shuah Khan wrote:
> > 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.
> 
> I know this comes quite late after the first patch series has been sent, 
> but I'd like to ask if you have you considered a different approach: 
> rather than implementing something entirely new, the Media controller 
> can almost do this already. It models the physical layout of the device, 
> instead of creating special use case specific Media entity like 
> constructs for tuner and audio. Also the Media token framework does not 
> appear to be as a perfect match for the Media controller framework which 
> is also planned to be used by DVB already:
> 
> <URL:http://linuxtv.org/news.php?entry=2014-10-21.mchehab>; look for "3) 
> DVB API improvements". There have been ALSA MC patches as well but I'm 
> not aware of the status of those at the moment.
> 
> The tokens appear much like media entities of specific kind to me.

Yeah, it could be seen as that.

> Currently, media entities may only be entities bound to a given 
> subsystem, but I don't think it has to (or perhaps even may) stay that way.

We had some discussions about that with Laurent in San Jose. Yeah,
we will likely need to change that at the media controller, for complex
embedded DVB devices.

The usage of the media controller for this specific usage is that
we should not force userspace apps to be aware of the media controller
just because of hardware locking.

> In case of the Media controller, mutual exclusion of different users is 
> currently performed by adding the entities to a pipeline and 
> incrementing the streaming count once streaming is enabled --- on 
> different interfaces streaming may mean a different thing.

Well, we'll still need to find a way for ALSA to prevent it to use
the audio demod and DMA engine that will be powered off when DVB is
streaming.

> The Media controller interface does not handle serialising potential 
> users that may wish to configure the device. If that's needed then we'll 
> need to think how to add it.

Yes, this would be needed needed if we take this approach. 

Reconfiguring the DMA engine and some other registers via V4L2 API 
should be blocked. The same applies to firmware load, if the device 
is using tuner input for analog TV.

If we use the media controller, we'll need to add a state to it,
to indicate that a block at the pipeline is being reconfigured.

Takashi,

What's the status of Media Controller adoption on ALSA?

Regards,
Mauro