Re: [PATCH v14 32/53] ALSA: usb-audio: Check for support for requested audio format

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Takashi,

On 2/17/2024 2:08 AM, Takashi Iwai wrote:
> On Sat, 17 Feb 2024 00:42:18 +0100,
> Wesley Cheng wrote:
>>
>> Hi Takashi,
>>
>> On 2/9/2024 1:34 PM, Wesley Cheng wrote:
>>> Hi Takashi,
>>>
>>> On 2/9/2024 2:42 AM, Takashi Iwai wrote:
>>>> On Fri, 09 Feb 2024 00:13:45 +0100,
>>>> Wesley Cheng wrote:
>>>>>
>>>>> Allow for checks on a specific USB audio device to see if a
>>>>> requested PCM
>>>>> format is supported.  This is needed for support when playback is
>>>>> initiated by the ASoC USB backend path.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> ---
>>>>>    sound/usb/card.c | 31 +++++++++++++++++++++++++++++++
>>>>>    sound/usb/card.h | 11 +++++++++++
>>>>>    2 files changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>>>>> index 7dc8007ba839..1ad99a462038 100644
>>>>> --- a/sound/usb/card.c
>>>>> +++ b/sound/usb/card.c
>>>>> @@ -155,6 +155,37 @@ int snd_usb_unregister_platform_ops(void)
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
>>>>> +/*
>>>>> + * Checks to see if requested audio profile, i.e sample rate, # of
>>>>> + * channels, etc... is supported by the substream associated to the
>>>>> + * USB audio device.
>>>>> + */
>>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction)
>>>>> +{
>>>>> +    struct snd_usb_audio *chip;
>>>>> +    struct snd_usb_substream *subs;
>>>>> +    struct snd_usb_stream *as;
>>>>> +
>>>>> +    /*
>>>>> +     * Register mutex is held when populating and clearing usb_chip
>>>>> +     * array.
>>>>> +     */
>>>>> +    guard(mutex)(&register_mutex);
>>>>> +    chip = usb_chip[card_idx];
>>>>> +
>>>>> +    if (chip && enable[card_idx]) {
>>>>> +        list_for_each_entry(as, &chip->pcm_list, list) {
>>>>> +            subs = &as->substream[direction];
>>>>> +            if (snd_usb_find_substream_format(subs, params))
>>>>> +                return as;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return NULL;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
>>>>> +
>>>>>    /*
>>>>>     * disconnect streams
>>>>>     * called from usb_audio_disconnect()
>>>>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>>>>> index 02e4ea898db5..ed4a664e24e5 100644
>>>>> --- a/sound/usb/card.h
>>>>> +++ b/sound/usb/card.h
>>>>> @@ -217,4 +217,15 @@ struct snd_usb_platform_ops {
>>>>>    int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops);
>>>>>    int snd_usb_unregister_platform_ops(void);
>>>>> +
>>>>> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO)
>>>>> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction);
>>>>> +#else
>>>>> +static struct snd_usb_stream
>>>>> *snd_usb_find_suppported_substream(int card_idx,
>>>>> +            struct snd_pcm_hw_params *params, int direction)
>>>>> +{
>>>>> +    return NULL;
>>>>> +}
>>>>> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */
>>>>
>>>> The usefulness of ifdef guard here is doubtful, IMO.  This header is
>>>> only for USB-audio driver enablement, and not seen as generic
>>>> helpers.  So, just add the new function declarations without dummy
>>>> definitions.
>>>>
>>>
>>> Got it, will remove it.  We also have a dependency in place for the
>>> qc_audio_offload driver and SND USB AUDIO in the Kconfig.
>>>
>>
>> Looking at this again after trying some mixed Kconfig settings.  These
>> declarations aren't specific for USB-audio.  They are helpers that are
>> exposed to soc usb, so that it can do some basic verification with soc
>> usb before allowing the enable stream to continue.
> 
> Then rather the question is why snd-soc-usb calls those functions
> *unconditionally*.  No matter whether we have dependencies in Kconfig,
> calling the function means that the callee shall be drug when the
> corresponding code is running.
> 
> If it were generic core API stuff such as power-management or ACPI,
> it'd make sense to define dummy functions without the enablement, as
> many code may have optional calls.  If the API is enabled, it's anyway
> in the core.  If not, it's optional.  That'll be fine.
> 
> OTOH, the stuff you're calling certainly belongs to snd-usb-audio.
> Even if the call is really optional, it means that you'll have a hard
> dependency when snd-usb-audio is built, no matter whether you need or
> not.
> 
>> Since the ASoC
>> layer doesn't have insight on what audio profiles are supported by the
>> usb device, this API will ensure that the request profile is
>> supported.
>>
>> Issues are seen when we disable SND USB audio config and enable the
>> ASoC parts.
> 
> If snd-usb-audio is disabled, what snd-soc-usb would serve at all?
> Does it still make sense to keep it enabled?
> That said, the statement above (building snd-soc-usb without
> snd-usb-audio) looks already dubious; isn't it better to have a proper
> dependency in Kconfig, instead?
> 

Ok, took a look at it a bit more and should have gotten all the 
dependencies addressed through Kconfigs.  Thanks for the review comments 
and feedback.

Thanks
Wesley Cheng