Re: [PATCH v7 18/22] ASoC: qdsp6: audioreach: add topology support

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks Pierre for quick review,

On 21/09/2021 19:44, Pierre-Louis Bossart wrote:
> 
>> +static struct audioreach_module *audioreach_tplg_alloc_module(struct q6apm *apm,
>> +							      struct audioreach_container *cont,
>> +							      struct snd_soc_dapm_widget *w,
>> +							      uint32_t module_id, bool *found)
>> +{
>> +	struct audioreach_module *mod;
>> +	int ret;
>> +
>> +	mutex_lock(&apm->lock);
>> +	mod = idr_find(&apm->modules_idr, module_id);
>> +	mutex_unlock(&apm->lock);
>> +
>> +	if (mod) {
>> +		*found = true;
>> +		return mod;
>> +	}
>> +	*found = false;
>> +	mod = kzalloc(sizeof(*mod), GFP_KERNEL);
>> +	if (!mod)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mutex_lock(&apm->lock);
>> +	if (!module_id) { /* alloc module id dynamically */
>> +		ret = idr_alloc_cyclic(&apm->modules_idr, mod,
>> +				       AR_MODULE_DYNAMIC_INSTANCE_ID_START,
>> +				       AR_MODULE_DYNAMIC_INSTANCE_ID_END, GFP_KERNEL);
>> +	} else {
>> +		ret = idr_alloc(&apm->modules_idr, mod, module_id, module_id + 1, GFP_KERNEL);
>> +	}
>> +	mutex_unlock(&apm->lock);
>> +
>> +	if (ret < 0) {
>> +		dev_err(apm->dev,
>> +			"Failed to allocate Module Instance ID (%x)\n", module_id);
>> +		kfree(mod);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	mod->instance_id = ret;
>> +	dev_err(apm->dev, "Module Instance ID (0x%08x) allocated\n", ret);
> 
> dev_dbg?
> 

Sure, I removed these in next version.
>> +	/* add to module list */
>> +	list_add_tail(&mod->node, &cont->modules_list);
>> +	mod->container = cont;
>> +	mod->widget = w;
>> +	cont->num_modules++;
>> +
>> +	return mod;
>> +}
> 
> 
> 
>> +static int audioreach_widget_ready(struct snd_soc_component *component,
>> +				   int index, struct snd_soc_dapm_widget *w,
>> +				   struct snd_soc_tplg_dapm_widget *tplg_w)
>> +{
>> +	switch (w->id) {
>> +	case snd_soc_dapm_aif_in:
>> +	case snd_soc_dapm_aif_out:
>> +		audioreach_widget_load_buffer(component, index, w, tplg_w);
>> +		break;
>> +	case snd_soc_dapm_decoder:
>> +	case snd_soc_dapm_encoder:
>> +	case snd_soc_dapm_src:
>> +		audioreach_widget_load_enc_dec_cnv(component, index, w, tplg_w);
>> +		break;
>> +	case snd_soc_dapm_buffer:
>> +		audioreach_widget_load_buffer(component, index, w, tplg_w);
>> +		break;
>> +	case snd_soc_dapm_mixer:
>> +		return audioreach_widget_load_mixer(component, index, w, tplg_w);
>> +	case snd_soc_dapm_pga:
>> +		return audioreach_widget_load_pga(component, index, w, tplg_w);
>> +	case snd_soc_dapm_dai_link:
>> +	case snd_soc_dapm_scheduler:
>> +	case snd_soc_dapm_out_drv:
>> +	default:
>> +		dev_err(component->dev, "Widget type (0x%x) not yet supported\n", w->id);
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
> 
> spurious newline
> 
>> +static int audioreach_widget_unload(struct snd_soc_component *scomp,
>> +				    struct snd_soc_dobj *dobj)
>> +{
>> +	struct snd_soc_dapm_widget *w = container_of(dobj, struct snd_soc_dapm_widget, dobj);
>> +	struct q6apm *apm = dev_get_drvdata(scomp->dev);
>> +	struct audioreach_container *cont;
>> +	struct audioreach_module *mod;
>> +
>> +	mod = dobj->private;
>> +	cont = mod->container;
>> +
>> +	if (w->id == snd_soc_dapm_mixer) {
>> +		/* virtual widget */
>> +		kfree(dobj->private);
>> +		return 0;
>> +	}
>> +
>> +	mutex_lock(&apm->lock);
>> +	idr_remove(&apm->modules_idr, mod->instance_id);
>> +	cont->num_modules--;
>> +
>> +	list_del(&mod->node);
>> +	kfree(mod);
>> +
>> +	if (list_empty(&cont->modules_list)) { /* remove container */
>> +		struct audioreach_sub_graph *sg = cont->sub_graph;
>> +
>> +		idr_remove(&apm->containers_idr, cont->container_id);
>> +		list_del(&cont->node);
>> +		sg->num_containers--;
>> +		kfree(cont);
>> +		if (list_empty(&sg->container_list)) { /* remove sg */
>> +			struct audioreach_graph_info *info = sg->info;
>> +
>> +			idr_remove(&apm->sub_graphs_idr, sg->sub_graph_id);
>> +			list_del(&sg->node);
>> +			info->num_sub_graphs--;
>> +			kfree(sg);
>> +			if (list_empty(&info->sg_list)) { /* remove graph info */
>> +				idr_remove(&apm->graph_info_idr, info->id);
>> +				kfree(info);
>> +			}
>> +		}
>> +	}
> 
> It's not very clear if the nested removes actually free-up everything?
> You may want to add a comment on the hierarchy.
I will remove the module and check if the container is empty and then 
remove the container and checks if sub-graph is empty and then removes 
subgraphs.
Hierarchy was mentioned in cover letter, but I will add some comment 
here for more clarity to readers.
> 
>> +
>> +	mutex_unlock(&apm->lock);
>> +
>> +	return 0;
>> +}
>> +
> 
>> +int audioreach_tplg_init(struct snd_soc_component *component)
>> +{
>> +	struct snd_soc_card *card = component->card;
>> +	struct device *dev = component->dev;
>> +	const struct firmware *fw;
>> +	char tplg_fw_name[128];
>> +	int ret;
>> +
>> +	/* Inline with Qualcomm UCM configs and linux-firmware path */
>> +	snprintf(tplg_fw_name, sizeof(tplg_fw_name), "qcom/%s/%s-tplg.bin", card->driver_name,
>> +		 card->name);
> 
> use kasprintf instead of fixed 128-char array?
I moved this to kasprintf in next version.

> 
> Also you should use a qcom/audioreach/ prefix to possible interference
> with other parts of qcom...
> 

So Qualcomm linux-firmwares are arranged something like

qcom/sdm845/*
qcom/sm8250/*
qcom/sm8150/*

and UCM something like 
this:Qualcomm/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0.conf

Qualcomm/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0.conf


Atleast in Qualcomm soundcard case we have driver name set to SoC name 
and we tend to reuse this driver across multiple platforms.

second part card name actually is from model device tree property, in 
this case which is "Qualcomm-RB5-WSA8815-Speakers-DMIC0"

so we will endup looking for
/lib/firmare/qcom/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0-tplg.bin

AFAIU, it should not interface with any other qcom parts.

for Other qcom parts this model will change so the topology file name.


>> +
>> +	ret = request_firmware(&fw, tplg_fw_name, dev);
>> +	if (ret < 0) {
>> +		dev_info(dev, "loading %s failed %d, falling back to dfw-audioreach-tplg.bin\n",
>> +			 tplg_fw_name, ret);
>> +		/* default firmware */
>> +		ret = request_firmware(&fw, "dfw-audioreach-tplg.bin", dev);
>> +		if (ret < 0) {
>> +			dev_err(dev, "tplg fw dfw-audioreach-tplg.bin load failed with %d\n", ret);
> 
> the dfw prefix isn't very helpful...Intel's example of "dfw_sst.bin" is
> a historical reference, not something you should reuse.

Rethinking on this once again, Am not sure if it even makes sense to 
support this default setup. It will be very hard to get a working 
defalut tplg on every platform. So am planning to remove this in next 
version.

Do you see any issues?

--srini
> 
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = snd_soc_tplg_component_load(component, &audioreach_tplg_ops, fw);
>> +	if (ret < 0) {
>> +		dev_err(dev, "tplg component load failed%d\n", ret);
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	release_firmware(fw);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(audioreach_tplg_init);
>>