Re: [PATCH 02/14] ASoC: Intel: avs: Generic soc component driver

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-04-26 11:33 PM, Pierre-Louis Bossart wrote:
>> +struct avs_dma_data {
>> +	struct avs_tplg_path_template *template;
>> +	struct avs_path *path;
>> +	/*
>> +	 * link stream is stored within substream's runtime
>> +	 * private_data to fulfill the needs of codec BE path
>> +	 *
>> +	 * host stream assigned
> 
> not able to parse that comment, what are you trying to say?


Sure. This actually stems from the legacy driver architecture. PCM 
operations for legacy HD-Audio are found in 
sound/pci/hda/hda_controller.c with respective types declared in 
sound/pci/hda/hda_controller.h. Operations found there make use of 
struct azx_dev and get_azx_dev(). So, to be able to re-use sound/pci/hda 
as much as possible plus declare virtually no custom HD-Audio related 
logic, we satisfy the needs of lower level code by assigning BE stream 
to substream->runtime->private_data.

As HD-Audio legacy operates in coupled mode only, there is no need for 
it to differentiate between HOST and LINK side. It's not true for DSP 
configurations though. So, we declare separate pointer for HOST stream 
allowing PCM operations found here to have easy access to both, LINK and 
HOST streams whenever necessary.

>> +	 */
>> +	struct hdac_ext_stream *host_stream;
>> +};
>> +
>> +static ssize_t topology_name_read(struct file *file, char __user *user_buf, size_t count,
>> +				  loff_t *ppos)
>> +{
>> +	struct snd_soc_component *component = file->private_data;
>> +	struct snd_soc_card *card = component->card;
>> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(card->dev);
>> +	char buf[64];
>> +	size_t len;
>> +
>> +	len = snprintf(buf, sizeof(buf), "%s/%s\n", component->driver->topology_name_prefix,
>> +		       mach->tplg_filename);
>> +
>> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>> +}
>> +
>> +static const struct file_operations topology_name_fops = {
>> +	.open = simple_open,
>> +	.read = topology_name_read,
>> +	.llseek = default_llseek,
>> +};
> 
> can you clarify why this is needed?


The usage of default_llseek() or the topology_name_fops as a whole? The 
latter is here to obtain name of the firmware file requested by given 
machine board easily from the debugfs. The former is probably just a 
copy-paste from other declarations of file_operation entries for 
READ-only files.

>> +
>> +static int avs_component_load_libraries(struct avs_soc_component *acomp)
>> +{
>> +	struct avs_tplg *tplg = acomp->tplg;
>> +	struct avs_dev *adev = to_avs_dev(acomp->base.dev);
>> +	int ret;
>> +
>> +	if (!tplg->num_libs)
>> +		return 0;
>> +
>> +	/* Parent device may be asleep and library loading involves IPCs. */
>> +	ret = pm_runtime_get_sync(adev->dev);
>> +	if (ret < 0 && ret != -EACCES) {
>> +		pm_runtime_put_noidle(adev->dev);
>> +		return ret;
>> +	}
> 
> Mark recommends the use of pm_runtime_resume_and_get(), see patches from today.


Will definitely check this out, thanks for pointing this out.

>> +
>> +	avs_hda_clock_gating_enable(adev, false);
>> +	avs_hda_l1sen_enable(adev, false);
>> +
>> +	ret = avs_dsp_load_libraries(adev, tplg->libs, tplg->num_libs);
>> +
>> +	avs_hda_l1sen_enable(adev, true);
>> +	avs_hda_clock_gating_enable(adev, true);
>> +
>> +	if (!ret)
>> +		ret = avs_module_info_init(adev, false);
>> +
>> +	pm_runtime_mark_last_busy(adev->dev);
>> +	pm_runtime_put_autosuspend(adev->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int avs_component_probe(struct snd_soc_component *component)
>> +{
>> +	struct snd_soc_card *card = component->card;
>> +	struct snd_soc_acpi_mach *mach;
>> +	struct avs_soc_component *acomp;
>> +	struct avs_dev *adev;
>> +	char *filename;
>> +	int ret;
>> +
>> +	dev_dbg(card->dev, "probing %s card %s\n", component->name, card->name);
>> +	mach = dev_get_platdata(card->dev);
>> +	acomp = to_avs_soc_component(component);
>> +	adev = to_avs_dev(component->dev);
>> +
>> +	acomp->tplg = avs_tplg_new(component);
>> +	if (!acomp->tplg)
>> +		return -ENOMEM;
>> +
>> +	if (!mach->tplg_filename)
>> +		goto finalize;
>> +
>> +	/* Load specified topology and create sysfs for it. */
>> +	filename = kasprintf(GFP_KERNEL, "%s/%s", component->driver->topology_name_prefix,
>> +			     mach->tplg_filename);
> 
> what is the link between topology and sysfs?


This comment is misleading as debugfs entry is being created as seen in 
the code below.

To answer the question assuming s/sysfs/debugfs/:

Ability to allow for reading name of the file requested by given machine 
board from user space. AVS driver spawns multiple machine boards, each 
requesting a topology file. To make it easier for the validation and 
normal users to understand what has requested what, we provide a simple, 
read-only debugfs entry for each of the boards within the avs tree.

>> +	if (!filename)
>> +		return -ENOMEM;
>> +
>> +	ret = avs_load_topology(component, filename);
>> +	kfree(filename);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = avs_component_load_libraries(acomp);
>> +	if (ret < 0) {
>> +		dev_err(card->dev, "libraries loading failed: %d\n", ret);
>> +		goto err_load_libs;
>> +	}
>> +
>> +finalize:
>> +	debugfs_create_file("topology_name", 0444, component->debugfs_root, component,
>> +			    &topology_name_fops);
> 
> that's debugfs here, is this to make it possible to select an alternate topology file? If yes, a comment earlier wouldn't hurt.


Nah, its purpose is very basic: get the exact name of the topology file 
loaded. Example:

intel/avs/hda-8086280b-tplg.bin

>> +
>> +	mutex_lock(&adev->comp_list_mutex);
>> +	list_add_tail(&acomp->node, &adev->comp_list);
>> +	mutex_unlock(&adev->comp_list_mutex);
>> +
>> +	return 0;
>> +
>> +err_load_libs:
>> +	avs_remove_topology(component);
>> +	return ret;
>> +}
>> +
> 
> 
>> +static const struct snd_soc_component_driver avs_component_driver = {
>> +	.name			= "avs-pcm",
>> +	.probe			= avs_component_probe,
>> +	.remove			= avs_component_remove,
>> +	.open			= avs_component_open,
>> +	.pointer		= avs_component_pointer,
>> +	.mmap			= avs_component_mmap,
>> +	.pcm_construct		= avs_component_construct,
>> +	.module_get_upon_open	= 1, /* increment refcount when a pcm is opened */
>> +	.topology_name_prefix	= "intel/avs",
> 
> is this intentional that the firmware binaries and topologies will be stored in the same intel/avs directory?
> 
>> +	.non_legacy_dai_naming	= true,
> 
> is this needed? we've never used this for Intel?


I'll recheck this but back in time when we drawn PCM ops for the first 
time there was a reason, certainly. Out of my head - impacts behavior of 
snd_soc_register_dais().

>> +};
>> +