[PATCH 0/5] media token resource framework

[PATCH 0/5] 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, and au0828
driver.

With these changes dvb and v4l2 can share the tuner without
disrupting each other. Used tvtime, xawtv, kaffeine, and
vlc to 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.

Shuah Khan (5):
  media: add media token device resource framework
  media: v4l2-core changes to use media tuner token api
  media: au0828-video changes to use media tuner token api
  media: dvb-core changes to use media tuner token api
  media: au0828-core changes to create and destroy media token res

 MAINTAINERS                             |    2 +
 drivers/media/dvb-core/dvb_frontend.c   |   10 +
 drivers/media/usb/au0828/au0828-core.c  |   23 ++
 drivers/media/usb/au0828/au0828-video.c |   43 +++-
 drivers/media/v4l2-core/v4l2-fh.c       |   16 ++
 drivers/media/v4l2-core/v4l2-ioctl.c    |   96 +++++++-
 include/linux/media_tknres.h            |   98 +++++++++
 lib/Makefile                            |    2 +
 lib/media_tknres.c                      |  361 +++++++++++++++++++++++++++++++
 9 files changed, 648 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/media_tknres.h
 create mode 100644 lib/media_tknres.c

-- 
1.7.10.4

[PATCH 1/5] 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.

The media token resource contains token for tuner, dma, and
audio. Each token has valid modes or states it can be in.
When a token is in one of the exclusive modes, only one owner
is allowed. When a token is in shared mode, owners with the
request the token in the same mode in shared mode are allowed
to share the token.

As an example, when tuner token is held digital and exclusive
mode, no other requests are granted. On the other hand, when
the tuner token is held in shared analog mode, other requests
for shared analog access are granted.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index aefa948..861f6c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5821,6 +5821,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..65a24df
--- /dev/null
+++ b/include/linux/media_tknres.h
@@ -0,0 +1,98 @@
+/*
+ * 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;
+
+enum media_tkn_mode {
+	MEDIA_MODE_DVB,
+	MEDIA_MODE_ANALOG,
+	MEDIA_MODE_RADIO,
+};
+
+#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, enum media_tkn_mode mode);
+extern int media_get_shared_tuner_tkn(struct device *dev,
+				enum media_tkn_mode mode);
+extern int media_put_tuner_tkn(struct device *dev, enum media_tkn_mode mode);
+extern int media_reset_shared_tuner_tkn(struct device *dev,
+				enum media_tkn_mode mode);
+
+extern int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode);
+extern int media_get_shared_dma_tkn(struct device *dev,
+				enum media_tkn_mode mode);
+extern int media_put_dma_tkn(struct device *dev, enum media_tkn_mode mode);
+
+extern int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode);
+extern int media_get_shared_audio_tkn(struct device *dev,
+				enum media_tkn_mode mode);
+extern int media_put_audio_tkn(struct device *dev, enum media_tkn_mode mode);
+#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,
+					enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_get_shared_tuner_tkn(struct device *dev,
+						enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_put_tuner_tkn(struct device *dev)
+{
+	return 0;
+}
+static inline int media_reset_shared_tuner_tkn(struct device *dev,
+						enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_get_dma_tkn(struct device *dev,
+					enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_get_shared_dma_tkn(struct device *dev,
+						enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_put_dma_tkn(struct device *dev)
+{
+	return 0;
+}
+static inline int media_get_audio_tkn(struct device *dev,
+					enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline int media_get_shared_audio_tkn(struct device *dev,
+						enum media_tkn_mode mode)
+{
+	return 0;
+}
+static inline 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..863336b
--- /dev/null
+++ b/lib/media_tknres.c
@@ -0,0 +1,361 @@
+/*
+ * 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, dma,
+ * and audio. Each token has valid modes or states it can be in.
+ * When a token is in one of the exclusive modes, only one owner is
+ * allowed. When a token is in shared mode, owners with the same
+ * mode request are allowed to share the token.
+ *
+ * API
+ *	int media_tknres_create(struct device *dev);
+ *	int media_tknres_destroy(struct device *dev);
+ *
+ *	int media_get_tuner_tkn(struct device *dev, enum media_tkn_mode mode);
+ *	int media_get_shared_tuner_tkn(struct device *dev,
+ *					enum media_tkn_mode mode);
+ *	int media_put_tuner_tkn(struct device *dev)
+ *					enum media_tkn_mode mode);
+ *	int media_reset_shared_tuner_tkn(struct device *dev,
+ *						enum media_tkn_mode mode);
+ *
+ *	int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode);
+ *	int media_get_shared_dma_tkn(struct device *dev,
+ *					enum media_tkn_mode mode);
+ *	int media_put_dma_tkn(struct device *dev)
+ *					enum media_tkn_mode mode);
+ *
+ *	int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode);
+ *	int media_get_shared_audio_tkn(struct device *dev,
+ *					enum media_tkn_mode mode);
+ *	int media_put_audio_tkn(struct device *dev)
+ *					enum media_tkn_mode mode);
+ * Not yet implemented:
+ *	int media_reset_shared_dma_tkn(struct device *dev,
+ *					enum media_tkn_mode mode);
+ *	int media_reset_shared_adudio_tkn(struct device *dev,
+ *					enum media_tkn_mode mode);
+*/
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/media_tknres.h>
+
+struct media_tkn {
+	spinlock_t lock;
+	enum media_tkn_mode mode;
+	int owners;
+	bool is_exclusive;
+};
+
+struct media_tknres {
+	struct media_tkn tuner;
+	struct media_tkn dma;
+	struct media_tkn 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;
+
+	tkn->tuner.mode = MEDIA_MODE_DVB;
+	spin_lock_init(&tkn->tuner.lock);
+	tkn->tuner.owners = 0;
+	tkn->tuner.is_exclusive = false;
+
+	tkn->dma.mode = MEDIA_MODE_DVB;
+	spin_lock_init(&tkn->dma.lock);
+	tkn->dma.owners = 0;
+	tkn->tuner.is_exclusive = false;
+
+	tkn->audio.mode = MEDIA_MODE_DVB;
+	spin_lock_init(&tkn->audio.lock);
+	tkn->audio.owners = 0;
+	tkn->tuner.is_exclusive = false;
+
+	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,
+				enum media_tkn_mode mode, bool exclusive)
+{
+	int rc = 0;
+
+	spin_lock(&tkn->lock);
+	if (tkn->is_exclusive)
+		rc = -EBUSY;
+	else if (tkn->owners && ((mode != tkn->mode) || exclusive))
+		rc = -EBUSY;
+	else {
+		if (tkn->owners < INT_MAX)
+			tkn->owners++;
+		else
+			tkn->owners = 1;
+		tkn->mode = mode;
+		tkn->is_exclusive = exclusive;
+		pr_debug("%s: Media Token Resource get (%d - %d - %d)\n",
+			__func__, tkn->mode, tkn->owners, tkn->is_exclusive);
+	}
+	spin_unlock(&tkn->lock);
+	pr_debug("%s: Media Token Resource get rc = %d exclusive %d\n",
+		__func__, rc, exclusive);
+	return rc;
+}
+
+static int __media_put_tkn(struct media_tkn *tkn,
+				enum media_tkn_mode mode)
+{
+	int rc = 0;
+
+	spin_lock(&tkn->lock);
+	if (tkn->owners == 0) {
+		pr_debug("%s: Media Token Resource put with %d owners\n",
+				__func__, tkn->owners);
+		rc = -EINVAL;
+	} else if (mode != tkn->mode) {
+		pr_debug("%s: Media Token Resource put wrong mode (%d - %d)\n",
+			__func__, mode, tkn->mode);
+		rc = -EINVAL;
+	} else {
+		tkn->owners--;
+		if (tkn->owners == 0 && tkn->is_exclusive)
+			tkn->is_exclusive = false;
+		pr_debug("%s: Media Token Resource put (%d - %d - %d)\n",
+			__func__, tkn->mode, tkn->owners, tkn->is_exclusive);
+	}
+	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, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->tuner, mode, true);
+}
+EXPORT_SYMBOL_GPL(media_get_tuner_tkn);
+
+int media_get_shared_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->tuner, mode, false);
+}
+EXPORT_SYMBOL_GPL(media_get_shared_tuner_tkn);
+
+int media_put_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource put mode %d\n",
+			__func__, mode);
+	return __media_put_tkn(&tkn_ptr->tuner, mode);
+}
+EXPORT_SYMBOL_GPL(media_put_tuner_tkn);
+
+int media_reset_shared_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	struct media_tknres *tkn_ptr;
+	struct media_tkn *tkn;
+
+	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;
+	}
+
+	tkn = &tkn_ptr->tuner;
+	spin_lock(&tkn->lock);
+	if (tkn->is_exclusive || mode != tkn->mode || tkn->owners == 0)
+		goto done;
+	tkn->owners = 0;
+	tkn->mode = MEDIA_MODE_DVB;
+	tkn->is_exclusive = false;
+	dev_dbg(dev, "%s: Media Token Resource reset done mode %d\n",
+			__func__, mode);
+done:
+	spin_unlock(&tkn->lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_reset_shared_tuner_tkn);
+
+int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->dma, mode, true);
+}
+EXPORT_SYMBOL_GPL(media_get_dma_tkn);
+
+int media_get_shared_dma_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->dma, mode, false);
+}
+EXPORT_SYMBOL_GPL(media_get_shared_dma_tkn);
+
+int media_put_dma_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource put mode=%d\n",
+			__func__, mode);
+	return __media_put_tkn(&tkn_ptr->dma, mode);
+}
+EXPORT_SYMBOL_GPL(media_put_dma_tkn);
+
+int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->audio, mode, true);
+}
+EXPORT_SYMBOL_GPL(media_get_audio_tkn);
+
+int media_get_shared_audio_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
+			__func__, mode);
+	return __media_get_tkn(&tkn_ptr->audio, mode, false);
+}
+EXPORT_SYMBOL_GPL(media_get_shared_audio_tkn);
+
+int media_put_audio_tkn(struct device *dev, enum media_tkn_mode mode)
+{
+	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;
+	}
+
+	dev_dbg(dev, "%s: Media Token Resource put mode=%d\n",
+			__func__, mode);
+	return __media_put_tkn(&tkn_ptr->audio, mode);
+}
+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

[PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

Changes to v4l2-core to hold tuner token in v4l2 ioctl that change
the tuner modes, and reset 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_OUTPUT, S_FMT, S_TUNER, S_MODULATOR, S_FREQUENCY,
S_STD, S_HW_FREQ_SEEK:

- hold tuner in shared analog mode before calling appropriate
  ops->vidioc_s_*
- return leaving tuner in shared mode.
- Note that S_MODULATOR is now implemented in the core
  previously FCN.

QUERYSTD:
- hold tuner in shared analog mode before calling
  ops->vidioc_querystd
- return after calling put tuner - this simply decrements the
  owners. Leaves tuner in shared analog mode if owners > 0

v4l2_fh_exit:
- resets the media tuner token. A simple put token could leave
  the token in shared mode. The nature of analog token holds
  is unbalanced requiring a reset to clear it.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/v4l2-core/v4l2-fh.c    |   16 ++++++
 drivers/media/v4l2-core/v4l2-ioctl.c |   96 +++++++++++++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index c97067a..81ce3f9 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,19 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 {
 	if (fh->vdev == NULL)
 		return;
+
+	if (fh->vdev->dev_parent) {
+		enum media_tkn_mode mode;
+
+		mode = (fh->vdev->vfl_type == V4L2_TUNER_RADIO) ?
+			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
+		/* reset the token - the nature of token get in
+		   analog mode is shared and unbalanced. There is
+		   no clear start and stop, so shared token might
+		   never get cleared */
+		media_reset_shared_tuner_tkn(fh->vdev->dev_parent, mode);
+	}
+
 	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 d15e167..9e1f042 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,37 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
 	       sizeof(fmt->fmt.pix) - offset);
 }
 
+static int v4l_get_tuner_tkn(struct video_device *vfd,
+				enum v4l2_tuner_type type)
+{
+	int ret = 0;
+
+	if (vfd->dev_parent) {
+		enum media_tkn_mode mode;
+
+		mode = (type == V4L2_TUNER_RADIO) ?
+			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
+		ret = media_get_shared_tuner_tkn(vfd->dev_parent, mode);
+		if (ret)
+			dev_info(vfd->dev_parent,
+				"%s: Tuner is busy\n", __func__);
+	}
+	dev_dbg(vfd->dev_parent, "%s: No token?? %d\n", __func__, ret);
+	return ret;
+}
+
+static void v4l_put_tuner_tkn(struct video_device *vfd,
+				enum v4l2_tuner_type type)
+{
+	if (vfd->dev_parent) {
+		enum media_tkn_mode mode;
+
+		mode = (type == V4L2_TUNER_RADIO) ?
+			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
+		media_put_tuner_tkn(vfd->dev_parent, mode);
+	}
+}
+
 static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1022,12 +1054,24 @@ 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, V4L2_TUNER_ANALOG_TV);
+	if (ret)
+		return ret;
 	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
 }
 
 static int v4l_s_output(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, V4L2_TUNER_ANALOG_TV);
+	if (ret)
+		return ret;
 	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
 }
 
@@ -1236,6 +1280,10 @@ 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, V4L2_TUNER_ANALOG_TV);
+	if (ret)
+		return ret;
+
 	v4l_sanitize_format(p);
 
 	switch (p->type) {
@@ -1415,9 +1463,13 @@ 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, p->type);
+	if (ret)
+		return ret;
 	return ops->vidioc_s_tuner(file, fh, p);
 }
 
@@ -1433,6 +1485,26 @@ static int v4l_g_modulator(const struct v4l2_ioctl_ops *ops,
 	return err;
 }
 
+static int v4l_s_modulator(const struct v4l2_ioctl_ops *ops,
+				struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+	struct v4l2_fh *vfh =
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+
+	if (vfh != NULL) {
+		int ret;
+		enum v4l2_tuner_type type;
+
+		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+		ret = v4l_get_tuner_tkn(vfd, type);
+		if (ret)
+			return ret;
+	}
+	return ops->vidioc_s_modulator(file, fh, arg);
+}
+
 static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1453,6 +1525,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 +1535,9 @@ 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, type);
+		if (ret)
+			return ret;
 	}
 	return ops->vidioc_s_frequency(file, fh, p);
 }
@@ -1508,11 +1584,16 @@ 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, V4L2_TUNER_ANALOG_TV);
+	if (ret)
+		return ret;
+
 	/* Calls the specific handler */
 	return ops->vidioc_s_std(file, fh, norm);
 }
@@ -1522,7 +1603,11 @@ 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, V4L2_TUNER_ANALOG_TV);
+	if (ret)
+		return ret;
 	/*
 	 * If no signal is detected, then the driver should return
 	 * V4L2_STD_UNKNOWN. Otherwise it should return tvnorms with
@@ -1532,7 +1617,9 @@ static int v4l_querystd(const struct v4l2_ioctl_ops *ops,
 	 * their efforts to improve the standards detection.
 	 */
 	*p = vfd->tvnorms;
-	return ops->vidioc_querystd(file, fh, arg);
+	ret = ops->vidioc_querystd(file, fh, arg);
+	v4l_put_tuner_tkn(vfd, V4L2_TUNER_ANALOG_TV);
+	return ret;
 }
 
 static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
@@ -1541,6 +1628,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 +1638,9 @@ 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, type);
+	if (ret)
+		return ret;
 	return ops->vidioc_s_hw_freq_seek(file, fh, p);
 }
 
@@ -2217,7 +2308,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout, v4l_print_audioout, 0),
 	IOCTL_INFO_STD(VIDIOC_S_AUDOUT, vidioc_s_audout, v4l_print_audioout, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_G_MODULATOR, v4l_g_modulator, v4l_print_modulator, INFO_FL_CLEAR(v4l2_modulator, index)),
-	IOCTL_INFO_STD(VIDIOC_S_MODULATOR, vidioc_s_modulator, v4l_print_modulator, INFO_FL_PRIO),
+	IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_modulator,
+			v4l_print_frequency, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_G_FREQUENCY, v4l_g_frequency, v4l_print_frequency, INFO_FL_CLEAR(v4l2_frequency, tuner)),
 	IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_frequency, v4l_print_frequency, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
-- 
1.7.10.4

[PATCH 3/5] media: au0828-video changes to use media tuner 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 token 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 in shared analog mode
- return leaving tuner in shared mode

vb1 streamon:
- hold tuner in shared analog mode
- return leaving tuner in shared mode

release v4l2_file_operations:
- hold tuner in exclusive analog mode to power down.
  Don't call s_power when tuner is busy.

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 |   43 ++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
index 5f337b1..52a3644 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, MEDIA_MODE_ANALOG);
+		if (ret) {
+			dev_info(&dev->usbdev->dev,
+					"%s: Tuner is busy\n", __func__);
+			/* don't touch tuner and usb device interface */
+			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, MEDIA_MODE_ANALOG);
 
+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_shared_tuner_tkn(&dev->usbdev->dev, MEDIA_MODE_ANALOG);
+	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_shared_tuner_tkn(&dev->usbdev->dev, MEDIA_MODE_ANALOG);
+	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,16 +1572,24 @@ 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_shared_tuner_tkn(&dev->usbdev->dev, MEDIA_MODE_ANALOG);
+	if (ret) {
+		dev_info(&dev->usbdev->dev, "%s: Tuner is busy\n", __func__);
+		return -EBUSY;
+	}
+
 	strcpy(t->name, "Auvitek tuner");
 
 	au0828_init_tuner(dev);
 	i2c_gate_ctrl(dev, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_tuner, t);
 	i2c_gate_ctrl(dev, 0);
+	media_put_tuner_tkn(&dev->usbdev->dev, MEDIA_MODE_ANALOG);
 	return 0;
 }
 
@@ -1682,6 +1714,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_shared_tuner_tkn(&dev->usbdev->dev, MEDIA_MODE_ANALOG);
+	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 +2121,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 4/5] media: dvb-core changes to use media tuner token api

[Shuah Khan]

Change to hold media tuner token exclusive mode in
dvb_frontend_start() before starting the dvb thread
and release from dvb_frontend_thread() when thread
exits. dvb frontend thread is started only when the
dvb device is opened in Read/Write mode.

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

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index c862ad7..22833c6 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"
@@ -742,6 +743,7 @@ restart:
 	if (semheld)
 		up(&fepriv->sem);
 	dvb_frontend_wakeup(fe);
+	media_put_tuner_tkn(fe->dvb->device, MEDIA_MODE_DVB);
 	return 0;
 }
 
@@ -836,6 +838,13 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 	fepriv->state = FESTATE_IDLE;
 	fe->exit = DVB_FE_NO_EXIT;
 	fepriv->thread = NULL;
+
+	ret = media_get_tuner_tkn(fe->dvb->device, MEDIA_MODE_DVB);
+	if (ret == -EBUSY) {
+		dev_info(fe->dvb->device, "dvb: Tuner is busy\n");
+		return ret;
+	}
+
 	mb();
 
 	fe_thread = kthread_run(dvb_frontend_thread, fe,
@@ -846,6 +855,7 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 				"dvb_frontend_start: failed to start kthread (%d)\n",
 				ret);
 		up(&fepriv->sem);
