Hi Takashi,
On 12/05/2016 11:50 PM, Takashi Iwai wrote:
> On Wed, 30 Nov 2016 23:01:16 +0100,
> Shuah Khan wrote:
>>
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
> (snip)
>> @@ -616,6 +617,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>> if (err < 0)
>> goto __error;
>>
>> + if (quirk && quirk->media_device) {
>> + /* don't want to fail when media_snd_device_create() fails */
>> + media_snd_device_create(chip, intf);
>
> Note that the usb-audio driver is probed for each usb interface, and
> there are multiple interfaces per device. That is, usb_audio_probe()
> may be called multiple times per device, and at the second or the
> later calls, it appends the stuff onto the previously created card
> object.
>
> So, you'd have to make this call also conditional (e.g. check
> chip->num_interfaces == 0, indicating the very first one), or allow to
> be called multiple times.
>
> In the former case, it'd be better to split media_snd_device_create()
> and media_snd_mixer_init(). The *_device_create() needs to be called
> only once, while *_mixer_init() still has to be called for each time
> because the new mixer element may be added for each interface.
>
Thanks for the catch. I am able to fix this in media_snd_device_create()
I made a change to do media_dev allocate only once. media_snd_mixer_init()
will get called every time media_snd_device_create() is called. This way
new mixers can be handled. media_snd_mixer_init() has logic to deal with
mixers that are already initialized. We are good here with the following
change:
@@ -272,6 +258,14 @@ int media_snd_device_create(struct snd_usb_audio *chip,
struct usb_device *usbdev = interface_to_usbdev(iface);
int ret;
+ /* usb-audio driver is probed for each usb interface, and
+ * there are multiple interfaces per device. Avoid calling
+ * media_device_usb_allocate() each time usb_audio_probe()
+ * is called. Do it only once.
+ */
+ if (chip->media_dev)
+ goto snd_mixer_init;
+
mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME);
if (!mdev)
return -ENOMEM;
@@ -291,6 +285,7 @@ int media_snd_device_create(struct snd_usb_audio *chip,
/* save media device - avoid lookups */
chip->media_dev = mdev;
+snd_mixer_init:
/* Create media entities for mixer and control dev */
ret = media_snd_mixer_init(chip);
if (ret) {
>
>> + }
>> +
>> usb_chip[chip->index] = chip;
>> chip->num_interfaces++;
>> usb_set_intfdata(intf, chip);
>> @@ -672,6 +678,14 @@ static void usb_audio_disconnect(struct usb_interface
>> *intf)
>> list_for_each(p, &chip->midi_list) {
>> snd_usbmidi_disconnect(p);
>> }
>> + /*
>> + * Nice to check quirk && quirk->media_device and
>> + * then call media_snd_device_delete(). Don't have
>> + * access to quirk here. media_snd_device_delete()
>> + * acceses mixer_list
>> + */
>> + media_snd_device_delete(chip);
>
> ... meanwhile this is OK, as it's called only once.
>
> (BTW, is it OK to call media_* stuff while the device is still in use?
> The disconnect callback gets called for hot-unplug.)
>
Yes. All of the media_* functions that get called during run-time check for
chip->media_dev or media_ctl and do nothing when these are null.
media_device itself will not be free'd until all reference are gone. When
usb_audio_disconnect() happens via unbind snd_usb_audio or physical remove,
media_dev sticks around until au0828 (media driver) goes away. There is
handling for any user apps. that have /dev/mediaX open.
Does this sound correct? Did I miss any of your concerns?
>
>> --- /dev/null
>> +++ b/sound/usb/media.c
> (snip)
>> +void media_snd_stream_delete(struct snd_usb_substream *subs)
>> +{
>> + struct media_ctl *mctl = subs->media_ctl;
>> +
>> + if (mctl && mctl->media_dev) {
>
> mctl->media_dev NULL check here is superfluous, as it's checked
> mctl->below.
>
Done.
>> + struct media_device *mdev;
>> +
>> + mdev = mctl->media_dev;
>> + if (mdev && media_devnode_is_registered(mdev->devnode)) {
>> + media_devnode_remove(mctl->intf_devnode);
>> + media_device_unregister_entity(&mctl->media_entity);
>> + media_entity_cleanup(&mctl->media_entity);
>> + }
>> + kfree(mctl);
>> + subs->media_ctl = NULL;
>> + }
>> +}
>> +
>> +int media_snd_start_pipeline(struct snd_usb_substream *subs)
>> +{
>> + struct media_ctl *mctl = subs->media_ctl;
>> +
>> + if (mctl)
>> + return media_snd_enable_source(mctl);
>> + return 0;
>> +}
>
> It's merely a wrapper, and media_snd_enable_source() itself checks
> NULL mctl. So we can replace media_snd_enable_source() with
> media_snd_start_pipeline().
Done.
>
>> +void media_snd_stop_pipeline(struct snd_usb_substream *subs)
>> +{
>> + struct media_ctl *mctl = subs->media_ctl;
>> +
>> + if (mctl)
>> + media_snd_disable_source(mctl);
>> +}
>
> Ditto.
Done.
>
>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>> index 44d178e..0e4e0640 100644
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
> (snip)
>> @@ -717,10 +718,14 @@ static int snd_usb_hw_params(struct snd_pcm_substream
>> *substream,
>> struct audioformat *fmt;
>> int ret;
>>
>> + ret = media_snd_start_pipeline(subs);
>> + if (ret)
>> + return ret;
>
> It's an open question at which timing we should call
> media_snd_start_pipeline(). The hw_params is mostly OK, while the
> real timing where the stream might be started is the prepare
> callback. I guess we can keep as is for now.
Okay.
>
> Also, one more thing to be considered is whether
> media_snd_start_pipeline() can be called multiple times. hw_params
> and prepare callbacks may be called multiple times. I suppose it's
> OK, but just to be sure.
Yes. media_snd_start_pipeline() can be called multiple times. v4l2 apps.
also call start_pipeline multiple times. So we are good here.
>
>
>> @@ -1234,7 +1246,12 @@ static int snd_usb_pcm_open(struct snd_pcm_substream
>> *substream, int direction)
>> subs->dsd_dop.channel = 0;
>> subs->dsd_dop.marker = 1;
>>
>> - return setup_hw_info(runtime, subs);
>> + ret = setup_hw_info(runtime, subs);
>> + if (ret == 0)
>> + ret = media_snd_stream_init(subs, as->pcm, direction);
>> + if (ret)
>> + snd_usb_autosuspend(subs->stream->chip);
>> + return ret;
>
> This leads to the PM refcount unbalance. The call of
> snd_usb_autosuspend() must be in the former if block,
>
> ret = setup_hw_info(runtime, subs);
> if (ret == 0) {
> ret = media_snd_stream_init(subs, as->pcm, direction);
> if (ret)
> snd_usb_autosuspend(subs->stream->chip);
> }
> return ret;
Done.
>
>
> thanks,
>
> Takashi
>
Mauro,
I will send patch v7 and include the others in the series that don't need
changes to keep them grouped.
thanks,
-- Shuah
--
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
