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

Shuah Khan
2016-02-29T06:01:34Z

On 02/27/2016 12:48 AM, Takashi Iwai wrote:
> 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.
>
>

Hi Takashi,

I sent patch v4 that works for all combinations
of MEDIA_SUPPORT and SND_USB_AUDIO.

Please review and Ack if you are okay with this
version. Thanks for your help with the Kconfig
file changes.

-- Shuah

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