[PATCH] ASoC: Intel: Unify HDAudio-ext bus initialization
HDAudio-extended bus initialization parts are scattered throughout Intel ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide unified initialization point.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hdaudio_ext.h | 9 ++++++--- sound/hda/ext/hdac_ext_bus.c | 29 ++++++++++++++++++++--------- sound/soc/intel/skylake/skl.c | 9 +-------- sound/soc/sof/intel/hda-bus.c | 16 +++++++++------- sound/soc/sof/intel/hda.c | 5 +++-- sound/soc/sof/intel/hda.h | 3 ++- 6 files changed, 41 insertions(+), 30 deletions(-)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 375581634143..d1f6e9f7c057 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -2,11 +2,14 @@ #ifndef __SOUND_HDAUDIO_EXT_H #define __SOUND_HDAUDIO_EXT_H
+#include <linux/pci.h> #include <sound/hdaudio.h> +#include <sound/hda_codec.h>
-int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev, - const struct hdac_bus_ops *ops, - const struct hdac_ext_bus_ops *ext_ops); +int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev, + const struct hdac_bus_ops *ops, + const struct hdac_ext_bus_ops *ext_ops, + const char *model);
void snd_hdac_ext_bus_exit(struct hdac_bus *bus); int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index 765c40a6ccba..a89e2e80ea4c 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -10,15 +10,17 @@ */
#include <linux/module.h> +#include <linux/pci.h> #include <linux/slab.h> #include <linux/io.h> #include <sound/hdaudio_ext.h> +#include <sound/hda_codec.h>
MODULE_DESCRIPTION("HDA extended core"); MODULE_LICENSE("GPL v2");
/** - * snd_hdac_ext_bus_init - initialize a HD-audio extended bus + * snd_hda_ext_bus_init - initialize a HD-audio extended bus * @bus: the pointer to HDAC bus object * @dev: device pointer * @ops: bus verb operators @@ -26,28 +28,37 @@ MODULE_LICENSE("GPL v2"); * * Returns 0 if successful, or a negative error code. */ -int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev, - const struct hdac_bus_ops *ops, - const struct hdac_ext_bus_ops *ext_ops) +int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev, + const struct hdac_bus_ops *ops, + const struct hdac_ext_bus_ops *ext_ops, + const char *model) { + struct hdac_bus *base = &bus->core; int ret;
- ret = snd_hdac_bus_init(bus, dev, ops); + ret = snd_hdac_bus_init(base, &pdev->dev, ops); if (ret < 0) return ret;
- bus->ext_ops = ext_ops; + base->ext_ops = ext_ops; /* FIXME: * Currently only one bus is supported, if there is device with more * buses, bus->idx should be greater than 0, but there needs to be a * reliable way to always assign same number. */ - bus->idx = 0; - bus->cmd_dma_state = true; + base->idx = 0; + base->cmd_dma_state = true; + base->use_posbuf = 1; + base->bdl_pos_adj = 0; + base->sync_write = 1; + bus->pci = pdev; + bus->modelname = model; + bus->mixer_assigned = -1; + mutex_init(&bus->prepare_mutex);
return 0; } -EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); +EXPORT_SYMBOL_GPL(snd_hda_ext_bus_init);
/** * snd_hdac_ext_bus_exit - clean up a HD-audio extended bus diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..95de41d14e56 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -886,16 +886,9 @@ static int skl_create(struct pci_dev *pci, #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) ext_ops = snd_soc_hdac_hda_get_ops(); #endif - snd_hdac_ext_bus_init(bus, &pci->dev, NULL, ext_ops); - bus->use_posbuf = 1; + snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus"); skl->pci = pci; INIT_WORK(&skl->probe_work, skl_probe_work); - bus->bdl_pos_adj = 0; - - mutex_init(&hbus->prepare_mutex); - hbus->pci = pci; - hbus->mixer_assigned = -1; - hbus->modelname = "sklbus";
*rskl = skl;
diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c index 30025d3c16b6..5d5081f80e88 100644 --- a/sound/soc/sof/intel/hda-bus.c +++ b/sound/soc/sof/intel/hda-bus.c @@ -8,6 +8,7 @@ // Authors: Keyon Jie yang.jie@linux.intel.com
#include <linux/io.h> +#include <linux/pci.h> #include <sound/hdaudio.h> #include <sound/hda_i915.h> #include "../sof-priv.h" @@ -53,24 +54,25 @@ static const struct hdac_bus_ops bus_core_ops = { /* * This can be used for both with/without hda link support. */ -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev, + const char *model) { #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) - snd_hdac_ext_bus_init(bus, dev, &bus_core_ops, sof_hda_ext_ops); + snd_hda_ext_bus_init(bus, pdev, &bus_core_ops, sof_hda_ext_ops, model); #else /* CONFIG_SND_SOC_SOF_HDA */ memset(bus, 0, sizeof(*bus)); - bus->dev = dev; + bus->core.dev = &pdev->dev;
- INIT_LIST_HEAD(&bus->stream_list); + INIT_LIST_HEAD(&bus->core.stream_list);
- bus->irq = -1; + bus->core.irq = -1;
/* * There is only one HDA bus atm. keep the index as 0. * Need to fix when there are more than one HDA bus. */ - bus->idx = 0; + bus->core.idx = 0;
- spin_lock_init(&bus->reg_lock); + spin_lock_init(&bus->core.reg_lock); #endif /* CONFIG_SND_SOC_SOF_HDA */ } diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index fbc2421c77f8..03a68d286c7c 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -609,8 +609,9 @@ static int hda_init(struct snd_sof_dev *sdev) bus = sof_to_bus(sdev);
/* HDA bus init */ - sof_hda_bus_init(bus, &pci->dev); + sof_hda_bus_init(hbus, pci, hda_model);
+#if !IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) bus->use_posbuf = 1; bus->bdl_pos_adj = 0; bus->sync_write = 1; @@ -619,7 +620,7 @@ static int hda_init(struct snd_sof_dev *sdev) hbus->pci = pci; hbus->mixer_assigned = -1; hbus->modelname = hda_model; - +#endif /* initialise hdac bus */ bus->addr = pci_resource_start(pci, 0); bus->remap_addr = pci_ioremap_bar(pci, 0); diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 1195018a1f4f..a4ec88f36512 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -635,7 +635,8 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev); /* * HDA bus operations. */ -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev); +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev, + const char *model);
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) /*
On 10/18/21 2:21 PM, Cezary Rojewski wrote:
HDAudio-extended bus initialization parts are scattered throughout Intel ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide unified initialization point.
What unification are we talking about?
The code for HDaudio differs a great deal between the two Intel drivers, and specifically the 'nocodec' mode in SOF does not rely on this library, so there's no burning desire on my side to add this dependency when we carefully tried to avoid it to use the DMA parts only.
I would add we recently looked at the code and the coupling/decoupling in this library seems questionable if not broken.
edit: this patch also seems to add a layer of indirection through a 'core' layer, not sure where this is going at all. I must be missing something.
CC: Ranjani and Kai.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
include/sound/hdaudio_ext.h | 9 ++++++--- sound/hda/ext/hdac_ext_bus.c | 29 ++++++++++++++++++++--------- sound/soc/intel/skylake/skl.c | 9 +-------- sound/soc/sof/intel/hda-bus.c | 16 +++++++++------- sound/soc/sof/intel/hda.c | 5 +++-- sound/soc/sof/intel/hda.h | 3 ++- 6 files changed, 41 insertions(+), 30 deletions(-)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 375581634143..d1f6e9f7c057 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -2,11 +2,14 @@ #ifndef __SOUND_HDAUDIO_EXT_H #define __SOUND_HDAUDIO_EXT_H
+#include <linux/pci.h> #include <sound/hdaudio.h> +#include <sound/hda_codec.h>
-int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops);+int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops,const char *model);void snd_hdac_ext_bus_exit(struct hdac_bus *bus); int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index 765c40a6ccba..a89e2e80ea4c 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -10,15 +10,17 @@ */
#include <linux/module.h> +#include <linux/pci.h> #include <linux/slab.h> #include <linux/io.h> #include <sound/hdaudio_ext.h> +#include <sound/hda_codec.h>
MODULE_DESCRIPTION("HDA extended core"); MODULE_LICENSE("GPL v2");
/**
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- snd_hda_ext_bus_init - initialize a HD-audio extended bus
- @bus: the pointer to HDAC bus object
- @dev: device pointer
- @ops: bus verb operators
@@ -26,28 +28,37 @@ MODULE_LICENSE("GPL v2");
- Returns 0 if successful, or a negative error code.
*/ -int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops)+int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops,const char *model)
missing kernel doc update?
{
- struct hdac_bus *base = &bus->core; int ret;
- ret = snd_hdac_bus_init(bus, dev, ops);
- ret = snd_hdac_bus_init(base, &pdev->dev, ops); if (ret < 0) return ret;
- bus->ext_ops = ext_ops;
- base->ext_ops = ext_ops; /* FIXME:
*/
- Currently only one bus is supported, if there is device with more
- buses, bus->idx should be greater than 0, but there needs to be a
- reliable way to always assign same number.
- bus->idx = 0;
- bus->cmd_dma_state = true;
base->idx = 0;
base->cmd_dma_state = true;
base->use_posbuf = 1;
base->bdl_pos_adj = 0;
base->sync_write = 1;
bus->pci = pdev;
bus->modelname = model;
bus->mixer_assigned = -1;
mutex_init(&bus->prepare_mutex);
return 0;
} -EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); +EXPORT_SYMBOL_GPL(snd_hda_ext_bus_init);
/**
- snd_hdac_ext_bus_exit - clean up a HD-audio extended bus
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..95de41d14e56 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -886,16 +886,9 @@ static int skl_create(struct pci_dev *pci, #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) ext_ops = snd_soc_hdac_hda_get_ops(); #endif
- snd_hdac_ext_bus_init(bus, &pci->dev, NULL, ext_ops);
- bus->use_posbuf = 1;
- snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus"); skl->pci = pci; INIT_WORK(&skl->probe_work, skl_probe_work);
bus->bdl_pos_adj = 0;
mutex_init(&hbus->prepare_mutex);
hbus->pci = pci;
hbus->mixer_assigned = -1;
hbus->modelname = "sklbus";
*rskl = skl;
diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c index 30025d3c16b6..5d5081f80e88 100644 --- a/sound/soc/sof/intel/hda-bus.c +++ b/sound/soc/sof/intel/hda-bus.c @@ -8,6 +8,7 @@ // Authors: Keyon Jie yang.jie@linux.intel.com
#include <linux/io.h> +#include <linux/pci.h> #include <sound/hdaudio.h> #include <sound/hda_i915.h> #include "../sof-priv.h" @@ -53,24 +54,25 @@ static const struct hdac_bus_ops bus_core_ops = { /*
- This can be used for both with/without hda link support.
*/ -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const char *model){ #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
- snd_hdac_ext_bus_init(bus, dev, &bus_core_ops, sof_hda_ext_ops);
- snd_hda_ext_bus_init(bus, pdev, &bus_core_ops, sof_hda_ext_ops, model);
#else /* CONFIG_SND_SOC_SOF_HDA */ memset(bus, 0, sizeof(*bus));
- bus->dev = dev;
- bus->core.dev = &pdev->dev;
- INIT_LIST_HEAD(&bus->stream_list);
- INIT_LIST_HEAD(&bus->core.stream_list);
- bus->irq = -1;
bus->core.irq = -1;
/*
- There is only one HDA bus atm. keep the index as 0.
- Need to fix when there are more than one HDA bus.
*/
- bus->idx = 0;
- bus->core.idx = 0;
why is this level of indirection through 'core' needed in this code which doesn't use the hdaudio-ext library?
the changes here have nothing to do with snd_hda_ext_bus_init()?
- spin_lock_init(&bus->reg_lock);
- spin_lock_init(&bus->core.reg_lock);
same, we've just added reg_lock everywhere, why use a different one
#endif /* CONFIG_SND_SOC_SOF_HDA */ } diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index fbc2421c77f8..03a68d286c7c 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -609,8 +609,9 @@ static int hda_init(struct snd_sof_dev *sdev) bus = sof_to_bus(sdev);
/* HDA bus init */
- sof_hda_bus_init(bus, &pci->dev);
- sof_hda_bus_init(hbus, pci, hda_model);
+#if !IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) bus->use_posbuf = 1; bus->bdl_pos_adj = 0; bus->sync_write = 1; @@ -619,7 +620,7 @@ static int hda_init(struct snd_sof_dev *sdev) hbus->pci = pci; hbus->mixer_assigned = -1; hbus->modelname = hda_model;
spurious line change
+#endif /* initialise hdac bus */ bus->addr = pci_resource_start(pci, 0); bus->remap_addr = pci_ioremap_bar(pci, 0); diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 1195018a1f4f..a4ec88f36512 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -635,7 +635,8 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev); /*
- HDA bus operations.
*/ -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev); +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const char *model);#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) /*
On 2021-10-18 11:18 PM, Pierre-Louis Bossart wrote:
On 10/18/21 2:21 PM, Cezary Rojewski wrote:
HDAudio-extended bus initialization parts are scattered throughout Intel ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide unified initialization point.
What unification are we talking about?
The code for HDaudio differs a great deal between the two Intel drivers, and specifically the 'nocodec' mode in SOF does not rely on this library, so there's no burning desire on my side to add this dependency when we carefully tried to avoid it to use the DMA parts only.
I would add we recently looked at the code and the coupling/decoupling in this library seems questionable if not broken.
edit: this patch also seems to add a layer of indirection through a 'core' layer, not sure where this is going at all. I must be missing something.
CC: Ranjani and Kai.
Thanks for the comments!
Pretty sure you overestimated the goal of this patch, though. In both skylake and sof -drivers various bus->xxx and bus->core.xxx fields are scattered and initialized with basically the exact same values. These values more often than not even match the sound/pci/hda ones. The ultimate goal is probably to extract similar parts and move them to snd_hdac_bus_init() or some other wrapper. For now, 'ext-bus' is being addressed.
Patch would have 'greater' effect if not for the fact that sof-intel-hda code conditionally initializes several fields for the reasons unknown to me. So, instead of just removing those assignments, preproccesor directive is used instead.
...
/**
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- snd_hda_ext_bus_init - initialize a HD-audio extended bus
- @bus: the pointer to HDAC bus object
- @dev: device pointer
- @ops: bus verb operators
@@ -26,28 +28,37 @@ MODULE_LICENSE("GPL v2");
- Returns 0 if successful, or a negative error code.
*/ -int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops)+int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops,const char *model)missing kernel doc update?
Ack.
...
@@ -53,24 +54,25 @@ static const struct hdac_bus_ops bus_core_ops = { /*
- This can be used for both with/without hda link support.
*/ -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const char *model)
- snd_hdac_ext_bus_init(bus, dev, &bus_core_ops, sof_hda_ext_ops);
- snd_hda_ext_bus_init(bus, pdev, &bus_core_ops, sof_hda_ext_ops, model); #else /* CONFIG_SND_SOC_SOF_HDA */ memset(bus, 0, sizeof(*bus));
- bus->dev = dev;
- bus->core.dev = &pdev->dev;
- INIT_LIST_HEAD(&bus->stream_list);
- INIT_LIST_HEAD(&bus->core.stream_list);
- bus->irq = -1;
bus->core.irq = -1;
/*
- There is only one HDA bus atm. keep the index as 0.
- Need to fix when there are more than one HDA bus.
*/
- bus->idx = 0;
- bus->core.idx = 0;
why is this level of indirection through 'core' needed in this code which doesn't use the hdaudio-ext library?
the changes here have nothing to do with snd_hda_ext_bus_init()?
I don't understand the comment. First argument in parameter-list for function sof_hda_bus_init() has its type changed from 'struct hdac_bus *' to 'struct hda_bus *'. Without updating those assignments, code wouldn't compile with CONFIG_SND_SOC_SOF_HDA disabled.
- spin_lock_init(&bus->reg_lock);
- spin_lock_init(&bus->core.reg_lock);
same, we've just added reg_lock everywhere, why use a different one
It's not a different one, it's exactly the same one : )
#endif /* CONFIG_SND_SOC_SOF_HDA */ } diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index fbc2421c77f8..03a68d286c7c 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -609,8 +609,9 @@ static int hda_init(struct snd_sof_dev *sdev) bus = sof_to_bus(sdev);
/* HDA bus init */
- sof_hda_bus_init(bus, &pci->dev);
- sof_hda_bus_init(hbus, pci, hda_model);
+#if !IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) bus->use_posbuf = 1; bus->bdl_pos_adj = 0; bus->sync_write = 1; @@ -619,7 +620,7 @@ static int hda_init(struct snd_sof_dev *sdev) hbus->pci = pci; hbus->mixer_assigned = -1; hbus->modelname = hda_model;
spurious line change
+#endif
Well, I've just inserted #endif in place of this newline. No problem with appending additional Enter if that's what you prefer.
Czarek
Hey,
On Mon, 18 Oct 2021, Cezary Rojewski wrote:
HDAudio-extended bus initialization parts are scattered throughout Intel ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide unified initialization point.
[...]
--- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c
[..]
-int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops)+int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops,const char *model)
[...]
- bus->idx = 0;
- bus->cmd_dma_state = true;
- base->idx = 0;
- base->cmd_dma_state = true;
- base->use_posbuf = 1;
- base->bdl_pos_adj = 0;
- base->sync_write = 1;
- bus->pci = pdev;
- bus->modelname = model;
- bus->mixer_assigned = -1;
- mutex_init(&bus->prepare_mutex);
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
Br, Kai
On 2021-10-19 11:16 AM, Kai Vehmanen wrote:
Hey,
On Mon, 18 Oct 2021, Cezary Rojewski wrote:
HDAudio-extended bus initialization parts are scattered throughout Intel ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide unified initialization point.
[...]
--- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c
[..]
-int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops)+int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
const struct hdac_bus_ops *ops,const struct hdac_ext_bus_ops *ext_ops,const char *model)[...]
- bus->idx = 0;
- bus->cmd_dma_state = true;
- base->idx = 0;
- base->cmd_dma_state = true;
- base->use_posbuf = 1;
- base->bdl_pos_adj = 0;
- base->sync_write = 1;
- bus->pci = pdev;
- bus->modelname = model;
- bus->mixer_assigned = -1;
- mutex_init(&bus->prepare_mutex);
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
I'm rather in favor of bigger cleanups. We can definitely move further in the future : )
Notice that some 'core' field are being initialized in snd_hda_ext_bus_init() for a very long time. The problem with moving all 'core' bits to snd_hdac_bus_init() is that some of these are not 1:1 between ASoC and ALSA hda-drivers. Also, hda_tegra differs from hda_intel too. I could update said function while not removing any assignments which differ from the default. Maybe let's just go for it..
TLDR: treat snd_hdac_bus_init() as function initializing fields with defaults. Any you don't like? Change after its invocation.
BTW, I don't see any problems with ->sync_write=1 as from software perspective it is a bus property. Otherwise, interface change is required to reflect this 'misleading' information.
Regards, Czarek
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
I'm rather in favor of bigger cleanups. We can definitely move further in the future : )
I am not opposed to updates in this hdaudio-ext part, but I am in favor of less ambitious step-by-step changes.
a) This is legacy code where the initial authors have moved on, and it's very hard to figure out what the intent was. It's quite common to have discussion on feature v. bug, see e.g. the discussion we had on this in https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
b) In addition, this code is shared between two teams with separate testing/CI capabilities and limited number of commercial/shipping devices. There is a very large risk of introducing bugs even with the best intentions.
Before we do any changes in functionality, there are already basic steps that can be done, e.g. using consistent naming between variables, the existing code is just confusing as can be:
struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream;
I started basic cleanups here: https://github.com/thesofproject/linux/pull/3158, I haven't had time to complete this because of more important locking issues, but I intend to send those clarifications for the next cycle.
Again before we do large changes let's think of smaller steps and how we are going to validate those changes.
On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
I'm rather in favor of bigger cleanups. We can definitely move further in the future : )
I am not opposed to updates in this hdaudio-ext part, but I am in favor of less ambitious step-by-step changes.
a) This is legacy code where the initial authors have moved on, and it's very hard to figure out what the intent was. It's quite common to have discussion on feature v. bug, see e.g. the discussion we had on this in https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
b) In addition, this code is shared between two teams with separate testing/CI capabilities and limited number of commercial/shipping devices. There is a very large risk of introducing bugs even with the best intentions.
Before we do any changes in functionality, there are already basic steps that can be done, e.g. using consistent naming between variables, the existing code is just confusing as can be:
struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream;
I started basic cleanups here: https://github.com/thesofproject/linux/pull/3158, I haven't had time to complete this because of more important locking issues, but I intend to send those clarifications for the next cycle.
Again before we do large changes let's think of smaller steps and how we are going to validate those changes.
Agree, step-by-step is the way to go.
Isn't this patch a 'step' though? This isn't remotely a refactor, just gathering of common parts of ext-bus initialization. We could start with this so legacy users are unaffected, then have snd_hdac_bus_init() updated so snd_hdac_ext_bus_init() is devoid of 'core' fields assignments as suggested by Kai.
Regards, Czarek
On 10/19/21 12:32 PM, Cezary Rojewski wrote:
On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
I'm rather in favor of bigger cleanups. We can definitely move further in the future : )
I am not opposed to updates in this hdaudio-ext part, but I am in favor of less ambitious step-by-step changes.
a) This is legacy code where the initial authors have moved on, and it's very hard to figure out what the intent was. It's quite common to have discussion on feature v. bug, see e.g. the discussion we had on this in https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
b) In addition, this code is shared between two teams with separate testing/CI capabilities and limited number of commercial/shipping devices. There is a very large risk of introducing bugs even with the best intentions.
Before we do any changes in functionality, there are already basic steps that can be done, e.g. using consistent naming between variables, the existing code is just confusing as can be:
struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream;
I started basic cleanups here: https://github.com/thesofproject/linux/pull/3158, I haven't had time to complete this because of more important locking issues, but I intend to send those clarifications for the next cycle.
Again before we do large changes let's think of smaller steps and how we are going to validate those changes.
Agree, step-by-step is the way to go.
Isn't this patch a 'step' though? This isn't remotely a refactor, just gathering of common parts of ext-bus initialization. We could start with this so legacy users are unaffected, then have snd_hdac_bus_init() updated so snd_hdac_ext_bus_init() is devoid of 'core' fields assignments as suggested by Kai.
If it was just moving common parts, I would have no issue. My main objection is that you went one step further and started renaming stuff in rather confusing ways, e.g.
-void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
- * snd_hdac_ext_bus_init - initialize a HD-audio extended bus + * snd_hda_ext_bus_init - initialize a HD-audio extended bus
hda_bus is a definition from hda_codec.h, I don't see a reason why we should use this structure when the vast majority of the code uses hdac_bus. And the purpose of hdac_ext is really to deal with *controller* extensions to couple/decouple DMAs. There is no dependency on codecs at that level.
Even if there was, I also don't really see why/when we should start using hda_ext v. hdac_ext, the naming convention completely escapes me. It would seem logical to me to only use hdac_ext as an extension of hdac_, no?
I also don't get what this means: + snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
what is 'sklbus' and what purpose are you trying to accomplish with this change?
On 2021-10-19 8:42 PM, Pierre-Louis Bossart wrote:
On 10/19/21 12:32 PM, Cezary Rojewski wrote:
On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
...
I am not opposed to updates in this hdaudio-ext part, but I am in favor of less ambitious step-by-step changes.
a) This is legacy code where the initial authors have moved on, and it's very hard to figure out what the intent was. It's quite common to have discussion on feature v. bug, see e.g. the discussion we had on this in https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
b) In addition, this code is shared between two teams with separate testing/CI capabilities and limited number of commercial/shipping devices. There is a very large risk of introducing bugs even with the best intentions.
Before we do any changes in functionality, there are already basic steps that can be done, e.g. using consistent naming between variables, the existing code is just confusing as can be:
struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream;
I started basic cleanups here: https://github.com/thesofproject/linux/pull/3158, I haven't had time to complete this because of more important locking issues, but I intend to send those clarifications for the next cycle.
Again before we do large changes let's think of smaller steps and how we are going to validate those changes.
Agree, step-by-step is the way to go.
Isn't this patch a 'step' though? This isn't remotely a refactor, just gathering of common parts of ext-bus initialization. We could start with this so legacy users are unaffected, then have snd_hdac_bus_init() updated so snd_hdac_ext_bus_init() is devoid of 'core' fields assignments as suggested by Kai.
If it was just moving common parts, I would have no issue. My main objection is that you went one step further and started renaming stuff in rather confusing ways, e.g.
-void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- snd_hda_ext_bus_init - initialize a HD-audio extended bus
Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't believe that's confusing to anybody.
No problem with reverting naming change for now - we can streamline naming later.
In regard to sof_hda_bus_init(): I don't see any renaming here, just argument type changes to match new snd_hda_ext_bus_init() usage.
hda_bus is a definition from hda_codec.h, I don't see a reason why we should use this structure when the vast majority of the code uses hdac_bus. And the purpose of hdac_ext is really to deal with *controller* extensions to couple/decouple DMAs. There is no dependency on codecs at that level.
hda_bus is the base for all HDAudio drivers: struct azx struct skl_dev struct sof_intel_hda_dev
So no, what vast majority of code actually does is hda_bus/codec to hdac_bus/codec (and vice-versa) translation when in fact all the drivers are hda_* based. If you speak of confusion, that's the confusion that should be addressed in the future..
Even if there was, I also don't really see why/when we should start using hda_ext v. hdac_ext, the naming convention completely escapes me. It would seem logical to me to only use hdac_ext as an extension of hdac_, no?
I also don't get what this means:
- snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
what is 'sklbus' and what purpose are you trying to accomplish with this change?
Well, please see the updated declaration of snd_hda_ext_bus_init() in this very patch and then the existing code of sound/soc/intel/skylake/skl.c - skl_create(). Last argument in updated declaration reads 'modelname'. Skylake-driver has its own, SOF initializes it differently.
Regads, Czarek
If it was just moving common parts, I would have no issue. My main objection is that you went one step further and started renaming stuff in rather confusing ways, e.g.
-void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- snd_hda_ext_bus_init - initialize a HD-audio extended bus
Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't believe that's confusing to anybody.
it is confusing to me, myself and I. The main point is that it blurs layers.
The hdaudio_ext library deals with Intel controller extensions for DMA management and does not need to know about a larger container.
No problem with reverting naming change for now - we can streamline naming later.
In regard to sof_hda_bus_init(): I don't see any renaming here, just argument type changes to match new snd_hda_ext_bus_init() usage.
hda_bus is a definition from hda_codec.h, I don't see a reason why we should use this structure when the vast majority of the code uses hdac_bus. And the purpose of hdac_ext is really to deal with *controller* extensions to couple/decouple DMAs. There is no dependency on codecs at that level.
hda_bus is the base for all HDAudio drivers: struct azx struct skl_dev struct sof_intel_hda_dev
So no, what vast majority of code actually does is hda_bus/codec to hdac_bus/codec (and vice-versa) translation when in fact all the drivers are hda_* based. If you speak of confusion, that's the confusion that should be addressed in the future..
I maintain that the hdaudio_ext library is about Intel-specific changes on the controller level and only with DMAs.
/** * hdac_ext_stream: HDAC extended stream for extended HDA caps * * @hstream: hdac_stream * @pphc_addr: processing pipe host stream pointer * @pplc_addr: processing pipe link stream pointer * @spib_addr: software position in buffers stream pointer * @fifo_addr: software position Max fifos stream pointer * @dpibr_addr: DMA position in buffer resume pointer * @dpib: DMA position in buffer * @lpib: Linear position in buffer * @decoupled: stream host and link is decoupled * @link_locked: link is locked * @link_prepared: link is prepared * @link_substream: link substream */
It's not really a surprise that there's confusion, even the HDaudio spec describes controller, DMA, link and codec without clearly calling out boundaries between layers.
Even if there was, I also don't really see why/when we should start using hda_ext v. hdac_ext, the naming convention completely escapes me. It would seem logical to me to only use hdac_ext as an extension of hdac_, no?
I also don't get what this means: + snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
what is 'sklbus' and what purpose are you trying to accomplish with this change?
Well, please see the updated declaration of snd_hda_ext_bus_init() in this very patch and then the existing code of sound/soc/intel/skylake/skl.c - skl_create(). Last argument in updated declaration reads 'modelname'. Skylake-driver has its own, SOF initializes it differently.
Not sure why you have your own?
On 2021-10-21 7:19 PM, Pierre-Louis Bossart wrote:
If it was just moving common parts, I would have no issue. My main objection is that you went one step further and started renaming stuff in rather confusing ways, e.g.
-void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- snd_hda_ext_bus_init - initialize a HD-audio extended bus
Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't believe that's confusing to anybody.
it is confusing to me, myself and I. The main point is that it blurs layers.
The hdaudio_ext library deals with Intel controller extensions for DMA management and does not need to know about a larger container.
Ok. Will leave the naming part for future patches while leaving argument list as is.
...
Even if there was, I also don't really see why/when we should start using hda_ext v. hdac_ext, the naming convention completely escapes me. It would seem logical to me to only use hdac_ext as an extension of hdac_, no?
I also don't get what this means: + snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
what is 'sklbus' and what purpose are you trying to accomplish with this change?
Well, please see the updated declaration of snd_hda_ext_bus_init() in this very patch and then the existing code of sound/soc/intel/skylake/skl.c - skl_create(). Last argument in updated declaration reads 'modelname'. Skylake-driver has its own, SOF initializes it differently.
Not sure why you have your own?
Not sure I understand the question. If you are talking about changing string 'sklbus' to something else, then I don't believe mixing changes: update to actual values assigned and assignment relocation in one patch is good. I used 'sklbus' as that's what is being currently assigned to ->modelname within skl_create(). Such approach makes the change more transparent.
Regards, Czarek
Well, please see the updated declaration of snd_hda_ext_bus_init() in this very patch and then the existing code of sound/soc/intel/skylake/skl.c - skl_create(). Last argument in updated declaration reads 'modelname'. Skylake-driver has its own, SOF initializes it differently.
Not sure why you have your own?
Not sure I understand the question. If you are talking about changing string 'sklbus' to something else, then I don't believe mixing changes: update to actual values assigned and assignment relocation in one patch is good. I used 'sklbus' as that's what is being currently assigned to ->modelname within skl_create(). Such approach makes the change more transparent.
What I meant is that this 'modelname' is a module parameter for legacy and SOF driver, it's attached to the bus, but eventually used by the codec
hda_codec.c: if (codec->bus->modelname) { hda_codec.c: codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL);
and even further down used to apply board-specific fixups.
"sklbus" doesn't seem to be related to codecs, boards or fixups, so not sure what this parameter does in the end?
participants (3)
-
Cezary Rojewski -
Kai Vehmanen -
Pierre-Louis Bossart