+		media_put_tuner_tkn(fe->dvb->device, MEDIA_MODE_DVB);
 		return ret;
 	}
 	fepriv->thread = fe_thread;
-- 
1.7.10.4

[PATCH 5/5] media: au0828-core changes to create and destroy media token res

[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 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

Hi Devin,

On 09/22/2014 01:21 PM, Devin Heitmueller wrote:
> Hi Shuah,
> 
> What about G_INPUT and G_TUNER?  Consider the following use case, which is
> entirely legal in the V4L2 API:

Did you mean G_INPUT and G_STD here? I didn't see G_TUNER mentioned
below in the use-case.

I didn't know this use-case, I did notice ENUM_INPUT getting called
in a loop during testing. I think I didn't run into this because I
changed the au0828: vidioc_g_tuner to hold the lock as it does a tuner
init and messes up the tuner when it is in use by dvb. But, that doesn't
cover this use-case for other drivers. So I need to make more changes
to cover it. Thanks for pointing this out.

> 
> 1.  Program opens /dev/video0
> 2.  Program calls G_INPUT/G_STD and sees that the appropriate input and
> standard are already set, since all devices have a default input at
> initialization
> 3.  Program never calls S_INPUT, S_STD, or S_TUNER
> 4.  Program goes into a loop calling ENUM_INPUT, waiting until it returns
> the input as having signal lock
> 5.  When signal lock is seen, program calls STREAMON.

I am missing vb2 streamon change to hold the tuner in this patch set.
Without that change vb2 work isn't complete. Unfortunately I don't
have hybrid hardware that uses a vb2 driver.

> 
> In the above case, you would be actively using the au8522 video decoder but
> not holding the lock, so thr DVB device can be opened and screw everything
> up.  Likewise if the DVB device were in use and such a program were run, it
> wouls break.
> 

I think this use-case will be covered with changes to vb2 streamon
to check and hold tuner. I am thinking it might not be necessary to
change g_tuner, g_std, g_input and enum_input at v4l2-core level.
Does that sounds right??

thanks,
-- Shuah


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

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Devin Heitmueller]

Hello Shuah,

>> What about G_INPUT and G_TUNER?  Consider the following use case, which is
>> entirely legal in the V4L2 API:
>
> Did you mean G_INPUT and G_STD here? I didn't see G_TUNER mentioned
> below in the use-case.

It can be either ENUM_INPUT or G_TUNER.  Both return status
information that requires communication with the video decoder chip
and/or tuner.  It's probably worth mentioning that ENUMINPUT isn't
like the other ENUM_ calls in that it doesn't return a static list
without  talking to the driver - it contains a field (called .status)
which actually needs to talk to the hardware in order to populate it.

>> 1.  Program opens /dev/video0
>> 2.  Program calls G_INPUT/G_STD and sees that the appropriate input and
>> standard are already set, since all devices have a default input at
>> initialization
>> 3.  Program never calls S_INPUT, S_STD, or S_TUNER
>> 4.  Program goes into a loop calling ENUM_INPUT, waiting until it returns
>> the input as having signal lock
>> 5.  When signal lock is seen, program calls STREAMON.
>
> I am missing vb2 streamon change to hold the tuner in this patch set.
> Without that change vb2 work isn't complete. Unfortunately I don't
> have hybrid hardware that uses a vb2 driver.

I don't think you quite understood my concern.  The concern is that in
the use case above I'm actively using the tuner *before*
VIDIOC_STREAMON is called.  Hence from a locking standpoint you
probably don't want to allow the DVB device to take control of the
tuner.

>>
>> In the above case, you would be actively using the au8522 video decoder but
>> not holding the lock, so thr DVB device can be opened and screw everything
>> up.  Likewise if the DVB device were in use and such a program were run, it
>> wouls break.
>>
>
> I think this use-case will be covered with changes to vb2 streamon
> to check and hold tuner. I am thinking it might not be necessary to
> change g_tuner, g_std, g_input and enum_input at v4l2-core level.
> Does that sounds right??

The more I think about it, the less confident I am that you actually
can take a fine-grained locking approach without adding additional
ioctls to make it explicit.  When is the tuner unlocked?  Is it when
the filehandle is closed?  If so, then the the following script would
behave in an unexpected manner:

#!/bin/sh

while [ 1 ]; do
  v4l2-ctl -n
  # Some code to parse the output and see if the "status" field for
current input shows no signal
  # If status shows as locked break
done
v4l2-ctl --stream-mmap=500 --stream-to=/tmp/foo.bin

In the above case I'm actively using the tuner but not holding the
lock most of the time, so a separate process can grab the DVB device
between calls to "v4l2-ctl -n".

However if you're keeping the device locked until STREAMOFF, then
you'll break all sorts of applications which might just close the
filehandle without calling STREAMOFF, and hence you'll have cases
where the tuner is left locked in analog mode *forever*, preventing
apps from using it in digital mode.

Without adding a new ioctl to lock/unlock the analog side of the
device, there is no real way to deal with this perfectly legal use
case.  The downside of that of course is that applications would have
to be modified in order for the locking to be used, and the default
would have to be to not do locking in order to preserve backward
compatibility with existing applications.

What other ioctls have we not thought of?  I think there is an
argument to be made that we're being too aggressive in trying to
control the locking based on the ioctls called.  It might make sense
to simplify the approach to lock on when the device is opened, and
unlock when closed.  This avoids the complexity of trying to figure
out *which* ioctls we need to set the lock on (which likely varies on
a per device basis anyway), at the cost of not allowing the device to
be used when something has the filehandle opened for the other side of
the device (the behavior of which is currently undefined in the spec).
I know there are concerns that some apps might leave the FD open even
when done using it, but that seems like a less likely case than
properly handling fine-grained locking (causing unexpected behavior
for the applications that don't expect -EBUSY to be returned after the
device has been successfully opened).

We can always start with coarse locking on open/close, and do finer
grained locking down the road if needed - or simply change the
currently undefined behavior in the spec to say that you have to close
the device handle before attempting to open the other side of the
device.

On a related note, you should be testing with MythTV - none of the
applications you are currently testing with support both analog *and*
digital, so you are not seeing all the race conditions that can occur
when you close one side of the device and then *immediately* open the
other side.  In particular, there is a known race that occurs when
closing the DVB device and then opening the V4L device, because the
DVB frontend shuts down the tuner asynchronously after the close()
call returns to userland.  Exiting one application and starting
another provides plenty of time for the close() logic to be run, so
you're missing all the race conditions.

Devin


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

Hi Devin/Mauro/Hans,

Summarizing the discussion on v4l to keep others on this
thread in the loop. Please see below:

Hans! Could you please take a look and see if it raises
any red flags for you.

On 09/23/2014 08:17 AM, Devin Heitmueller wrote:

> 
> We can always start with coarse locking on open/close, and do finer
> grained locking down the road if needed - or simply change the
> currently undefined behavior in the spec to say that you have to close
> the device handle before attempting to open the other side of the
> device.
> 

I share the same concerns about fine grain locking approach that
requires changes to various v4l2 ioctls to hold the token. My
concern with the current approach is that we won't be able to find
all the v4l paths to secure. During my testing, it is clear that
several applications don't seem to check ioctls return codes and
even if one of the ioctls returns -EBUSY, applications keep calling
other ioctls instead of exiting with device busy condition.

Compared to the current approach, holding lock in open and releasing
it in close is cleaner with predictable failure conditions. It is lot
easier to maintain. In addition, this approach keeps it same as the
dvb core token hold approach as outlined below.

dvb on the other hand is easier with its clean entry and exit points.
In the case of dvb, the lock is held when the device is opened in
read/write mode before dvb front-end thread gets started and released
when thread exits.

I discussed this a couple of times in the past during development
for this current patch series. The concern has been that a number of
currently supported use-cases will break with the simpler approach
to lock when v4l device is opened and unlock when it is closed.

As we discussed this morning and agreed on giving the simpler
approach a try first keeping the finer grain locking option
open for revisit. This would be acceptable provided the token
code is tested on existing apps, including mythtv, kradio,
gnome-radio.

In addition to releasing the token at device close, release the token
if an app decides to use S_PRIORITY to release the streaming ownership
e. g. V4L2_PRIORITY_BACKGROUND

Devin recommended testing on devices that have an encoder to cover
the cases where there are multiple /dev/videoX nodes tied to the
same tuner.

Please check if I captured it correctly. I can get started on the
simpler approach and see where it takes us. I have to change the
v4l2 and driver v4l2 patches. dvb and the rest can stay the same.

thanks,
-- Shuah

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

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

[Hans Verkuil]

Hi Shuah,

Here is my review...

On 09/22/2014 05:00 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.
>
> The media token resource contains token for tuner, dma, and
> audio.

Why dma and audio? Neither is being used in this patch series. I
would leave them out until you actually show how they are used in a
driver.

