Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify BYT/CHT/BDW card names when used with sof #2021

Merged
merged 3 commits into from
May 29, 2020

Conversation

plbossart
Copy link
Member

Follow-up on @perexg 's suggestion to use simpler names. I removed the byt/cht mention since it's confusing to have a 'byt' card on a 'cht' machine and it adds no value to the user.

I hope it doesn't break all the CI tests...

@ranj063
Copy link
Collaborator

ranj063 commented Apr 16, 2020

looks good except for the one blip

ranj063
ranj063 previously approved these changes Apr 16, 2020
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

@plbossart This feels a bit strange to have nothing about the platform in the card name. E.g. if you have a non-Intel device using SOF and e.g. rt5640 codec, the same UCM file would probably not apply.

Hmm, of course, with limited space, maybe first-come-first-served applies here and in the case another SOF card with rt5650 comes, they will just have to pick another name. I guess this is the intention?

@plbossart
Copy link
Member Author

@plbossart This feels a bit strange to have nothing about the platform in the card name. E.g. if you have a non-Intel device using SOF and e.g. rt5640 codec, the same UCM file would probably not apply.

Hmm, of course, with limited space, maybe first-come-first-served applies here and in the case another SOF card with rt5650 comes, they will just have to pick another name. I guess this is the intention?

total budget is 16 chars:
sof- 4 chars
-rt5640\0: 8 chars
-cx2072x\0: 9 chars
-max98090\0: 10 chars

that leaves 2 chars worst case for the platform definition, so to remain consistent I omitted it. I felt it was better to avoid truncating the codec name than add something specific about Intel.
Also if the device has a DMI name it can be used for UCM, the driver name is the fallback.
Anyways, all these codecs are somewhat old so the odds of a non-Intel SOF-based design are slim at best.

makes sense?

@perexg
Copy link

perexg commented Apr 17, 2020

The question is, if the driver name should contain those details like codec and platform. The driver name might be like "sof" and you can put details to the standard card name. The conditions in ucm2 can handle the rest (we may even add lazy load to optimize config loading later).

@plbossart
Copy link
Member Author

The question is, if the driver name should contain those details like codec and platform. The driver name might be like "sof" and you can put details to the standard card name. The conditions in ucm2 can handle the rest (we may even add lazy load to optimize config loading later).

@perexg what do you mean by the standard card name? we set the name once in the snd_soc_card structure, we don't have another mechanism, do we? Wondering if you are talking about extending the ASoC framework here?

@plbossart
Copy link
Member Author

The question is, if the driver name should contain those details like codec and platform. The driver name might be like "sof" and you can put details to the standard card name. The conditions in ucm2 can handle the rest (we may even add lazy load to optimize config loading later).

@perexg what do you mean by the standard card name? we set the name once in the snd_soc_card structure, we don't have another mechanism, do we? Wondering if you are talking about extending the ASoC framework here?

Answering my own question, we do...

struct snd_soc_card {
	const char *name;
	const char *long_name;
	const char *driver_name;

That said why would the name become the driver_name....

@perexg
Copy link

perexg commented Apr 17, 2020

Yes, you can set different name via snd_soc_card->driver_name. Appearently, this field is NULL in your case, thus snd_soc_bind_card() will use the snd_soc_card->name field for the driver name.

@plbossart
Copy link
Member Author

Yes, you can set different name via snd_soc_card->driver_name. Appearently, this field is NULL in your case, thus snd_soc_bind_card() will use the snd_soc_card->name field for the driver name.

Just to get the character limitation right, would this mapping be correct?

snd_soc_card.driver_name ->	unsigned char driver[16];	/* Driver name */
   would be "SOF"
snd_soc_card.name ->	unsigned char name[32];		/* Short name of soundcard */
   would be sof-bytcht-rt5640
snd_soc_card.long_name ->	unsigned char longname[80];	/* name + info text about soundcard */
   not touched, so would be sof-bytcht-rt5640 or a DMI-based name, if present.

This one I don't know, would it be based on the driver name?
	unsigned char id[16];		/* ID of card (user selectable) */

@plbossart
Copy link
Member Author

@perexg I did the suggested fixes so now things look better:

root@Zotac:/sys/class/sound/card0# cat /proc/asound/cards
 0 [sofbytchtrt5640]: SOF - sof-bytcht-rt5640
                      ZOTAC-XXXXXX-XX-CherryTrailFFD

The ID still looks horrible, if it's build automatically from the name, we need to require that the name be unique enough for the first 15 chars.

@perexg
Copy link

perexg commented Apr 17, 2020

ID is an alias to the card number which may be set via the module parameters (another missing part in ASoC). This string is expected to be user configurable (to address the cards in the system more nicely). Unfortunately, because you use '-' instead space for the card name, the mechanism which picks the last word from the card name does not work like for other drivers.

@plbossart
Copy link
Member Author

ID is an alias to the card number which should be set via the module parameters (another missing part in ASoC). This string is expected to be user configurable (to address the cards in the system more nicely). Unfortunately, because you use '-' instead space for the card name, the mechanism which picks the last word does not work like for other drivers.

I can add a space for the codec name, that's no issue. I don't think anyone was aware of that space requirement :-)

