Allow sending of compress parameters to SOF firmware by dbaluta · Pull Request #3552 · thesofproject/linux

dbaluta · 2022-03-28T17:56:54Z

This is the kernel part of SOF FW series in (thesofproject/sof#5591, thesofproject/sof#5592, thesofproject/sof#5596).

In particular we are interested right now to send the codec id to SOF FW, in order to properly select the correct API processing function.

plbossart

Looks mostly good to me @dbaluta

dbaluta · 2022-05-03T14:26:39Z

@lgirdwood @plbossart @ujfalusi @ranj063 this is ready for merge as the previous ABI patches were merged.

Corresponding patch for FW is already in.

dbaluta · 2022-05-17T15:07:25Z

@plbossart @ranj063 @paulstelian97 @lgirdwood this should be ready for merge.

ranj063 · 2022-05-17T15:10:30Z

 /* SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 21
+#define SOF_ABI_MINOR 22


@dbaluta looks like this can be squashed with the previous patch right?

@ranj063 I've seen previous changes preferred this approach of having a separate patch. I wouldn't mind either.

@plbossart what do you think?

A separate patch is fine, this allows for a more space in the commit message to explain the interoperability between firmware and kernel versions.

"Older FW will work with newer kernels as they will directly ignore ext_data_length field."

that's true, but functionally the parameters will be ignored and the result is undefined. If the firmware version is < 3.22 you probably don't want to even such an IPC at all, and probably want to prevent this path from being created in the first place.

plbossart

Not fully clear on new kernel + old firmware cases.

plbossart · 2022-05-18T13:35:54Z

-
-	uint8_t reserved[5];
+	uint8_t reserved0;
+	int16_t ext_data_length; /** < 0, means no extended data */


spurious space between /** and < ?

plbossart · 2022-05-18T13:39:03Z

-	pcm = kzalloc(sizeof(*pcm), GFP_KERNEL);
+	data_size = sizeof(params->codec);
+
+	if (data_size + sizeof(*pcm) > SOF_IPC_MSG_MAX_SIZE)


nit-pick: for consistency with the rest of the code, maybe sizeof(*pcm) + data_size?

plbossart · 2022-05-18T13:41:32Z

 /* SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 21
+#define SOF_ABI_MINOR 22


A separate patch is fine, this allows for a more space in the commit message to explain the interoperability between firmware and kernel versions.

plbossart · 2022-05-18T13:44:16Z

 /* SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 21
+#define SOF_ABI_MINOR 22


"Older FW will work with newer kernels as they will directly ignore ext_data_length field."

that's true, but functionally the parameters will be ignored and the result is undefined. If the firmware version is < 3.22 you probably don't want to even such an IPC at all, and probably want to prevent this path from being created in the first place.

dbaluta · 2022-05-25T14:07:15Z

@plbossart fixed the nitpicks. The only thing remaining is:

If the firmware version is < 3.22 you probably don't want to even such an IPC at all, and probably want to prevent this path from being created in the first place.

Not sure how to deal with this. If the kernel figures out that the FW is older than 3.22 should we return an error in sof_compr_set_params?

marc-hb · 2022-05-25T17:50:59Z

BYT failures in https://sof-ci.01.org/linuxpr/PR3552/build164/devicetest should be addressed by fresh sof-test PR

lib.sh: lower BYT expectations in new storage_checks() sof-test#911

Sorry for the inconvenience.

plbossart · 2022-05-25T20:00:21Z

@plbossart fixed the nitpicks. The only thing remaining is:
If the firmware version is < 3.22 you probably don't want to even such an IPC at all, and probably want to prevent this path from being created in the first place.
Not sure how to deal with this. If the kernel figures out that the FW is older than 3.22 should we return an error in sof_compr_set_params?

I think so. We clearly have a problem in our definition of ABI. There are cases where the communication will work with an older firmware, the firmware will be able to only use the parameters that were defined when it was build, so in theory there's backwards compatibility in terms of communication protocol, but from the application level the results will be broken.

marc-hb · 2022-05-26T04:23:17Z

I think so. We clearly have a problem in our definition of ABI.

Discussed previously in

SOF FW 1.9 ABI vs kernel 5.14.17 ABI mismatch sof#4986

and others linked from there

dbaluta · 2022-05-30T07:07:56Z

@plbossart should be fine now. Should i squash the FW version check patch into the previous one?

dbaluta · 2022-06-09T08:29:27Z

@ranj063 @lyakh @lgirdwood can you please have a look?

ujfalusi · 2022-06-09T11:26:11Z

-
-	uint8_t reserved[5];
+	uint8_t reserved0;
+	int16_t ext_data_length; /**< 0, means no extended data */


It might have been already answered, but why we need the reserved0? To bring the ext_data_length to 16bit alignment?

for that one could just place ext_data_length after the 3 reserved bytes.

Unfortunately the change was already merged in the FW side so we cannot change it here. Sorry for late answer I just came back from vacation :)

