[PATCH 0/4] ALSA: hda: Expose codec organization functions
Changes expose several function that are currently unavailable for HDA-DSP drivers for use. Those functions are:
snd_hda_codec_cleanup_for_unbind() snd_hda_codec_set_power_save() snd_hda_codec_register() snd_hda_codec_unregister() snd_hda_codec_device_init()
This allows upcoming AVS driver [1] to re-use even mode of HDA related code that is currently available in sound/pci/hda and sound/hda and prevent any code duplication within avs-driver that would otherwise had to happen.
Last patch in the series provides snd_hdac_ext_bus_link_at() - a helper function which allows for retrieval of HDA segment (link) based on codec address directly. This is simpler than parsing codec-name first to extract the address what is the case for snd_hdac_ext_bus_get_link(). The latter function is updated to re-use newly added one so core logic is not duplicated after the addition.
[1]: https://lore.kernel.org/all/20211208111301.1817725-1-cezary.rojewski@intel.c...
Cezary Rojewski (4): ALSA: hda: Add snd_hdac_ext_bus_link_at() helper ALSA: hda: Update and expose snd_hda_codec_device_init() ALSA: hda: Update and expose codec register procedures ALSA: hda: Expose codec cleanup and power-save functions
include/sound/hda_codec.h | 11 +++- include/sound/hdaudio_ext.h | 1 + sound/hda/ext/hdac_ext_controller.c | 31 +++++++--- sound/pci/hda/hda_codec.c | 94 ++++++++++++++++++++--------- sound/pci/hda/hda_local.h | 2 - sound/soc/codecs/hdac_hda.c | 2 +- 6 files changed, 100 insertions(+), 41 deletions(-)
This patch exposes a new helper to directly retrieve the link from the codec address, and makes use of this helper when retrieving the link from the codec name.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hdaudio_ext.h | 1 + sound/hda/ext/hdac_ext_controller.c | 31 +++++++++++++++++++---------- 2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 77123c3e4095..b0c8e4936168 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -28,6 +28,7 @@ void snd_hdac_ext_stream_spbcap_enable(struct hdac_bus *chip, bool enable, int index);
int snd_hdac_ext_bus_get_ml_capabilities(struct hdac_bus *bus); +struct hdac_ext_link *snd_hdac_ext_bus_link_at(struct hdac_bus *bus, int addr); struct hdac_ext_link *snd_hdac_ext_bus_get_link(struct hdac_bus *bus, const char *codec_name);
diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c index b2df7b4f9227..b072392725c7 100644 --- a/sound/hda/ext/hdac_ext_controller.c +++ b/sound/hda/ext/hdac_ext_controller.c @@ -132,6 +132,26 @@ void snd_hdac_link_free_all(struct hdac_bus *bus) } EXPORT_SYMBOL_GPL(snd_hdac_link_free_all);
+/** + * snd_hdac_ext_bus_link_at - get link at specified address + * @bus: link's parent bus device + * @addr: codec device address + * + * Returns link object or NULL if matching link is not found. + */ +struct hdac_ext_link *snd_hdac_ext_bus_link_at(struct hdac_bus *bus, int addr) +{ + struct hdac_ext_link *hlink; + int i; + + list_for_each_entry(hlink, &bus->hlink_list, list) + for (i = 0; i < HDA_MAX_CODECS; i++) + if (hlink->lsdiid & (0x1 << addr)) + return hlink; + return NULL; +} +EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_link_at); + /** * snd_hdac_ext_bus_get_link - get link based on codec name * @bus: the pointer to HDAC bus object @@ -140,8 +160,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_link_free_all); struct hdac_ext_link *snd_hdac_ext_bus_get_link(struct hdac_bus *bus, const char *codec_name) { - int i; - struct hdac_ext_link *hlink = NULL; int bus_idx, addr;
if (sscanf(codec_name, "ehdaudio%dD%d", &bus_idx, &addr) != 2) @@ -151,14 +169,7 @@ struct hdac_ext_link *snd_hdac_ext_bus_get_link(struct hdac_bus *bus, if (addr < 0 || addr > 31) return NULL;
- list_for_each_entry(hlink, &bus->hlink_list, list) { - for (i = 0; i < HDA_MAX_CODECS; i++) { - if (hlink->lsdiid & (0x1 << addr)) - return hlink; - } - } - - return NULL; + return snd_hdac_ext_bus_link_at(bus, addr); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_get_link);
With few changes, snd_hda_codec_device_init() can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed function.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hda_codec.h | 3 +++ sound/pci/hda/hda_codec.c | 45 +++++++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 82d9daa17851..5e3cbcca42f0 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -306,6 +306,9 @@ struct hda_codec { /* * constructors */ +__printf(3, 4) struct hda_codec * +snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, + const char *fmt, ...); int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp); int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 7016b48227bf..3787060ad77f 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -877,36 +877,48 @@ static void snd_hda_codec_dev_release(struct device *dev)
#define DEV_NAME_LEN 31
-static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card, - unsigned int codec_addr, struct hda_codec **codecp) +/** + * snd_hda_codec_device_init - allocate HDA codec device + * @bus: codec's parent bus + * @codec_addr: the codec address on the parent bus + * @fmt: format string for the device's name + * + * Returns newly allocated codec device or ERR_PTR() on failure. + */ +struct hda_codec * +snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, + const char *fmt, ...) { + va_list vargs; char name[DEV_NAME_LEN]; struct hda_codec *codec; int err;
- dev_dbg(card->dev, "%s: entry\n", __func__); - if (snd_BUG_ON(!bus)) - return -EINVAL; + return ERR_PTR(-EINVAL); if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) - return -EINVAL; + return ERR_PTR(-EINVAL);
codec = kzalloc(sizeof(*codec), GFP_KERNEL); if (!codec) - return -ENOMEM; + return ERR_PTR(-ENOMEM); + + va_start(vargs, fmt); + vsprintf(name, fmt, vargs); + va_end(vargs);
- sprintf(name, "hdaudioC%dD%d", card->number, codec_addr); err = snd_hdac_device_init(&codec->core, &bus->core, name, codec_addr); if (err < 0) { kfree(codec); - return err; + return ERR_PTR(err); }
+ codec->bus = bus; codec->core.type = HDA_DEV_LEGACY; - *codecp = codec;
- return err; + return codec; } +EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);
/** * snd_hda_codec_new - create a HDA codec @@ -920,11 +932,13 @@ static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card, int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp) { - int ret; + struct hda_codec *codec;
- ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp); - if (ret < 0) - return ret; + codec = snd_hda_codec_device_init(bus, codec_addr, "hdaudioC%dD%d", + card->number, codec_addr); + if (IS_ERR(codec)) + return PTR_ERR(codec); + *codecp = codec;
return snd_hda_codec_device_new(bus, card, codec_addr, *codecp); } @@ -951,7 +965,6 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, codec->core.dev.release = snd_hda_codec_dev_release; codec->core.exec_verb = codec_exec_verb;
- codec->bus = bus; codec->card = card; codec->addr = codec_addr; mutex_init(&codec->spdif_mutex);
With few changes, snd_hda_codec_register() and its unregister-counterpart can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed functions.
Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hda_codec.h | 5 ++++- sound/pci/hda/hda_codec.c | 35 ++++++++++++++++++++++++++--------- sound/pci/hda/hda_local.h | 1 - sound/soc/codecs/hdac_hda.c | 2 +- 4 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 5e3cbcca42f0..f74abc13414f 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -312,9 +312,12 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp); int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, - unsigned int codec_addr, struct hda_codec *codec); + unsigned int codec_addr, struct hda_codec *codec, + bool snddev_managed); int snd_hda_codec_configure(struct hda_codec *codec); int snd_hda_codec_update_widgets(struct hda_codec *codec); +void snd_hda_codec_register(struct hda_codec *codec); +void snd_hda_codec_unregister(struct hda_codec *codec);
/* * low level functions diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 3787060ad77f..4406b7983c07 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -813,7 +813,12 @@ void snd_hda_codec_display_power(struct hda_codec *codec, bool enable) snd_hdac_display_power(&codec->bus->core, codec->addr, enable); }
-/* also called from hda_bind.c */ +/** + * snd_hda_codec_register - Finalize codec initialization + * @codec: codec device to register + * + * Also called from hda_bind.c + */ void snd_hda_codec_register(struct hda_codec *codec) { if (codec->registered) @@ -826,6 +831,7 @@ void snd_hda_codec_register(struct hda_codec *codec) codec->registered = 1; } } +EXPORT_SYMBOL_GPL(snd_hda_codec_register);
static int snd_hda_codec_dev_register(struct snd_device *device) { @@ -833,10 +839,12 @@ static int snd_hda_codec_dev_register(struct snd_device *device) return 0; }
-static int snd_hda_codec_dev_free(struct snd_device *device) +/** + * snd_hda_codec_unregister - Unregister specified codec device + * @codec: codec device to unregister + */ +void snd_hda_codec_unregister(struct hda_codec *codec) { - struct hda_codec *codec = device->device_data; - codec->in_freeing = 1; /* * snd_hda_codec_device_new() is used by legacy HDA and ASoC driver. @@ -853,7 +861,12 @@ static int snd_hda_codec_dev_free(struct snd_device *device) */ if (codec->core.type == HDA_DEV_LEGACY) put_device(hda_codec_dev(codec)); +} +EXPORT_SYMBOL_GPL(snd_hda_codec_unregister);
+static int snd_hda_codec_dev_free(struct snd_device *device) +{ + snd_hda_codec_unregister(device->device_data); return 0; }
@@ -940,12 +953,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, return PTR_ERR(codec); *codecp = codec;
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp); + return snd_hda_codec_device_new(bus, card, codec_addr, *codecp, false); } EXPORT_SYMBOL_GPL(snd_hda_codec_new);
int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, - unsigned int codec_addr, struct hda_codec *codec) + unsigned int codec_addr, struct hda_codec *codec, + bool snddev_managed) { char component[31]; hda_nid_t fg; @@ -1020,9 +1034,12 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, codec->core.subsystem_id, codec->core.revision_id); snd_component_add(card, component);
- err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops); - if (err < 0) - goto error; + if (snddev_managed) { + /* ASoC features component management instead */ + err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops); + if (err < 0) + goto error; + }
/* PM runtime needs to be enabled later after binding codec */ pm_runtime_forbid(&codec->core.dev); diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 8621f576446b..4c52dfb615bc 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -135,7 +135,6 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, #define snd_hda_add_vmaster(codec, name, tlv, followers, suffix, access) \ __snd_hda_add_vmaster(codec, name, tlv, followers, suffix, true, access, NULL) int snd_hda_codec_reset(struct hda_codec *codec); -void snd_hda_codec_register(struct hda_codec *codec); void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec); void snd_hda_codec_disconnect_pcms(struct hda_codec *codec);
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index de5955db0a5f..667f3df239c7 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -413,7 +413,7 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component) HDA_CODEC_IDX_CONTROLLER, true);
ret = snd_hda_codec_device_new(hcodec->bus, component->card->snd_card, - hdev->addr, hcodec); + hdev->addr, hcodec, true); if (ret < 0) { dev_err(&hdev->dev, "failed to create hda codec %d\n", ret); goto error_no_pm;
Hi,
On Mon, 7 Feb 2022, Cezary Rojewski wrote:
With few changes, snd_hda_codec_register() and its unregister-counterpart can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed functions.
thanks, there is no doubt room to improve the HDA<->asoc interaction still and increase reuse. Some questions:
Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process.
Can you maybe clarify this? The existing code to handle the different paths is already quite hairy (e.g. code in hda_codec.c:snd_hda_codec_dev_free() that does different actions depending on whether "codec->core.type == HDA_DEV_LEGACY) is true or false), so we'd need to have very clear semantics for the "snddev_managed".
@@ -940,12 +953,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, return PTR_ERR(codec); *codecp = codec;
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp, false);
So this sets snddev_managed to "false" for snd-hda-intel? How is this expected to work?
int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
unsigned int codec_addr, struct hda_codec *codec)
unsigned int codec_addr, struct hda_codec *codec,bool snddev_managed){ char component[31]; hda_nid_t fg; @@ -1020,9 +1034,12 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, codec->core.subsystem_id, codec->core.revision_id); snd_component_add(card, component);
[...]
- err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
- if (err < 0)
goto error;
- if (snddev_managed) {
/* ASoC features component management instead */err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);if (err < 0)goto error;- }
I might misunderstand semantics of snddev_managed here, but given in case of non-ASoC use, snddev_managed is false, this would mean we don't call snd_device_new() at all...? I don't see where this is added elsewhere in the series, so this would seem to break the flow for non-ASoC case.
Br, Kai
On 2022-02-08 4:54 PM, Kai Vehmanen wrote:
Hi,
On Mon, 7 Feb 2022, Cezary Rojewski wrote:
With few changes, snd_hda_codec_register() and its unregister-counterpart can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed functions.
thanks, there is no doubt room to improve the HDA<->asoc interaction still and increase reuse. Some questions:
Thanks for taking time in reviewing the patches, Kai!
Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process.
Can you maybe clarify this? The existing code to handle the different paths is already quite hairy (e.g. code in hda_codec.c:snd_hda_codec_dev_free() that does different actions depending on whether "codec->core.type == HDA_DEV_LEGACY) is true or false), so we'd need to have very clear semantics for the "snddev_managed".
It's rather straightforward - ASoC does not provide you with pointer to struct snd_card until all components are accounted for. snd_hda_codec_device_new() in its current form needs such information upfront though. As we want to create only as many DAIs (and other ASoC components like links and routes) as needed, codec's ->pcm_list_head needs to be built before codec's probing can be completed. But in order to have hda codec to fill ->pcm_list_head for, you need to create it. And in order to create it you need snd_card pointer which ASoC won't give you before you complete component probing.
Typical chicken and egg problem. And that's why additional option is provided - it solves that problem.
@@ -940,12 +953,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, return PTR_ERR(codec); *codecp = codec;
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp, false);
So this sets snddev_managed to "false" for snd-hda-intel? How is this expected to work?
Good catch! It is supposed to be 'true' by default. I checked previous 'releases' of this patch: between internal RFC v1 -> RFC v2 this mistake got in, and probably because I've rebased the driver during that time and run into several conflicts which I had to fix manually.
Will update in v2.
int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
unsigned int codec_addr, struct hda_codec *codec)
unsigned int codec_addr, struct hda_codec *codec,bool snddev_managed)@@ -1020,9 +1034,12 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, codec->core.subsystem_id, codec->core.revision_id); snd_component_add(card, component);
[...]
- err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
- if (err < 0)
goto error;
- if (snddev_managed) {
/* ASoC features component management instead */err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);if (err < 0)goto error;- }
I might misunderstand semantics of snddev_managed here, but given in case of non-ASoC use, snddev_managed is false, this would mean we don't call snd_device_new() at all...? I don't see where this is added elsewhere in the series, so this would seem to break the flow for non-ASoC case.
Same as above.
Regards, Czarek
On Tue, 08 Feb 2022 17:34:47 +0100, Cezary Rojewski wrote:
On 2022-02-08 4:54 PM, Kai Vehmanen wrote:
Hi,
On Mon, 7 Feb 2022, Cezary Rojewski wrote:
With few changes, snd_hda_codec_register() and its unregister-counterpart can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed functions.
thanks, there is no doubt room to improve the HDA<->asoc interaction still and increase reuse. Some questions:
Thanks for taking time in reviewing the patches, Kai!
Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process.
Can you maybe clarify this? The existing code to handle the different paths is already quite hairy (e.g. code in hda_codec.c:snd_hda_codec_dev_free() that does different actions depending on whether "codec->core.type == HDA_DEV_LEGACY) is true or false), so we'd need to have very clear semantics for the "snddev_managed".
It's rather straightforward - ASoC does not provide you with pointer to struct snd_card until all components are accounted for. snd_hda_codec_device_new() in its current form needs such information upfront though. As we want to create only as many DAIs (and other ASoC components like links and routes) as needed, codec's ->pcm_list_head needs to be built before codec's probing can be completed. But in order to have hda codec to fill ->pcm_list_head for, you need to create it. And in order to create it you need snd_card pointer which ASoC won't give you before you complete component probing.
Typical chicken and egg problem. And that's why additional option is provided - it solves that problem.
Hmm, but snd_hda_codec_device_new() also does the actual hardware initialization including the power-up sequence, and there is a call snd_component_add() with the card instance. How the function would work properly before the card instantiation? You seem to have handled only snd_device_new() and not touching other code where the card (or the hardware access) is involved.
If the purpose is to get the fields of snd_hda_codec to be initialized, those init code can be factored out to another function, so that it works certainly without card.
Takashi
On 2022-02-08 6:14 PM, Takashi Iwai wrote:
Hmm, but snd_hda_codec_device_new() also does the actual hardware initialization including the power-up sequence, and there is a call snd_component_add() with the card instance. How the function would work properly before the card instantiation? You seem to have handled only snd_device_new() and not touching other code where the card (or the hardware access) is involved.
If the purpose is to get the fields of snd_hda_codec to be initialized, those init code can be factored out to another function, so that it works certainly without card.
I was anticipating snd_hda_codec_device_init() related question so much and was so ready for it that I answered like an autobot, regardless of fact that people were not asking about snd_hda_codec_device_init() vs snd_hda_codec_device_new() directly and that's not even the patch where the change is added. And I wasn't even drinking coffee for the past few days..
Meh.. In regard to snddev_managed, If I'm not mistaken, the problem is connected to the 'dev_ops' (.dev_register, .dev_free) provided for snd_device_new(). To have basically 0% custom HDA logic in our code, rely solely on code found in sound/hda and sound/pci/hda ability to control when snd_hda_codec_register() and snd_hda_codec_unregister() is called was needed.
snd_soc_bind_card() invokes snd_card_register() which in consequence leads to snd_device_register_all() and that to automatic ->dev_register call. That call involves PM operations, and at that moment, codec is not ready for it. In the end, ability to call these in correct order allowed all codecs that communicate with avs-driver to remain on HDA_DEV_LEGACY level.
To answer the remaining question found in your message: The purpose of the change was not to get the fields of snd_hda_codec out of the function and have them initialized elsewhere. All other operations found in the snd_hda_codec_device_new() are required and their ordering is not problematic so they have been left untouched.
Regards, Czarek
On 2022-02-09 11:21 AM, Cezary Rojewski wrote:
snd_soc_bind_card() invokes snd_card_register() which in consequence leads to snd_device_register_all() and that to automatic ->dev_register call. That call involves PM operations, and at that moment, codec is not ready for it.
By that I mean: bus driver (here, avs-driver) has some saying on the PM matter too e.g.: sets their (codecs) status to suspended via pm_runtime_set_suspended() so the bus runtime suspend is not blocked by codecs that could possibly never complete their probing.
Regards, Czarek
On Wed, 09 Feb 2022 11:38:23 +0100, Cezary Rojewski wrote:
On 2022-02-09 11:21 AM, Cezary Rojewski wrote:
snd_soc_bind_card() invokes snd_card_register() which in consequence leads to snd_device_register_all() and that to automatic ->dev_register call. That call involves PM operations, and at that moment, codec is not ready for it.
By that I mean: bus driver (here, avs-driver) has some saying on the PM matter too e.g.: sets their (codecs) status to suspended via pm_runtime_set_suspended() so the bus runtime suspend is not blocked by codecs that could possibly never complete their probing.
Hm, but the runtime PM of hda_codec device is suppressed at first in snd_hda_codec_device_new() via pm_runtime_forbid(), i.e. the codec should be kept running until it reaches to snd_hda_codec_register(), it shouldn't be runtime-suspended beforehand.
I might overlook something, but it's a bit hard to follow...
thanks,
Takashi
Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process.
Can you maybe clarify this? The existing code to handle the different paths is already quite hairy (e.g. code in hda_codec.c:snd_hda_codec_dev_free() that does different actions depending on whether "codec->core.type == HDA_DEV_LEGACY) is true or false), so we'd need to have very clear semantics for the "snddev_managed".
It's rather straightforward - ASoC does not provide you with pointer to struct snd_card until all components are accounted for. snd_hda_codec_device_new() in its current form needs such information upfront though. As we want to create only as many DAIs (and other ASoC components like links and routes) as needed, codec's ->pcm_list_head needs to be built before codec's probing can be completed. But in order to have hda codec to fill ->pcm_list_head for, you need to create it. And in order to create it you need snd_card pointer which ASoC won't give you before you complete component probing.
Typical chicken and egg problem. And that's why additional option is provided - it solves that problem.
New capabilities are always welcome, what I am missing is how important your suggestion is for end users or OEMs.
The main reason for using a DSP-based driver on a HDaudio Intel platform is when DMICs connected to the PCH, since this link cannot be handled by the legacy driver. Those sort of form factors typically have analog playback and capture only. UCM exposes only analog playback and capture.
Desktops without DMICs generally rely on the legacy driver and have different sorts of problems with jack retasking and other time-consuming problems.
Exposing additional DAIs in a DSP-based driver when supported by the codec is a good idea on paper, but it's far from straightforward.
a) it assumes that there are indeed additional DAIs. Is this really the case on the SKL/KBL devices you are targeting? It's not on newer CML..ADL devices we've been supporting with SOF.
b) it assumes that what is exposed by the codec actually does work - and the number of quirks tells us to be cautious. We routinely get reports that even Intel NUCs don't have the right quirks in the kernel...
c) and then it creates new problems for the topology that may expose paths that may or may not be supported by the codec. I am not aware of any existing APIs to take down or enable a path conditionally - though it's been a problem for a very long time for SSP and DMIC enablement that are not always correctly described, and any suggestions to improve this limitation would have my full support.
FWIW, in our latest SOF work we went back to handling ONE DAI with analog playback and capture and ditched the 'digital playback'. Trying to do more led us to too many issues of 'works on platform A' and 'does not work on platform B', and sometimes with different answers depending on which BIOS version is used.
IMHO what's really problematic for HDaudio is the support for amplifiers located behind the HDaudio codec, for which we more often than not are missing the I2C configuration sequences. Suspend-resume is a recurring problem as well.
I am not saying no for the sake of saying no, I have just never heard of anyone complain about restrictions on the number of DAIs in the HDaudio world.
On 2022-02-08 6:46 PM, Pierre-Louis Bossart wrote:
New capabilities are always welcome, what I am missing is how important your suggestion is for end users or OEMs.
Users won't notice and why would we like them to notice? The intended behavior is 1:1 with hda legacy one.
The main reason for using a DSP-based driver on a HDaudio Intel platform is when DMICs connected to the PCH, since this link cannot be handled by the legacy driver. Those sort of form factors typically have analog playback and capture only. UCM exposes only analog playback and capture.
Desktops without DMICs generally rely on the legacy driver and have different sorts of problems with jack retasking and other time-consuming problems.
I believe we're all aware why and when DSP-drivers are chosen over HDA legacy driver. That's orthogonal to the current subject.
Exposing additional DAIs in a DSP-based driver when supported by the codec is a good idea on paper, but it's far from straightforward.
The outcome is reduction in number of DAIs exposed for basically all codecs except for HDMIs as DSP-based solutions are hardcoding 3-DAIs. Here, the intended behavior is to be 1:1 with hda legacy behavior, not hardcoding.
a) it assumes that there are indeed additional DAIs. Is this really the case on the SKL/KBL devices you are targeting? It's not on newer CML..ADL devices we've been supporting with SOF.
It does not _assume_, it _reads_. The outcome is reduction of DAIs exposed rather than hardcoding Analog/Alt Analog/Digital endpoints what is currently the case.
b) it assumes that what is exposed by the codec actually does work - and the number of quirks tells us to be cautious. We routinely get reports that even Intel NUCs don't have the right quirks in the kernel...
Patches just expose functions. Logic stays the same. As it is sound/pci/hda code we're talking about, that code is quirk-friendly - it contains several "patches" and quirks already.
c) and then it creates new problems for the topology that may expose paths that may or may not be supported by the codec. I am not aware of any existing APIs to take down or enable a path conditionally - though it's been a problem for a very long time for SSP and DMIC enablement that are not always correctly described, and any suggestions to improve this limitation would have my full support.
The default for topology is to expose just single analog endpoint as majority of codecs expose nothing else. Moreover, having just 1 FE defined in topology can be used to limit the usecases where we know codec says it exposes more but in fact these endpoints aren't functional. For specific codecs that expose more, a specific topology can be provided just like it's done for any DSP-driver today.
FWIW, in our latest SOF work we went back to handling ONE DAI with analog playback and capture and ditched the 'digital playback'. Trying to do more led us to too many issues of 'works on platform A' and 'does not work on platform B', and sometimes with different answers depending on which BIOS version is used.
IMHO what's really problematic for HDaudio is the support for amplifiers located behind the HDaudio codec, for which we more often than not are missing the I2C configuration sequences. Suspend-resume is a recurring problem as well.
I am not saying no for the sake of saying no, I have just never heard of anyone complain about restrictions on the number of DAIs in the HDaudio world.
I believe our goals align. Rather than hardcoding Analog/Alt Analog/Digital endpoints as it's done currently, when codec most of the time do not have them working anyway, rely on behavior found in sound/hda and sound/pci/hda. If there are some problems there it's win:win for us and legacy driver. Fix one spot, have both drivers happy.
Regards, Czarek
On 2/9/22 06:05, Cezary Rojewski wrote:
FWIW, in our latest SOF work we went back to handling ONE DAI with analog playback and capture and ditched the 'digital playback'. Trying to do more led us to too many issues of 'works on platform A' and 'does not work on platform B', and sometimes with different answers depending on which BIOS version is used.
IMHO what's really problematic for HDaudio is the support for amplifiers located behind the HDaudio codec, for which we more often than not are missing the I2C configuration sequences. Suspend-resume is a recurring problem as well.
I am not saying no for the sake of saying no, I have just never heard of anyone complain about restrictions on the number of DAIs in the HDaudio world.
I believe our goals align. Rather than hardcoding Analog/Alt Analog/Digital endpoints as it's done currently, when codec most of the time do not have them working anyway, rely on behavior found in sound/hda and sound/pci/hda. If there are some problems there it's win:win for us and legacy driver. Fix one spot, have both drivers happy.
I don't quite see the alignment: the only thing that we've seen work reliably and that we do need is the analog part, and we want to get rid of the other paths which we can't test in the first place. I must admit I don't recall why we bothered to expose those alt analog and digital paths back in 2018, they have not been used nor tested by anyone.
Now if this patch helped to make sure we do indeed have an analog path, that'd be fine. That would indeed avoid a hard-coded decision and report configuration errors.
I just don't see what exposing additional paths brings, they never worked reliably for us when we tried. IIRC half of our team gets an error with the Digital playback stream on Up Extreme and the other half can play just fine - albeit with no connector to actually see what the output is.
FWIW there are many things made possible by HDaudio, in practice we often have to limit ourselves to what is known to work and what is needed by end-products. You could spend all your time chasing configuration issues, missing NIDS and bad verbs. Been there, done that.
On 2022-02-09 5:08 PM, Pierre-Louis Bossart wrote:
On 2/9/22 06:05, Cezary Rojewski wrote:
I believe our goals align. Rather than hardcoding Analog/Alt Analog/Digital endpoints as it's done currently, when codec most of the time do not have them working anyway, rely on behavior found in sound/hda and sound/pci/hda. If there are some problems there it's win:win for us and legacy driver. Fix one spot, have both drivers happy.
I don't quite see the alignment: the only thing that we've seen work reliably and that we do need is the analog part, and we want to get rid of the other paths which we can't test in the first place. I must admit I don't recall why we bothered to expose those alt analog and digital paths back in 2018, they have not been used nor tested by anyone.
Now if this patch helped to make sure we do indeed have an analog path, that'd be fine. That would indeed avoid a hard-coded decision and report configuration errors.
I just don't see what exposing additional paths brings, they never worked reliably for us when we tried. IIRC half of our team gets an error with the Digital playback stream on Up Extreme and the other half can play just fine - albeit with no connector to actually see what the output is.
FWIW there are many things made possible by HDaudio, in practice we often have to limit ourselves to what is known to work and what is needed by end-products. You could spend all your time chasing configuration issues, missing NIDS and bad verbs. Been there, done that.
Again, we are not trying to force-expose stuff which does not work. In majority of the cases, non-HDMI codecs we're dealing with notify about just single analog endpoint. For now, it's 100% of the cases, but I'm aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not equal to entire market.
Remember that you can always use topology to "gate" userspace from streaming through endpoints which we do not work. And right now, we are working with topologies supporting single endpoint for non-HDMI devices.
So, this is a clear upgrade when compared to Analog/Alt Analog/Digitalh-hardcoded configuration used currently. That's on top of aligning with hda legacy behavior.
Regards, Czarek
Again, we are not trying to force-expose stuff which does not work. In majority of the cases, non-HDMI codecs we're dealing with notify about just single analog endpoint. For now, it's 100% of the cases, but I'm aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not equal to entire market.
We are in agreement, but since we don't have any ability to test those alt/digital parts my take is 'don't even bother about them'.
Remember that you can always use topology to "gate" userspace from streaming through endpoints which we do not work. And right now, we are working with topologies supporting single endpoint for non-HDMI devices.
May I ask how you 'gate' parts of a topology? Or did you mean that you use different topologies?
So, this is a clear upgrade when compared to Analog/Alt Analog/Digitalh-hardcoded configuration used currently. That's on top of aligning with hda legacy behavior.
Agree, but it still leaves the door open to exposing those paths which may or may not work - no one has ever tested them. It's better IMHO to only allow for the analog path. If we can detect the presence of this path, great.
On 2022-02-11 4:23 PM, Pierre-Louis Bossart wrote:
Again, we are not trying to force-expose stuff which does not work. In majority of the cases, non-HDMI codecs we're dealing with notify about just single analog endpoint. For now, it's 100% of the cases, but I'm aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not equal to entire market.
We are in agreement, but since we don't have any ability to test those alt/digital parts my take is 'don't even bother about them'.
Remember that you can always use topology to "gate" userspace from streaming through endpoints which we do not work. And right now, we are working with topologies supporting single endpoint for non-HDMI devices.
May I ask how you 'gate' parts of a topology? Or did you mean that you use different topologies?
So, this is a clear upgrade when compared to Analog/Alt Analog/Digitalh-hardcoded configuration used currently. That's on top of aligning with hda legacy behavior.
Agree, but it still leaves the door open to exposing those paths which may or may not work - no one has ever tested them. It's better IMHO to only allow for the analog path. If we can detect the presence of this path, great.
You may define topology with lower number of FE, thus limiting number of options available to user, possibly shielding them from untested scenarios. E.g.: Have a 3-DAI HDMI codec yet provide 2-FE topology file for it because you know 3rd endpoint is faulty. Only two devices will show up in userspace effectively making users unable to play on the 3rd one.
Again, that's a default - expose fewer endpoints (here, just single analog one) for non-HDMI codecs.
Of course, there is small number of users who may want to stream on their Alt Analog/Digital endpoints. We do not want 'Conexant story' to repeat all over again. So, for them, non-default (topology) could be provided to enable streaming on whatever they want.
The chosen approach does not hinder either of the sides.
Regards, Czarek
With few changes, snd_hda_codec_set_power_save() and snd_hda_codec_cleanup_for_unbind() can be re-used by ASoC drivers. While at it, provide kernel doc for the exposed functions.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hda_codec.h | 3 +++ sound/pci/hda/hda_codec.c | 14 ++++++++++++-- sound/pci/hda/hda_local.h | 1 - 3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index f74abc13414f..77426ff58338 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -318,6 +318,7 @@ int snd_hda_codec_configure(struct hda_codec *codec); int snd_hda_codec_update_widgets(struct hda_codec *codec); void snd_hda_codec_register(struct hda_codec *codec); void snd_hda_codec_unregister(struct hda_codec *codec); +void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec);
/* * low level functions @@ -496,9 +497,11 @@ int hda_call_check_power_status(struct hda_codec *codec, hda_nid_t nid) #define snd_hda_power_down(codec) snd_hdac_power_down(&(codec)->core) #define snd_hda_power_down_pm(codec) snd_hdac_power_down_pm(&(codec)->core) #ifdef CONFIG_PM +void snd_hda_codec_set_power_save(struct hda_codec *codec, int delay); void snd_hda_set_power_save(struct hda_bus *bus, int delay); void snd_hda_update_power_acct(struct hda_codec *codec); #else +static inline void snd_hda_codec_set_power_save(struct hda_codec *codec, int delay) {} static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {} #endif
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 4406b7983c07..34db37ce8847 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -766,6 +766,10 @@ static void codec_release_pcms(struct hda_codec *codec) } }
+/** + * snd_hda_codec_cleanup_for_unbind - Prepare codec for removal + * @codec: codec device to cleanup + */ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec) { if (codec->registered) { @@ -3397,7 +3401,12 @@ int snd_hda_add_new_ctls(struct hda_codec *codec, EXPORT_SYMBOL_GPL(snd_hda_add_new_ctls);
#ifdef CONFIG_PM -static void codec_set_power_save(struct hda_codec *codec, int delay) +/** + * snd_hda_codec_set_power_save - Configure codec's runtime PM + * @codec: codec device to configure + * @delay: autosuspend delay + */ +void snd_hda_codec_set_power_save(struct hda_codec *codec, int delay) { struct device *dev = hda_codec_dev(codec);
@@ -3415,6 +3424,7 @@ static void codec_set_power_save(struct hda_codec *codec, int delay) pm_runtime_forbid(dev); } } +EXPORT_SYMBOL_GPL(snd_hda_codec_set_power_save);
/** * snd_hda_set_power_save - reprogram autosuspend for the given delay @@ -3428,7 +3438,7 @@ void snd_hda_set_power_save(struct hda_bus *bus, int delay) struct hda_codec *c;
list_for_each_codec(c, bus) - codec_set_power_save(c, delay); + snd_hda_codec_set_power_save(c, delay); } EXPORT_SYMBOL_GPL(snd_hda_set_power_save);
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 4c52dfb615bc..aca592651870 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -135,7 +135,6 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, #define snd_hda_add_vmaster(codec, name, tlv, followers, suffix, access) \ __snd_hda_add_vmaster(codec, name, tlv, followers, suffix, true, access, NULL) int snd_hda_codec_reset(struct hda_codec *codec); -void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec); void snd_hda_codec_disconnect_pcms(struct hda_codec *codec);
#define snd_hda_regmap_sync(codec) snd_hdac_regmap_sync(&(codec)->core)
On 2/7/22 05:49, Cezary Rojewski wrote:
Changes expose several function that are currently unavailable for HDA-DSP drivers for use. Those functions are:
snd_hda_codec_cleanup_for_unbind() snd_hda_codec_set_power_save() snd_hda_codec_register() snd_hda_codec_unregister() snd_hda_codec_device_init()
It would be useful to explain why a platform driver would need to make use of codec-management related routines, which would typically be needed only in a codec or machine driver, or hidden as part of a generic bus layer.
In addition, both the Skylake and SOF/HDA drivers make use of e.g., snd_hdac_ext_bus_device_init(), what was wrong with this approach that's been used since 2018?
On 2022-02-07 1:35 PM, Pierre-Louis Bossart wrote:
On 2/7/22 05:49, Cezary Rojewski wrote:
Changes expose several function that are currently unavailable for HDA-DSP drivers for use. Those functions are:
snd_hda_codec_cleanup_for_unbind() snd_hda_codec_set_power_save() snd_hda_codec_register() snd_hda_codec_unregister() snd_hda_codec_device_init()
It would be useful to explain why a platform driver would need to make use of codec-management related routines, which would typically be needed only in a codec or machine driver, or hidden as part of a generic bus layer.
In addition, both the Skylake and SOF/HDA drivers make use of e.g., snd_hdac_ext_bus_device_init(), what was wrong with this approach that's been used since 2018?
Thanks for chiming in! So, all HDA drivers currently available in ASoC are _assuming_ codec resources, they are not _reading_ them. To be efficient and only create DAIs and other components when needed, codec's ->pcm_list_head need to be filled in with data before ASoC sound card can be fully enumerated.
Initialization routines for HDA device require pointer to instance of struct snd_card upfront whereas ASoC framework gives you such pointer only once all components are accounted for. Also, component granulation seen in ASoC causes need for adjustment for order of operations when registering/probing codec device to achieve correctness (resource wise) I'd mentioned above. We could have coded that logic ourselves but that's a duplication as the logic is already there.
Regards, Czarek
participants (4)
-
Cezary Rojewski -
Kai Vehmanen -
Pierre-Louis Bossart -
Takashi Iwai