Skip to content

Support for dynamic codec adapter id - Part 2#5592

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.

Sign up for GitHub

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

Merged
lgirdwood merged 5 commits into from
Apr 25, 2022
Merged

Support for dynamic codec adapter id - Part 2 #5592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5704d95
codec_adapter: Remove assert after memcpy_s
dbaluta Mar 24, 2022
0831d9c
codec_adapter: Dynamically allocate stream_params
dbaluta Mar 23, 2022
a24bcf8
ipc: stream: Add extended data at the end of sof_ipc_stream_params
dbaluta Mar 23, 2022
0914fbe
codec_adapter: Copy extended data found at the end of sof_ipc_stream_…
dbaluta Mar 23, 2022
97792e2
ipc3: Check ipc size check for pcm_params to account for ext_data
dbaluta Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/audio/codec_adapter/codec/cadence.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -537,8 +537,8 @@ cadence_codec_process(struct processing_module *mod,
struct module_data *codec = comp_get_module_data(dev);
struct cadence_codec_data *cd = codec->private;
int output_bytes = cadence_codec_get_samples(dev) *
mod->stream_params.sample_container_bytes *
mod->stream_params.channels;
mod->stream_params->sample_container_bytes *
mod->stream_params->channels;
uint32_t remaining = input_buffers[0].size;
int ret;

Expand Down
32 changes: 27 additions & 5 deletions src/audio/codec_adapter/codec_adapter.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -249,7 +249,7 @@ int codec_adapter_prepare(struct comp_dev *dev)
goto free;
}
list_item_append(&buffer->sink_list, &mod->sink_buffer_list);
buffer_set_params(buffer, &mod->stream_params, BUFFER_UPDATE_FORCE);
buffer_set_params(buffer, mod->stream_params, BUFFER_UPDATE_FORCE);
buffer_reset_pos(buffer, NULL);
}
} else {
Expand All@@ -263,7 +263,7 @@ int codec_adapter_prepare(struct comp_dev *dev)
buff_size);
goto free;
}
buffer_set_params(buffer, &mod->stream_params, BUFFER_UPDATE_FORCE);
buffer_set_params(buffer, mod->stream_params, BUFFER_UPDATE_FORCE);
buffer_reset_pos(buffer, NULL);
}
}
Expand DownExpand Up@@ -306,9 +306,28 @@ int codec_adapter_params(struct comp_dev *dev,
return ret;
}

ret = memcpy_s(&mod->stream_params, sizeof(struct sof_ipc_stream_params),
/* allocate stream_params each time */
Comment thread
paulstelian97 marked this conversation as resolved.
Outdated
if (mod->stream_params)
rfree(mod->stream_params);

mod->stream_params = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(*mod->stream_params) + params->ext_data_length);
if (!mod->stream_params)
return -ENOMEM;

ret = memcpy_s(mod->stream_params, sizeof(struct sof_ipc_stream_params),
params, sizeof(struct sof_ipc_stream_params));
assert(!ret);
if (ret < 0)
return ret;

@lyakhlyakhApr 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this doesn't change anything, right? The only way for memcpy_s() above to fail would be if params were overlapping with &mod->stream_params which would probably be a bad bug deserving a panic anyway...

@dbalutadbalutaApr 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@ranj063 suggested to remote the assert.


if (params->ext_data_length) {
ret = memcpy_s((uint8_t *)mod->stream_params->data,
params->ext_data_length,
(uint8_t *)params->data,
params->ext_data_length);
if (ret < 0)
return ret;
}

mod->period_bytes = params->sample_container_bytes *
params->channels * params->rate / 1000;
Expand DownExpand Up@@ -428,7 +447,7 @@ static void module_copy_samples(struct comp_dev *dev, struct comp_buffer *src_bu
if (!copy_bytes)
return;
audio_stream_copy(&src_buffer->stream, 0, &sink_buffer->stream, 0,
copy_bytes / mod->stream_params.sample_container_bytes);
copy_bytes / mod->stream_params->sample_container_bytes);
buffer_stream_writeback(sink_buffer, copy_bytes);

