-
Notifications
You must be signed in to change notification settings - Fork 2
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
[RFC][WIP] Dmic nhlt parse #4
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work @juimonen - mostly needs a cleanup and more comments around the code blocks to describe what we are doing. It really has to be clear from comments too so everyone who has never heard of NHLT will know what its doing and also why.
@beatabaranowska fyi - please feel free to comment or ask questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random comments based on my superficial understanding of all things NHLT.
topology/intel/dmic-nhlt.c
Outdated
struct dmic_calc_decim_modes { | ||
int16_t clkdiv[DMIC_MAX_MODES]; | ||
int16_t mcic[DMIC_MAX_MODES]; | ||
int16_t mfir[DMIC_MAX_MODES]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INTEL_DMIC?
topology/intel/dmic-nhlt.c
Outdated
uint32_t pdmsm; | ||
uint32_t reuse_fir_from_pdm; | ||
uint32_t reserved1[2]; | ||
struct dmic_intel_fir_config fir_config[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 -> define?
@plbossart thanks for the comments, they are good. Kind of nasty crossfire here as I need to use nhlt (kernel internal header) + sof dmic headers and calculations. Exactly I was thinking to replicate the code directly from sof, but it brings in files with many not needed macros and functions i.e. the dmic code for this part in sof is not done to be easily exported. And Liam wanted me to clean it up -> so it maybe comes cleaner/neater, but I will miss the portability with sof. But I need to think about it, the separate licensing and exact replica of sof code kind of sounds good to me, because I need to fix/maintain this. The SSP is still in development, not even compiling it yet, tried to explain that poorly in PR comment. |
8111deb
to
ebe7dfc
Compare
updated with changes:
|
and I tried to close the comments I've addressed, those not addressed should be still open. |
ebe7dfc
to
97022e6
Compare
@beatabaranowska ping ? |
@lgirdwood I have task with priority. would try next week. |
@lgirdwood basic SSP support is now in this updated PR. However, it is supporting the single duplex SSP endpoint we currently have in our test topologies (at least I 've been able to run SSP playback with it). I need to add support for multiple SSP endpoints and clean up the SSP code (mostly as I have some sof definitions there). So dmic has always 1 nhlt endpoint, but SSP can have several, hence the difference. |
@juimonen this is because in nhlt we provide dmic (the intel one) so one edpoint. and ssp depends the number of ports. currently 6 I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just quick review. not finished yet.
topology/intel/nhlt.h
Outdated
|
||
#define NHLT_VENDOR_ID_INTEL 0x8086 | ||
|
||
#define NHLT_DEVICE_ID_INTEL_PDM_DMIC 0xAE20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be read from link type and device type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry didn't get this... If I am creating the nhlt blob for Intel hw and dmic, shouldn't set these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! Some unnecessary code to remove, maybe improve debug prints.
c7ba1a6
to
9c5379b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK but please remove the rest of gain ramp related stuff.
9c5379b
to
c627d53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split into more patches i.e. one for nhtl core, one for ssp, one for dmic. This will make it easier for Jaroslav to review.
We also need to explain very early on in the commit message that the NHLT blobs are really register programming values that will be consumed by firmware based on topology data. This explains why the code looks like a driver.
c627d53
to
8a70bc8
Compare
8a70bc8
to
5d59814
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that Jaroslav likes to see is a config example before each parse function as a comment i.e.
/*
* This function parses the following config items.
*
* config_blah1 {
* data1 [
* item1
* ]
* }
*/
int parse_blah1()
{}
Every function that parses any data should have this. If the parsing is done by other code than instead I would paste an example of each NHLT data item in topology format so Jaroslav can see whet is the input and how we convert to output.
d43fa0c
to
3403ea4
Compare
ping |
3403ea4
to
7e17f09
Compare
new version:
still to check:
|
48cae7c
to
7cc8af5
Compare
7cc8af5
to
b4ab447
Compare
@ranj063 easy comments fixed, I need to little bit think&test the open ones... |
1d14380
to
137427f
Compare
@libinyang fyi - PR for the NHLT work you are using. |
9538044
to
cb7e859
Compare
@juimonen is this still WIP ? |
@lgirdwood we should pretty close, I'll do final checking and remove the WIP end of week... |
My last status is: I tested it on tgl-h platform. DMIC works while SSP not. And I don't have change to hear the real recorded file. I will check the sound today and update the status. |
It seems my tgl-h platform is something wrong. DMIC blob only records the noise. And I test the daily built images on my tgl-h platform, there is still only noise. So it should not be the DMIC blob issue. |
@RanderWang can you review the SSP calculation in this PR? It is a "mix" from sof fw code and your ssp blob generation code (so it has still all other SSP formats supported besides TDM/DSP_B). |
@juimonen is there a specific reason why you're using the 'sof' master instead of the upstream one? 4 out of 6 commits are upstream already. |
tplg_pp_debug("create_nhlt_blob ep count %d total size %u", eps_count, nhlt_size); | ||
|
||
/* allocate the bytes string, every byte is alsa conf byte string format "0xA1," */ | ||
bytes_string_buffer = calloc(nhlt_size * 5, sizeof(uint8_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the answer above as a comment
All rights reserved. | ||
|
||
This program is free software; you can redistribute it and/or modify | ||
it under the terms of version 2 of the GNU General Public License as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ssp code is lifted from Sound Open Firmware (sof) code base, thus it
will have BSD-3 license."
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ssp blob generation code is lifted" then maybe... so the intel specific blob thing is from sof, generic acpi nhlt is just new from me.
goto err; | ||
eps_count++; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should really have a comment in the earlier code that tells eps_count is modified in future patches... Or move the eps_count to when it's actually used, otherwise static analysis will have a field day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yes, I'll run the patches through some tool and try to fix this
cb7e859
to
002088a
Compare
int ret; | ||
|
||
/* add the nhlt bytes as new config into existing SectionData*/ | ||
ret = snd_config_search(tplg_pp->output_cfg, "SectionData", &top_data_section); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use tplg_create_config_template() to create the Data config with bytes in it?
continue; | ||
|
||
/* item already exists */ | ||
if (!strcmp(name, data_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work? so you have one NHLT blob for types of DAI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is just 1 acpi nhlt blob, that might contain 1 to many nhlt endpoints (DAI configs). So I'm just trying to add 1 data element into manifest, nothing more. I think the code is a copy from some other part from topology2 processing, can be changed if there's standard way of adding data element.
return -EINVAL; | ||
|
||
if(tplg_pp->private_data) | ||
free(tplg_pp->private_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you dont seem to allocate preivate_data in init()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocated in nhlt_dmic_init_params(tplg_pp) or nhlt_ssp_init_params(tplg_pp). Not very clear, but I' trying to separate the nhlt and the blob generation (headers and code) from each other.
/* the index is always 0 in dmic case */ | ||
ret = nhlt_dmic_get_ep(tplg_pp, &eps[eps_count], 0); | ||
if (ret < 0) | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to unwind and free previously successful ep alloc's right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn't any ep's allocated before this...
@@ -316,6 +329,11 @@ int nhlt_set_dai(struct tplg_pre_processor *tplg_pp, | |||
tplg_pp_debug("nhlt_set_dai id %s", id); | |||
|
|||
/* set dai parameters here */ | |||
if (!strncmp(id, "DMIC", 4)) { | |||
ret = nhlt_dmic_set_params(tplg_pp, cfg, parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does cfg point to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does nhlt_dmic_set_params() end up with generating the blob? Its hard to tell where the blob generation is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg should be the DAI config from topology?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting the dai params from topology2 parsing (with auto_attr) -> setting the values to internal structs -> all blobs are generated at the end of parsing from these internal structs. Sof fw code is doing it like this -> I don't want to start to modify too much for e.g. the dmic blob code -> will differ from sof too much -> horrible amount of possible errors for me
} | ||
} | ||
|
||
/* set object values */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to set class values? The object should have inherited class values already no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so are the class values already set at this point in auto_attr processing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so
polarity_b, clk_edge, skew); | ||
} | ||
|
||
static int set_mic_data(struct tplg_pre_processor *tplg_pp, snd_config_t *cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you call this a set but its really a get isnt it? you're getting the user values from the input conf file and updating your local stuct for processing later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh well, I guess it could be thought both ways. I was thinking it this way: "I'm setting the data to internal structs from topology" then "getting the calculated endpoints from the engine". So I see the nhlt part as an external engine where you need to set data, then ask it to generate the blobs
* } | ||
* } | ||
*/ | ||
int nhlt_dmic_set_params(struct tplg_pre_processor *tplg_pp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, its really get_params and to be more precise, we should call it nhlt_dmic_get_input_params()
@@ -0,0 +1,1365 @@ | |||
// SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would make it easier if you split the dmic-process.* files into a separate commit. There are pretty much exactly copy from the SOF code right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean whatever makes things easier to read. I've already done 2 iterations from comments to back and forth with single commits to multiple commits :) but yeah as it is not so clear -> there's probably issue in the patch division
return 0; | ||
} | ||
|
||
static struct intel_dmic_params * get_dmic(struct tplg_pre_processor *tplg_pp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your get and set are inverted. get should be used for reading input values form conf and set should be used to set the values in the generated blob.
should be now rebased on upstream. |
Add optional generation of nhlt acpi table into topology2.0 processing. Nhlt internal structure is defined in: https://01.org/sites/default/files/595976_intel_sst_nhlt.pdf Nhlt acpi table contain vendor specific binary data blobs that are used in some Intel dsp platforms for configuring the dmic and ssp hardware. The beef in this code is to generate the vendor specific binary blobs, but as there is existing nhlt parser in kernel there's no point of re-inventing the container: just use the existing nhlt acpi table format. Basically this code is creating similar nhlt acpi table that you would get from: cat /sys/firmware/acpi/tables/NHLT Nhlt generation is done by adding auto attribute function for topology2.0 dai processing, saving parameters and generating the nhlt struct after parsing. Nhlt struct is added into the manifest as bytes field. Nhlt processing uses a private data in tplg_pre_processor struct and needs to be initialized before parsing starts. After all dais have been parsed the nhlt can be created. Nhtl structs are almost direct copy from related kernel header. Vendor specific blob generation will be added in later commits. Signed-off-by: Jaska Uimonen <[email protected]>
In some Intel platforms the nhlt vendor specific blobs are sent to the dsp over ipc. Add the generation of Intel dmic blob into the nhlt code. Dmic code is lifted from Sound Open Firmware (sof) code base, thus it will have BSD-3 license. Signed-off-by: Jaska Uimonen <[email protected]>
In some Intel platforms the nhlt vendor specific blobs are sent to the dsp over ipc. Add the generation of Intel ssp blob into the nhlt code. Ssp code is lifted from Sound Open Firmware (sof) code base, thus it will have BSD-3 license. Signed-off-by: Jaska Uimonen <[email protected]>
002088a
to
267c967
Compare
@plbossart I ran now checkpatch, sparse and cppcheck on all patches individually. There we're a lot of style issues and couple of coding things... which should be now fixed. But could not get them to complain about the eps_count thing you mentioned :) |
well that's a good sign if humans are still needed. Tools are still somewhat unreliable, I've seen static analysis results show up much later due to completely unrelated changes. I don't have any specific recommendations other than use the following options sparse: cppcheck: |
alsactl distributed as part of Fedora 40 got a SEGV: # journalctl ... May 17 00:55:58 dev64.localdomain kernel: alsactl[1923]: segfault at 28 ip 00005600705b3373 sp 00007ffd9712bef0 error 4 in alsactl[5600705af000+13000] likely on CPU 5 (core 8, socket 0) ... As the following output of the debug session, card_free() tried a card pointing NULL: $ sudo coredumpctl debug alsactl PID: 1923 (alsactl) UID: 0 (root) GID: 0 (root) Signal: 11 (SEGV) Timestamp: Fri 2024-05-17 00:55:58 JST (3h 34min ago) Command Line: /usr/sbin/alsactl -s -n 19 -c -E ALSA_CONFIG_PATH=/etc/alsa/alsactl.conf --initfile=/lib/alsa/init/00main rdaemon Executable: /usr/sbin/alsactl Control Group: /system.slice/alsa-state.service Unit: alsa-state.service Slice: system.slice Boot ID: 241b5a2ef86f4940bb3d340583c80d88 Machine ID: 437365709a8c488c9481ee4b6651c2ec Hostname: dev64.localdomain Storage: /var/lib/systemd/coredump/core.alsactl.0.241b5a2ef86f4940bb3d340583c80d88.1923.1715874958000000.zst (present) Size on Disk: 81.7K Package: alsa-utils/1.2.11-1.fc40 build-id: 3b6fec58b3566d666d6e9fd48e8fcf04f03f0152 Message: Process 1923 (alsactl) of user 0 dumped core. Module libasound.so.2 from rpm alsa-lib-1.2.11-2.fc40.x86_64 Module alsactl from rpm alsa-utils-1.2.11-1.fc40.x86_64 Stack trace of thread 1923: #0 0x00005600705b3373 card_free (alsactl + 0xa373) #1 0x00005600705c0e54 state_daemon (alsactl + 0x17e54) #2 0x00005600705b2339 main (alsactl + 0x9339) #3 0x00007f4c0b9b7088 __libc_start_call_main (libc.so.6 + 0x2a088) #4 0x00007f4c0b9b714b __libc_start_main_impl (libc.so.6 + 0x2a14b) #5 0x00005600705b2df5 _start (alsactl + 0x9df5) ELF object binary architecture: AMD x86-64 GNU gdb (Fedora Linux) 14.2-1.fc40 Copyright (C) 2023 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-redhat-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <https://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /usr/sbin/alsactl... Reading symbols from /usr/lib/debug/usr/sbin/alsactl-1.2.11-1.fc40.x86_64.debug... [New LWP 1923] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Core was generated by `/usr/sbin/alsactl -s -n 19 -c -E ALSA_CONFIG_PATH=/etc/alsa/alsactl.conf --init'. Program terminated with signal SIGSEGV, Segmentation fault. #0 free_list (list=0x20) at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/daemon.c:73 73 for (i = 0; i < list->size; i++) (gdb) where #0 free_list (list=0x20) at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/daemon.c:73 #1 card_free (card=card@entry=0x5600707455f0) at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/daemon.c:82 #2 0x00005600705c0e54 in state_daemon (file=file@entry=0x5600705c31a1 "/var/lib/alsa/asound.state", cardname=cardname@entry=0x0, period=period@entry=300, pidfile=pidfile@entry=0x5600705c3170 "/var/run/alsactl.pid") at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/daemon.c:455 #3 0x00005600705b2339 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/alsactl.c:459 (gdb) list 68 69 static void free_list(struct id_list *list) 70 { 71 int i; 72 73 for (i = 0; i < list->size; i++) 74 free(list->list[i]); 75 free(list->list); 76 } 77 (gdb) up #1 card_free (card=card@entry=0x5600707455f0) at /usr/src/debug/alsa-utils-1.2.11-1.fc40.x86_64/alsactl/daemon.c:82 82 free_list(&c->blacklist); (gdb) p c $1 = (struct card *) 0x0 (gdb) Closes: alsa-project#267 Signed-off-by: Masatake YAMATO <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>
Idea here is to tap into topology2 pre-processing to generate nhlt blob containing endpoint descriptors, which include intel vendor specific blobs -> and add that to topology manifest. The nhlt blob generated should have the same structure as you would get from "cat /sys/firmware/acpi/tables/NHLT". Currently only dmic blob is implemented and I've been able to use that to configure dmic dai through kernel. SSP file is included but doesn't do anything yet.