| Message ID | c8bae1d475b1086302fcb83bc463ec01437c3f95.1413246372.git.shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Tue, 14 Oct 2014 08:58:37 -0600, Shuah Khan wrote: > > Add media token device resource framework to allow sharing > resources such as tuner, dma, audio etc. across media drivers > and non-media sound drivers that control media hardware. The > Media token resource is created at the main struct device that > is common to all drivers that claim various pieces of the main > media device, which allows them to find the resource using the > main struct device. As an example, digital, analog, and > snd-usb-audio drivers can use the media token resource API > using the main struct device for the interface the media device > is attached to. > > A shared media tokens resource is created using devres framework > for drivers to find and lock/unlock. Creating a shared devres > helps avoid creating data structure dependencies between drivers. > This media token resource contains media token for tuner, and > audio. When tuner token is requested, audio token is issued. > Subsequent token (for tuner and audio) gets from the same task > and task in the same tgid succeed. This allows applications that > make multiple v4l2 ioctls to work with the first call acquiring > the token and applications that create separate threads to handle > video and audio functions. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > MAINTAINERS | 2 + > include/linux/media_tknres.h | 50 +++++++++ > lib/Makefile | 2 + > lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 291 insertions(+) > create mode 100644 include/linux/media_tknres.h > create mode 100644 lib/media_tknres.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e80a275..9216179 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-* > F: include/uapi/linux/meye.h > F: include/uapi/linux/ivtv* > F: include/uapi/linux/uvcvideo.h > +F: include/linux/media_tknres.h > +F: lib/media_tknres.c > > MEDIAVISION PRO MOVIE STUDIO DRIVER > M: Hans Verkuil <hverkuil@xs4all.nl> > diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h > new file mode 100644 > index 0000000..6d37327 > --- /dev/null > +++ b/include/linux/media_tknres.h > @@ -0,0 +1,50 @@ > +/* > + * media_tknres.h - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +#ifndef __LINUX_MEDIA_TOKEN_H > +#define __LINUX_MEDIA_TOKEN_H > + > +struct device; > + > +#if defined(CONFIG_MEDIA_SUPPORT) > +extern int media_tknres_create(struct device *dev); > +extern int media_tknres_destroy(struct device *dev); > + > +extern int media_get_tuner_tkn(struct device *dev); > +extern int media_put_tuner_tkn(struct device *dev); > + > +extern int media_get_audio_tkn(struct device *dev); > +extern int media_put_audio_tkn(struct device *dev); The words "tknres" and "tkn" (especially latter) look ugly and not clear to me. IMO, it should be "token". > +#else > +static inline int media_tknres_create(struct device *dev) > +{ > + return 0; > +} > +static inline int media_tknres_destroy(struct device *dev) > +{ > + return 0; > +} > +static inline int media_get_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static inline int media_put_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_get_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_put_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +#endif > + > +#endif /* __LINUX_MEDIA_TOKEN_H */ > diff --git a/lib/Makefile b/lib/Makefile > index d6b4bc4..6f21695 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o > > obj-$(CONFIG_GLOB) += glob.o > > +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o > + > obj-$(CONFIG_MPILIB) += mpi/ > obj-$(CONFIG_SIGNATURE) += digsig.o > > diff --git a/lib/media_tknres.c b/lib/media_tknres.c > new file mode 100644 > index 0000000..e0a36cb > --- /dev/null > +++ b/lib/media_tknres.c > @@ -0,0 +1,237 @@ > +/* > + * media_tknres.c - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +/* > + * Media devices often have hardware resources that are shared > + * across several functions. For instance, TV tuner cards often > + * have MUXes, converters, radios, tuners, etc. that are shared > + * across various functions. However, v4l2, alsa, DVB, usbfs, and > + * all other drivers have no knowledge of what resources are > + * shared. For example, users can't access DVB and alsa at the same > + * time, or the DVB and V4L analog API at the same time, since many > + * only have one converter that can be in either analog or digital > + * mode. Accessing and/or changing mode of a converter while it is > + * in use by another function results in video stream error. > + * > + * A shared media tokens resource is created using devres framework > + * for drivers to find and lock/unlock. Creating a shared devres > + * helps avoid creating data structure dependencies between drivers. > + * This media token resource contains media token for tuner, and > + * audio. When tuner token is requested, audio token is issued. > + * Subsequent token (for tuner and audio) gets from the same task > + * and task in the same tgid succeed. This allows applications that > + * make multiple v4l2 ioctls to work with the first call acquiring > + * the token and applications that create separate threads to handle > + * video and audio functions. > + * > + * API > + * int media_tknres_create(struct device *dev); > + * int media_tknres_destroy(struct device *dev); > + * > + * int media_get_tuner_tkn(struct device *dev); > + * int media_put_tuner_tkn(struct device *dev); > + * > + * int media_get_audio_tkn(struct device *dev); > + * int media_put_audio_tkn(struct device *dev); > +*/ > + > +#include <linux/kernel.h> > +#include <linux/device.h> > +#include <linux/sched.h> > +#include <linux/media_tknres.h> > + > +struct media_tkn { > + spinlock_t lock; > + unsigned int owner; /* owner task pid */ > + unsigned int tgid; /* owner task gid */ > + struct task_struct *task; > +}; > + > +struct media_tknres { > + struct media_tkn tuner; > + struct media_tkn audio; > +}; Why do you need to have both tuner and audio tokens? If I understand correctly, no matter whether it's tuner or audio, if it's being used, the open must fail, right? > + > +#define TUNER "Tuner" > +#define AUDIO "Audio" > + > +static void media_tknres_release(struct device *dev, void *res) > +{ > + dev_info(dev, "%s: Media Token Resource released\n", __func__); > +} > + > +int media_tknres_create(struct device *dev) > +{ > + struct media_tknres *tkn; > + > + tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres), > + GFP_KERNEL); > + if (!tkn) > + return -ENOMEM; > + > + spin_lock_init(&tkn->tuner.lock); > + tkn->tuner.owner = 0; > + tkn->tuner.tgid = 0; > + tkn->tuner.task = NULL; > + > + spin_lock_init(&tkn->audio.lock); > + tkn->audio.owner = 0; > + tkn->audio.tgid = 0; > + tkn->audio.task = NULL; > + > + devres_add(dev, tkn); > + > + dev_info(dev, "%s: Media Token Resource created\n", __func__); > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_create); > + > +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); You should use spin_lock_irqsave() here and in all other places. The trigger callback in usb-audio, for example, may be called in irq context. > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ IMO, it's not "release" but "steal"... But what happens if the stolen owner calls put? Then it'll be released although the stealing owner still thinks it's being held. > + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { Shouldn't the second "&&" be "||" instead? And too many parentheses there. > + rc = -EBUSY; Wrong indentation. I have a feeling that the whole thing can be a bit more simplified in the end... thanks, Takashi > + } else { > + tkn->owner = tpid; > + tkn->tgid = tgid; > + tkn->task = current; > + } > + pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); > + return rc; > +} > + > +static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ > + if (tkn->task == NULL || > + ((tkn->task != current) && (tkn->tgid != tgid))) { > + rc = -EINVAL; > + } else { > + tkn->owner = 0; > + tkn->tgid = 0; > + tkn->task = NULL; > + } > + pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); > + return rc; > +} > + > +/* > + * When media tknres doesn't exist, get and put interfaces > + * return 0 to let the callers take legacy code paths. This > + * will also cover the drivers that don't create media tknres. > + * Returning -ENODEV will require additional checks by callers. > + * Instead handle the media tknres not present case as a driver > + * not supporting media tknres and return 0. > +*/ > +int media_get_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + int ret = 0; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + ret = __media_get_tkn(&tkn_ptr->tuner, TUNER); > + if (ret) > + return ret; > + > + /* get audio token */ > + ret = __media_get_tkn(&tkn_ptr->audio, AUDIO); > + if (ret) > + __media_put_tkn(&tkn_ptr->tuner, TUNER); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(media_get_tuner_tkn); > + > +int media_put_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + /* put audio token and then tuner token */ > + __media_put_tkn(&tkn_ptr->audio, AUDIO); > + > + return __media_put_tkn(&tkn_ptr->tuner, TUNER); > +} > +EXPORT_SYMBOL_GPL(media_put_tuner_tkn); > + > +int media_get_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_get_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_get_audio_tkn); > + > +int media_put_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_put_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_put_audio_tkn); > + > +int media_tknres_destroy(struct device *dev) > +{ > + int rc; > + > + rc = devres_release(dev, media_tknres_release, NULL, NULL); > + WARN_ON(rc); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_destroy); > -- > 1.7.10.4 >
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
At Wed, 15 Oct 2014 18:53:28 -0600, Shuah Khan wrote: > > On 10/15/2014 11:05 AM, Takashi Iwai wrote: > > >> +#if defined(CONFIG_MEDIA_SUPPORT) > >> +extern int media_tknres_create(struct device *dev); > >> +extern int media_tknres_destroy(struct device *dev); > >> + > >> +extern int media_get_tuner_tkn(struct device *dev); > >> +extern int media_put_tuner_tkn(struct device *dev); > >> + > >> +extern int media_get_audio_tkn(struct device *dev); > >> +extern int media_put_audio_tkn(struct device *dev); > > > > The words "tknres" and "tkn" (especially latter) look ugly and not > > clear to me. IMO, it should be "token". > > No problem. I can change that to media_token_create/destroy() > and expand token in other functions. > > >> +struct media_tkn { > >> + spinlock_t lock; > >> + unsigned int owner; /* owner task pid */ > >> + unsigned int tgid; /* owner task gid */ > >> + struct task_struct *task; > >> +}; > >> + > >> +struct media_tknres { > >> + struct media_tkn tuner; > >> + struct media_tkn audio; > >> +}; > > > > Why do you need to have both tuner and audio tokens? If I understand > > correctly, no matter whether it's tuner or audio, if it's being used, > > the open must fail, right? > > As it evolved during development, it turns out at the moment I don't > have any use-cases that require holding audio and tuner separately. > It probably could be collapsed into just a media token. I have to > think about this some. > > >> + > >> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) > >> +{ > >> + int rc = 0; > >> + unsigned tpid; > >> + unsigned tgid; > >> + > >> + spin_lock(&tkn->lock); > > > > You should use spin_lock_irqsave() here and in all other places. > > The trigger callback in usb-audio, for example, may be called in irq > > context. > > ok. Good point, will change that. > > > > >> + > >> + tpid = task_pid_nr(current); > >> + tgid = task_tgid_nr(current); > >> + > >> + /* allow task in the same group id to release */ > > > > IMO, it's not "release" but "steal"... But what happens if the stolen > > owner calls put? Then it'll be released although the stealing owner > > still thinks it's being held. > > Yeah it could be called a steal. :) Essentially tgid happens to be > the real owner. I am overwriting the pid with current pid when > tgid is same. > > media dvb and v4l apps start two or more threads that all share the > tgid and subsequent token gets should be allowed based on the tgid. > > Scenario 1: > > Please note that there are 3 device files in question and media > token resource is the lock for all. Hence the changes to v4l-core, > dvb-core, and snd-usb-audio to hold the token for exclusive access > when the task or tgid don't match. > > program starts: > - first thread opens device file in R/W mode - open gets the token > or thread makes ioctls calls that clearly indicates intent to > change tuner settings > - creates one thread for audio > - creates another for video or continues video function > - video thread does a close and re-opens the device file > > In this case when thread tries to close, token put fails unless > tasks with same tgid are allowed to release. > ( I ran into this one of the media applications and it is a valid > case to handle, thread can close the file and should be able to > open again without running into token busy case ) > > - or continue to just use the device file > - audio thread does snd_pcm_capture ops - trigger start > > program exits: > - video thread closes the device file > - audio thread does snd_pcm_capture ops - trigger stop > > This got me thinking about the scenario when an application > does a fork() as opposed to create a thread. I have to think > about this and see how I can address that. > > > > >> + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { > > > > Shouldn't the second "&&" be "||" instead? > > And too many parentheses there. > > Right - this is my bad. The comment right above this conditional > is a give away that, at some point I did a copy and paste from > _put to _get and only changed the first task null check. > I am yelling at myself at the moment. > > > > >> + rc = -EBUSY; > > > > Wrong indentation. > > Yes. Will fix that. > > > > > I have a feeling that the whole thing can be a bit more simplified in > > the end... > > > > Any ideas to simplify are welcome. There are a few points to make things more consistent and simplified, IMO. First of all, the whole concept can be more generalized. It's not necessarily only for media and audio. Rather we can make it a generic dev_token. Then it'll become more convincing to land into lib/*. The media-specific handling is only about the pid and gid checks. This can be implemented as a callback that is passed at creating a token, for example. This will reduce the core code in lib/*. Also, get and put should handle a proper refcount. The point I mentioned in my previous post is the possible unbalance, and you'll see unexpected unlock in the scenario. In addition, with use of kref, the lock can be removed. So, essentially the lib code would be just a devres_alloc() for an object with kref, and dev_token_get() will take a kref with the exclusion check. Takashi
On 10/16/2014 08:00 AM, Takashi Iwai wrote: > At Wed, 15 Oct 2014 18:53:28 -0600, > Shuah Khan wrote: >> >> On 10/15/2014 11:05 AM, Takashi Iwai wrote: >> >>>> +#if defined(CONFIG_MEDIA_SUPPORT) >>>> +extern int media_tknres_create(struct device *dev); >>>> +extern int media_tknres_destroy(struct device *dev); >>>> + >>>> +extern int media_get_tuner_tkn(struct device *dev); >>>> +extern int media_put_tuner_tkn(struct device *dev); >>>> + >>>> +extern int media_get_audio_tkn(struct device *dev); >>>> +extern int media_put_audio_tkn(struct device *dev); >>> >>> The words "tknres" and "tkn" (especially latter) look ugly and not >>> clear to me. IMO, it should be "token". >> >> No problem. I can change that to media_token_create/destroy() >> and expand token in other functions. >> >>>> +struct media_tkn { >>>> + spinlock_t lock; >>>> + unsigned int owner; /* owner task pid */ >>>> + unsigned int tgid; /* owner task gid */ >>>> + struct task_struct *task; >>>> +}; >>>> + >>>> +struct media_tknres { >>>> + struct media_tkn tuner; >>>> + struct media_tkn audio; >>>> +}; >>> >>> Why do you need to have both tuner and audio tokens? If I understand >>> correctly, no matter whether it's tuner or audio, if it's being used, >>> the open must fail, right? >> >> As it evolved during development, it turns out at the moment I don't >> have any use-cases that require holding audio and tuner separately. >> It probably could be collapsed into just a media token. I have to >> think about this some. >> >>>> + >>>> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) >>>> +{ >>>> + int rc = 0; >>>> + unsigned tpid; >>>> + unsigned tgid; >>>> + >>>> + spin_lock(&tkn->lock); >>> >>> You should use spin_lock_irqsave() here and in all other places. >>> The trigger callback in usb-audio, for example, may be called in irq >>> context. >> >> ok. Good point, will change that. >> >>> >>>> + >>>> + tpid = task_pid_nr(current); >>>> + tgid = task_tgid_nr(current); >>>> + >>>> + /* allow task in the same group id to release */ >>> >>> IMO, it's not "release" but "steal"... But what happens if the stolen >>> owner calls put? Then it'll be released although the stealing owner >>> still thinks it's being held. >> >> Yeah it could be called a steal. :) Essentially tgid happens to be >> the real owner. I am overwriting the pid with current pid when >> tgid is same. >> >> media dvb and v4l apps start two or more threads that all share the >> tgid and subsequent token gets should be allowed based on the tgid. >> >> Scenario 1: >> >> Please note that there are 3 device files in question and media >> token resource is the lock for all. Hence the changes to v4l-core, >> dvb-core, and snd-usb-audio to hold the token for exclusive access >> when the task or tgid don't match. >> >> program starts: >> - first thread opens device file in R/W mode - open gets the token >> or thread makes ioctls calls that clearly indicates intent to >> change tuner settings >> - creates one thread for audio >> - creates another for video or continues video function >> - video thread does a close and re-opens the device file >> >> In this case when thread tries to close, token put fails unless >> tasks with same tgid are allowed to release. >> ( I ran into this one of the media applications and it is a valid >> case to handle, thread can close the file and should be able to >> open again without running into token busy case ) >> >> - or continue to just use the device file >> - audio thread does snd_pcm_capture ops - trigger start >> >> program exits: >> - video thread closes the device file >> - audio thread does snd_pcm_capture ops - trigger stop >> >> This got me thinking about the scenario when an application >> does a fork() as opposed to create a thread. I have to think >> about this and see how I can address that. >> >>> >>>> + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { >>> >>> Shouldn't the second "&&" be "||" instead? >>> And too many parentheses there. >> >> Right - this is my bad. The comment right above this conditional >> is a give away that, at some point I did a copy and paste from >> _put to _get and only changed the first task null check. >> I am yelling at myself at the moment. >> >>> >>>> + rc = -EBUSY; >>> >>> Wrong indentation. >> >> Yes. Will fix that. >> >>> >>> I have a feeling that the whole thing can be a bit more simplified in >>> the end... >>> >> >> Any ideas to simplify are welcome. > > There are a few points to make things more consistent and simplified, > IMO. First of all, the whole concept can be more generalized. It's > not necessarily only for media and audio. Rather we can make it a > generic dev_token. Then it'll become more convincing to land into > lib/*. Right. At the moment this is very media specific. > > The media-specific handling is only about the pid and gid checks. > This can be implemented as a callback that is passed at creating a > token, for example. This will reduce the core code in lib/*. > > Also, get and put should handle a proper refcount. The point I > mentioned in my previous post is the possible unbalance, and you'll > see unexpected unlock in the scenario. In addition, with use of kref, > the lock can be removed. Yes. kref would eliminate the need for locks and potential for unbalanced scenario. > > So, essentially the lib code would be just a devres_alloc() for an > object with kref, and dev_token_get() will take a kref with the > exclusion check. > I considered callback for media specific handling to make this token construct generic. Talked myself out of it with the idea to get this working for media use-case first and then extend it to be generic. With this patch set covering media cases including non-media driver, I think I am confident that the approach itself works to address the issues and I don't mind pursuing a generic approach you are suggesting. The current work isn't that far off and I can get the generic working without many changes. Thanks again for talking through the problem and ideas. -- Shuah
Hi Shuah, As promised, here is my review for this patch series. On 10/14/2014 04:58 PM, Shuah Khan wrote: > Add media token device resource framework to allow sharing > resources such as tuner, dma, audio etc. across media drivers > and non-media sound drivers that control media hardware. The > Media token resource is created at the main struct device that > is common to all drivers that claim various pieces of the main > media device, which allows them to find the resource using the > main struct device. As an example, digital, analog, and > snd-usb-audio drivers can use the media token resource API > using the main struct device for the interface the media device > is attached to. > > A shared media tokens resource is created using devres framework > for drivers to find and lock/unlock. Creating a shared devres > helps avoid creating data structure dependencies between drivers. > This media token resource contains media token for tuner, and > audio. When tuner token is requested, audio token is issued. Did you mean: 'tuner token is issued' instead of audio token? I also have the same question as Takashi: why do we have an audio token in the first place? While you are streaming audio over alsa the underlying tuner must be marked as being in use. It's all about the tuner, since that's the resource that is being shared, not about audio as such. For the remainder of my review I will ignore the audio-related code and concentrate only on the tuner part. > Subsequent token (for tuner and audio) gets from the same task > and task in the same tgid succeed. This allows applications that > make multiple v4l2 ioctls to work with the first call acquiring > the token and applications that create separate threads to handle > video and audio functions. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > MAINTAINERS | 2 + > include/linux/media_tknres.h | 50 +++++++++ > lib/Makefile | 2 + > lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ I am still not convinced myself that this should be a generic API. The only reason we need it today is for sharing tuners. Which is almost a purely media thing with USB audio as the single non-media driver that will be affected. Today I see no use case outside of tuners. I would probably want to keep this inside drivers/media. If this is going to be expanded it can always be moved to lib later. > 4 files changed, 291 insertions(+) > create mode 100644 include/linux/media_tknres.h > create mode 100644 lib/media_tknres.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e80a275..9216179 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-* > F: include/uapi/linux/meye.h > F: include/uapi/linux/ivtv* > F: include/uapi/linux/uvcvideo.h > +F: include/linux/media_tknres.h > +F: lib/media_tknres.c > > MEDIAVISION PRO MOVIE STUDIO DRIVER > M: Hans Verkuil <hverkuil@xs4all.nl> > diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h > new file mode 100644 > index 0000000..6d37327 > --- /dev/null > +++ b/include/linux/media_tknres.h > @@ -0,0 +1,50 @@ > +/* > + * media_tknres.h - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +#ifndef __LINUX_MEDIA_TOKEN_H > +#define __LINUX_MEDIA_TOKEN_H > + > +struct device; > + > +#if defined(CONFIG_MEDIA_SUPPORT) > +extern int media_tknres_create(struct device *dev); > +extern int media_tknres_destroy(struct device *dev); > + > +extern int media_get_tuner_tkn(struct device *dev); > +extern int media_put_tuner_tkn(struct device *dev); > + > +extern int media_get_audio_tkn(struct device *dev); > +extern int media_put_audio_tkn(struct device *dev); > +#else > +static inline int media_tknres_create(struct device *dev) > +{ > + return 0; > +} > +static inline int media_tknres_destroy(struct device *dev) > +{ > + return 0; > +} > +static inline int media_get_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static inline int media_put_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_get_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_put_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +#endif > + > +#endif /* __LINUX_MEDIA_TOKEN_H */ > diff --git a/lib/Makefile b/lib/Makefile > index d6b4bc4..6f21695 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o > > obj-$(CONFIG_GLOB) += glob.o > > +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o > + > obj-$(CONFIG_MPILIB) += mpi/ > obj-$(CONFIG_SIGNATURE) += digsig.o > > diff --git a/lib/media_tknres.c b/lib/media_tknres.c > new file mode 100644 > index 0000000..e0a36cb > --- /dev/null > +++ b/lib/media_tknres.c > @@ -0,0 +1,237 @@ > +/* > + * media_tknres.c - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +/* > + * Media devices often have hardware resources that are shared > + * across several functions. For instance, TV tuner cards often > + * have MUXes, converters, radios, tuners, etc. that are shared > + * across various functions. However, v4l2, alsa, DVB, usbfs, and > + * all other drivers have no knowledge of what resources are > + * shared. For example, users can't access DVB and alsa at the same > + * time, or the DVB and V4L analog API at the same time, since many > + * only have one converter that can be in either analog or digital > + * mode. Accessing and/or changing mode of a converter while it is > + * in use by another function results in video stream error. > + * > + * A shared media tokens resource is created using devres framework > + * for drivers to find and lock/unlock. Creating a shared devres > + * helps avoid creating data structure dependencies between drivers. > + * This media token resource contains media token for tuner, and > + * audio. When tuner token is requested, audio token is issued. > + * Subsequent token (for tuner and audio) gets from the same task > + * and task in the same tgid succeed. This allows applications that > + * make multiple v4l2 ioctls to work with the first call acquiring > + * the token and applications that create separate threads to handle > + * video and audio functions. > + * > + * API > + * int media_tknres_create(struct device *dev); > + * int media_tknres_destroy(struct device *dev); > + * > + * int media_get_tuner_tkn(struct device *dev); > + * int media_put_tuner_tkn(struct device *dev); > + * > + * int media_get_audio_tkn(struct device *dev); > + * int media_put_audio_tkn(struct device *dev); > +*/ > + > +#include <linux/kernel.h> > +#include <linux/device.h> > +#include <linux/sched.h> > +#include <linux/media_tknres.h> > + > +struct media_tkn { > + spinlock_t lock; > + unsigned int owner; /* owner task pid */ > + unsigned int tgid; /* owner task gid */ > + struct task_struct *task; > +}; > + > +struct media_tknres { > + struct media_tkn tuner; > + struct media_tkn audio; > +}; > + > +#define TUNER "Tuner" > +#define AUDIO "Audio" > + > +static void media_tknres_release(struct device *dev, void *res) > +{ > + dev_info(dev, "%s: Media Token Resource released\n", __func__); dev_dbg would be more appropriate. > +} > + > +int media_tknres_create(struct device *dev) > +{ > + struct media_tknres *tkn; > + > + tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres), > + GFP_KERNEL); > + if (!tkn) > + return -ENOMEM; > + > + spin_lock_init(&tkn->tuner.lock); > + tkn->tuner.owner = 0; > + tkn->tuner.tgid = 0; > + tkn->tuner.task = NULL; > + > + spin_lock_init(&tkn->audio.lock); > + tkn->audio.owner = 0; > + tkn->audio.tgid = 0; > + tkn->audio.task = NULL; > + > + devres_add(dev, tkn); > + > + dev_info(dev, "%s: Media Token Resource created\n", __func__); Ditto. > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_create); > + > +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ > + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { > + rc = -EBUSY; As I understand it this makes no sense: if tkn->task == current, then tkn->tgid == tgid. Why would you allow different task in the same group id to release anyway? > + } else { > + tkn->owner = tpid; > + tkn->tgid = tgid; > + tkn->task = current; > + } > + pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); > + return rc; > +} > + > +static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ > + if (tkn->task == NULL || > + ((tkn->task != current) && (tkn->tgid != tgid))) { > + rc = -EINVAL; > + } else { > + tkn->owner = 0; > + tkn->tgid = 0; > + tkn->task = NULL; > + } > + pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); > + return rc; > +} > + > +/* > + * When media tknres doesn't exist, get and put interfaces > + * return 0 to let the callers take legacy code paths. This > + * will also cover the drivers that don't create media tknres. > + * Returning -ENODEV will require additional checks by callers. > + * Instead handle the media tknres not present case as a driver > + * not supporting media tknres and return 0. > +*/ > +int media_get_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + int ret = 0; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + ret = __media_get_tkn(&tkn_ptr->tuner, TUNER); OK, this makes no sense. I am completely missing the tuner mode here. Speaking for V4L2 (I am less sure about DVB) any application can access the tuner even if it is owned by another application as long as you don't switch mode. The way it works now it would block any other application from accessing the tuner, or even other threads in the same process group. That's pretty much guaranteed to break existing code. So these tokens should be refcounted (certainly for the analog tuner mode, but probably for all modes). In the V4L2 wrapper function you just check if the token has been obtained already by setting a flag in struct v4l2_fh. If not, then you get the token. When the file handle is released and if it has the token, then you release that token. It is my understanding (correct me if I am wrong) that DVB and ALSA allow only one user of the interface at a time, so you can still use refcounting there, but the refcount will never go above 1 anyway. I just hacked tknres support into vivid to test this and it really disables this important feature. I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this module to the GPL devres_* functions. It took me some time to figure that out. > + if (ret) > + return ret; > + > + /* get audio token */ > + ret = __media_get_tkn(&tkn_ptr->audio, AUDIO); > + if (ret) > + __media_put_tkn(&tkn_ptr->tuner, TUNER); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(media_get_tuner_tkn); > + > +int media_put_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + /* put audio token and then tuner token */ > + __media_put_tkn(&tkn_ptr->audio, AUDIO); > + > + return __media_put_tkn(&tkn_ptr->tuner, TUNER); > +} > +EXPORT_SYMBOL_GPL(media_put_tuner_tkn); > + > +int media_get_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_get_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_get_audio_tkn); > + > +int media_put_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_put_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_put_audio_tkn); > + > +int media_tknres_destroy(struct device *dev) > +{ > + int rc; > + > + rc = devres_release(dev, media_tknres_release, NULL, NULL); > + WARN_ON(rc); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_destroy); > I'm really sorry but I have to nack this patch series. It simply breaks existing V4L functionality that I know people are using. I'm not going to review the other patches, they seemed mostly OK to me although I would expect to see changes to videobuf2-core.c as well which I didn't. You might want to think about adding media token support to vivid to test this. The vivid driver supports both radio and TV tuners. Currently those are emulated as independent tuners, but you could try to use media token to emulate it as a shared resource. That way you can test this with a vb2 driver as well. Anyway: Nacked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans
At Tue, 21 Oct 2014 12:46:03 +0200, Hans Verkuil wrote: > > Hi Shuah, > > As promised, here is my review for this patch series. > > On 10/14/2014 04:58 PM, Shuah Khan wrote: > > Add media token device resource framework to allow sharing > > resources such as tuner, dma, audio etc. across media drivers > > and non-media sound drivers that control media hardware. The > > Media token resource is created at the main struct device that > > is common to all drivers that claim various pieces of the main > > media device, which allows them to find the resource using the > > main struct device. As an example, digital, analog, and > > snd-usb-audio drivers can use the media token resource API > > using the main struct device for the interface the media device > > is attached to. > > > > A shared media tokens resource is created using devres framework > > for drivers to find and lock/unlock. Creating a shared devres > > helps avoid creating data structure dependencies between drivers. > > This media token resource contains media token for tuner, and > > audio. When tuner token is requested, audio token is issued. > > Did you mean: 'tuner token is issued' instead of audio token? > > I also have the same question as Takashi: why do we have an audio > token in the first place? While you are streaming audio over alsa > the underlying tuner must be marked as being in use. It's all about > the tuner, since that's the resource that is being shared, not about > audio as such. > > For the remainder of my review I will ignore the audio-related code > and concentrate only on the tuner part. > > > Subsequent token (for tuner and audio) gets from the same task > > and task in the same tgid succeed. This allows applications that > > make multiple v4l2 ioctls to work with the first call acquiring > > the token and applications that create separate threads to handle > > video and audio functions. > > > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > > --- > > MAINTAINERS | 2 + > > include/linux/media_tknres.h | 50 +++++++++ > > lib/Makefile | 2 + > > lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ > > I am still not convinced myself that this should be a generic API. > The only reason we need it today is for sharing tuners. Which is almost > a purely media thing with USB audio as the single non-media driver that > will be affected. Today I see no use case outside of tuners. I would > probably want to keep this inside drivers/media. > > If this is going to be expanded it can always be moved to lib later. Well, my argument is that it should be more generic if it were intended to be put in lib. It'd be fine to put into drivers/media, but this code snippet must be a separate module. Otherwise usb-audio would grab the whole media stuff even if not needed at all. (snip) > I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION > and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this > module to the GPL devres_* functions. It took me some time to figure that > out. It was a code in lib, so it cannot be a module at all :) Takashi
On 10/21/2014 01:51 PM, Takashi Iwai wrote: > At Tue, 21 Oct 2014 12:46:03 +0200, > Hans Verkuil wrote: >> >> Hi Shuah, >> >> As promised, here is my review for this patch series. >> >> On 10/14/2014 04:58 PM, Shuah Khan wrote: >>> Add media token device resource framework to allow sharing >>> resources such as tuner, dma, audio etc. across media drivers >>> and non-media sound drivers that control media hardware. The >>> Media token resource is created at the main struct device that >>> is common to all drivers that claim various pieces of the main >>> media device, which allows them to find the resource using the >>> main struct device. As an example, digital, analog, and >>> snd-usb-audio drivers can use the media token resource API >>> using the main struct device for the interface the media device >>> is attached to. >>> >>> A shared media tokens resource is created using devres framework >>> for drivers to find and lock/unlock. Creating a shared devres >>> helps avoid creating data structure dependencies between drivers. >>> This media token resource contains media token for tuner, and >>> audio. When tuner token is requested, audio token is issued. >> >> Did you mean: 'tuner token is issued' instead of audio token? >> >> I also have the same question as Takashi: why do we have an audio >> token in the first place? While you are streaming audio over alsa >> the underlying tuner must be marked as being in use. It's all about >> the tuner, since that's the resource that is being shared, not about >> audio as such. >> >> For the remainder of my review I will ignore the audio-related code >> and concentrate only on the tuner part. >> >>> Subsequent token (for tuner and audio) gets from the same task >>> and task in the same tgid succeed. This allows applications that >>> make multiple v4l2 ioctls to work with the first call acquiring >>> the token and applications that create separate threads to handle >>> video and audio functions. >>> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> MAINTAINERS | 2 + >>> include/linux/media_tknres.h | 50 +++++++++ >>> lib/Makefile | 2 + >>> lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ >> >> I am still not convinced myself that this should be a generic API. >> The only reason we need it today is for sharing tuners. Which is almost >> a purely media thing with USB audio as the single non-media driver that >> will be affected. Today I see no use case outside of tuners. I would >> probably want to keep this inside drivers/media. >> >> If this is going to be expanded it can always be moved to lib later. > > Well, my argument is that it should be more generic if it were > intended to be put in lib. It'd be fine to put into drivers/media, > but this code snippet must be a separate module. Otherwise usb-audio > would grab the whole media stuff even if not needed at all. Certainly. > > (snip) >> I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION >> and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this >> module to the GPL devres_* functions. It took me some time to figure that >> out. > > It was a code in lib, so it cannot be a module at all :) Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it compiles as a module :-) Regards, Hans
At Tue, 21 Oct 2014 13:58:59 +0200, Hans Verkuil wrote: > > > > On 10/21/2014 01:51 PM, Takashi Iwai wrote: > > At Tue, 21 Oct 2014 12:46:03 +0200, > > Hans Verkuil wrote: > >> > >> Hi Shuah, > >> > >> As promised, here is my review for this patch series. > >> > >> On 10/14/2014 04:58 PM, Shuah Khan wrote: > >>> Add media token device resource framework to allow sharing > >>> resources such as tuner, dma, audio etc. across media drivers > >>> and non-media sound drivers that control media hardware. The > >>> Media token resource is created at the main struct device that > >>> is common to all drivers that claim various pieces of the main > >>> media device, which allows them to find the resource using the > >>> main struct device. As an example, digital, analog, and > >>> snd-usb-audio drivers can use the media token resource API > >>> using the main struct device for the interface the media device > >>> is attached to. > >>> > >>> A shared media tokens resource is created using devres framework > >>> for drivers to find and lock/unlock. Creating a shared devres > >>> helps avoid creating data structure dependencies between drivers. > >>> This media token resource contains media token for tuner, and > >>> audio. When tuner token is requested, audio token is issued. > >> > >> Did you mean: 'tuner token is issued' instead of audio token? > >> > >> I also have the same question as Takashi: why do we have an audio > >> token in the first place? While you are streaming audio over alsa > >> the underlying tuner must be marked as being in use. It's all about > >> the tuner, since that's the resource that is being shared, not about > >> audio as such. > >> > >> For the remainder of my review I will ignore the audio-related code > >> and concentrate only on the tuner part. > >> > >>> Subsequent token (for tuner and audio) gets from the same task > >>> and task in the same tgid succeed. This allows applications that > >>> make multiple v4l2 ioctls to work with the first call acquiring > >>> the token and applications that create separate threads to handle > >>> video and audio functions. > >>> > >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > >>> --- > >>> MAINTAINERS | 2 + > >>> include/linux/media_tknres.h | 50 +++++++++ > >>> lib/Makefile | 2 + > >>> lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ > >> > >> I am still not convinced myself that this should be a generic API. > >> The only reason we need it today is for sharing tuners. Which is almost > >> a purely media thing with USB audio as the single non-media driver that > >> will be affected. Today I see no use case outside of tuners. I would > >> probably want to keep this inside drivers/media. > >> > >> If this is going to be expanded it can always be moved to lib later. > > > > Well, my argument is that it should be more generic if it were > > intended to be put in lib. It'd be fine to put into drivers/media, > > but this code snippet must be a separate module. Otherwise usb-audio > > would grab the whole media stuff even if not needed at all. > > Certainly. > > > > > (snip) > >> I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION > >> and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this > >> module to the GPL devres_* functions. It took me some time to figure that > >> out. > > > > It was a code in lib, so it cannot be a module at all :) > > Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it > compiles as a module :-) Ah, I thought it was a boolean, but yes, this can be a module indeed. Then I don't think it's worth to put in lib. Takashi
Hi Shuah, A few comments below. On Tue, Oct 14, 2014 at 08:58:37AM -0600, Shuah Khan wrote: > Add media token device resource framework to allow sharing > resources such as tuner, dma, audio etc. across media drivers > and non-media sound drivers that control media hardware. The > Media token resource is created at the main struct device that > is common to all drivers that claim various pieces of the main > media device, which allows them to find the resource using the > main struct device. As an example, digital, analog, and > snd-usb-audio drivers can use the media token resource API > using the main struct device for the interface the media device > is attached to. > > A shared media tokens resource is created using devres framework > for drivers to find and lock/unlock. Creating a shared devres > helps avoid creating data structure dependencies between drivers. > This media token resource contains media token for tuner, and > audio. When tuner token is requested, audio token is issued. > Subsequent token (for tuner and audio) gets from the same task > and task in the same tgid succeed. This allows applications that > make multiple v4l2 ioctls to work with the first call acquiring > the token and applications that create separate threads to handle > video and audio functions. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > MAINTAINERS | 2 + > include/linux/media_tknres.h | 50 +++++++++ > lib/Makefile | 2 + > lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 291 insertions(+) > create mode 100644 include/linux/media_tknres.h > create mode 100644 lib/media_tknres.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e80a275..9216179 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-* > F: include/uapi/linux/meye.h > F: include/uapi/linux/ivtv* > F: include/uapi/linux/uvcvideo.h > +F: include/linux/media_tknres.h > +F: lib/media_tknres.c > > MEDIAVISION PRO MOVIE STUDIO DRIVER > M: Hans Verkuil <hverkuil@xs4all.nl> > diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h > new file mode 100644 > index 0000000..6d37327 > --- /dev/null > +++ b/include/linux/media_tknres.h > @@ -0,0 +1,50 @@ > +/* > + * media_tknres.h - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +#ifndef __LINUX_MEDIA_TOKEN_H > +#define __LINUX_MEDIA_TOKEN_H > + > +struct device; > + > +#if defined(CONFIG_MEDIA_SUPPORT) > +extern int media_tknres_create(struct device *dev); > +extern int media_tknres_destroy(struct device *dev); > + > +extern int media_get_tuner_tkn(struct device *dev); > +extern int media_put_tuner_tkn(struct device *dev); > + > +extern int media_get_audio_tkn(struct device *dev); > +extern int media_put_audio_tkn(struct device *dev); > +#else > +static inline int media_tknres_create(struct device *dev) > +{ > + return 0; > +} > +static inline int media_tknres_destroy(struct device *dev) > +{ > + return 0; > +} > +static inline int media_get_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static inline int media_put_tuner_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_get_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +static int media_put_audio_tkn(struct device *dev) > +{ > + return 0; > +} > +#endif > + > +#endif /* __LINUX_MEDIA_TOKEN_H */ > diff --git a/lib/Makefile b/lib/Makefile > index d6b4bc4..6f21695 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o > > obj-$(CONFIG_GLOB) += glob.o > > +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o I'd add a new Kconfig option for this, as it's only needed by a very specific kind of drivers. It could be automatically selected by those that need it. > + > obj-$(CONFIG_MPILIB) += mpi/ > obj-$(CONFIG_SIGNATURE) += digsig.o > > diff --git a/lib/media_tknres.c b/lib/media_tknres.c > new file mode 100644 > index 0000000..e0a36cb > --- /dev/null > +++ b/lib/media_tknres.c > @@ -0,0 +1,237 @@ > +/* > + * media_tknres.c - managed media token resource > + * > + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * > + * This file is released under the GPLv2. > + */ > +/* > + * Media devices often have hardware resources that are shared > + * across several functions. For instance, TV tuner cards often > + * have MUXes, converters, radios, tuners, etc. that are shared > + * across various functions. However, v4l2, alsa, DVB, usbfs, and > + * all other drivers have no knowledge of what resources are > + * shared. For example, users can't access DVB and alsa at the same > + * time, or the DVB and V4L analog API at the same time, since many > + * only have one converter that can be in either analog or digital > + * mode. Accessing and/or changing mode of a converter while it is > + * in use by another function results in video stream error. > + * > + * A shared media tokens resource is created using devres framework > + * for drivers to find and lock/unlock. Creating a shared devres > + * helps avoid creating data structure dependencies between drivers. > + * This media token resource contains media token for tuner, and > + * audio. When tuner token is requested, audio token is issued. > + * Subsequent token (for tuner and audio) gets from the same task > + * and task in the same tgid succeed. This allows applications that > + * make multiple v4l2 ioctls to work with the first call acquiring > + * the token and applications that create separate threads to handle > + * video and audio functions. > + * > + * API > + * int media_tknres_create(struct device *dev); > + * int media_tknres_destroy(struct device *dev); > + * > + * int media_get_tuner_tkn(struct device *dev); > + * int media_put_tuner_tkn(struct device *dev); > + * > + * int media_get_audio_tkn(struct device *dev); > + * int media_put_audio_tkn(struct device *dev); > +*/ > + > +#include <linux/kernel.h> > +#include <linux/device.h> > +#include <linux/sched.h> > +#include <linux/media_tknres.h> Alphabetical order, please. > + > +struct media_tkn { > + spinlock_t lock; > + unsigned int owner; /* owner task pid */ > + unsigned int tgid; /* owner task gid */ > + struct task_struct *task; > +}; > + > +struct media_tknres { > + struct media_tkn tuner; > + struct media_tkn audio; > +}; > + > +#define TUNER "Tuner" > +#define AUDIO "Audio" > + > +static void media_tknres_release(struct device *dev, void *res) > +{ > + dev_info(dev, "%s: Media Token Resource released\n", __func__); > +} > + > +int media_tknres_create(struct device *dev) > +{ > + struct media_tknres *tkn; > + > + tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres), > + GFP_KERNEL); > + if (!tkn) > + return -ENOMEM; > + > + spin_lock_init(&tkn->tuner.lock); > + tkn->tuner.owner = 0; > + tkn->tuner.tgid = 0; > + tkn->tuner.task = NULL; > + > + spin_lock_init(&tkn->audio.lock); > + tkn->audio.owner = 0; > + tkn->audio.tgid = 0; > + tkn->audio.task = NULL; > + > + devres_add(dev, tkn); > + > + dev_info(dev, "%s: Media Token Resource created\n", __func__); > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_create); > + > +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ > + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { > + rc = -EBUSY; Indentation. I'd like to someone else comment on this approach in general, but I'm not aware of better ways to do this either as there's no common device node (so file handles can't be used either). > + } else { > + tkn->owner = tpid; > + tkn->tgid = tgid; > + tkn->task = current; > + } > + pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); > + return rc; > +} > + > +static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str) > +{ > + int rc = 0; > + unsigned tpid; > + unsigned tgid; > + > + spin_lock(&tkn->lock); > + > + tpid = task_pid_nr(current); > + tgid = task_tgid_nr(current); > + > + /* allow task in the same group id to release */ > + if (tkn->task == NULL || > + ((tkn->task != current) && (tkn->tgid != tgid))) { > + rc = -EINVAL; Indentation. > + } else { > + tkn->owner = 0; > + tkn->tgid = 0; > + tkn->task = NULL; > + } > + pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n", > + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); > + > + spin_unlock(&tkn->lock); No need to print things while holding the lock. > + return rc; > +} > + > +/* > + * When media tknres doesn't exist, get and put interfaces > + * return 0 to let the callers take legacy code paths. This > + * will also cover the drivers that don't create media tknres. > + * Returning -ENODEV will require additional checks by callers. > + * Instead handle the media tknres not present case as a driver > + * not supporting media tknres and return 0. > +*/ > +int media_get_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + int ret = 0; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + ret = __media_get_tkn(&tkn_ptr->tuner, TUNER); > + if (ret) > + return ret; > + > + /* get audio token */ > + ret = __media_get_tkn(&tkn_ptr->audio, AUDIO); > + if (ret) > + __media_put_tkn(&tkn_ptr->tuner, TUNER); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(media_get_tuner_tkn); You're calling media_get_tuner_tkn() from a number of V4L2 framework functions that handle common and often performance critical IOCTLs. devres_find() will loop over the entire list of resources associated with that device. I think this would be just fine if it was done just once in a non-performance critical time, but not every time everywhere. > + > +int media_put_tuner_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + /* put audio token and then tuner token */ > + __media_put_tkn(&tkn_ptr->audio, AUDIO); > + > + return __media_put_tkn(&tkn_ptr->tuner, TUNER); > +} > +EXPORT_SYMBOL_GPL(media_put_tuner_tkn); > + > +int media_get_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_get_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_get_audio_tkn); > + > +int media_put_audio_tkn(struct device *dev) > +{ > + struct media_tknres *tkn_ptr; > + > + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); > + if (tkn_ptr == NULL) { > + dev_dbg(dev, "%s: Media Token Resource not found\n", > + __func__); > + return 0; > + } > + > + return __media_put_tkn(&tkn_ptr->audio, AUDIO); > +} > +EXPORT_SYMBOL_GPL(media_put_audio_tkn); > + > +int media_tknres_destroy(struct device *dev) > +{ > + int rc; > + > + rc = devres_release(dev, media_tknres_release, NULL, NULL); > + WARN_ON(rc); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_tknres_destroy);
diff --git a/MAINTAINERS b/MAINTAINERS index e80a275..9216179 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5864,6 +5864,8 @@ F: include/uapi/linux/v4l2-* F: include/uapi/linux/meye.h F: include/uapi/linux/ivtv* F: include/uapi/linux/uvcvideo.h +F: include/linux/media_tknres.h +F: lib/media_tknres.c MEDIAVISION PRO MOVIE STUDIO DRIVER M: Hans Verkuil <hverkuil@xs4all.nl> diff --git a/include/linux/media_tknres.h b/include/linux/media_tknres.h new file mode 100644 index 0000000..6d37327 --- /dev/null +++ b/include/linux/media_tknres.h @@ -0,0 +1,50 @@ +/* + * media_tknres.h - managed media token resource + * + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * + * This file is released under the GPLv2. + */ +#ifndef __LINUX_MEDIA_TOKEN_H +#define __LINUX_MEDIA_TOKEN_H + +struct device; + +#if defined(CONFIG_MEDIA_SUPPORT) +extern int media_tknres_create(struct device *dev); +extern int media_tknres_destroy(struct device *dev); + +extern int media_get_tuner_tkn(struct device *dev); +extern int media_put_tuner_tkn(struct device *dev); + +extern int media_get_audio_tkn(struct device *dev); +extern int media_put_audio_tkn(struct device *dev); +#else +static inline int media_tknres_create(struct device *dev) +{ + return 0; +} +static inline int media_tknres_destroy(struct device *dev) +{ + return 0; +} +static inline int media_get_tuner_tkn(struct device *dev) +{ + return 0; +} +static inline int media_put_tuner_tkn(struct device *dev) +{ + return 0; +} +static int media_get_audio_tkn(struct device *dev) +{ + return 0; +} +static int media_put_audio_tkn(struct device *dev) +{ + return 0; +} +#endif + +#endif /* __LINUX_MEDIA_TOKEN_H */ diff --git a/lib/Makefile b/lib/Makefile index d6b4bc4..6f21695 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -139,6 +139,8 @@ obj-$(CONFIG_DQL) += dynamic_queue_limits.o obj-$(CONFIG_GLOB) += glob.o +obj-$(CONFIG_MEDIA_SUPPORT) += media_tknres.o + obj-$(CONFIG_MPILIB) += mpi/ obj-$(CONFIG_SIGNATURE) += digsig.o diff --git a/lib/media_tknres.c b/lib/media_tknres.c new file mode 100644 index 0000000..e0a36cb --- /dev/null +++ b/lib/media_tknres.c @@ -0,0 +1,237 @@ +/* + * media_tknres.c - managed media token resource + * + * Copyright (c) 2014 Shuah Khan <shuahkh@osg.samsung.com> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * + * This file is released under the GPLv2. + */ +/* + * Media devices often have hardware resources that are shared + * across several functions. For instance, TV tuner cards often + * have MUXes, converters, radios, tuners, etc. that are shared + * across various functions. However, v4l2, alsa, DVB, usbfs, and + * all other drivers have no knowledge of what resources are + * shared. For example, users can't access DVB and alsa at the same + * time, or the DVB and V4L analog API at the same time, since many + * only have one converter that can be in either analog or digital + * mode. Accessing and/or changing mode of a converter while it is + * in use by another function results in video stream error. + * + * A shared media tokens resource is created using devres framework + * for drivers to find and lock/unlock. Creating a shared devres + * helps avoid creating data structure dependencies between drivers. + * This media token resource contains media token for tuner, and + * audio. When tuner token is requested, audio token is issued. + * Subsequent token (for tuner and audio) gets from the same task + * and task in the same tgid succeed. This allows applications that + * make multiple v4l2 ioctls to work with the first call acquiring + * the token and applications that create separate threads to handle + * video and audio functions. + * + * API + * int media_tknres_create(struct device *dev); + * int media_tknres_destroy(struct device *dev); + * + * int media_get_tuner_tkn(struct device *dev); + * int media_put_tuner_tkn(struct device *dev); + * + * int media_get_audio_tkn(struct device *dev); + * int media_put_audio_tkn(struct device *dev); +*/ + +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/sched.h> +#include <linux/media_tknres.h> + +struct media_tkn { + spinlock_t lock; + unsigned int owner; /* owner task pid */ + unsigned int tgid; /* owner task gid */ + struct task_struct *task; +}; + +struct media_tknres { + struct media_tkn tuner; + struct media_tkn audio; +}; + +#define TUNER "Tuner" +#define AUDIO "Audio" + +static void media_tknres_release(struct device *dev, void *res) +{ + dev_info(dev, "%s: Media Token Resource released\n", __func__); +} + +int media_tknres_create(struct device *dev) +{ + struct media_tknres *tkn; + + tkn = devres_alloc(media_tknres_release, sizeof(struct media_tknres), + GFP_KERNEL); + if (!tkn) + return -ENOMEM; + + spin_lock_init(&tkn->tuner.lock); + tkn->tuner.owner = 0; + tkn->tuner.tgid = 0; + tkn->tuner.task = NULL; + + spin_lock_init(&tkn->audio.lock); + tkn->audio.owner = 0; + tkn->audio.tgid = 0; + tkn->audio.task = NULL; + + devres_add(dev, tkn); + + dev_info(dev, "%s: Media Token Resource created\n", __func__); + return 0; +} +EXPORT_SYMBOL_GPL(media_tknres_create); + +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str) +{ + int rc = 0; + unsigned tpid; + unsigned tgid; + + spin_lock(&tkn->lock); + + tpid = task_pid_nr(current); + tgid = task_tgid_nr(current); + + /* allow task in the same group id to release */ + if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) { + rc = -EBUSY; + } else { + tkn->owner = tpid; + tkn->tgid = tgid; + tkn->task = current; + } + pr_debug("%s: Media %s Token get: owner (%d,%d) req (%d,%d) rc %d\n", + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); + + spin_unlock(&tkn->lock); + return rc; +} + +static int __media_put_tkn(struct media_tkn *tkn, char *tkn_str) +{ + int rc = 0; + unsigned tpid; + unsigned tgid; + + spin_lock(&tkn->lock); + + tpid = task_pid_nr(current); + tgid = task_tgid_nr(current); + + /* allow task in the same group id to release */ + if (tkn->task == NULL || + ((tkn->task != current) && (tkn->tgid != tgid))) { + rc = -EINVAL; + } else { + tkn->owner = 0; + tkn->tgid = 0; + tkn->task = NULL; + } + pr_debug("%s: Media %s Token put: owner (%d,%d) req (%d,%d) rc %d\n", + __func__, tkn_str, tkn->owner, tkn->tgid, tpid, tgid, rc); + + spin_unlock(&tkn->lock); + return rc; +} + +/* + * When media tknres doesn't exist, get and put interfaces + * return 0 to let the callers take legacy code paths. This + * will also cover the drivers that don't create media tknres. + * Returning -ENODEV will require additional checks by callers. + * Instead handle the media tknres not present case as a driver + * not supporting media tknres and return 0. +*/ +int media_get_tuner_tkn(struct device *dev) +{ + struct media_tknres *tkn_ptr; + int ret = 0; + + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); + if (tkn_ptr == NULL) { + dev_dbg(dev, "%s: Media Token Resource not found\n", + __func__); + return 0; + } + + ret = __media_get_tkn(&tkn_ptr->tuner, TUNER); + if (ret) + return ret; + + /* get audio token */ + ret = __media_get_tkn(&tkn_ptr->audio, AUDIO); + if (ret) + __media_put_tkn(&tkn_ptr->tuner, TUNER); + + return ret; +} +EXPORT_SYMBOL_GPL(media_get_tuner_tkn); + +int media_put_tuner_tkn(struct device *dev) +{ + struct media_tknres *tkn_ptr; + + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); + if (tkn_ptr == NULL) { + dev_dbg(dev, "%s: Media Token Resource not found\n", + __func__); + return 0; + } + + /* put audio token and then tuner token */ + __media_put_tkn(&tkn_ptr->audio, AUDIO); + + return __media_put_tkn(&tkn_ptr->tuner, TUNER); +} +EXPORT_SYMBOL_GPL(media_put_tuner_tkn); + +int media_get_audio_tkn(struct device *dev) +{ + struct media_tknres *tkn_ptr; + + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); + if (tkn_ptr == NULL) { + dev_dbg(dev, "%s: Media Token Resource not found\n", + __func__); + return 0; + } + + return __media_get_tkn(&tkn_ptr->audio, AUDIO); +} +EXPORT_SYMBOL_GPL(media_get_audio_tkn); + +int media_put_audio_tkn(struct device *dev) +{ + struct media_tknres *tkn_ptr; + + tkn_ptr = devres_find(dev, media_tknres_release, NULL, NULL); + if (tkn_ptr == NULL) { + dev_dbg(dev, "%s: Media Token Resource not found\n", + __func__); + return 0; + } + + return __media_put_tkn(&tkn_ptr->audio, AUDIO); +} +EXPORT_SYMBOL_GPL(media_put_audio_tkn); + +int media_tknres_destroy(struct device *dev) +{ + int rc; + + rc = devres_release(dev, media_tknres_release, NULL, NULL); + WARN_ON(rc); + + return 0; +} +EXPORT_SYMBOL_GPL(media_tknres_destroy);
Add media token device resource framework to allow sharing resources such as tuner, dma, audio etc. across media drivers and non-media sound drivers that control media hardware. The Media token resource is created at the main struct device that is common to all drivers that claim various pieces of the main media device, which allows them to find the resource using the main struct device. As an example, digital, analog, and snd-usb-audio drivers can use the media token resource API using the main struct device for the interface the media device is attached to. A shared media tokens resource is created using devres framework for drivers to find and lock/unlock. Creating a shared devres helps avoid creating data structure dependencies between drivers. This media token resource contains media token for tuner, and audio. When tuner token is requested, audio token is issued. Subsequent token (for tuner and audio) gets from the same task and task in the same tgid succeed. This allows applications that make multiple v4l2 ioctls to work with the first call acquiring the token and applications that create separate threads to handle video and audio functions. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- MAINTAINERS | 2 + include/linux/media_tknres.h | 50 +++++++++ lib/Makefile | 2 + lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 291 insertions(+) create mode 100644 include/linux/media_tknres.h create mode 100644 lib/media_tknres.c