Skip to content

Update host position value in every period to improve precision #5137

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 2 commits into from
Mar 3, 2022
Merged

Update host position value in every period to improve precision #5137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11cc712
audio: host: update position every period to improve precision
yaochunhung Dec 28, 2021
6589c49
ipc: ipc3: reset host position in ipc_stream_pcm_params before reply
yaochunhung Dec 28, 2021
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
20 changes: 16 additions & 4 deletions src/audio/host.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ struct host_data {
uint32_t host_period_bytes;
uint16_t stream_tag;
uint16_t no_stream_position; /**< 1 means don't send stream position */
uint8_t cont_update_posn; /**< 1 means continuous update stream position */

/* host component attributes */
enum comp_copy_type copy_type; /**< Current host copy type */
Expand Down Expand Up @@ -307,6 +308,8 @@ static void host_update_position(struct comp_dev *dev, uint32_t bytes)
struct comp_buffer *source;
struct comp_buffer *sink;
int ret;
bool update_mailbox = false;
bool send_ipc = false;


if (dev->direction == SOF_IPC_STREAM_PLAYBACK)
Expand Down Expand Up @@ -345,6 +348,8 @@ static void host_update_position(struct comp_dev *dev, uint32_t bytes)
#else
hd->local_pos = 0;
#endif
if (hd->cont_update_posn)
update_mailbox = true;

/* Don't send stream position if no_stream_position == 1 */
if (!hd->no_stream_position) {
Expand All @@ -361,12 +366,18 @@ static void host_update_position(struct comp_dev *dev, uint32_t bytes)
/* send timestamped position to host
* (updates position first, by calling ops.position())
*/
pipeline_get_timestamp(dev->pipeline, dev, &hd->posn);
mailbox_stream_write(dev->pipeline->posn_offset,
&hd->posn, sizeof(hd->posn));
ipc_msg_send(hd->msg, &hd->posn, false);
update_mailbox = true;
send_ipc = true;
}
}

if (update_mailbox) {
pipeline_get_timestamp(dev->pipeline, dev, &hd->posn);
mailbox_stream_write(dev->pipeline->posn_offset,
&hd->posn, sizeof(hd->posn));
if (send_ipc)
ipc_msg_send(hd->msg, &hd->posn, false);
}
}

/* The host memory is not guaranteed to be continuous and also not guaranteed
Expand Down Expand Up @@ -761,6 +772,7 @@ static int host_params(struct comp_dev *dev,
hd->stream_tag = params->stream_tag;
hd->no_stream_position = params->no_stream_position;
hd->host_period_bytes = params->host_period_bytes;
hd->cont_update_posn = params->cont_update_posn;

/* retrieve DMA buffer address alignment */
err = dma_get_attribute(hd->dma, DMA_ATTR_BUFFER_ADDRESS_ALIGNMENT,
Expand Down
3 changes: 2 additions & 1 deletion src/include/ipc/stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,9 @@ 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 */

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

Expand Down
4 changes: 2 additions & 2 deletions src/include/kernel/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@

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

Copy link
Collaborator

@dbaluta dbaluta Mar 2, 2022

Choose a reason for hiding this comment

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

@yaochunhung @lgirdwood what is SOF_ABI_PATCH and why is decremented?

Copy link
Member

@lgirdwood lgirdwood Mar 2, 2022

Choose a reason for hiding this comment

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

@dbaluta see https://semver.org/

/** \brief SOF ABI version number. Format within 32bit word is MMmmmppp */
#define SOF_ABI_MAJOR_SHIFT 24
Expand Down
6 changes: 6 additions & 0 deletions src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ static int ipc_stream_pcm_params(uint32_t stream)
struct sof_ipc_pcm_params pcm_params;
struct sof_ipc_pcm_params_reply reply;
struct ipc_comp_dev *pcm_dev;
struct sof_ipc_stream_posn posn;
int err, reset_err;

/* copy message with ABI safe method */
Expand Down Expand Up @@ -299,6 +300,11 @@ static int ipc_stream_pcm_params(uint32_t stream)
reply.rhdr.error = 0;
reply.comp_id = pcm_params.comp_id;
reply.posn_offset = pcm_dev->cd->pipeline->posn_offset;

/* reset position value before send ipc */
memset(&posn, 0, sizeof(posn));
mailbox_stream_write(reply.posn_offset, &posn, sizeof(posn));
Copy link
Collaborator

@lyakh lyakh Dec 29, 2021

Choose a reason for hiding this comment

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

Could you describe in which situations you see invalid position data being delivered to Linux?

Copy link
Contributor Author

@yaochunhung yaochunhung Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh yes,I saw this invalid value when I run alsa conformance test and I implement PCM pointer function to get host position value but see invalid value at the beginning. After I provide this code, the invalid value will be replaced with zero. Not sure if this issue could be reproducible on Intels platforms because it looks like Intel uses different ways to get position at Linux side. thanks.

Copy link
Collaborator

@lyakh lyakh Dec 30, 2021

Choose a reason for hiding this comment

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

got it, thanks!


mailbox_hostbox_write(0, &reply, sizeof(reply));
return 1;

Expand Down