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

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

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