UUID usage on SOF #245
UUID usage on SOF #245
Conversation
keyonjie
requested review from
deb-intel,
intelkevinputnam and
lgirdwood
as code owners
There was a problem hiding this comment.
I think this is good start, but I think we need to level-up a bit and tackle the topic from a component developers point of view. This is now partly in conflict with:
firmware/components/component-overview.rst
... we'd need to write down how component_type and UUID relate to each other. I think that's where most of the arguments stem from (and reason to introduce UUIDs in general).
Yes, there should be joint update in the documentation of the component part.
Let me add a chapter to talk about this. Thanks for the points. |
updated now. @kv2019i |
|
have been busy on some other PM related issues, will update this UUID tomorrow. @lgirdwood @kv2019i |
|
@kv2019i @plbossart @lgirdwood @mmaka1 @ktrzcinx update to reflect to the latest code change in the PRs, please help to review, thanks. |
developer_guides/uuid/index.rst
Outdated
| Why UUID Needed | ||
| *************** | ||
|
|
||
| To develope a new audio signal processing component, we traditionally |
developer_guides/uuid/index.rst
Outdated
|
|
||
| To develope a new audio signal processing component, we traditionally | ||
| implement the component driver with a new unique component type | ||
| introduced, the painpoints with this method including how to keep ABI |
developer_guides/uuid/index.rst
Outdated
| introduced, the painpoints with this method including how to keep ABI | ||
| aligned and backward compatible for different version combination of | ||
| topology/driver/firmware for each new added components, there could be | ||
| component type collision if versions are not aligned. |
There was a problem hiding this comment.
consider a sentence with fewer than 6 lines?
developer_guides/uuid/index.rst
Outdated
| component type collision if versions are not aligned. | ||
|
|
||
| UUIDs (Universally Unique Identifiers) provide a more scalable and | ||
| collision-free way of component identification. UUIDs are currently |
There was a problem hiding this comment.
currently? this is still work in progress?
There was a problem hiding this comment.
I assumed this will get merged after the implementation is merged, so saying "currently" here.
There was a problem hiding this comment.
@keyonjie I wouldn't have picked this, but "currently" gives the impression you are describing something that is valid for the moment but may change later. I think this paragraph describes the intended design with no plan to change, so you can just leave "currently" out.
There was a problem hiding this comment.
OK, let me remove "currently", thanks.
developer_guides/uuid/index.rst
Outdated
| topology driver. | ||
|
|
||
| Allocating a new UUID for the new added component is recommended, as | ||
| the component type might be replaced by UUID in the future. For the |
There was a problem hiding this comment.
so that's unclear now? Is the type replaced yes or no? The previous paragraph strongly hinted that UUID was the only identification method, the use of 'might' makes it a mere possibility. What is the direction?
There was a problem hiding this comment.
um, per my understanding, the component type will be moved out of the IPC structures, but it is still used inside the firmware, let me re-organize the sentence here.
developer_guides/uuid/index.rst
Outdated
| widget (corresponding to the new added component), since we have | ||
| implmented marco to help to handle the converting task, just use the | ||
| exactly same macro ``DECLARE_SOF_RT_UUID`` as depited in the firmware | ||
| source, e.g for src component the below should be addded to the topology |
| ************************** | ||
| Only the UUID arrays for runtime created components are stored to the | ||
| .rodata section as static data, it will occupy small memory, e.g. | ||
| 19 component types * 16 Bytes/component type = 304 Bytes. |
There was a problem hiding this comment.
this is unclear, this implies there are built-time components so where are those UUIDs stored?
There was a problem hiding this comment.
The wording 'runtime' is confused here, let me correct it.
There was a problem hiding this comment.
well this begs a follow-up question. how do we determine which components are used in topology files and which ones are not?
There was a problem hiding this comment.
Basically, the macro 'DECLARE_SOF_RT_UUID' will declare the corresponding UUID static data to the .rodata section. So, if a component(or module, or a pure driver only) is declared with macro 'DECLARE_SOF_RT_UUID' in the firmware, then its UUID should be specified in the topology. Otherwise, when declared with the macro 'DECLARE_SOF_UUID' only, it will be used for the system internal only, the tracing system will use it, but since its UUID will not located in the .rodata section, this will save the memory footprint (we need only 4 Bytes offset for each of this kind of definition).
If you grep the 'DECLARE_SOF_RT_UUID' usage in the firmware source, it is about 39 for the real use at this moment, if UUID of a component is defined in the firmware with 'DECLARE_SOF_RT_UUID', it will occupy 16 Bytes
of the memory, no matter if the component will be used in your topology files or not.
developer_guides/uuid/index.rst
Outdated
| UUID Arrays Stored Section | ||
| ************************** | ||
| Only the UUID arrays for runtime created components are stored to the | ||
| .rodata section as static data, it will occupy small memory, e.g. |
There was a problem hiding this comment.
, using a limited memory footprint, e.g.
developer_guides/uuid/index.rst
Outdated
| ******************************** | ||
| The firmware will use UUID byte array to match the component driver, if | ||
| it is provided from the Linux driver side, otherwise, fallback to use | ||
| the traditional component type for backward compatible issue. |
There was a problem hiding this comment.
backwards-compatible behavior
developer_guides/uuid/index.rst
Outdated
| "new", "old", "old", "supported, offset/reserved 0 passed to the FW, component type will be used." | ||
| "new", "old", "new", "supported, UUID 0s, component type will be used in FW." | ||
| "new", "new", "old", "supported, offset/reserved 0 passed to the FW, component type will be used." | ||
| "new", "new", "new", "supported, UUID will be supported." |
There was a problem hiding this comment.
can we define old and new here?
There was a problem hiding this comment.
Hmm, this is still a bit hard to read. How about:
UUID will be used in component creation only when support is available across the stack in FW, topology and driver. If an older FW/tplg/driver without UUID support is used, component type information is used instead.
Following table describes the firmware behaviour with different combinations of FW/tplg/driver.
.. csv-table:: Firmware/tplg/driver ABI alignment for UUID support
:header: "Firmware", "Topology file", "Linux driver", "Comments"
:widths: 10, 10, 15, 40
"no", "no", "no", "supported, no UUID"
"no", "no", "yes", "supported, UUID 0s, FW ignore it"
"no", "yes", "no", "supported, no UUID"
"no", "yes", "yes", "supported, UUIDs valid, but FW ignore them, component type will be used."
"yes", "no", "no", "supported, offset/reserved 0 passed to the FW, component type will be used."
"yes", "no", "yes", "supported, UUID 0s, component type will be used in FW."
"yes", "yes", "no", "supported, offset/reserved 0 passed to the FW, component type will be used."
"yes", "yes", "yes", "supported, UUID will be supported."
Thanks, looks the dictionary for my spell checker is not good enough. |
|
@plbossart updated. |
developer_guides/uuid/index.rst
Outdated
| component type collision if versions are not aligned. | ||
|
|
||
| UUIDs (Universally Unique Identifiers) provide a more scalable and | ||
| collision-free way of component identification. UUIDs are currently |
There was a problem hiding this comment.
@keyonjie I wouldn't have picked this, but "currently" gives the impression you are describing something that is valid for the moment but may change later. I think this paragraph describes the intended design with no plan to change, so you can just leave "currently" out.
developer_guides/uuid/index.rst
Outdated
| "new", "old", "old", "supported, offset/reserved 0 passed to the FW, component type will be used." | ||
| "new", "old", "new", "supported, UUID 0s, component type will be used in FW." | ||
| "new", "new", "old", "supported, offset/reserved 0 passed to the FW, component type will be used." | ||
| "new", "new", "new", "supported, UUID will be supported." |
There was a problem hiding this comment.
Hmm, this is still a bit hard to read. How about:
UUID will be used in component creation only when support is available across the stack in FW, topology and driver. If an older FW/tplg/driver without UUID support is used, component type information is used instead.
Following table describes the firmware behaviour with different combinations of FW/tplg/driver.
.. csv-table:: Firmware/tplg/driver ABI alignment for UUID support
:header: "Firmware", "Topology file", "Linux driver", "Comments"
:widths: 10, 10, 15, 40
"no", "no", "no", "supported, no UUID"
"no", "no", "yes", "supported, UUID 0s, FW ignore it"
"no", "yes", "no", "supported, no UUID"
"no", "yes", "yes", "supported, UUIDs valid, but FW ignore them, component type will be used."
"yes", "no", "no", "supported, offset/reserved 0 passed to the FW, component type will be used."
"yes", "no", "yes", "supported, UUID 0s, component type will be used in FW."
"yes", "yes", "no", "supported, offset/reserved 0 passed to the FW, component type will be used."
"yes", "yes", "yes", "supported, UUID will be supported."
|
@keyonjie it's probably far simpler to say tha UUID will only be used when ABI is 3.17 or greater on FW, kernel and topology otherwise type is used. This is far easier to understand than a large table. |
developer_guides/uuid/index.rst
Outdated
| *************** | ||
|
|
||
| To develop a new audio signal processing component, we traditionally | ||
| implement the component driver with a new unique component type |
There was a problem hiding this comment.
implemented, it's now the past.
| Allocating a new UUID for the new added component is recommended, as | ||
| the component type will be replaced by UUID in the IPC structures, | ||
| in the future. For the whole SOF stack, the usage of the UUID should | ||
| follow rules as below: |
There was a problem hiding this comment.
again you are using recommended and 'should' when it's now mandatory. You have to be consistent with the language used between paragraphs.
developer_guides/uuid/index.rst
Outdated
| **************** | ||
| The same UUID shall be used in topology .m4 file for a new added | ||
| widget (corresponding to the new added component), since we have | ||
| implemented macro to help to handle the converting task, just use the |
developer_guides/uuid/index.rst
Outdated
|
|
||
| Linux Topology Driver | ||
| ********************* | ||
| The topology driver will parse the 16Bytes UUID token, append it to the |
There was a problem hiding this comment.
16-byte UUID token
the grammar is N- , and unit is singular if I am not mistaken.
| ************************** | ||
| Only the UUID arrays for runtime created components are stored to the | ||
| .rodata section as static data, it will occupy small memory, e.g. | ||
| 19 component types * 16 Bytes/component type = 304 Bytes. |
There was a problem hiding this comment.
well this begs a follow-up question. how do we determine which components are used in topology files and which ones are not?
| tracing subsystem, the topology .m4 files, and the Linux topology driver. | ||
| Using the ``type`` to define topology and create component is still supported | ||
| today, but the ``type`` could be moved out of the IPC struct in the future, | ||
| so allocating UUID for the new added component driver is strongly recommended. |
Thanks for patience and mentoring on this, will update them next. |
Add a page to describe how to use UUID on SOF, the UUID implementation should follow this page. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
The UUID now is ready on topology and driver, update the documentation part. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
|
@keyonjie btw, if you are ever unsure abour grammar then please just ask @deb-intel for review or she can also make corrections and reflow the text in English for you. |
|
@lgirdwood Thanks for your endorsement. I am happy to help with language. I really enjoy language, especially removing ambiguity for our readers. @plbossart is correct in that we need to make sure readers know when something is required and when something is optional or even simply encouraged. The more modern (and direct) term for "shall" is "must." If something is not required but we want readers to know that they CAN do it if they want, we can use language such as "you can," "you may," "you might want to . . .," "we recommend that you . . .," or "consider doing this . . ." The easiest way to imply that readers must perform certain actions is to use active verbs in lists. Actions that are optional or highly recommended can be indicated as such:
See all of the cool things we can do with language?! :-) |
Add a page to describe how to use UUID on SOF, the UUID will be the replacement of the traditional component type.