>  Each token has valid modes or states it can be in.
> When a token is in one of the exclusive modes, only one owner
> is allowed. When a token is in shared mode, owners with the
> request the token in the same mode in shared mode are allowed
> to share the token.
>
> As an example, when tuner token is held digital and exclusive
> mode, no other requests are granted. On the other hand, when
> the tuner token is held in shared analog mode, other requests
> for shared analog access are granted.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>   MAINTAINERS                  |    2 +
>   include/linux/media_tknres.h |   98 ++++++++++++
>   lib/Makefile                 |    2 +
>   lib/media_tknres.c           |  361 ++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 463 insertions(+)
>   create mode 100644 include/linux/media_tknres.h
>   create mode 100644 lib/media_tknres.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aefa948..861f6c6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5821,6 +5821,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..65a24df
> --- /dev/null
> +++ b/include/linux/media_tknres.h
> @@ -0,0 +1,98 @@
> +/*
> + * 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;
> +
> +enum media_tkn_mode {
> +	MEDIA_MODE_DVB,
> +	MEDIA_MODE_ANALOG,
> +	MEDIA_MODE_RADIO,
> +};
> +
> +#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, enum media_tkn_mode mode);
> +extern int media_get_shared_tuner_tkn(struct device *dev,
> +				enum media_tkn_mode mode);
> +extern int media_put_tuner_tkn(struct device *dev, enum media_tkn_mode mode);
> +extern int media_reset_shared_tuner_tkn(struct device *dev,
> +				enum media_tkn_mode mode);
> +
> +extern int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode);
> +extern int media_get_shared_dma_tkn(struct device *dev,
> +				enum media_tkn_mode mode);
> +extern int media_put_dma_tkn(struct device *dev, enum media_tkn_mode mode);
> +
> +extern int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode);
> +extern int media_get_shared_audio_tkn(struct device *dev,
> +				enum media_tkn_mode mode);
> +extern int media_put_audio_tkn(struct device *dev, enum media_tkn_mode mode);
> +#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,
> +					enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_get_shared_tuner_tkn(struct device *dev,
> +						enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_put_tuner_tkn(struct device *dev)
> +{
> +	return 0;
> +}
> +static inline int media_reset_shared_tuner_tkn(struct device *dev,
> +						enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_get_dma_tkn(struct device *dev,
> +					enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_get_shared_dma_tkn(struct device *dev,
> +						enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_put_dma_tkn(struct device *dev)
> +{
> +	return 0;
> +}
> +static inline int media_get_audio_tkn(struct device *dev,
> +					enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline int media_get_shared_audio_tkn(struct device *dev,
> +						enum media_tkn_mode mode)
> +{
> +	return 0;
> +}
> +static inline 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..863336b
> --- /dev/null
> +++ b/lib/media_tknres.c
> @@ -0,0 +1,361 @@
> +/*
> + * 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, dma,
> + * and audio. Each token has valid modes or states it can be in.
> + * When a token is in one of the exclusive modes, only one owner is
> + * allowed. When a token is in shared mode, owners with the same
> + * mode request are allowed to share the token.
> + *
> + * API
> + *	int media_tknres_create(struct device *dev);
> + *	int media_tknres_destroy(struct device *dev);
> + *
> + *	int media_get_tuner_tkn(struct device *dev, enum media_tkn_mode mode);
> + *	int media_get_shared_tuner_tkn(struct device *dev,
> + *					enum media_tkn_mode mode);
> + *	int media_put_tuner_tkn(struct device *dev)
> + *					enum media_tkn_mode mode);
> + *	int media_reset_shared_tuner_tkn(struct device *dev,
> + *						enum media_tkn_mode mode);
> + *
> + *	int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode);
> + *	int media_get_shared_dma_tkn(struct device *dev,
> + *					enum media_tkn_mode mode);
> + *	int media_put_dma_tkn(struct device *dev)
> + *					enum media_tkn_mode mode);
> + *
> + *	int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode);
> + *	int media_get_shared_audio_tkn(struct device *dev,
> + *					enum media_tkn_mode mode);
> + *	int media_put_audio_tkn(struct device *dev)
> + *					enum media_tkn_mode mode);
> + * Not yet implemented:
> + *	int media_reset_shared_dma_tkn(struct device *dev,
> + *					enum media_tkn_mode mode);
> + *	int media_reset_shared_adudio_tkn(struct device *dev,
> + *					enum media_tkn_mode mode);
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/media_tknres.h>
> +
> +struct media_tkn {
> +	spinlock_t lock;
> +	enum media_tkn_mode mode;
> +	int owners;
> +	bool is_exclusive;
> +};
> +
> +struct media_tknres {
> +	struct media_tkn tuner;
> +	struct media_tkn dma;
> +	struct media_tkn 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;
> +
> +	tkn->tuner.mode = MEDIA_MODE_DVB;
> +	spin_lock_init(&tkn->tuner.lock);
> +	tkn->tuner.owners = 0;
> +	tkn->tuner.is_exclusive = false;
> +
> +	tkn->dma.mode = MEDIA_MODE_DVB;
> +	spin_lock_init(&tkn->dma.lock);
> +	tkn->dma.owners = 0;
> +	tkn->tuner.is_exclusive = false;
> +
> +	tkn->audio.mode = MEDIA_MODE_DVB;
> +	spin_lock_init(&tkn->audio.lock);
> +	tkn->audio.owners = 0;
> +	tkn->tuner.is_exclusive = false;
> +
> +	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,
> +				enum media_tkn_mode mode, bool exclusive)
> +{
> +	int rc = 0;
> +
> +	spin_lock(&tkn->lock);
> +	if (tkn->is_exclusive)
> +		rc = -EBUSY;
> +	else if (tkn->owners && ((mode != tkn->mode) || exclusive))
> +		rc = -EBUSY;
> +	else {
> +		if (tkn->owners < INT_MAX)
> +			tkn->owners++;
> +		else
> +			tkn->owners = 1;

Somewhat weird. Can owners ever become INT_MAX?

> +		tkn->mode = mode;
> +		tkn->is_exclusive = exclusive;
> +		pr_debug("%s: Media Token Resource get (%d - %d - %d)\n",
> +			__func__, tkn->mode, tkn->owners, tkn->is_exclusive);
> +	}
> +	spin_unlock(&tkn->lock);
> +	pr_debug("%s: Media Token Resource get rc = %d exclusive %d\n",
> +		__func__, rc, exclusive);
> +	return rc;
> +}
> +
> +static int __media_put_tkn(struct media_tkn *tkn,
> +				enum media_tkn_mode mode)
> +{
> +	int rc = 0;
> +
> +	spin_lock(&tkn->lock);
> +	if (tkn->owners == 0) {
> +		pr_debug("%s: Media Token Resource put with %d owners\n",
> +				__func__, tkn->owners);
> +		rc = -EINVAL;
> +	} else if (mode != tkn->mode) {
> +		pr_debug("%s: Media Token Resource put wrong mode (%d - %d)\n",
> +			__func__, mode, tkn->mode);
> +		rc = -EINVAL;
> +	} else {
> +		tkn->owners--;
> +		if (tkn->owners == 0 && tkn->is_exclusive)
> +			tkn->is_exclusive = false;
> +		pr_debug("%s: Media Token Resource put (%d - %d - %d)\n",
> +			__func__, tkn->mode, tkn->owners, tkn->is_exclusive);
> +	}
> +	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, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->tuner, mode, true);
> +}
> +EXPORT_SYMBOL_GPL(media_get_tuner_tkn);
> +
> +int media_get_shared_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->tuner, mode, false);
> +}
> +EXPORT_SYMBOL_GPL(media_get_shared_tuner_tkn);
> +
> +int media_put_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource put mode %d\n",
> +			__func__, mode);
> +	return __media_put_tkn(&tkn_ptr->tuner, mode);
> +}
> +EXPORT_SYMBOL_GPL(media_put_tuner_tkn);
> +
> +int media_reset_shared_tuner_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	struct media_tknres *tkn_ptr;
> +	struct media_tkn *tkn;
> +
> +	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;
> +	}
> +
> +	tkn = &tkn_ptr->tuner;
> +	spin_lock(&tkn->lock);
> +	if (tkn->is_exclusive || mode != tkn->mode || tkn->owners == 0)
> +		goto done;
> +	tkn->owners = 0;
> +	tkn->mode = MEDIA_MODE_DVB;
> +	tkn->is_exclusive = false;
> +	dev_dbg(dev, "%s: Media Token Resource reset done mode %d\n",
> +			__func__, mode);
> +done:
> +	spin_unlock(&tkn->lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_reset_shared_tuner_tkn);

Hmm, I think this is asking for problems. You are working around a problem
in the v4l2 API by adding this reset function, but you should address the
real issue. I have more about this in my review of patch 2.

If you do refcounting as is done here, then reset functions like this
basically defeat the whole refcounting idea. It's too easy to be abused.

> +
> +int media_get_dma_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->dma, mode, true);
> +}
> +EXPORT_SYMBOL_GPL(media_get_dma_tkn);
> +
> +int media_get_shared_dma_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->dma, mode, false);
> +}
> +EXPORT_SYMBOL_GPL(media_get_shared_dma_tkn);
> +
> +int media_put_dma_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource put mode=%d\n",
> +			__func__, mode);
> +	return __media_put_tkn(&tkn_ptr->dma, mode);
> +}
> +EXPORT_SYMBOL_GPL(media_put_dma_tkn);
> +
> +int media_get_audio_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->audio, mode, true);
> +}
> +EXPORT_SYMBOL_GPL(media_get_audio_tkn);
> +
> +int media_get_shared_audio_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource get mode=%d\n",
> +			__func__, mode);
> +	return __media_get_tkn(&tkn_ptr->audio, mode, false);
> +}
> +EXPORT_SYMBOL_GPL(media_get_shared_audio_tkn);
> +
> +int media_put_audio_tkn(struct device *dev, enum media_tkn_mode mode)
> +{
> +	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;
> +	}
> +
> +	dev_dbg(dev, "%s: Media Token Resource put mode=%d\n",
> +			__func__, mode);
> +	return __media_put_tkn(&tkn_ptr->audio, mode);
> +}
> +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);
>

As I mentioned at the beginning, these dma and audio tokens aren't used anywhere,
so just drop them for now.

Regards,

	Hans

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Hans Verkuil]

Hi Shuah,

Here is my review...

On 09/22/2014 05:00 PM, Shuah Khan wrote:
> Changes to v4l2-core to hold tuner token in v4l2 ioctl that change
> the tuner modes, and reset 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_OUTPUT, S_FMT, S_TUNER, S_MODULATOR, S_FREQUENCY,

S_MODULATOR doesn't need to take a token. Certainly not a tuner token,
since it is a modulator, not a tuner. There aren't many modulator drivers,
and none of them have different modes.

The same is true for S_OUTPUT: it deals with outputs, so it has nothing
to do with tuners since those are for input.

> S_STD, S_HW_FREQ_SEEK:
>
> - hold tuner in shared analog mode before calling appropriate
>    ops->vidioc_s_*
> - return leaving tuner in shared mode.
> - Note that S_MODULATOR is now implemented in the core
>    previously FCN.
>
> QUERYSTD:
> - hold tuner in shared analog mode before calling
>    ops->vidioc_querystd
> - return after calling put tuner - this simply decrements the
>    owners. Leaves tuner in shared analog mode if owners > 0

I would handle QUERYSTD the same as the ones in the first group.
It's a fair assumption that once you call QUERYSTD you expect to
switch the tuner to analog mode and keep it there.

I'm missing STREAMON in this list as well.

With regards to G_TUNER and ENUM_INPUT, I will reply to the post that
discusses that topic.

>
> v4l2_fh_exit:
> - resets the media tuner token. A simple put token could leave
>    the token in shared mode. The nature of analog token holds
>    is unbalanced requiring a reset to clear it.

Nack on the reset. It should just put the tuner token.

The way it should work is that whenever an ioctl is issued that needs to
take a tuner token, then it should check if the filehandle already has a
token. If not, then take the token if possible. If it has, then check if
the token is in the right mode and continue if that's the case, otherwise
return EBUSY. I don't think it can ever be in the wrong mode, but it
doesn't hurt to check. Or do WARN_ON or something like that.

This way each filehandle will take a token only once, and it is released
when the filehandle is closed.

You do need to store in struct v4l2_fh whether or not a token was obtained
so the v4l2_fh_exit knows to release it.

Almost all drivers support v4l2_fh by now, so I wouldn't bother with drivers
that don't support v4l2_fh.

>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>   drivers/media/v4l2-core/v4l2-fh.c    |   16 ++++++
>   drivers/media/v4l2-core/v4l2-ioctl.c |   96 +++++++++++++++++++++++++++++++++-
>   2 files changed, 110 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> index c97067a..81ce3f9 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,19 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>   {
>   	if (fh->vdev == NULL)
>   		return;
> +
> +	if (fh->vdev->dev_parent) {
> +		enum media_tkn_mode mode;
> +
> +		mode = (fh->vdev->vfl_type == V4L2_TUNER_RADIO) ?
> +			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
> +		/* reset the token - the nature of token get in
> +		   analog mode is shared and unbalanced. There is
> +		   no clear start and stop, so shared token might
> +		   never get cleared */
> +		media_reset_shared_tuner_tkn(fh->vdev->dev_parent, mode);