comp_update_buffer_produce(sink_buffer, copy_bytes);
Expand DownExpand Up@@ -677,6 +696,9 @@ int codec_adapter_reset(struct comp_dev *dev)
buffer_zero(buffer);
}

rfree(mod->stream_params);
mod->stream_params = NULL;

comp_dbg(dev, "codec_adapter_reset(): done");

return comp_set_state(dev, COMP_TRIGGER_RESET);
Expand Down
5 changes: 4 additions & 1 deletion src/include/ipc/stream.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,9 +95,12 @@ struct sof_ipc_stream_params {
uint32_t host_period_bytes;
uint16_t no_stream_position; /**< 1 means don't send stream position */
uint8_t cont_update_posn; /**< 1 means continuous update stream position */
uint8_t reserved0;
uint16_t ext_data_length; /**< 0 means no extended data */
Comment thread
dbaluta marked this conversation as resolved.
Outdated

@lyakhlyakhApr 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this structure is packed, please, move this up before cont_update_posn to avoid unaligned access especially on ARM

@dbalutadbalutaApr 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@lyakh you have a good point. but i don't like changing the order of fields as it is already part of the ABI. I have added 1 uint8_t reseved field. See the new updated code.

@lyakhlyakhApr 22, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@dbaluta ah, right sure, but then let's put this 16-bit field after the reserved bytes to avoid splitting them. Even though you're unlikely to use 3 bytes together, still it might happen that exactly 3 bytes would be needed for an array or a structure


uint8_t reserved[5];
uint8_t reserved[2];
uint16_t chmap[SOF_IPC_MAX_CHANNELS]; /**< channel map - SOF_CHMAP_ */
int8_t data[]; /**< extended data */
} __attribute__((packed, aligned(4)));

/* PCM params info - SOF_IPC_STREAM_PCM_PARAMS */
Expand Down
2 changes: 1 addition & 1 deletion src/include/kernel/abi.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -29,7 +29,7 @@

/** \brief SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 21
#define SOF_ABI_MINOR 22
#define SOF_ABI_PATCH 0

/** \brief SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down
2 changes: 1 addition & 1 deletion src/include/sof/audio/codec_adapter/codec/generic.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -275,7 +275,7 @@ struct module_data {
/* codec_adapter private, runtime data */
struct processing_module {
struct module_data priv; /**< module private data */
struct sof_ipc_stream_params stream_params;
struct sof_ipc_stream_params *stream_params;
struct list_item sink_buffer_list; /* list of sink buffers to save produced output */

/*
Expand Down
22 changes: 20 additions & 2 deletions src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -219,8 +219,26 @@ static int ipc_stream_pcm_params(uint32_t stream)
return -EINVAL;
}

if (IPC_IS_SIZE_INVALID(pcm_params.params)) {
IPC_SIZE_ERROR_TRACE(&ipc_tr, pcm_params.params);
/* sanity check for pcm_params size */
if (pcm_params.hdr.size !=
sizeof(pcm_params) + pcm_params.params.ext_data_length) {
tr_err(&ipc_tr, "pcm_params invalid size, hdr.size=%d, ext_data_len=%d",
pcm_params.hdr.size, pcm_params.params.ext_data_length);
return -EINVAL;
}

/* sanity check for pcm_params.params size */
if (pcm_params.params.hdr.size !=
sizeof(pcm_params.params) + pcm_params.params.ext_data_length) {
tr_err(&ipc_tr, "pcm_params.params invalid size, hdr.size=%d, ext_data_len=%d",
pcm_params.params.hdr.size, pcm_params.params.ext_data_length);
return -EINVAL;
}

if (sizeof(pcm_params) + pcm_params.params.ext_data_length > SOF_IPC_MSG_MAX_SIZE) {
tr_err(&ipc_tr, "pcm_params ext_data_length invalid size %d max allowed %d",
pcm_params.params.ext_data_length,
SOF_IPC_MSG_MAX_SIZE - sizeof(pcm_params));
return -EINVAL;
}

Expand Down