But I must admit I am lost here. Earlier you wrote "For the filename lookup, only the driver and longname strings are used (longname as first)."

so in this case, I have a longname which will never have any UCM match, so we will fallback to the driver name which has become too generic. It's unmanageable if we need to deal with all baytrail/cherrytrail configurations in a single directory?

@plbossart
Copy link
Member Author

plbossart commented Apr 17, 2020

@perexg adding a space is magical :-)

root@Zotac:~# aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: rt5640 [sof-bytcht rt5640], device 0: Low Latency (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 0: rt5640 [sof-bytcht rt5640], device 1: Media Playback 1 (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
root@Zotac:~# arecord -l
**** List of CAPTURE Hardware Devices ****
card 0: rt5640 [sof-bytcht rt5640], device 0: Low Latency (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
root@Zotac:~# cat /proc/asound/cards 
 0 [rt5640         ]: SOF - sof-bytcht rt5640
                      ZOTAC-XXXXXX-XX-CherryTrailFFD

so now my only concern is how the UCM file is found (order between long card name, short card name, driver name).

@perexg
Copy link

perexg commented Apr 20, 2020

I would create SOF/SOF.conf and conditionaly include verb configs from there. You can add one directory level. Ideally based on the string with the components (codec IDs) like we settled for soundwire.

syntax 2
If.bytcht_rt5640 {
   Condition {
      Type String
      Haystack "${CardComponents}"
      Needle "plat:bytcht codec0:rt5640"
   }
   True {
      <SOF/bytcht-rt5640/bytcht-rt5640.conf>
   }
}

or use the card name:

syntax 2
If.bytcht_rt5640 {
   Condition {
      Type String
      Haystack "${CardName}"
      Needle "bytcht rt5640"
   }
   True {
      <SOF/bytcht-rt5640/bytcht-rt5640.conf>
   }
}

We have similar situation for USB / legacy HDA where the driver name is common for all variants (codecs, devices) and we need to do the split based on other information than driver name / long name (DMI name for ASoC).

@plbossart
Copy link
Member Author

plbossart commented Apr 21, 2020

@perexg I hit a snag with the UCM include files.
What I wanted to do is this in SOF/SOF.conf

If.bytcht_rt5640 {
	 Condition {
	 	Type String
		Haystack "${CardName}"
		Needle "bytcht rt5640"
	}
	True {
		<bytcr-rt5640/bycr-rt5640.conf>
	}
}

in other words, load the legacy config - which would be tested to see if its used in legacy mode or not. This fails because there are hard-coded paths in alsa-ucm. isn't there a way to specify a local path?

@plbossart
Copy link
Member Author

The matching UCM configurations are here: alsa-project/alsa-ucm-conf#20

@plbossart plbossart requested review from perexg and ranj063 May 23, 2020 19:47
@plbossart plbossart requested a review from kv2019i May 23, 2020 19:47
@plbossart
Copy link
Member Author

@ranj063 @kv2019i can you take a look, I'd like to send all Baytrail stuff upstream before the merge window.

Blindly adding an sof- prefix to the card name is not user friendly
and causes UCM issues with a driver name truncated to 16 characters.

Simplify to use "sof-bytcht <codec_name>" pattern for all byt* machine
drivers. The sof- prefix is added by the core. A generic "SOF" driver
name is used, and UCMv2 will detect the configuration for this driver
by testing the card name.

Legacy uses are unmodified.

Suggested-by: Jaroslav Kysela <perex@perex.cz>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Blindly adding an sof- prefix to the card name is not user friendly
and causes UCM issues with a driver name truncated to 16 characters.

Simplify to use "sof-bytcht <codec_name>" pattern for all cht* machine
drivers. The sof- prefix is added by the core. A generic "SOF" driver
name is used, and UCMv2 will detect the configuration for this driver
by testing the card name.

Legacy uses are unmodified.

Suggested-by: Jaroslav Kysela <perex@perex.cz>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Blindly adding an sof- prefix to the card name is not user friendly
and causes UCM issues with a driver name truncated to 16 characters.

Simplify to use "sof-bdw <codec_name>" pattern for all Broadwell
machine drivers. The sof- prefix is added by the core. A generic "SOF"
driver name is used, and UCMv2 will detect the configuration for this
driver by testing the card name.

Legacy uses are unmodified.

Suggested-by: Jaroslav Kysela <perex@perex.cz>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
Copy link
Member Author

@kv2019i @ranj063 @bardliao need your reviews, this PR works nicely with the updated UCM2 files so we should really push this upstream ASAP.

Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@kv2019i
Copy link
Collaborator

kv2019i commented May 29, 2020

Ack @plbossart , looks good to go.

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.

None yet

6 participants