As mentioned above, this should just be a put, not a reset.

> +	}
> +
>   	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 d15e167..9e1f042 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,37 @@ static void v4l_sanitize_format(struct v4l2_format *fmt)
>   	       sizeof(fmt->fmt.pix) - offset);
>   }
>
> +static int v4l_get_tuner_tkn(struct video_device *vfd,
> +				enum v4l2_tuner_type type)
> +{
> +	int ret = 0;
> +
> +	if (vfd->dev_parent) {
> +		enum media_tkn_mode mode;
> +
> +		mode = (type == V4L2_TUNER_RADIO) ?
> +			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
> +		ret = media_get_shared_tuner_tkn(vfd->dev_parent, mode);
> +		if (ret)
> +			dev_info(vfd->dev_parent,
> +				"%s: Tuner is busy\n", __func__);
> +	}
> +	dev_dbg(vfd->dev_parent, "%s: No token?? %d\n", __func__, ret);

Shouldn't this be in a 'else'? Now the 'No token' message is also printed when
media_get_shared_tuner_tkn got the tuner.

> +	return ret;
> +}
> +
> +static void v4l_put_tuner_tkn(struct video_device *vfd,
> +				enum v4l2_tuner_type type)
> +{
> +	if (vfd->dev_parent) {
> +		enum media_tkn_mode mode;
> +
> +		mode = (type == V4L2_TUNER_RADIO) ?
> +			MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
> +		media_put_tuner_tkn(vfd->dev_parent, mode);
> +	}
> +}
> +
>   static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>   				struct file *file, void *fh, void *arg)
>   {
> @@ -1022,12 +1054,24 @@ 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, V4L2_TUNER_ANALOG_TV);
> +	if (ret)
> +		return ret;
>   	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>   }
>
>   static int v4l_s_output(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, V4L2_TUNER_ANALOG_TV);
> +	if (ret)
> +		return ret;
>   	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>   }

As mentioned, tuner tokens make no sense for s_output.

>
> @@ -1236,6 +1280,10 @@ 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, V4L2_TUNER_ANALOG_TV);
> +	if (ret)
> +		return ret;
> +
>   	v4l_sanitize_format(p);
>
>   	switch (p->type) {
> @@ -1415,9 +1463,13 @@ 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, p->type);
> +	if (ret)
> +		return ret;
>   	return ops->vidioc_s_tuner(file, fh, p);
>   }
>
> @@ -1433,6 +1485,26 @@ static int v4l_g_modulator(const struct v4l2_ioctl_ops *ops,
>   	return err;
>   }
>
> +static int v4l_s_modulator(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +	struct v4l2_fh *vfh =
> +		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
> +
> +	if (vfh != NULL) {
> +		int ret;
> +		enum v4l2_tuner_type type;
> +
> +		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> +		ret = v4l_get_tuner_tkn(vfd, type);
> +		if (ret)
> +			return ret;
> +	}
> +	return ops->vidioc_s_modulator(file, fh, arg);
> +}

Again, tuner tokens make no sense for a modulator.

> +
>   static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>   				struct file *file, void *fh, void *arg)
>   {
> @@ -1453,6 +1525,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 +1535,9 @@ 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, type);
> +		if (ret)
> +			return ret;
>   	}
>   	return ops->vidioc_s_frequency(file, fh, p);
>   }
> @@ -1508,11 +1584,16 @@ 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, V4L2_TUNER_ANALOG_TV);
> +	if (ret)
> +		return ret;
> +
>   	/* Calls the specific handler */
>   	return ops->vidioc_s_std(file, fh, norm);
>   }
> @@ -1522,7 +1603,11 @@ 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, V4L2_TUNER_ANALOG_TV);
> +	if (ret)
> +		return ret;
>   	/*
>   	 * If no signal is detected, then the driver should return
>   	 * V4L2_STD_UNKNOWN. Otherwise it should return tvnorms with
> @@ -1532,7 +1617,9 @@ static int v4l_querystd(const struct v4l2_ioctl_ops *ops,
>   	 * their efforts to improve the standards detection.
>   	 */
>   	*p = vfd->tvnorms;
> -	return ops->vidioc_querystd(file, fh, arg);
> +	ret = ops->vidioc_querystd(file, fh, arg);
> +	v4l_put_tuner_tkn(vfd, V4L2_TUNER_ANALOG_TV);

No put needed.

> +	return ret;
>   }
>
>   static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
> @@ -1541,6 +1628,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 +1638,9 @@ 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, type);
> +	if (ret)
> +		return ret;
>   	return ops->vidioc_s_hw_freq_seek(file, fh, p);
>   }
>
> @@ -2217,7 +2308,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>   	IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout, v4l_print_audioout, 0),
>   	IOCTL_INFO_STD(VIDIOC_S_AUDOUT, vidioc_s_audout, v4l_print_audioout, INFO_FL_PRIO),
>   	IOCTL_INFO_FNC(VIDIOC_G_MODULATOR, v4l_g_modulator, v4l_print_modulator, INFO_FL_CLEAR(v4l2_modulator, index)),
> -	IOCTL_INFO_STD(VIDIOC_S_MODULATOR, vidioc_s_modulator, v4l_print_modulator, INFO_FL_PRIO),
> +	IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_modulator,
> +			v4l_print_frequency, INFO_FL_PRIO),

Huh? S_MODULATOR is removed and a duplicate S_FREQUENCY is added?

>   	IOCTL_INFO_FNC(VIDIOC_G_FREQUENCY, v4l_g_frequency, v4l_print_frequency, INFO_FL_CLEAR(v4l2_frequency, tuner)),
>   	IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_frequency, v4l_print_frequency, INFO_FL_PRIO),
>   	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
>

BTW, I still haven't seen how you are going to handle snd-usb-audio. I want to
see how that is going to be handled before I will Ack anything.

Regards,

	Hans

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Hans Verkuil]

Hi Shuah,

On 09/23/2014 10:46 PM, Shuah Khan wrote:
> Hi Devin/Mauro/Hans,
>
> Summarizing the discussion on v4l to keep others on this
> thread in the loop. Please see below:
>
> Hans! Could you please take a look and see if it raises
> any red flags for you.
>
> On 09/23/2014 08:17 AM, Devin Heitmueller wrote:
>
>>
>> We can always start with coarse locking on open/close, and do finer
>> grained locking down the road if needed - or simply change the
>> currently undefined behavior in the spec to say that you have to close
>> the device handle before attempting to open the other side of the
>> device.
>>
>
> I share the same concerns about fine grain locking approach that
> requires changes to various v4l2 ioctls to hold the token. My
> concern with the current approach is that we won't be able to find
> all the v4l paths to secure. During my testing, it is clear that
> several applications don't seem to check ioctls return codes and
> even if one of the ioctls returns -EBUSY, applications keep calling
> other ioctls instead of exiting with device busy condition.

Let's be realistic: if an application doesn't test for error codes,
then that's the problem of the application. Also, EBUSY will only be
returned if someone else is holding the tuner in a different mode.
And that didn't work anyway (and probably in worse ways too if the
driver would forcefully change the tuner mode).

So I really don't see a problem with this.

>
> Compared to the current approach, holding lock in open and releasing
> it in close is cleaner with predictable failure conditions. It is lot
> easier to maintain. In addition, this approach keeps it same as the
> dvb core token hold approach as outlined below.

Absolutely not an option for v4l2. You should always be able to open
a v4l2 device, regardless of the tuner mode or any other mode.

The only exception is a radio tuner device: that should take the tuner
on open. I think this is a very unfortunate design and I wish that that
could be dropped.

One thing that we haven't looked at at all is what should be done if
the current input is not a tuner but a composite or S-Video input.

It is likely (I would have to test this to be sure) that you can capture
from a DVB tuner and at the same time from an S-Video input without any
problems. By taking the tuner token unconditionally this would become
impossible. But doing this right will require more work.

BTW, what happens if the analog video part of a hybrid board doesn't have
a tuner but only S-Video/Composite inputs? I think I've seen such boards
actually. I would have to dig through my collection though.

>
> dvb on the other hand is easier with its clean entry and exit points.
> In the case of dvb, the lock is held when the device is opened in
> read/write mode before dvb front-end thread gets started and released
> when thread exits.
>
> I discussed this a couple of times in the past during development
> for this current patch series. The concern has been that a number of
> currently supported use-cases will break with the simpler approach
> to lock when v4l device is opened and unlock when it is closed.
>
> As we discussed this morning and agreed on giving the simpler
> approach a try first keeping the finer grain locking option
> open for revisit. This would be acceptable provided the token
> code is tested on existing apps, including mythtv, kradio,
> gnome-radio.

Nack from me. Taking a tuner token should only happen when the device
actually needs the tuner. For DVB that's easy, since whenever you open
the frontend you *know* you want the tuner.

But that's much more difficult for V4L2 since there are so many combinations,
including many that don't need a tuner at all such as HDMI, Composite etc.
inputs.

>
> In addition to releasing the token at device close, release the token
> if an app decides to use S_PRIORITY to release the streaming ownership
> e. g. V4L2_PRIORITY_BACKGROUND

S_PRIORITY has nothing to do with tuner ownership. If there is a real need
to release the token at another time than on close (and I don't see
such a need), then make a new ioctl. Let's not overload S_PRIO with an
unrelated action.

>
> Devin recommended testing on devices that have an encoder to cover
> the cases where there are multiple /dev/videoX nodes tied to the
> same tuner.

Good examples are cx23885 (already vb2) and cx88 (the vb2 patches have
been posted, but not yet merged). It shouldn't be too hard to find
hardware based on those chipsets.

>
> Please check if I captured it correctly. I can get started on the
> simpler approach and see where it takes us. I have to change the
> v4l2 and driver v4l2 patches. dvb and the rest can stay the same.

As mentioned, Nack for the simpler approach from me. That's simply
too simple :-)

Note: I'm travelling and attending a conference, so my availability for
the rest of the week will probably be limited.

Regards,

	Hans

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

[Shuah Khan]

On 09/24/2014 05:26 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> Here is my review...
> 
> On 09/22/2014 05:00 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.
>>
>> The media token resource contains token for tuner, dma, and
>> audio.
> 
> Why dma and audio? Neither is being used in this patch series. I
> would leave them out until you actually show how they are used in a
> driver.

Yeah I can remove these. I am not using them in this series.

>> +static int __media_get_tkn(struct media_tkn *tkn,
>> +                enum media_tkn_mode mode, bool exclusive)
>> +{
>> +    int rc = 0;
>> +
>> +    spin_lock(&tkn->lock);
>> +    if (tkn->is_exclusive)
>> +        rc = -EBUSY;
>> +    else if (tkn->owners && ((mode != tkn->mode) || exclusive))
>> +        rc = -EBUSY;
>> +    else {
>> +        if (tkn->owners < INT_MAX)
>> +            tkn->owners++;
>> +        else
>> +            tkn->owners = 1;
> 
> Somewhat weird. Can owners ever become INT_MAX?
> 

I didn't have this at first. I was testing with tvtime and
noticed the count going up to 40k+ while it is streaming.
The count kept going up. So I figured I might as well add
the check to cover for cases where application like tvtime
run for a very longtime like a couple of hours.

thanks,
-- Shuah


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

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

On 09/24/2014 05:57 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> Here is my review...
> 
> On 09/22/2014 05:00 PM, Shuah Khan wrote:
>> Changes to v4l2-core to hold tuner token in v4l2 ioctl that change
>> the tuner modes, and reset 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_OUTPUT, S_FMT, S_TUNER, S_MODULATOR, S_FREQUENCY,
> 
> S_MODULATOR doesn't need to take a token. Certainly not a tuner token,
> since it is a modulator, not a tuner. There aren't many modulator drivers,
> and none of them have different modes.
> 
> The same is true for S_OUTPUT: it deals with outputs, so it has nothing
> to do with tuners since those are for input.

ok I will remove these. I will have to check why I added
S_OUTPUT - maybe au0828 does something in its vidioc_
If it does something it can be handled in the driver.
S_MODULATOR can go - I kept adding ioctls as I detected them
to cause problems, and this might have been one of the ones
I added as the first set.

> 
>> S_STD, S_HW_FREQ_SEEK:
>>
>> - hold tuner in shared analog mode before calling appropriate
>>    ops->vidioc_s_*
>> - return leaving tuner in shared mode.
>> - Note that S_MODULATOR is now implemented in the core
>>    previously FCN.
>>
>> QUERYSTD:
>> - hold tuner in shared analog mode before calling
>>    ops->vidioc_querystd
>> - return after calling put tuner - this simply decrements the
>>    owners. Leaves tuner in shared analog mode if owners > 0
> 
> I would handle QUERYSTD the same as the ones in the first group.
> It's a fair assumption that once you call QUERYSTD you expect to
> switch the tuner to analog mode and keep it there.
> 
> I'm missing STREAMON in this list as well.

Yes on STREAMON. ok I will make the changes. Do you want to see
STREAMON changes in vb1 or vb2? In our previous discussion, we
decides to make the STERANON changes in vb2 layer and handle
vb1 in the driver.

> 
> With regards to G_TUNER and ENUM_INPUT, I will reply to the post that
> discusses that topic.
> 
>>
>> v4l2_fh_exit:
>> - resets the media tuner token. A simple put token could leave
>>    the token in shared mode. The nature of analog token holds
>>    is unbalanced requiring a reset to clear it.
> 
> Nack on the reset. It should just put the tuner token.
> 
> The way it should work is that whenever an ioctl is issued that needs to
> take a tuner token, then it should check if the filehandle already has a
> token. If not, then take the token if possible. If it has, then check if
> the token is in the right mode and continue if that's the case, otherwise
> return EBUSY. I don't think it can ever be in the wrong mode, but it
> doesn't hurt to check. Or do WARN_ON or something like that.

Yes that would work. Thanks.

> 
> This way each filehandle will take a token only once, and it is released
> when the filehandle is closed.
> 
> You do need to store in struct v4l2_fh whether or not a token was obtained
> so the v4l2_fh_exit knows to release it.
> 
> Almost all drivers support v4l2_fh by now, so I wouldn't bother with
> drivers
> that don't support v4l2_fh.

ok. au0828 calls fh_exit() from its close routine and then does a tuner
reset later. I did change the driver close to handle this case.

> 
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-fh.c    |   16 ++++++
>>   drivers/media/v4l2-core/v4l2-ioctl.c |   96
>> +++++++++++++++++++++++++++++++++-
>>   2 files changed, 110 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c
>> b/drivers/media/v4l2-core/v4l2-fh.c
>> index c97067a..81ce3f9 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,19 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>>   {
>>       if (fh->vdev == NULL)
>>           return;
>> +
>> +    if (fh->vdev->dev_parent) {
>> +        enum media_tkn_mode mode;
>> +
>> +        mode = (fh->vdev->vfl_type == V4L2_TUNER_RADIO) ?
>> +            MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
>> +        /* reset the token - the nature of token get in
>> +           analog mode is shared and unbalanced. There is
>> +           no clear start and stop, so shared token might
>> +           never get cleared */
>> +        media_reset_shared_tuner_tkn(fh->vdev->dev_parent, mode);
> 
> As mentioned above, this should just be a put, not a reset.

ok on that.

> 
>> +    }
>> +
>>       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 d15e167..9e1f042 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,37 @@ static void v4l_sanitize_format(struct
>> v4l2_format *fmt)
>>              sizeof(fmt->fmt.pix) - offset);
>>   }
>>
>> +static int v4l_get_tuner_tkn(struct video_device *vfd,
>> +                enum v4l2_tuner_type type)
>> +{
>> +    int ret = 0;
>> +
>> +    if (vfd->dev_parent) {
>> +        enum media_tkn_mode mode;
>> +
>> +        mode = (type == V4L2_TUNER_RADIO) ?
>> +            MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
>> +        ret = media_get_shared_tuner_tkn(vfd->dev_parent, mode);
>> +        if (ret)
>> +            dev_info(vfd->dev_parent,
>> +                "%s: Tuner is busy\n", __func__);
>> +    }
>> +    dev_dbg(vfd->dev_parent, "%s: No token?? %d\n", __func__, ret);
> 
> Shouldn't this be in a 'else'? Now the 'No token' message is also
> printed when
> media_get_shared_tuner_tkn got the tuner.

Yes it should have been.

> 
>> +    return ret;
>> +}
>> +
>> +static void v4l_put_tuner_tkn(struct video_device *vfd,
>> +                enum v4l2_tuner_type type)
>> +{
>> +    if (vfd->dev_parent) {
>> +        enum media_tkn_mode mode;
>> +
>> +        mode = (type == V4L2_TUNER_RADIO) ?
>> +            MEDIA_MODE_RADIO : MEDIA_MODE_ANALOG;
>> +        media_put_tuner_tkn(vfd->dev_parent, mode);
>> +    }
>> +}
>> +
>>   static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>>                   struct file *file, void *fh, void *arg)
>>   {
>> @@ -1022,12 +1054,24 @@ 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, V4L2_TUNER_ANALOG_TV);
>> +    if (ret)
>> +        return ret;
>>       return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>>   }
>>
>>   static int v4l_s_output(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, V4L2_TUNER_ANALOG_TV);
>> +    if (ret)
>> +        return ret;
>>       return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>   }
> 
> As mentioned, tuner tokens make no sense for s_output.

