Kcontrol name no prefix #4226
Kcontrol name no prefix #4226
Conversation
jsarha
force-pushed
the
kcontrol_name_no_prefix
branch
from
7f794a3 to
2469872
Compare
sound/soc/soc-dapm.c
Outdated
There was a problem hiding this comment.
The two branch does the same thing?
if (shared || w->kcontrol_name_no_prefix)?
There was a problem hiding this comment.
would this work similarly, but earlier:
if (w->kcontrol_name_no_prefix) {
prefix_len = 0;
} else {
prefix = soc_dapm_prefix(dapm);
if (prefix)
prefix_len = strlen(prefix) + 1;
else
prefix_len = 0;
}Answer: No, the prefix is not this prefix.
There was a problem hiding this comment.
@jsarha, so the flag is ignore_wname_in_long_name
and you can just simply:
if (w->ignore_wname_in_long_name)
wname_in_long_name = false;There was a problem hiding this comment.
Changed "ignore_wname_in_long_name" to "no_wname_in_long_name", to make the flag name a bit shorter, but otherwise applied.
sound/soc/sof/topology.c
Outdated
There was a problem hiding this comment.
get_token_nop() / nop_get_token() The get_token_void() is hard to put in place, it is to get void data?
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
is it more like drop_kcontrol_prefix in essense, or ingore_prefix?
jsarha
force-pushed
the
kcontrol_name_no_prefix
branch
from
2469872 to
50193c5
Compare
|
I'll make this ready for review, if it would then get more attention. |
jsarha
requested review from
ajitkupandey,
bardliao,
dbaluta,
juimonen,
kuanhsuncheng,
kv2019i,
lgirdwood,
lyakh,
plbossart,
ranj063,
singalsu and
yaochunhung
as code owners
|
@jsarha I must admit I've completely lost track of this PR. I know I was the one person who asked for the kcontrol names to be simpler with thesofproject/sof#6576, but I just don't connect the dots with the 'no prefix' part and the ASoC details. Would you mind adding in the commit messages a description of the 'before' and 'after' names, so that we can all agree on the start point and the result? Thanks! |
jsarha
force-pushed
the
kcontrol_name_no_prefix
branch
from
50193c5 to
cff08c2
Compare
@plbossart , I hope the updated last commit message explains better what this series does. The other changes are a rebase on top of latest sof-dev and a minor typo fix in the second commit message. |
|
Ping @plbossart and @ranj063 ? |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
topology-related changes are very hard to understand for the average developer @jsarha
I could not figure out what some of the patches try to improve and the consistency between code and commit message can be improved a great deal.
sound/soc/sof/topology.c
Outdated
There was a problem hiding this comment.
I don't understand the commit message at all.
"
no need to assign the tuple value anywhere, but the
tuple should be copied to struct snd_sof_widget member.
"
what is the difference between assign and copy? I don't get the issue at all.
There was a problem hiding this comment.
I try to explain in more detail.
There was a problem hiding this comment.
no luck, this is still very confusing. I don't know the details of the topology and I don't need to learn about them, the patch commit message should be high-level enough to describe the functionality.
In this case, I think using the same 'get_type' naming is very misleading, since the type is irrelevant.
There was a problem hiding this comment.
this doesn't appear to have changed, and all 3 reviewers didn't like the code, so this probably needs more work?
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
I am really confused here.
Is this the case that we provide "a full control over the user visible mixer name" ?
The example just seems to drop the prefix, that's hardly full control.
There was a problem hiding this comment.
We already had the full control except for the prefix, that is always added, if the newly added flag is not set. The rest of the name comes from topology2 mixer object's "name"-attribute:
The point of this series is to provide means to get rid of the non sense widget name e.g. "gain.2.1" in the beginning of the user visible mixer name.
There was a problem hiding this comment.
suggested edit to the commit message
Add SOF_TKN_GAIN_NAME_NO_PREFIX token, sof_topology_token entry in
gain_tokens array, and the associated code in sof_ipc4_widget_setup_comp_pga()
to set no_wname_in_long_name flag in the associated snd_soc_dapm_widget.
In practice setting the "name_no_prefix" attribute in the gain node
means that the widget name is not added as a
prefix to the mixer name.
E.g. "gain.2.1 Post Mixer Analog Playback Volume" becomes just
"Post Mixer Analog Playback Volume".
The decision to skip the prefix addition is made when the topology binary file is created. The topology can be modified on a need-basis to make the control names more user-friendly.
There was a problem hiding this comment.
this part needs to go to sof_widget_ready()
There was a problem hiding this comment.
nothing seems to have changed here either, so this is probably not quite ready for a merge.
Add no_wname_in_long_name flag to struct snd_soc_dapm_widget, and the logic in dapm_create_or_share_kcontrol() to not to add widget name as prefix for kcontrol name if the flag is set. Without this flag new kcontrols will have a name that has the widget name in the beginning followed by the name given in the struct snd_kcontrol_new. Some times the widget name is not very informative and this flag gives the driver an option to drop it if the name from the snd_kcontrol_new good as it is. Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
Add get_token_void() to be used in struct sof_topology_token arrays, when there is no need to assign the tuple value anywhere, but the tuple should be copied to struct snd_sof_widget member. sof_widget_parse_tokens() only copies the tuples into struct snd_sof_widget's tuples array if the token is found from some of the widget's struct sof_topology_token arrays. The struct sof_topology_token has four members, u32 token, u32 type, get_token function pointer, and u32 offset. The function pointer is supposed to copy the tuple value of the matching token at the offset in object pointer given to it as a parameter. Sometimes there is no real need to copy the value anywhere, but the still the tuple (token and value pair) is needed in the struct snd_sof_widget's tuples array. So this function helps in such a case. With get_token_void() there can be an entry in the struct sof_topology_token array that does not write anything to the given object. Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
Add SOF_TKN_GAIN_NAME_NO_PREFIX token, sof_topology_token entry in gain_tokens array, and the associated code in sof_ipc4_widget_setup_comp_pga() to set no_wname_in_long_name flag in the associated snd_soc_dapm_widget. In practice setting the "name_no_prefix" attribute in the gain node means that the systematically assigned widget name is not added as a prefix the mixer name. E.g. "gain.2.1 Post Mixer Analog Playback Volume" becomes just "Post Mixer Analog Playback Volume". This gives the topology mixer object's "name"-attribute a full control over the user visible mixer name, without anything extra added to it. Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
jsarha
force-pushed
the
kcontrol_name_no_prefix
branch
from
cff08c2 to
970f8c1
Compare
|
@plbossart and @ranj063 , new version pushed. Tried to address Pierre's comments. |
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
suggested edit to the commit message
Add SOF_TKN_GAIN_NAME_NO_PREFIX token, sof_topology_token entry in
gain_tokens array, and the associated code in sof_ipc4_widget_setup_comp_pga()
to set no_wname_in_long_name flag in the associated snd_soc_dapm_widget.
In practice setting the "name_no_prefix" attribute in the gain node
means that the widget name is not added as a
prefix to the mixer name.
E.g. "gain.2.1 Post Mixer Analog Playback Volume" becomes just
"Post Mixer Analog Playback Volume".
The decision to skip the prefix addition is made when the topology binary file is created. The topology can be modified on a need-basis to make the control names more user-friendly.
sound/soc/sof/topology.c
Outdated
There was a problem hiding this comment.
no luck, this is still very confusing. I don't know the details of the topology and I don't need to learn about them, the patch commit message should be high-level enough to describe the functionality.
In this case, I think using the same 'get_type' naming is very misleading, since the type is irrelevant.
| unsigned char power_checked:1; /* power checked this run */ | ||
| unsigned char is_supply:1; /* Widget is a supply type widget */ | ||
| unsigned char is_ep:2; /* Widget is a endpoint type widget */ | ||
| unsigned char no_wname_in_long_name:1; /* No widget name prefix in kcontrol name */ |
There was a problem hiding this comment.
I think no_wname_in_kcontrol_name makes more sense.
There was a problem hiding this comment.
I guess the naming was selected based on existing variable names, such as
wname_in_long_name = false;
kcname_in_long_name = true;??
| int get_token_dai_type(void *elem, void *object, u32 offset); | ||
| int get_token_uuid(void *elem, void *object, u32 offset); | ||
| int get_token_string(void *elem, void *object, u32 offset); | ||
| int get_token_void(void *elem, void *object, u32 offset); |
There was a problem hiding this comment.
@jsarha this is unnecessary. You dont really need to parse the token anywwhere at all. Instead what you can do in in sof_widget_ready() in topology.c, iterate though the list of tuples in the swidget's tuples-array, find the token you are interested in and set the field in swidget based on that
There was a problem hiding this comment.
just to be clearer, this token is IPC agnostic. So why restrict it to IPC4? it should go to the common topology parser
|
Ranjani pointed out quite correctly that the name_no_prefix should come from topoplogy mixer object not from gain object. After banging my head against the wall for two days and still not getting the mixer attribute to work, I decided that its time to push the no_prefix part back on proceed with the mixer renaming separately. There is a new PR here: thesofproject/sof#7641 I will continue with this one on lower priority, in parallel with the mixer renaming part on higher priority. |
Now that I finally know how I could make the no_prefix property a mixer property, I am not any more sure its a good idea. On the topology and sof driver side there is no problem. However, in the ASoC / ALSA side the flag would need to go to "struct snd_kcontrol_new" [1], but there is no behavioral flags there at the moment. I am a little hesitant to start adding them there as we do not really need that. I am leaning applying the comments here but keeping the flag on the widget side, and making the topology attribute to default to not adding the prefix. @ranj063 and @plbossart what do you say? [1] Line 47 in 9fd7ca4 |
Not following at all, sorry. |
|
@plbossart , the question is whether to put the topology attribute in to mixer object and the flag in the driver side in to "struct snd_kcontrol_new". Or if to keep the topology attribute in the gain widget object and the flag in "struct snd_soc_dapm_widget" as they are in this PR ATM. I am leaning towards the later simply because I intuitively dislike the idea of adding the flag in "struct snd_kcontrol_new", and because we do not need per mixer control of kcontrol name prefixes, per widget is quite good enough. However, I can easily go either way. |
|
this is all way beyond my comfort zone, best to see what @ranj063 thinks of all this. |
|
@ranj063 and @plbossart , to make things more concrete I updated the PR and now all the comment this far should be addressed, apart from moving the attribute to topology mixer object. Please look the updated PR and my latest comment from topology side too: thesofproject/sof#7227 |
| unsigned char power_checked:1; /* power checked this run */ | ||
| unsigned char is_supply:1; /* Widget is a supply type widget */ | ||
| unsigned char is_ep:2; /* Widget is a endpoint type widget */ | ||
| unsigned char no_wname_in_long_name:1; /* No widget name prefix in kcontrol name */ |
There was a problem hiding this comment.
I guess the naming was selected based on existing variable names, such as
wname_in_long_name = false;
kcname_in_long_name = true;??
| unsigned char power_checked:1; /* power checked this run */ | ||
| unsigned char is_supply:1; /* Widget is a supply type widget */ | ||
| unsigned char is_ep:2; /* Widget is a endpoint type widget */ | ||
| unsigned char no_wname_in_long_name:1; /* No widget name prefix in kcontrol name */ |
There was a problem hiding this comment.
commit message: the last sentence is missing something, the grammar does not work.
this flag gives the driver an option to drop it if the name
from the snd_kcontrol_new is meaningful enough.
sound/soc/sof/topology.c
Outdated
There was a problem hiding this comment.
this doesn't appear to have changed, and all 3 reviewers didn't like the code, so this probably needs more work?
| {SOF_TKN_GAIN_VAL, SND_SOC_TPLG_TUPLE_TYPE_WORD, | ||
| get_token_u32, offsetof(struct sof_ipc4_gain_data, init_val)}, | ||
| {SOF_TKN_GAIN_NAME_NO_PREFIX, SND_SOC_TPLG_TUPLE_TYPE_BOOL, | ||
| get_token_void, 0}, |
There was a problem hiding this comment.
why is this limited to gains? Could this not be generalized to e.g. EQs?
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
nothing seems to have changed here either, so this is probably not quite ready for a merge.
4 participants
I've been trying to find a clean and elegant way to do this for a while now. Not sure if this is it. I still have trouble in seeing the big picture on how the dapm widgets are pulled together by SOF driver based on the used topology. But at least this is not very intrusive, when it comes to generic ASoC code. @ranj063 , @lgirdwood, @ujfalusi what do you think?
This is associated with this topology PR: thesofproject/sof#7227