Skip to content

Conversation

@jsarha
Copy link
Contributor

@jsarha jsarha commented Mar 7, 2023

"topology2: gain: Add "name_no_prefix" attribute to gain class" the main commit here. "topology2: cavs-mixin-mixout-hda: Rename mixin-mixout volumes, no prefix" is an example how to use the new feature (and the old variable value expansion), but in it self its a serious attempt to fix the mixer naming problems found in cavs-mixin-mixout-hda.conf.

The associated Linux driver PR is needed this PR to work.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart fyi.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart pls review.

@jsarha
Copy link
Contributor Author

jsarha commented Mar 30, 2023

Set ready for review to get more attention.

@jsarha jsarha marked this pull request as ready for review March 30, 2023 09:29
@jsarha jsarha requested a review from ranj063 as a code owner March 30, 2023 09:29
@lgirdwood
Copy link
Member

@plbossart does this align with your direction ?

@jsarha jsarha changed the title [RFC] Gain no name prefix Gain no name prefix Apr 4, 2023
@jsarha jsarha force-pushed the gain_no_name_prefix branch from ec72f28 to 6e3db35 Compare April 5, 2023 15:47
@jsarha

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not following, the first part of this patch adds 'use_topology_name' but this sets 'name_no_prefix'

I don't see the link between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The both mean "do not add the widget name as a prefix to the kcontrol name that is visible in the alsa mixer, e.g. use the topology name only". However, on the Linux side referring to topology is not very accurate as the widget may not be coming from a topology file at all, but from some ASoC component driver for instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am also lost, we have both name_no_prefix and use_topology_name, but they are the same concept?

Why can't we have just one token added?

Copy link
Contributor Author

@jsarha jsarha May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got me there. I've been going back and forth with the names according to suggestions from different directions, and that was the original that I forgot to update. But I think I'll go now back to name_no_prefix everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsarha shouldn't this be a mixer control attribute instead of a gain? WHat happens if different component has a mixer control?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsarha shouldn't this be a mixer control attribute instead of a gain? What happens if different component has a mixer control?

Now that you said it @ranj063 , it would indeed make more sense to put the attribute into mixer class, but I have no idea where I can find the attribute value in the Linux driver side, if its not in the widget object. So far I can not find any place where mixer object tuples are used.

Well, I'll probably figure it out after a day or two of reverse-engineering, but all help is appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre- and post mix is fine, but I don't get what ANALOG means here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That refers to the PCM this pipeline is going to.

@sys-pt1s
Copy link

sys-pt1s commented Apr 6, 2023

Can one of the admins verify this patch?

@lgirdwood
Copy link
Member

@ranj063 any inputs ?

@lgirdwood
Copy link
Member

@jsarha @ranj063 any updates on alignment here ?

@jsarha
Copy link
Contributor Author

jsarha commented May 8, 2023

@jsarha @ranj063 any updates on alignment here ?

I am just hoping someone would review this or say that I am on a wrong track. @ranj063, @plbossart , @juimonen what do you say? Oh, and please remember that this PR requires thesofproject/linux#4226 for this to work.

@jsarha jsarha force-pushed the gain_no_name_prefix branch from 6e3db35 to 186e3c7 Compare May 8, 2023 21:15
@jsarha
Copy link
Contributor Author

jsarha commented May 8, 2023

Oh, just noticed that there was a conflict. Rebased the code and fixed it so all should be good for reviewing again.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as lost as the matching kernel PR @jsarha, sorry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am also lost, we have both name_no_prefix and use_topology_name, but they are the same concept?

Why can't we have just one token added?

@jsarha jsarha force-pushed the gain_no_name_prefix branch from 186e3c7 to 3ff77ef Compare May 11, 2023 22:08
@jsarha jsarha requested review from dbaluta, lbetlej and mmaka1 as code owners May 11, 2023 22:08
@jsarha
Copy link
Contributor Author

jsarha commented May 11, 2023

@plbossart and @ranj063 , new version pushed. Tried to address Pierre's comments.

@jsarha jsarha marked this pull request as draft May 17, 2023 22:00
@jsarha
Copy link
Contributor Author

jsarha commented May 17, 2023

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: #7641

I will continue with this one on lower priority, in parallel with the mixer renaming part on higher priority.

Adds "no_wname_in_kcontrol_name" boolean attribute to gain class, with
"true" as default value. With this change no mixer name will have widget
name added to it. For example "gain.2.1 Post Mixer Analog Playback Volume"
becomes just "Post Mixer Analog Playback Volume".

If for some reason the widget name is needed in the mixer name, then this
behavior can be reverted by setting "no_wname_in_kcontrol_name" to true
in the topology gain node in question.

Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
@jsarha jsarha force-pushed the gain_no_name_prefix branch from 3ff77ef to 36edc12 Compare June 19, 2023 21:20
@jsarha jsarha marked this pull request as ready for review June 19, 2023 21:31
@jsarha
Copy link
Contributor Author

jsarha commented Jun 19, 2023

@plbossart and @ranj063 there is now a new version of this PR. Not much has changed on the topology side. Just dropped the mixer name changes, changed the gain attribute name to more specific, and changed its value to default to true.

However, now that I am this far I think that we probably need this property for IirEQ, FirEQ, and SmartAmp widgets too (are there more modules with associated kcontrols), to make the kcontrol names uniform. Should probably make another PR for changing the names of those entities to follow the scheme we now use in the mixer names.

The driver side has more changes: thesofproject/linux#4226

@jsarha jsarha marked this pull request as draft June 28, 2023 21:00
@jsarha jsarha closed this Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants