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

[FEATURE] sof-ctl should be able to insert ABI header. #1535

Closed
lgirdwood opened this issue Jun 7, 2019 · 11 comments
Closed

[FEATURE] sof-ctl should be able to insert ABI header. #1535

lgirdwood opened this issue Jun 7, 2019 · 11 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@lgirdwood
Copy link
Member

Is your feature request related to a problem? Please describe.
sof-ctl currently can't insert ABI header. A common use case is to generate new binary blob (using proprietary tools) and upload to FW for runtime tuning. Proprietary tools may not be able to generate the header.

Describe the solution you'd like
A command line argument like sof-ctl -a 3:8:0 to insert header for ABI. If this arg is not given then sof-ctl should validate header instead.

@lgirdwood lgirdwood added the enhancement New feature or request label Jun 7, 2019
@lgirdwood lgirdwood added this to the 1.4 milestone Jun 7, 2019
@lgirdwood
Copy link
Member Author

@singalsu is this merged now ?

@singalsu
Copy link
Collaborator

singalsu commented Sep 2, 2019

@lgirdwood Sorry no. The usage of this feature is not clear. Why do we want to specify the version in command line if we do not know the rest of the ext binary structure? The sof-ctl tool can't have capability re-format requests for past ABIs.

In another feature issue #791 the idea is that sof-ctl would return the current ABI constant blurb and process component could then insert the stable own binary settings data into the end.

@singalsu
Copy link
Collaborator

singalsu commented Sep 2, 2019

@lgirdwood I noticed that @keyonjie has added option "-r" but it's not exactly this.

If you mean overwrite the bytes location with command line provided version it's a simple change.

How should the header validate ("If this arg is not given then sof-ctl should validate header instead.") behave from user point of view?

@singalsu
Copy link
Collaborator

singalsu commented Sep 5, 2019

@lgirdwood @keyonjie Please help describe this requirement (how it differs from existing "-r" and now proposed "-p" functionality). I'd like to keep PR #1792 as [RFC] until we have this one implemented as well.

@keyonjie
Copy link
Contributor

keyonjie commented Sep 6, 2019

Hi @lgirdwood can you help to describe the usage scenario that you mentioned with "A command line argument like sof-ctl -a 3:8:0 to insert header for ABI. If this arg is not given then sof-ctl should validate header instead."?

Per my understanding, we have 2 kinds of usage scenarios when using sof-ctl tool:

  1. get: to get tlv kcontrol value (run without '-s'). for this scenario, we will get the value, and output them either to the terminal (without '-o') or to an output file (with '-o' specified), in binary mode, with or without ABI header included(without or with '-r').
  2. set: to get tlv kcontrol value (run with '-s'). for this scenario, we will set the value, with input file specified (after '-s'), the input file can be txt file(without '-b') or binary blob(with '-b'), with ABI header included in the blob file already (without '-r') or not (with '-r', sof-ctl will help to generate the ABI header automatically).

So, @lgirdwood is what you described is the last scenario in #2 above? if it is, then I think it can already generate the ABI header automatically, with the ABI version that the tool(sof-ctl) was compiled in the sof source repo, I think it is better to add allows the user to specify random ABI version here?

@singalsu is adding another usage scenario, that is using sof-ctl like "ABI header generator",

  1. generate: to generate ABI header with specified value of the header, with that implemented, we can generate any header as we want:
struct sof_abi_hdr {
	uint32_t magic;		/**< 'S', 'O', 'F', '\0' */
	uint32_t type;		/**< component specific type */
	uint32_t size;		/**< size in bytes of data excl. this struct */
	uint32_t abi;		/**< SOF ABI version */
	uint32_t reserved[4];	/**< reserved for future use */
	uint32_t data[0];	/**< Component data - opaque to core */
} __attribute__((packed));

We can specify the 'size' flag as Seppo wanted, specify the 'abi' flag as you want(if your intention was this), or specify the magic number and type(e.g. in KWD case) as we want.

@lgirdwood please help confirm what's your intention, the #2 or #3 usage scenario?

@lgirdwood
Copy link
Member Author

@singalsu @keyonjie intention is to allow external tuning tools (which do not know anything about ABI) to be able to send and recieve data from the FW.

  1. Set data. external tool generates coefficients, but we cant send them to FW without header. Tools takes external coefficients, inserts the header and sends to FW.

  2. Get data. External tool needs to read data without ABI. Tool reads data and strips out ABI. Saves as binary.