ujfalusi · 2022-06-09T11:27:32Z

+	int16_t ext_data_length; /**< 0, means no extended data */
+	uint8_t reserved[2];
 	uint16_t chmap[SOF_IPC_MAX_CHANNELS];	/**< channel map - SOF_CHMAP_ */
+	uint8_t data[]; /**< extended data */


ext_data ?

I think you are right on this, but the change was already merged in the FW side and would like to keep the names the same.

at a second glance this is not ABI. I can rename it in the FW too. Thanks!

ujfalusi · 2022-06-09T11:28:43Z

 	struct sof_ipc_pcm_params *pcm;
 	struct snd_sof_pcm *spcm;
+	int data_size;
 	int ret;


there is no need for a new line for the data_size, can be added after the existing int ret;

or should data_size be size_t?

ujfalusi · 2022-06-09T11:30:22Z

-	pcm = kzalloc(sizeof(*pcm), GFP_KERNEL);
+	data_size = sizeof(params->codec);
+
+	if (sizeof(*pcm) + data_size > SOF_IPC_MSG_MAX_SIZE)


Better use sdev->ipc->max_payload_size instead SOF_IPC_MSG_MAX_SIZE to avoid surprises if this is used with other than IPC3?

Ok, i will fix this.

dbaluta · 2022-06-29T12:47:24Z

SOFCI TEST

We need to extend sof_ipc_pcm_parmas with additional data in order to send compress_params to SOF FW. The extensions will be done at runtime so we need to dynamically allocate pcm object of type struct sof_ipc_pcm_params. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Allocate memory at the end of sof_ipc_stream_params to store snd_compr_params in order to be sent them to SOF firmware. This will help firmware correctly configure codecs parameters. Notice, that we use 2 bytes from the reserved pool in order to store the extended data length. This is compatible with older FWs where there was no extended data. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

After introducing extended parameters we need to forbid older firmware versions to run with the current and future kernel versions. Although in theory the communication protocol will still work the semantics at application level are undefined. So, prevent this by disallowing older firmwares to run with newer kernels. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Add new field to sof_ipc_stream_params in order to extend stream params struct with extended data to store compress parameters. Older kernel will still work this as they ext_data_length will always be zero. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

dbaluta · 2022-07-04T13:08:39Z

@ujfalusi all comments should be addressed now. Care to have a look?

ujfalusi

@dbaluta, sorry for the delay...

dbaluta requested review from ajitkupandey, bardliao, juimonen, keyonjie, kuanhsuncheng, kv2019i, lgirdwood, lyakh, plbossart, ranj063, ujfalusi and yaochunhung as code owners March 28, 2022 17:56

ujfalusi reviewed Mar 29, 2022

View reviewed changes

Comment thread include/uapi/sound/sof/abi.h

Comment thread sound/soc/sof/compress.c Outdated

plbossart reviewed Apr 4, 2022

View reviewed changes

Comment thread include/uapi/sound/sof/abi.h

Comment thread sound/soc/sof/compress.c Outdated

dbaluta changed the title ~~[RFC] Allow sending of compress parameters to SOF firmware~~ Allow sending of compress parameters to SOF firmware May 3, 2022

ranj063 reviewed May 3, 2022

View reviewed changes

Comment thread sound/soc/sof/compress.c

ranj063 reviewed May 17, 2022

View reviewed changes

plbossart reviewed May 18, 2022

View reviewed changes

plbossart previously approved these changes Jun 7, 2022

View reviewed changes

ujfalusi reviewed Jun 9, 2022

View reviewed changes

ujfalusi requested changes Jun 29, 2022

View reviewed changes

Comment thread sound/soc/sof/compress.c

dbaluta dismissed plbossart’s stale review via 4146f79 June 29, 2022 12:39

dbaluta added 4 commits June 29, 2022 15:57

plbossart approved these changes Jun 29, 2022

View reviewed changes

plbossart requested review from ranj063 and ujfalusi June 29, 2022 16:04

ranj063 approved these changes Jun 29, 2022

View reviewed changes

ujfalusi approved these changes Jul 4, 2022

View reviewed changes

dbaluta merged commit caa7bc7 into thesofproject:topic/sof-dev Jul 5, 2022

dbaluta deleted the add_compr branch July 5, 2022 06:09

Uh oh!

Conversation

dbaluta commented Mar 28, 2022

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbaluta commented May 3, 2022

Uh oh!

Uh oh!

dbaluta commented May 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta commented May 25, 2022

Uh oh!

marc-hb commented May 25, 2022

Uh oh!

plbossart commented May 25, 2022

Uh oh!

marc-hb commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaluta commented May 30, 2022

Uh oh!

dbaluta commented Jun 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbaluta commented Jun 29, 2022

Uh oh!

dbaluta commented Jul 4, 2022

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

marc-hb commented May 26, 2022 •

edited

Loading