Right. ack on that.

> 
>>
>> @@ -1236,6 +1280,10 @@ 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, V4L2_TUNER_ANALOG_TV);
>> +    if (ret)
>> +        return ret;
>> +
>>       v4l_sanitize_format(p);
>>
>>       switch (p->type) {
>> @@ -1415,9 +1463,13 @@ 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, p->type);
>> +    if (ret)
>> +        return ret;
>>       return ops->vidioc_s_tuner(file, fh, p);
>>   }
>>
>> @@ -1433,6 +1485,26 @@ static int v4l_g_modulator(const struct
>> v4l2_ioctl_ops *ops,
>>       return err;
>>   }
>>
>> +static int v4l_s_modulator(const struct v4l2_ioctl_ops *ops,
>> +                struct file *file, void *fh, void *arg)
>> +{
>> +    struct video_device *vfd = video_devdata(file);
>> +    struct v4l2_fh *vfh =
>> +        test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>> +
>> +    if (vfh != NULL) {
>> +        int ret;
>> +        enum v4l2_tuner_type type;
>> +
>> +        type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>> +                V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>> +        ret = v4l_get_tuner_tkn(vfd, type);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +    return ops->vidioc_s_modulator(file, fh, arg);
>> +}
> 
> Again, tuner tokens make no sense for a modulator.

Yes. Will fix it.

> 
>> +
>>   static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>                   struct file *file, void *fh, void *arg)
>>   {
>> @@ -1453,6 +1525,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 +1535,9 @@ 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, type);
>> +        if (ret)
>> +            return ret;
>>       }
>>       return ops->vidioc_s_frequency(file, fh, p);
>>   }
>> @@ -1508,11 +1584,16 @@ 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, V4L2_TUNER_ANALOG_TV);
>> +    if (ret)
>> +        return ret;
>> +
>>       /* Calls the specific handler */
>>       return ops->vidioc_s_std(file, fh, norm);
>>   }
>> @@ -1522,7 +1603,11 @@ 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, V4L2_TUNER_ANALOG_TV);
>> +    if (ret)
>> +        return ret;
>>       /*
>>        * If no signal is detected, then the driver should return
>>        * V4L2_STD_UNKNOWN. Otherwise it should return tvnorms with
>> @@ -1532,7 +1617,9 @@ static int v4l_querystd(const struct
>> v4l2_ioctl_ops *ops,
>>        * their efforts to improve the standards detection.
>>        */
>>       *p = vfd->tvnorms;
>> -    return ops->vidioc_querystd(file, fh, arg);
>> +    ret = ops->vidioc_querystd(file, fh, arg);
>> +    v4l_put_tuner_tkn(vfd, V4L2_TUNER_ANALOG_TV);
> 
> No put needed.

ok - will fix it.

> 
>> +    return ret;
>>   }
>>
>>   static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
>> @@ -1541,6 +1628,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 +1638,9 @@ 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, type);
>> +    if (ret)
>> +        return ret;
>>       return ops->vidioc_s_hw_freq_seek(file, fh, p);
>>   }
>>
>> @@ -2217,7 +2308,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>>       IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout,
>> v4l_print_audioout, 0),
>>       IOCTL_INFO_STD(VIDIOC_S_AUDOUT, vidioc_s_audout,
>> v4l_print_audioout, INFO_FL_PRIO),
>>       IOCTL_INFO_FNC(VIDIOC_G_MODULATOR, v4l_g_modulator,
>> v4l_print_modulator, INFO_FL_CLEAR(v4l2_modulator, index)),
>> -    IOCTL_INFO_STD(VIDIOC_S_MODULATOR, vidioc_s_modulator,
>> v4l_print_modulator, INFO_FL_PRIO),
>> +    IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_modulator,
>> +            v4l_print_frequency, INFO_FL_PRIO),
> 
> Huh? S_MODULATOR is removed and a duplicate S_FREQUENCY is added?

Oops. This is a mistake. Actually I don't think I need this at all
based on your earlier comment on S_MODULATOR. I will remove this.

> 
>>       IOCTL_INFO_FNC(VIDIOC_G_FREQUENCY, v4l_g_frequency,
>> v4l_print_frequency, INFO_FL_CLEAR(v4l2_frequency, tuner)),
>>       IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_frequency,
>> v4l_print_frequency, INFO_FL_PRIO),
>>       IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap,
>> INFO_FL_CLEAR(v4l2_cropcap, type)),
>>
> 
> BTW, I still haven't seen how you are going to handle snd-usb-audio. I
> want to
> see how that is going to be handled before I will Ack anything.
> 

Yes. Agreed. I will get the snd-usb-audio changes in before I send
out the next version.

thanks,
-- Shuah


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

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

On 09/24/2014 05:57 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> Here is my review...
> 
> On 09/22/2014 05:00 PM, Shuah Khan wrote:
>> Changes to v4l2-core to hold tuner token in v4l2 ioctl that change
>> the tuner modes, and reset 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_OUTPUT, S_FMT, S_TUNER, S_MODULATOR, S_FREQUENCY,
> 
> S_MODULATOR doesn't need to take a token. Certainly not a tuner token,
> since it is a modulator, not a tuner. There aren't many modulator drivers,
> and none of them have different modes.
> 
> The same is true for S_OUTPUT: it deals with outputs, so it has nothing
> to do with tuners since those are for input.
> 
>> S_STD, S_HW_FREQ_SEEK:
>>
>> - hold tuner in shared analog mode before calling appropriate
>>    ops->vidioc_s_*
>> - return leaving tuner in shared mode.
>> - Note that S_MODULATOR is now implemented in the core
>>    previously FCN.
>>
>> QUERYSTD:
>> - hold tuner in shared analog mode before calling
>>    ops->vidioc_querystd
>> - return after calling put tuner - this simply decrements the
>>    owners. Leaves tuner in shared analog mode if owners > 0
> 
> I would handle QUERYSTD the same as the ones in the first group.
> It's a fair assumption that once you call QUERYSTD you expect to
> switch the tuner to analog mode and keep it there.
> 
> I'm missing STREAMON in this list as well.
> 
> With regards to G_TUNER and ENUM_INPUT, I will reply to the post that
> discusses that topic.

Hans,

Didn't see you address G_TUNER and ENUM_INPUT in your other response.
Hope I didn't miss it.

thanks,
-- Shuah


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

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Shuah Khan]

On 09/24/2014 06:25 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> 
> Let's be realistic: if an application doesn't test for error codes,
> then that's the problem of the application. Also, EBUSY will only be
> returned if someone else is holding the tuner in a different mode.
> And that didn't work anyway (and probably in worse ways too if the
> driver would forcefully change the tuner mode).
> 
> So I really don't see a problem with this.
> 

I didn't have high hopes you would agree to the simpler approach. :)

>>
>> Compared to the current approach, holding lock in open and releasing
>> it in close is cleaner with predictable failure conditions. It is lot
>> easier to maintain. In addition, this approach keeps it same as the
>> dvb core token hold approach as outlined below.
> 
> Absolutely not an option for v4l2. You should always be able to open
> a v4l2 device, regardless of the tuner mode or any other mode.
> 
> The only exception is a radio tuner device: that should take the tuner
> on open. I think this is a very unfortunate design and I wish that that
> could be dropped.

Right this is another problem that needs to be addressed in the
user-space.

> 
> One thing that we haven't looked at at all is what should be done if
> the current input is not a tuner but a composite or S-Video input.
> 
> It is likely (I would have to test this to be sure) that you can capture
> from a DVB tuner and at the same time from an S-Video input without any
> problems. By taking the tuner token unconditionally this would become
> impossible. But doing this right will require more work.
> 
> BTW, what happens if the analog video part of a hybrid board doesn't have
> a tuner but only S-Video/Composite inputs? I think I've seen such boards
> actually. I would have to dig through my collection though.
> 

I would recommend trying to bound the problems that need to be solved
for the first phase of this media token feature. If we don't we will
never be done with it. :)

I would propose the first step as addressing dvb and v4l2 conflicts
and include snd-usb-audio so there is confidence that the media token
approach can span non-media drivers.

I am currently testing with tvtime, xawtv, vlc, and kaffeine. I am
planning to add kradio for snd-usb-audio work for the next round of
patches.

> 
> S_PRIORITY has nothing to do with tuner ownership. If there is a real need
> to release the token at another time than on close (and I don't see
> such a need), then make a new ioctl. Let's not overload S_PRIO with an
> unrelated action.

This is not an issue for fine grained approach since simpler approach
is nacked. i.e Mauro suggested changing S_PRIORITY as another place
to release it if we were to go with simple appraoch (open/close).

> 
>>
>> Devin recommended testing on devices that have an encoder to cover
>> the cases where there are multiple /dev/videoX nodes tied to the
>> same tuner.
> 
> Good examples are cx23885 (already vb2) and cx88 (the vb2 patches have
> been posted, but not yet merged). It shouldn't be too hard to find
> hardware based on those chipsets.
> 

