[Sound-open-firmware] Commit c80270e7e49 (" ipc: structures should all be aligned on 4 bytes") causes issues on i.MX8
Hi Liam,
I noticed this SOF commit:
commit c80270e7e49d26a34fe284d1868e7d9cb80104f1 Author: Liam Girdwood liam.r.girdwood@linux.intel.com Date: Sat Apr 3 21:22:21 2021 +0100
ipc: structures should all be aligned on 4 bytes
Give the compiler a chance to further optimise IPC data access since it's all on a 4 byte alignment.
Signed-off-by: Liam Girdwood liam.r.girdwood@linux.intel.com
It looks like it triggers some issues on i.MX8.
Is there any equivalent for Linux kernel? Some structures are shared with the kernel and on arm64 it assumes default alignment as 8 hence I get a mismatch at least when printing DAI / Host position
+ dev_info(sdev->dev, "posn : host 0x%llx dai 0x%llx wall 0x%llx no_per %d\n", + posn.host_posn, posn.dai_posn, posn.wallclock, stream->substream->runtime->no_period_wakeup);
thanks, Daniel.
Hi Daniel,
On Tue, 2021-05-11 at 15:02 +0000, Daniel Baluta wrote:
Hi Liam,
I noticed this SOF commit:
commit c80270e7e49d26a34fe284d1868e7d9cb80104f1Author: Liam Girdwood liam.r.girdwood@linux.intel.com Date: Sat Apr 3 21:22:21 2021 +0100
ipc: structures should all be aligned on 4 bytes Give the compiler a chance to further optimise IPC data access since it's all on a 4 byte alignment. Signed-off-by: Liam Girdwood liam.r.girdwood@linux.intel.com
It looks like it triggers some issues on i.MX8.
Is there any equivalent for Linux kernel? Some structures are shared with the kernel and on arm64 it assumes default alignment as 8 hence I get a mismatch at least when printing DAI / Host position
- dev_info(sdev->dev, "posn : host 0x%llx dai 0x%llx wall
0x%llx no_per %d\n",
- posn.host_posn, posn.dai_posn, posn.wallclock,
stream->substream->runtime->no_period_wakeup);
It looks like the uint64_t on the sof_ipc_stream_position are not aligned on 8 bytes (with the packing, there are 7 * uint32_t before them) and the ARM compiler is expecting a 8 byte alingment for uint64 ?
I'm not sure why this is showing up now as the "packed" attribute would force the uint64_t to be aligned on 4 bytes instead of 8 here.
Btw, have you tried setting aligned(8) here ? or tried removing the aligned(4) for stream_posn ?
Liam
thanks, Daniel.
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
It looks like the uint64_t on the sof_ipc_stream_position are not aligned on 8 bytes (with the packing, there are 7 * uint32_t before them) and the ARM compiler is expecting a 8 byte alingment for uint64 ?
I'm not sure why this is showing up now as the "packed" attribute would force the uint64_t to be aligned on 4 bytes instead of 8 here.
Btw, have you tried setting aligned(8) here ? or tried removing the aligned(4) for stream_posn ?
I have tried setting aligned(4) on the Linux kernel header:
--- a/include/sound/sof/stream.h +++ b/include/sound/sof/stream.h @@ -143,6 +143,6 @@ struct sof_ipc_stream_posn { uint64_t timestamp; /**< system time stamp */ uint32_t xrun_comp_id; /**< comp ID of XRUN component */ int32_t xrun_size; /**< XRUN size in bytes */ -} __packed; +} __packed __aligned(4);
It works fine with this.
On Tue, 2021-05-11 at 15:20 +0000, Daniel Baluta wrote:
It looks like the uint64_t on the sof_ipc_stream_position are not aligned on 8 bytes (with the packing, there are 7 * uint32_t before them) and the ARM compiler is expecting a 8 byte alingment for uint64 ?
I'm not sure why this is showing up now as the "packed" attribute would force the uint64_t to be aligned on 4 bytes instead of 8 here.
Btw, have you tried setting aligned(8) here ? or tried removing the aligned(4) for stream_posn ?
I have tried setting aligned(4) on the Linux kernel header:
--- a/include/sound/sof/stream.h+++ b/include/sound/sof/stream.h @@ -143,6 +143,6 @@ struct sof_ipc_stream_posn { uint64_t timestamp; /**< system time stamp */ uint32_t xrun_comp_id; /**< comp ID of XRUN component */ int32_t xrun_size; /**< XRUN size in bytes */ -} __packed; +} __packed __aligned(4); It works fine with this.
Ok, this must be an ARM64 difference and I assume you have a kernel fix brewing ?.
Intention on FW side was to use only the L32 and S32 calls and not cater for any unaligned load/stores (with their extra baggage).
Liam
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Tue, May 11, 2021 at 6:23 PM Girdwood, Liam R liam.r.girdwood@intel.com wrote:
On Tue, 2021-05-11 at 15:20 +0000, Daniel Baluta wrote:
It looks like the uint64_t on the sof_ipc_stream_position are not aligned on 8 bytes (with the packing, there are 7 * uint32_t before them) and the ARM compiler is expecting a 8 byte alingment for uint64 ?
I'm not sure why this is showing up now as the "packed" attribute would force the uint64_t to be aligned on 4 bytes instead of 8 here.
Btw, have you tried setting aligned(8) here ? or tried removing the aligned(4) for stream_posn ?
I have tried setting aligned(4) on the Linux kernel header:
--- a/include/sound/sof/stream.h+++ b/include/sound/sof/stream.h @@ -143,6 +143,6 @@ struct sof_ipc_stream_posn { uint64_t timestamp; /**< system time stamp */ uint32_t xrun_comp_id; /**< comp ID of XRUN component */ int32_t xrun_size; /**< XRUN size in bytes */ -} __packed; +} __packed __aligned(4);
It works fine with this.
Ok, this must be an ARM64 difference and I assume you have a kernel fix brewing ?.
Intention on FW side was to use only the L32 and S32 calls and not cater for any unaligned load/stores (with their extra baggage).
Sure. Once, I understand what's exactly happening.
participants (3)
-
Daniel Baluta -
Daniel Baluta -
Girdwood, Liam R