Re: [PATCH v3 22/22] sound/usb: Use Media Controller API to share media resources

Takashi Iwai
2016-02-27T07:48:20Z

On Sat, 27 Feb 2016 03:55:39 +0100,
Shuah Khan wrote:
>
> On 02/26/2016 01:50 PM, Takashi Iwai wrote:
> > On Fri, 26 Feb 2016 21:08:43 +0100,
> > Shuah Khan wrote:
> >>
> >> On 02/26/2016 12:55 PM, Takashi Iwai wrote:
> >>> On Fri, 12 Feb 2016 00:41:38 +0100,
> >>> Shuah Khan wrote:
> >>>>
> >>>> Change ALSA driver to use Media Controller API to
> >>>> share media resources with DVB and V4L2 drivers
> >>>> on a AU0828 media device. Media Controller specific
> >>>> initialization is done after sound card is registered.
> >>>> ALSA creates Media interface and entity function graph
> >>>> nodes for Control, Mixer, PCM Playback, and PCM Capture
> >>>> devices.
> >>>>
> >>>> snd_usb_hw_params() will call Media Controller enable
> >>>> source handler interface to request the media resource.
> >>>> If resource request is granted, it will release it from
> >>>> snd_usb_hw_free(). If resource is busy, -EBUSY is returned.
> >>>>
> >>>> Media specific cleanup is done in usb_audio_disconnect().
> >>>>
> >>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >>>> ---
> >>>> sound/usb/Kconfig | 4 +
> >>>> sound/usb/Makefile | 2 +
> >>>> sound/usb/card.c | 14 +++
> >>>> sound/usb/card.h | 3 +
> >>>> sound/usb/media.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>> sound/usb/media.h | 72 +++++++++++
> >>>> sound/usb/mixer.h | 3 +
> >>>> sound/usb/pcm.c | 28 ++++-
> >>>> sound/usb/quirks-table.h | 1 +
> >>>> sound/usb/stream.c | 2 +
> >>>> sound/usb/usbaudio.h | 6 +
> >>>> 11 files changed, 448 insertions(+), 5 deletions(-)
> >>>> create mode 100644 sound/usb/media.c
> >>>> create mode 100644 sound/usb/media.h
> >>>>
> >>>> diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
> >>>> index a452ad7..ba117f5 100644
> >>>> --- a/sound/usb/Kconfig
> >>>> +++ b/sound/usb/Kconfig
> >>>> @@ -15,6 +15,7 @@ config SND_USB_AUDIO
> >>>> select SND_RAWMIDI
> >>>> select SND_PCM
> >>>> select BITREVERSE
> >>>> + select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && MEDIA_SUPPORT
> >>>
> >>> Looking at the media Kconfig again, this would be broken if
> >>> MEDIA_SUPPORT=m and SND_USB_AUDIO=y. The ugly workaround is something
> >>> like:
> >>> select SND_USB_AUDIO_USE_MEDIA_CONTROLLER \
> >>> if MEDIA_CONTROLLER && (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND)
> >>
> >> My current config is MEDIA_SUPPORT=m and SND_USB_AUDIO=y
> >> It is working and I didn't see any issues so far.
> >
> > Hmm, how does it be? In drivers/media/Makefile:
> >
> > ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> > obj-$(CONFIG_MEDIA_SUPPORT) += media.o
> > endif
> >
> > So it's a module. Meanwhile you have reference from usb-audio driver
> > that is built-in kernel. How is the symbol resolved?
>
> Sorry my mistake. I misspoke. My config had:
> CONFIG_MEDIA_SUPPORT=m
> CONFIG_MEDIA_CONTROLLER=y
> CONFIG_SND_USB_AUDIO=m
>
> The following doesn't work as you pointed out.
>
> CONFIG_MEDIA_SUPPORT=m
> CONFIG_MEDIA_CONTROLLER=y
> CONFIG_SND_USB_AUDIO=y
>
> okay here is what will work for all of the possible
> combinations of CONFIG_MEDIA_SUPPORT and CONFIG_SND_USB_AUDIO
>
> select SND_USB_AUDIO_USE_MEDIA_CONTROLLER \
> if MEDIA_CONTROLLER && ((MEDIA_SUPPORT=y) || (MEDIA_SUPPORT=m && SND_USB_AUDIO=m))
>
> The above will cover the cases when
>
> 1. CONFIG_MEDIA_SUPPORT and CONFIG_SND_USB_AUDIO are
> both modules
> CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER is selected
>
> 2. CONFIG_MEDIA_SUPPORT=y and CONFIG_SND_USB_AUDIO=m
> CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER is selected
>
> 3. CONFIG_MEDIA_SUPPORT=y and CONFIG_SND_USB_AUDIO=y
> CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER is selected
>
> 4. CONFIG_MEDIA_SUPPORT=m and CONFIG_SND_USB_AUDIO=y
> This is when we don't want
> CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER selected
>
> I verified all of the above combinations to make sure
> the logic works.
>
> If you think of a better way to do this please let me
> know. I will go ahead and send patch v4 with the above
> change and you can decide if that is acceptable.

I'm not 100% sure whether CONFIG_SND_USB_AUDIO=m can be put there as
conditional inside CONFIG_SND_USB_AUDIO definition. Maybe a safer
form would be like:

config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
bool
default y
depends on SND_USB_AUDIO
depends on MEDIA_CONTROLLER
depends on (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)

and drop select from SND_USB_AUDIO.

> >>> Other than that, it looks more or less OK to me.
> >>> The way how media_stream_init() gets called is a bit worrisome, but it
> >>> should work practically. Another concern is about the disconnection.
> >>> Can all function calls in media_device_delete() be safe even if it's
> >>> called while the application still opens the MC device?
> >>
> >> Right. I have been looking into device removal path when
> >> ioctls are active and I can resolve any issues that might
> >> surface while an audio app is active when device is removed.
> >
> > So, it's 100% safe to call all these media_*() functions while the
> > device is being accessed before closing?
> >
>
> There is a known problem with device removal when
> media device file is open and ioctl is in progress.
> This isn't specific to this patch series, a general
> problem that is related to media device removal in
> general.
>
> I am working on a fix for this problem. As you said,
> earlier, I can work on fixing issues after the merge.

OK, understood.

Takashi