Skip to content

Conversation

@lkoenig
Copy link
Contributor

@lkoenig lkoenig commented Mar 8, 2022

sof-ctl assumed the binary control were of size of a
multiple of unsigned integer.
This caused the control byte array to be truncated when the
size of the control was 6 bytes.
This change allow setting any size of control in binary form.
Using CSV will always end up with a number of unsigned integer size.

I tested this change using the Google RTC Audio processing with control size of
6 bytes, 2 bytes and 4 bytes.

Signed-off-by: Lionel Koenig lionelk@google.com

@lkoenig lkoenig requested a review from singalsu as a code owner March 8, 2022 12:28
@lkoenig lkoenig requested review from bzhg and cujomalainey March 8, 2022 12:29
@kv2019i kv2019i requested review from juimonen and lgirdwood March 8, 2022 12:39
@lgirdwood
Copy link
Member

@lkoenig I assume no impact for userspace clients or FW ABI that may use this command ?

@lkoenig
Copy link
Contributor Author

lkoenig commented Mar 9, 2022

FW ABI is unchanged.
Userspace command line is unchanged. I do not understand what you meant by userspace clients ?
What changed is that if someone call sof-ctl to set a binary control, the binary file will be read as is (all the bytes) and passed as is to the control via the ALSA interface.

Prior this change sof-ctl read those binary files in unsigned int assuming the size of the binary file would be a multiple of 4 bytes. When I tried to set a 2 bytes control it got rounded to 0 bytes control.
The firmware already supports no multiple of 4 bytes control as the ABI header specify the size of the data in bytes.
Hopefully I am clear in my explaination.

tools/ctl/ctl.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

just thinking for the arm folk, if this is running on arm 32-bit, isn't an unsigned int 16bits? Shouldnt this be uint32_t?

Copy link
Member

Choose a reason for hiding this comment

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

@dbaluta @paulstelian97 @afq984 @kuanhsuncheng any comments here for ARM32 ?

Copy link
Contributor

@afq984 afq984 Mar 16, 2022

Choose a reason for hiding this comment

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

if this is running on arm 32-bit, isn't an unsigned int 16bits?

I believe it depends on the toolchain (edit: or ABI?), but it's 4 on clang 11 and gcc 11: https://godbolt.org/z/bo8r5as1b

The comment use unsigned int to ensure alignment sounds a bit confusing to me. It doesn't ensure a misaligned pointer can't be stored into the buffer such as buffer = (unsigned int*)(uintptr_t)3.
Does it mean access to the buffer must be have an address of a multiple of 4?

Copy link
Collaborator

@paulstelian97 paulstelian97 Mar 16, 2022

Choose a reason for hiding this comment

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

@lgirdwood I have no idea about 32-bit ARM issues since all the variants of i.MX8 are 64-bit.

Copy link
Member

Choose a reason for hiding this comment

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

Using the uint32_t and friends here would guarantee compliance no matter the underlying architecture. @lkoenig is this something you can update ?

Copy link
Contributor

@cujomalainey cujomalainey Mar 16, 2022

Choose a reason for hiding this comment

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

Agreed, I think we should in general be defaulting to inttypes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some update there.

sof-ctl assumed the binary control were of size of a
multiple of unsigned integer.
This caused the control byte array to be truncated when the
size of the control was 6 bytes.
This change allow setting any size of control in binary form.
Using CSV will always end up with a number of unsigned integer size.

I tested this change using the Google RTC Audio processing with control
of size:
  - 6 bytes,
  - 2 bytes
  - and 4 bytes.

Signed-off-by: Lionel Koenig <lionelk@google.com>
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Thanks @lkoenig - this should be fine for 32bit users.

@lgirdwood lgirdwood merged commit b0b8bc7 into thesofproject:main Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants