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

[Takashi Iwai]

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.


> +	}
> +
>  	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.)


> --- /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.

> +		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().

> +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.

> 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.

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.


> @@ -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;


thanks,

Takashi