Please see my bounding the problem comment. Can these devices wait until
the second phase. We have multiple combinations with hardware features,
applications. The way I am designing the media token is if driver
doesn't create the token, no change in the dvb-core, v4l2-core behavior.
It is not required that driver must create a token to allow evolving
driver support and hardware support as we go.

thanks,
-- Shuah

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

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Hans Verkuil]

On 09/24/2014 05:57 PM, Shuah Khan wrote:
> On 09/24/2014 06:25 AM, Hans Verkuil wrote:
>> Hi Shuah,
>>
>>
>> Let's be realistic: if an application doesn't test for error codes,
>> then that's the problem of the application. Also, EBUSY will only be
>> returned if someone else is holding the tuner in a different mode.
>> And that didn't work anyway (and probably in worse ways too if the
>> driver would forcefully change the tuner mode).
>>
>> So I really don't see a problem with this.
>>
> 
> I didn't have high hopes you would agree to the simpler approach. :)
> 
>>>
>>> Compared to the current approach, holding lock in open and releasing
>>> it in close is cleaner with predictable failure conditions. It is lot
>>> easier to maintain. In addition, this approach keeps it same as the
>>> dvb core token hold approach as outlined below.
>>
>> Absolutely not an option for v4l2. You should always be able to open
>> a v4l2 device, regardless of the tuner mode or any other mode.
>>
>> The only exception is a radio tuner device: that should take the tuner
>> on open. I think this is a very unfortunate design and I wish that that
>> could be dropped.
> 
> Right this is another problem that needs to be addressed in the
> user-space.

This is however out-of-scope for your project. For radio devices you can
just take the tuner token on open.

BTW, the tuner token in v4l2 should *only* be taken if there actually is
a tuner. Luckily that's very easy to test for: struct v4l2_ioctl_ops will
have a non-NULL vidioc_s_tuner field.

> 
>>
>> One thing that we haven't looked at at all is what should be done if
>> the current input is not a tuner but a composite or S-Video input.
>>
>> It is likely (I would have to test this to be sure) that you can capture
>> from a DVB tuner and at the same time from an S-Video input without any
>> problems. By taking the tuner token unconditionally this would become
>> impossible. But doing this right will require more work.
>>
>> BTW, what happens if the analog video part of a hybrid board doesn't have
>> a tuner but only S-Video/Composite inputs? I think I've seen such boards
>> actually. I would have to dig through my collection though.
>>
> 
> I would recommend trying to bound the problems that need to be solved
> for the first phase of this media token feature. If we don't we will
> never be done with it. :)

I want to think about this myself (i.e. non-tuner inputs) and do some
testing as well. I'll get back to this when I know more myself.

> 
> I would propose the first step as addressing dvb and v4l2 conflicts
> and include snd-usb-audio so there is confidence that the media token
> approach can span non-media drivers.

Certainly.

> I am currently testing with tvtime, xawtv, vlc, and kaffeine. I am
> planning to add kradio for snd-usb-audio work for the next round of
> patches.
> 
>>
>> S_PRIORITY has nothing to do with tuner ownership. If there is a real need
>> to release the token at another time than on close (and I don't see
>> such a need), then make a new ioctl. Let's not overload S_PRIO with an
>> unrelated action.
> 
> This is not an issue for fine grained approach since simpler approach
> is nacked. i.e Mauro suggested changing S_PRIORITY as another place
> to release it if we were to go with simple appraoch (open/close).
> 
>>
>>>
>>> Devin recommended testing on devices that have an encoder to cover
>>> the cases where there are multiple /dev/videoX nodes tied to the
>>> same tuner.
>>
>> Good examples are cx23885 (already vb2) and cx88 (the vb2 patches have
>> been posted, but not yet merged). It shouldn't be too hard to find
>> hardware based on those chipsets.
>>
> 
> Please see my bounding the problem comment. Can these devices wait until
> the second phase. We have multiple combinations with hardware features,
> applications. The way I am designing the media token is if driver
> doesn't create the token, no change in the dvb-core, v4l2-core behavior.
> It is not required that driver must create a token to allow evolving
> driver support and hardware support as we go.

>From what I know I don't think these types of devices will pose any problem
for this approach. However, in general you should consider all sorts of
combinations and see if the design will still work for those. You don't
want to have to redesign it later just because you ignored a particular type
of device. Which I why I want to see the snd-usb-audio work first, since I
think that's probably the most complex part of the whole design.

And I do think you should try to get one of those cards (cx88 or cx23885
based). They are common, they are complex and have hybrid tuners, sometimes
with radio support as well. You can probably get some of them fairly cheaply
on ebay. These are good cards to test with, if not now then in the near
future. My philosophy is that you can never have too much hardware :-)

Regards,

	Hans

Re: [PATCH 2/5] media: v4l2-core changes to use media tuner token api

[Laurent Pinchart]

Hi Hans,

On Wednesday 24 September 2014 14:25:59 Hans Verkuil wrote:
> On 09/23/2014 10:46 PM, Shuah Khan wrote:
> > Hi Devin/Mauro/Hans,
> > 
> > Summarizing the discussion on v4l to keep others on this
> > thread in the loop. Please see below:
> > 
> > Hans! Could you please take a look and see if it raises
> > any red flags for you.
> > 
> > On 09/23/2014 08:17 AM, Devin Heitmueller wrote:
> >> We can always start with coarse locking on open/close, and do finer
> >> grained locking down the road if needed - or simply change the
> >> currently undefined behavior in the spec to say that you have to close
> >> the device handle before attempting to open the other side of the
> >> device.
> > 
> > I share the same concerns about fine grain locking approach that
> > requires changes to various v4l2 ioctls to hold the token. My
> > concern with the current approach is that we won't be able to find
> > all the v4l paths to secure. During my testing, it is clear that
> > several applications don't seem to check ioctls return codes and
> > even if one of the ioctls returns -EBUSY, applications keep calling
> > other ioctls instead of exiting with device busy condition.
> 
> Let's be realistic: if an application doesn't test for error codes,
> then that's the problem of the application. Also, EBUSY will only be
> returned if someone else is holding the tuner in a different mode.
> And that didn't work anyway (and probably in worse ways too if the
> driver would forcefully change the tuner mode).
> 
> So I really don't see a problem with this.

I somehow agree, but I believe for this to work we need to offer applications 
with a way to find out about the constraints associated with the shared tuner. 
We can't expect userspace to properly deal with errors if they can't find out 
from information exposed by the kernel how to deal with them. From an end-user 
point of view, a dialog box saying EBUSY is pretty much useless if we can't 
tell the user that they can't watch TV on their XYZ DVB device because the 
radio device ABC is currently in use.

> > Compared to the current approach, holding lock in open and releasing
> > it in close is cleaner with predictable failure conditions. It is lot
> > easier to maintain. In addition, this approach keeps it same as the
> > dvb core token hold approach as outlined below.
> 
> Absolutely not an option for v4l2. You should always be able to open
> a v4l2 device, regardless of the tuner mode or any other mode.
> 
> The only exception is a radio tuner device: that should take the tuner
> on open. I think this is a very unfortunate design and I wish that that
> could be dropped.
> 
> One thing that we haven't looked at at all is what should be done if
> the current input is not a tuner but a composite or S-Video input.
> 
> It is likely (I would have to test this to be sure) that you can capture
> from a DVB tuner and at the same time from an S-Video input without any
> problems. By taking the tuner token unconditionally this would become
> impossible. But doing this right will require more work.
> 
> BTW, what happens if the analog video part of a hybrid board doesn't have
> a tuner but only S-Video/Composite inputs? I think I've seen such boards
> actually. I would have to dig through my collection though.
> 
> > dvb on the other hand is easier with its clean entry and exit points.
> > In the case of dvb, the lock is held when the device is opened in
> > read/write mode before dvb front-end thread gets started and released
> > when thread exits.
> > 
> > I discussed this a couple of times in the past during development
> > for this current patch series. The concern has been that a number of
> > currently supported use-cases will break with the simpler approach
> > to lock when v4l device is opened and unlock when it is closed.
> > 
> > As we discussed this morning and agreed on giving the simpler
> > approach a try first keeping the finer grain locking option
> > open for revisit. This would be acceptable provided the token
> > code is tested on existing apps, including mythtv, kradio,
> > gnome-radio.
> 
> Nack from me. Taking a tuner token should only happen when the device
> actually needs the tuner. For DVB that's easy, since whenever you open
> the frontend you *know* you want the tuner.
> 
> But that's much more difficult for V4L2 since there are so many
> combinations, including many that don't need a tuner at all such as HDMI,
> Composite etc. inputs.
> 
> > In addition to releasing the token at device close, release the token
> > if an app decides to use S_PRIORITY to release the streaming ownership
> > e. g. V4L2_PRIORITY_BACKGROUND
> 
> S_PRIORITY has nothing to do with tuner ownership. If there is a real need
> to release the token at another time than on close (and I don't see
> such a need), then make a new ioctl. Let's not overload S_PRIO with an
> unrelated action.
> 
> > Devin recommended testing on devices that have an encoder to cover
> > the cases where there are multiple /dev/videoX nodes tied to the
> > same tuner.
> 
> Good examples are cx23885 (already vb2) and cx88 (the vb2 patches have
> been posted, but not yet merged). It shouldn't be too hard to find
> hardware based on those chipsets.
> 
> > Please check if I captured it correctly. I can get started on the
> > simpler approach and see where it takes us. I have to change the
> > v4l2 and driver v4l2 patches. dvb and the rest can stay the same.
> 
> As mentioned, Nack for the simpler approach from me. That's simply
> too simple :-)
> 
> Note: I'm travelling and attending a conference, so my availability for
> the rest of the week will probably be limited.

-- 
Regards,

Laurent Pinchart