Re: [PATCH v2 1/6] media: add media token device resource framework
Hi Shuah,
A few comments below.
On Tue, Oct 14, 2014 at 08:58:37AM -0600, Shuah Khan wrote:
> Add media token device resource framework to allow sharing
> resources such as tuner, dma, audio etc. across media drivers
> and non-media sound drivers that control media hardware. The
> Media token resource is created at the main struct device that
> is common to all drivers that claim various pieces of the main
> media device, which allows them to find the resource using the
> main struct device. As an example, digital, analog, and
> snd-usb-audio drivers can use the media token resource API
> using the main struct device for the interface the media device
> is attached to.
>
> A shared media tokens resource is created using devres framework
> for drivers to find and lock/unlock. Creating a shared devres
> helps avoid creating data structure dependencies between drivers.
> This media token resource contains media token for tuner, and
> audio. When tuner token is requested, audio token is issued.
> Subsequent token (for tuner and audio) gets from the same task
> and task in the same tgid succeed. This allows applications that
> make multiple v4l2 ioctls to work with the first call acquiring
> the token and applications that create separate threads to handle
> video and audio functions.
>
> Signed-off-by: Shuah Khan
> ---
> MAINTAINERS |2 +
> include/linux/media_tknres.h | 50 +
> lib/Makefile |2 +
> lib/media_tknres.c | 237
> ++
> 4 files changed, 291 insertions(+)
> create mode 100644 include/linux/media_tknres.h
> create mode 100644 lib/media_tknres.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e80a275..9216179 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-*
> F: include/uapi/linux/meye.h
> F: include/uapi/linux/ivtv*
> F: include/uapi/linux/uvcvideo.h
> +F: include/linux/media_tknres.h
> +F: lib/media_tknres.c
>
> MEDIAVISION PRO MOVIE STUDIO DRIVER
> M: Hans Verkuil
> diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
> new file mode 100644
> index 000..6d37327
> --- /dev/null
> +++ b/include/linux/media_tknres.h
> @@ -0,0 +1,50 @@
> +/*
> + * media_tknres.h - managed media token resource
> + *
> + * Copyright (c) 2014 Shuah Khan
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + */
> +#ifndef __LINUX_MEDIA_TOKEN_H
> +#define __LINUX_MEDIA_TOKEN_H
> +
> +struct device;
> +
> +#if defined(CONFIG_MEDIA_SUPPORT)
> +extern int media_tknres_create(struct device *dev);
> +extern int media_tknres_destroy(struct device *dev);
> +
> +extern int media_get_tuner_tkn(struct device *dev);
> +extern int media_put_tuner_tkn(struct device *dev);
> +
> +extern int media_get_audio_tkn(struct device *dev);
> +extern int media_put_audio_tkn(struct device *dev);
> +#else
> +static inline int media_tknres_create(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_tknres_destroy(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_get_tuner_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_put_tuner_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static int media_get_audio_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static int media_put_audio_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* __LINUX_MEDIA_TOKEN_H */
> diff --git a/lib/Makefile b/lib/Makefile
> index d6b4bc4..6f21695 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o
>
> obj-$(CONFIG_GLOB) += glob.o
>
> +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
I'd add a new Kconfig option for this, as it's only needed by a very
specific kind of drivers. It could be automatically selected by those that
need it.
> +
> obj-$(CONFIG_MPILIB) += mpi/
> obj-$(CONFIG_SIGNATURE) += digsig.o
>
> diff --git a/lib/media_tknres.c b/lib/media_tknres.c
> new file mode 100644
> index 000..e0a36cb
> --- /dev/null
> +++ b/lib/media_tknres.c
> @@ -0,0 +1,237 @@
> +/*
> + * media_tknres.c - managed media token resource
> + *
> + * Copyright (c) 2014 Shuah Khan
> + * 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
>
Re: [PATCH v2 1/6] media: add media token device resource framework
At Tue, 21 Oct 2014 13:58:59 +0200, Hans Verkuil wrote: > > > > On 10/21/2014 01:51 PM, Takashi Iwai wrote: > > At Tue, 21 Oct 2014 12:46:03 +0200, > > Hans Verkuil wrote: > >> > >> Hi Shuah, > >> > >> As promised, here is my review for this patch series. > >> > >> On 10/14/2014 04:58 PM, Shuah Khan wrote: > >>> Add media token device resource framework to allow sharing > >>> resources such as tuner, dma, audio etc. across media drivers > >>> and non-media sound drivers that control media hardware. The > >>> Media token resource is created at the main struct device that > >>> is common to all drivers that claim various pieces of the main > >>> media device, which allows them to find the resource using the > >>> main struct device. As an example, digital, analog, and > >>> snd-usb-audio drivers can use the media token resource API > >>> using the main struct device for the interface the media device > >>> is attached to. > >>> > >>> A shared media tokens resource is created using devres framework > >>> for drivers to find and lock/unlock. Creating a shared devres > >>> helps avoid creating data structure dependencies between drivers. > >>> This media token resource contains media token for tuner, and > >>> audio. When tuner token is requested, audio token is issued. > >> > >> Did you mean: 'tuner token is issued' instead of audio token? > >> > >> I also have the same question as Takashi: why do we have an audio > >> token in the first place? While you are streaming audio over alsa > >> the underlying tuner must be marked as being in use. It's all about > >> the tuner, since that's the resource that is being shared, not about > >> audio as such. > >> > >> For the remainder of my review I will ignore the audio-related code > >> and concentrate only on the tuner part. > >> > >>> Subsequent token (for tuner and audio) gets from the same task > >>> and task in the same tgid succeed. This allows applications that > >>> make multiple v4l2 ioctls to work with the first call acquiring > >>> the token and applications that create separate threads to handle > >>> video and audio functions. > >>> > >>> Signed-off-by: Shuah Khan > >>> --- > >>>MAINTAINERS |2 + > >>>include/linux/media_tknres.h | 50 + > >>>lib/Makefile |2 + > >>>lib/media_tknres.c | 237 > >>> ++ > >> > >> I am still not convinced myself that this should be a generic API. > >> The only reason we need it today is for sharing tuners. Which is almost > >> a purely media thing with USB audio as the single non-media driver that > >> will be affected. Today I see no use case outside of tuners. I would > >> probably want to keep this inside drivers/media. > >> > >> If this is going to be expanded it can always be moved to lib later. > > > > Well, my argument is that it should be more generic if it were > > intended to be put in lib. It'd be fine to put into drivers/media, > > but this code snippet must be a separate module. Otherwise usb-audio > > would grab the whole media stuff even if not needed at all. > > Certainly. > > > > > (snip) > >> I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION > >> and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this > >> module to the GPL devres_* functions. It took me some time to figure that > >> out. > > > > It was a code in lib, so it cannot be a module at all :) > > Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it > compiles as a module :-) Ah, I thought it was a boolean, but yes, this can be a module indeed. Then I don't think it's worth to put in lib. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] media: add media token device resource framework
On 10/21/2014 01:51 PM, Takashi Iwai wrote: At Tue, 21 Oct 2014 12:46:03 +0200, Hans Verkuil wrote: Hi Shuah, As promised, here is my review for this patch series. On 10/14/2014 04:58 PM, Shuah Khan wrote: Add media token device resource framework to allow sharing resources such as tuner, dma, audio etc. across media drivers and non-media sound drivers that control media hardware. The Media token resource is created at the main struct device that is common to all drivers that claim various pieces of the main media device, which allows them to find the resource using the main struct device. As an example, digital, analog, and snd-usb-audio drivers can use the media token resource API using the main struct device for the interface the media device is attached to. A shared media tokens resource is created using devres framework for drivers to find and lock/unlock. Creating a shared devres helps avoid creating data structure dependencies between drivers. This media token resource contains media token for tuner, and audio. When tuner token is requested, audio token is issued. Did you mean: 'tuner token is issued' instead of audio token? I also have the same question as Takashi: why do we have an audio token in the first place? While you are streaming audio over alsa the underlying tuner must be marked as being in use. It's all about the tuner, since that's the resource that is being shared, not about audio as such. For the remainder of my review I will ignore the audio-related code and concentrate only on the tuner part. Subsequent token (for tuner and audio) gets from the same task and task in the same tgid succeed. This allows applications that make multiple v4l2 ioctls to work with the first call acquiring the token and applications that create separate threads to handle video and audio functions. Signed-off-by: Shuah Khan --- MAINTAINERS |2 + include/linux/media_tknres.h | 50 + lib/Makefile |2 + lib/media_tknres.c | 237 ++ I am still not convinced myself that this should be a generic API. The only reason we need it today is for sharing tuners. Which is almost a purely media thing with USB audio as the single non-media driver that will be affected. Today I see no use case outside of tuners. I would probably want to keep this inside drivers/media. If this is going to be expanded it can always be moved to lib later. Well, my argument is that it should be more generic if it were intended to be put in lib. It'd be fine to put into drivers/media, but this code snippet must be a separate module. Otherwise usb-audio would grab the whole media stuff even if not needed at all. Certainly. (snip) I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this module to the GPL devres_* functions. It took me some time to figure that out. It was a code in lib, so it cannot be a module at all :) Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it compiles as a module :-) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] media: add media token device resource framework
At Tue, 21 Oct 2014 12:46:03 +0200, Hans Verkuil wrote: > > Hi Shuah, > > As promised, here is my review for this patch series. > > On 10/14/2014 04:58 PM, Shuah Khan wrote: > > Add media token device resource framework to allow sharing > > resources such as tuner, dma, audio etc. across media drivers > > and non-media sound drivers that control media hardware. The > > Media token resource is created at the main struct device that > > is common to all drivers that claim various pieces of the main > > media device, which allows them to find the resource using the > > main struct device. As an example, digital, analog, and > > snd-usb-audio drivers can use the media token resource API > > using the main struct device for the interface the media device > > is attached to. > > > > A shared media tokens resource is created using devres framework > > for drivers to find and lock/unlock. Creating a shared devres > > helps avoid creating data structure dependencies between drivers. > > This media token resource contains media token for tuner, and > > audio. When tuner token is requested, audio token is issued. > > Did you mean: 'tuner token is issued' instead of audio token? > > I also have the same question as Takashi: why do we have an audio > token in the first place? While you are streaming audio over alsa > the underlying tuner must be marked as being in use. It's all about > the tuner, since that's the resource that is being shared, not about > audio as such. > > For the remainder of my review I will ignore the audio-related code > and concentrate only on the tuner part. > > > Subsequent token (for tuner and audio) gets from the same task > > and task in the same tgid succeed. This allows applications that > > make multiple v4l2 ioctls to work with the first call acquiring > > the token and applications that create separate threads to handle > > video and audio functions. > > > > Signed-off-by: Shuah Khan > > --- > > MAINTAINERS |2 + > > include/linux/media_tknres.h | 50 + > > lib/Makefile |2 + > > lib/media_tknres.c | 237 > > ++ > > I am still not convinced myself that this should be a generic API. > The only reason we need it today is for sharing tuners. Which is almost > a purely media thing with USB audio as the single non-media driver that > will be affected. Today I see no use case outside of tuners. I would > probably want to keep this inside drivers/media. > > If this is going to be expanded it can always be moved to lib later. Well, my argument is that it should be more generic if it were intended to be put in lib. It'd be fine to put into drivers/media, but this code snippet must be a separate module. Otherwise usb-audio would grab the whole media stuff even if not needed at all. (snip) > I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION > and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this > module to the GPL devres_* functions. It took me some time to figure that > out. It was a code in lib, so it cannot be a module at all :) Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] media: add media token device resource framework
Hi Shuah,
As promised, here is my review for this patch series.
On 10/14/2014 04:58 PM, Shuah Khan wrote:
Add media token device resource framework to allow sharing
resources such as tuner, dma, audio etc. across media drivers
and non-media sound drivers that control media hardware. The
Media token resource is created at the main struct device that
is common to all drivers that claim various pieces of the main
media device, which allows them to find the resource using the
main struct device. As an example, digital, analog, and
snd-usb-audio drivers can use the media token resource API
using the main struct device for the interface the media device
is attached to.
A shared media tokens resource is created using devres framework
for drivers to find and lock/unlock. Creating a shared devres
helps avoid creating data structure dependencies between drivers.
This media token resource contains media token for tuner, and
audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?
I also have the same question as Takashi: why do we have an audio
token in the first place? While you are streaming audio over alsa
the underlying tuner must be marked as being in use. It's all about
the tuner, since that's the resource that is being shared, not about
audio as such.
For the remainder of my review I will ignore the audio-related code
and concentrate only on the tuner part.
Subsequent token (for tuner and audio) gets from the same task
and task in the same tgid succeed. This allows applications that
make multiple v4l2 ioctls to work with the first call acquiring
the token and applications that create separate threads to handle
video and audio functions.
Signed-off-by: Shuah Khan
---
MAINTAINERS |2 +
include/linux/media_tknres.h | 50 +
lib/Makefile |2 +
lib/media_tknres.c | 237 ++
I am still not convinced myself that this should be a generic API.
The only reason we need it today is for sharing tuners. Which is almost
a purely media thing with USB audio as the single non-media driver that
will be affected. Today I see no use case outside of tuners. I would
probably want to keep this inside drivers/media.
If this is going to be expanded it can always be moved to lib later.
4 files changed, 291 insertions(+)
create mode 100644 include/linux/media_tknres.h
create mode 100644 lib/media_tknres.c
diff --git a/MAINTAINERS b/MAINTAINERS
index e80a275..9216179 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5864,6 +5864,8 @@ F:include/uapi/linux/v4l2-*
F:include/uapi/linux/meye.h
F:include/uapi/linux/ivtv*
F:include/uapi/linux/uvcvideo.h
+F: include/linux/media_tknres.h
+F: lib/media_tknres.c
MEDIAVISION PRO MOVIE STUDIO DRIVER
M:Hans Verkuil
diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
new file mode 100644
index 000..6d37327
--- /dev/null
+++ b/include/linux/media_tknres.h
@@ -0,0 +1,50 @@
+/*
+ * media_tknres.h - managed media token resource
+ *
+ * Copyright (c) 2014 Shuah Khan
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+#ifndef __LINUX_MEDIA_TOKEN_H
+#define __LINUX_MEDIA_TOKEN_H
+
+struct device;
+
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
+#else
+static inline int media_tknres_create(struct device *dev)
+{
+ return 0;
+}
+static inline int media_tknres_destroy(struct device *dev)
+{
+ return 0;
+}
+static inline int media_get_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static inline int media_put_tuner_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_get_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+static int media_put_audio_tkn(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+#endif /* __LINUX_MEDIA_TOKEN_H */
diff --git a/lib/Makefile b/lib/Makefile
index d6b4bc4..6f21695 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o
obj-$(CONFIG_GLOB) += glob.o
+obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
+
obj-$(CONFIG_MPILIB) += mpi/
obj-$(CONFIG_SIGNATURE) += digsig.o
diff --git a/lib/media_tknres.c b/lib/media_tknres.c
new file mode 100644
index 000..e0a36cb
--- /dev/null
+++ b/lib/media_tknres.c
@@ -0,0 +1,237 @@
+/*
+ * media_tknres.c - managed media token resource
+ *
+ * Copyright (c) 2014 Shuah Khan
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ */
+/*
+ * Media devices often have hardware resources that are
Re: [PATCH v2 1/6] media: add media token device resource framework
On 10/16/2014 08:00 AM, Takashi Iwai wrote:
> At Wed, 15 Oct 2014 18:53:28 -0600,
> Shuah Khan wrote:
>>
>> On 10/15/2014 11:05 AM, Takashi Iwai wrote:
>>
+#if defined(CONFIG_MEDIA_SUPPORT)
+extern int media_tknres_create(struct device *dev);
+extern int media_tknres_destroy(struct device *dev);
+
+extern int media_get_tuner_tkn(struct device *dev);
+extern int media_put_tuner_tkn(struct device *dev);
+
+extern int media_get_audio_tkn(struct device *dev);
+extern int media_put_audio_tkn(struct device *dev);
>>>
>>> The words "tknres" and "tkn" (especially latter) look ugly and not
>>> clear to me. IMO, it should be "token".
>>
>> No problem. I can change that to media_token_create/destroy()
>> and expand token in other functions.
>>
+struct media_tkn {
+ spinlock_t lock;
+ unsigned int owner; /* owner task pid */
+ unsigned int tgid; /* owner task gid */
+ struct task_struct *task;
+};
+
+struct media_tknres {
+ struct media_tkn tuner;
+ struct media_tkn audio;
+};
>>>
>>> Why do you need to have both tuner and audio tokens? If I understand
>>> correctly, no matter whether it's tuner or audio, if it's being used,
>>> the open must fail, right?
>>
>> As it evolved during development, it turns out at the moment I don't
>> have any use-cases that require holding audio and tuner separately.
>> It probably could be collapsed into just a media token. I have to
>> think about this some.
>>
+
+static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
+{
+ int rc = 0;
+ unsigned tpid;
+ unsigned tgid;
+
+ spin_lock(&tkn->lock);
>>>
>>> You should use spin_lock_irqsave() here and in all other places.
>>> The trigger callback in usb-audio, for example, may be called in irq
>>> context.
>>
>> ok. Good point, will change that.
>>
>>>
+
+ tpid = task_pid_nr(current);
+ tgid = task_tgid_nr(current);
+
+ /* allow task in the same group id to release */
>>>
>>> IMO, it's not "release" but "steal"... But what happens if the stolen
>>> owner calls put? Then it'll be released although the stealing owner
>>> still thinks it's being held.
>>
>> Yeah it could be called a steal. :) Essentially tgid happens to be
>> the real owner. I am overwriting the pid with current pid when
>> tgid is same.
>>
>> media dvb and v4l apps start two or more threads that all share the
>> tgid and subsequent token gets should be allowed based on the tgid.
>>
>> Scenario 1:
>>
>> Please note that there are 3 device files in question and media
>> token resource is the lock for all. Hence the changes to v4l-core,
>> dvb-core, and snd-usb-audio to hold the token for exclusive access
>> when the task or tgid don't match.
>>
>> program starts:
>> - first thread opens device file in R/W mode - open gets the token
>> or thread makes ioctls calls that clearly indicates intent to
>> change tuner settings
>> - creates one thread for audio
>> - creates another for video or continues video function
>> - video thread does a close and re-opens the device file
>>
>> In this case when thread tries to close, token put fails unless
>> tasks with same tgid are allowed to release.
>> ( I ran into this one of the media applications and it is a valid
>> case to handle, thread can close the file and should be able to
>> open again without running into token busy case )
>>
>> - or continue to just use the device file
>> - audio thread does snd_pcm_capture ops - trigger start
>>
>> program exits:
>> - video thread closes the device file
>> - audio thread does snd_pcm_capture ops - trigger stop
>>
>> This got me thinking about the scenario when an application
>> does a fork() as opposed to create a thread. I have to think
>> about this and see how I can address that.
>>
>>>
+ if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
>>>
>>> Shouldn't the second "&&" be "||" instead?
>>> And too many parentheses there.
>>
>> Right - this is my bad. The comment right above this conditional
>> is a give away that, at some point I did a copy and paste from
>> _put to _get and only changed the first task null check.
>> I am yelling at myself at the moment.
>>
>>>
+ rc = -EBUSY;
>>>
>>> Wrong indentation.
>>
>> Yes. Will fix that.
>>
>>>
>>> I have a feeling that the whole thing can be a bit more simplified in
>>> the end...
>>>
>>
>> Any ideas to simplify are welcome.
>
> There are a few points to make things more consistent and simplified,
> IMO. First of all, the whole concept can be more generalized. It's
> not necessarily only for media and audio. Rather we can make it a
> generic dev_token. Then it'll become more convincing to land into
> lib/*.
Right. At the moment this is very media specific.
>
> The media-specific handling is only about the pid and gid checks.
> This can be implemented as a callback that is passed a
Re: [PATCH v2 1/6] media: add media token device resource framework
At Wed, 15 Oct 2014 18:53:28 -0600,
Shuah Khan wrote:
>
> On 10/15/2014 11:05 AM, Takashi Iwai wrote:
>
> >> +#if defined(CONFIG_MEDIA_SUPPORT)
> >> +extern int media_tknres_create(struct device *dev);
> >> +extern int media_tknres_destroy(struct device *dev);
> >> +
> >> +extern int media_get_tuner_tkn(struct device *dev);
> >> +extern int media_put_tuner_tkn(struct device *dev);
> >> +
> >> +extern int media_get_audio_tkn(struct device *dev);
> >> +extern int media_put_audio_tkn(struct device *dev);
> >
> > The words "tknres" and "tkn" (especially latter) look ugly and not
> > clear to me. IMO, it should be "token".
>
> No problem. I can change that to media_token_create/destroy()
> and expand token in other functions.
>
> >> +struct media_tkn {
> >> + spinlock_t lock;
> >> + unsigned int owner; /* owner task pid */
> >> + unsigned int tgid; /* owner task gid */
> >> + struct task_struct *task;
> >> +};
> >> +
> >> +struct media_tknres {
> >> + struct media_tkn tuner;
> >> + struct media_tkn audio;
> >> +};
> >
> > Why do you need to have both tuner and audio tokens? If I understand
> > correctly, no matter whether it's tuner or audio, if it's being used,
> > the open must fail, right?
>
> As it evolved during development, it turns out at the moment I don't
> have any use-cases that require holding audio and tuner separately.
> It probably could be collapsed into just a media token. I have to
> think about this some.
>
> >> +
> >> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
> >> +{
> >> + int rc = 0;
> >> + unsigned tpid;
> >> + unsigned tgid;
> >> +
> >> + spin_lock(&tkn->lock);
> >
> > You should use spin_lock_irqsave() here and in all other places.
> > The trigger callback in usb-audio, for example, may be called in irq
> > context.
>
> ok. Good point, will change that.
>
> >
> >> +
> >> + tpid = task_pid_nr(current);
> >> + tgid = task_tgid_nr(current);
> >> +
> >> + /* allow task in the same group id to release */
> >
> > IMO, it's not "release" but "steal"... But what happens if the stolen
> > owner calls put? Then it'll be released although the stealing owner
> > still thinks it's being held.
>
> Yeah it could be called a steal. :) Essentially tgid happens to be
> the real owner. I am overwriting the pid with current pid when
> tgid is same.
>
> media dvb and v4l apps start two or more threads that all share the
> tgid and subsequent token gets should be allowed based on the tgid.
>
> Scenario 1:
>
> Please note that there are 3 device files in question and media
> token resource is the lock for all. Hence the changes to v4l-core,
> dvb-core, and snd-usb-audio to hold the token for exclusive access
> when the task or tgid don't match.
>
> program starts:
> - first thread opens device file in R/W mode - open gets the token
> or thread makes ioctls calls that clearly indicates intent to
> change tuner settings
> - creates one thread for audio
> - creates another for video or continues video function
> - video thread does a close and re-opens the device file
>
> In this case when thread tries to close, token put fails unless
> tasks with same tgid are allowed to release.
> ( I ran into this one of the media applications and it is a valid
> case to handle, thread can close the file and should be able to
> open again without running into token busy case )
>
> - or continue to just use the device file
> - audio thread does snd_pcm_capture ops - trigger start
>
> program exits:
> - video thread closes the device file
> - audio thread does snd_pcm_capture ops - trigger stop
>
> This got me thinking about the scenario when an application
> does a fork() as opposed to create a thread. I have to think
> about this and see how I can address that.
>
> >
> >> + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
> >
> > Shouldn't the second "&&" be "||" instead?
> > And too many parentheses there.
>
> Right - this is my bad. The comment right above this conditional
> is a give away that, at some point I did a copy and paste from
> _put to _get and only changed the first task null check.
> I am yelling at myself at the moment.
>
> >
> >> + rc = -EBUSY;
> >
> > Wrong indentation.
>
> Yes. Will fix that.
>
> >
> > I have a feeling that the whole thing can be a bit more simplified in
> > the end...
> >
>
> Any ideas to simplify are welcome.
There are a few points to make things more consistent and simplified,
IMO. First of all, the whole concept can be more generalized. It's
not necessarily only for media and audio. Rather we can make it a
generic dev_token. Then it'll become more convincing to land into
lib/*.
The media-specific handling is only about the pid and gid checks.
This can be implemented as a callback that is passed at creating a
token, for example. This will reduce the core code in lib/*.
Also, get and put should handle a proper refcount. The point I
mentioned i
Re: [PATCH v2 1/6] media: add media token device resource framework
On 10/15/2014 11:05 AM, Takashi Iwai wrote:
>> +#if defined(CONFIG_MEDIA_SUPPORT)
>> +extern int media_tknres_create(struct device *dev);
>> +extern int media_tknres_destroy(struct device *dev);
>> +
>> +extern int media_get_tuner_tkn(struct device *dev);
>> +extern int media_put_tuner_tkn(struct device *dev);
>> +
>> +extern int media_get_audio_tkn(struct device *dev);
>> +extern int media_put_audio_tkn(struct device *dev);
>
> The words "tknres" and "tkn" (especially latter) look ugly and not
> clear to me. IMO, it should be "token".
No problem. I can change that to media_token_create/destroy()
and expand token in other functions.
>> +struct media_tkn {
>> +spinlock_t lock;
>> +unsigned int owner; /* owner task pid */
>> +unsigned int tgid; /* owner task gid */
>> +struct task_struct *task;
>> +};
>> +
>> +struct media_tknres {
>> +struct media_tkn tuner;
>> +struct media_tkn audio;
>> +};
>
> Why do you need to have both tuner and audio tokens? If I understand
> correctly, no matter whether it's tuner or audio, if it's being used,
> the open must fail, right?
As it evolved during development, it turns out at the moment I don't
have any use-cases that require holding audio and tuner separately.
It probably could be collapsed into just a media token. I have to
think about this some.
>> +
>> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
>> +{
>> +int rc = 0;
>> +unsigned tpid;
>> +unsigned tgid;
>> +
>> +spin_lock(&tkn->lock);
>
> You should use spin_lock_irqsave() here and in all other places.
> The trigger callback in usb-audio, for example, may be called in irq
> context.
ok. Good point, will change that.
>
>> +
>> +tpid = task_pid_nr(current);
>> +tgid = task_tgid_nr(current);
>> +
>> +/* allow task in the same group id to release */
>
> IMO, it's not "release" but "steal"... But what happens if the stolen
> owner calls put? Then it'll be released although the stealing owner
> still thinks it's being held.
Yeah it could be called a steal. :) Essentially tgid happens to be
the real owner. I am overwriting the pid with current pid when
tgid is same.
media dvb and v4l apps start two or more threads that all share the
tgid and subsequent token gets should be allowed based on the tgid.
Scenario 1:
Please note that there are 3 device files in question and media
token resource is the lock for all. Hence the changes to v4l-core,
dvb-core, and snd-usb-audio to hold the token for exclusive access
when the task or tgid don't match.
program starts:
- first thread opens device file in R/W mode - open gets the token
or thread makes ioctls calls that clearly indicates intent to
change tuner settings
- creates one thread for audio
- creates another for video or continues video function
- video thread does a close and re-opens the device file
In this case when thread tries to close, token put fails unless
tasks with same tgid are allowed to release.
( I ran into this one of the media applications and it is a valid
case to handle, thread can close the file and should be able to
open again without running into token busy case )
- or continue to just use the device file
- audio thread does snd_pcm_capture ops - trigger start
program exits:
- video thread closes the device file
- audio thread does snd_pcm_capture ops - trigger stop
This got me thinking about the scenario when an application
does a fork() as opposed to create a thread. I have to think
about this and see how I can address that.
>
>> +if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
>
> Shouldn't the second "&&" be "||" instead?
> And too many parentheses there.
Right - this is my bad. The comment right above this conditional
is a give away that, at some point I did a copy and paste from
_put to _get and only changed the first task null check.
I am yelling at myself at the moment.
>
>> +rc = -EBUSY;
>
> Wrong indentation.
Yes. Will fix that.
>
> I have a feeling that the whole thing can be a bit more simplified in
> the end...
>
Any ideas to simplify are welcome.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] media: add media token device resource framework
At Tue, 14 Oct 2014 08:58:37 -0600,
Shuah Khan wrote:
>
> Add media token device resource framework to allow sharing
> resources such as tuner, dma, audio etc. across media drivers
> and non-media sound drivers that control media hardware. The
> Media token resource is created at the main struct device that
> is common to all drivers that claim various pieces of the main
> media device, which allows them to find the resource using the
> main struct device. As an example, digital, analog, and
> snd-usb-audio drivers can use the media token resource API
> using the main struct device for the interface the media device
> is attached to.
>
> A shared media tokens resource is created using devres framework
> for drivers to find and lock/unlock. Creating a shared devres
> helps avoid creating data structure dependencies between drivers.
> This media token resource contains media token for tuner, and
> audio. When tuner token is requested, audio token is issued.
> Subsequent token (for tuner and audio) gets from the same task
> and task in the same tgid succeed. This allows applications that
> make multiple v4l2 ioctls to work with the first call acquiring
> the token and applications that create separate threads to handle
> video and audio functions.
>
> Signed-off-by: Shuah Khan
> ---
> MAINTAINERS |2 +
> include/linux/media_tknres.h | 50 +
> lib/Makefile |2 +
> lib/media_tknres.c | 237
> ++
> 4 files changed, 291 insertions(+)
> create mode 100644 include/linux/media_tknres.h
> create mode 100644 lib/media_tknres.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e80a275..9216179 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-*
> F: include/uapi/linux/meye.h
> F: include/uapi/linux/ivtv*
> F: include/uapi/linux/uvcvideo.h
> +F: include/linux/media_tknres.h
> +F: lib/media_tknres.c
>
> MEDIAVISION PRO MOVIE STUDIO DRIVER
> M: Hans Verkuil
> diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h
> new file mode 100644
> index 000..6d37327
> --- /dev/null
> +++ b/include/linux/media_tknres.h
> @@ -0,0 +1,50 @@
> +/*
> + * media_tknres.h - managed media token resource
> + *
> + * Copyright (c) 2014 Shuah Khan
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + */
> +#ifndef __LINUX_MEDIA_TOKEN_H
> +#define __LINUX_MEDIA_TOKEN_H
> +
> +struct device;
> +
> +#if defined(CONFIG_MEDIA_SUPPORT)
> +extern int media_tknres_create(struct device *dev);
> +extern int media_tknres_destroy(struct device *dev);
> +
> +extern int media_get_tuner_tkn(struct device *dev);
> +extern int media_put_tuner_tkn(struct device *dev);
> +
> +extern int media_get_audio_tkn(struct device *dev);
> +extern int media_put_audio_tkn(struct device *dev);
The words "tknres" and "tkn" (especially latter) look ugly and not
clear to me. IMO, it should be "token".
> +#else
> +static inline int media_tknres_create(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_tknres_destroy(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_get_tuner_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static inline int media_put_tuner_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static int media_get_audio_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +static int media_put_audio_tkn(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* __LINUX_MEDIA_TOKEN_H */
> diff --git a/lib/Makefile b/lib/Makefile
> index d6b4bc4..6f21695 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o
>
> obj-$(CONFIG_GLOB) += glob.o
>
> +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o
> +
> obj-$(CONFIG_MPILIB) += mpi/
> obj-$(CONFIG_SIGNATURE) += digsig.o
>
> diff --git a/lib/media_tknres.c b/lib/media_tknres.c
> new file mode 100644
> index 000..e0a36cb
> --- /dev/null
> +++ b/lib/media_tknres.c
> @@ -0,0 +1,237 @@
> +/*
> + * media_tknres.c - managed media token resource
> + *
> + * Copyright (c) 2014 Shuah Khan
> + * 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