Can you guys confirm this is supported.

@singalsu
Copy link
Collaborator

singalsu commented Sep 6, 2019

Option "-r" should do examples 1 and 2 for set data and get data but it doesn't work as I thought it would go. This test is with ASCII CSV data. I'm not using myself binaries at the moment.

$ amixer controls | grep IIR # Get control number/ID
numid=22,iface=MIXER,name='EQIIR1.0 EQIIR'

$ cat eq/eq_iir_bassboost_noheader.txt # Data without ABI header
116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910,

$ cat eq/eq_iir_bassboost.txt # Data with ABI header
4607827,0,116,50368512,0,0,0,0,116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910,

$ sof-ctl -n 22 -r -s eq/eq_iir_bassboost_noheader.txt # Set EQ response (fail)
Control size is 304.
Applying configuration "eq/eq_iir_bassboost_noheader.txt" into device hw:0 control numid=22.
116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910
Error: failed TLV write (-22)
Error: Could not set control, ret:-22

$ sof-ctl -n 22 -s eq/eq_iir_bassboost.txt # Set EQ response (success)
Control size is 304.
Applying configuration "eq/eq_iir_bassboost.txt" into device hw:0 control numid=22.
4607827,0,116,50368512,0,0,0,0,116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910
Success.
hdr: magic 0x00464f53
hdr: type 0
hdr: size 116 bytes
hdr: abi 3:9:0
4607827,0,116,50368512,0,0,0,0,116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910

$ sof-ctl -n 22 -r # Get response (header is still present)
Control size is 304.
Retrieving configuration for device hw:0 control numid=22.
Success.
hdr: magic 0x00464f53
hdr: type 0
hdr: size 116 bytes
hdr: abi 3:10:0
4607827,0,116,50372608,0,0,0,0,116,2,1,0,0,0,0,0,0,2,2,0,0,0,0,3227172081,2141520527,536653443,3221660410,536653443,0,16384,3260252783,2107733822,161646111,3961037800,172645501,4294967294,27910

Maybe this should be fixed? I'll test next with binary data, need to do a convert script...

@singalsu
Copy link
Collaborator

singalsu commented Sep 6, 2019

Here's my test with binary data without header, write works (a lot of bass can be heard 👍 ) :

singalsu@piikkisika:~$ sof-ctl -n 22 -r -b -s eq/eq_iir_bassboost_noheader.bin 
Control size is 148.
Applying configuration "eq/eq_iir_bassboost_noheader.bin" into device hw:0 control numid=22.
Success.
hdr: magic 0x00464f53
hdr: type 0
hdr: size 116 bytes
hdr: abi 3:9:0
00000000 0074 0000 0002 0000 0001 0000 0000 0000 
00000010 0000 0000 0000 0000 0000 0000 0000 0000 
00000020 0000 0000 0002 0000 0002 0000 0000 0000 
00000030 0000 0000 0000 0000 0000 0000 bcf1 c05a 
00000040 028f 7fa5 ae83 1ffc a2fa c006 ae83 1ffc 
00000050 0000 0000 4000 0000 826f c253 773e 7da1 
00000060 861f 09a2 a3e8 ec18 5c7d 0a4a fffe ffff 
00000070 6d06 0000 

Also binary data get without header works

$ sof-ctl -n 22 -r -b -o get_data_test.bin
Control size is 304.
Retrieving configuration for device hw:0 control numid=22.
Success.
116 bytes written to file.

$ ls -l *.bin
-rw------- 1 singalsu singalsu 116 syys   6 21:12 get_data_test.bin

$ cmp -l get_data_test.bin eq/eq_iir_bassboost_noheader.bin 
$

So in my opinion examples 1-2 by @lgirdwood work with binary data.

@keyonjie Should I work to fix the CSV ASCII data set/get with -r ? Am I using the correct command line switches?

@keyonjie
Copy link
Contributor

keyonjie commented Sep 9, 2019

@singalsu yes, you are using the correct command lines, set/get for CSV ASCII text data with -r(raw data without ABI header) is not supported yet (as we don't have this requirement in demux or kwd scenarios, I didn't work on that), it will be good if you can add this for EQ.

@lgirdwood
Copy link
Member Author

@singalsu @keyonjie please support this for binaries too. Most 3rd party tuning tools will emit only binary tuning data.

@singalsu
Copy link
Collaborator

The PR #1792 is merged now so we can close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants