Skip to content

Conversation

@plbossart
Copy link
Member

This field doesn't seem to be used, remove.
I don't believe there is a need for an ABI bump since it's not used on either side.

Don't merge for now since we may want to actually use this field to define the endianness of the compressed page table as discussed in #652

We set this field to zero and never used it again. The firmware does not
use it. Yank.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We set this field to zero and never used it again. The firmware does
not use it. Yank.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This field doesn't seem to be used by anyone, reclaim as reserved.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart force-pushed the fix/ipc-host-buffer branch from c286ad6 to e0e5a43 Compare March 6, 2019 02:14
@keyonjie
Copy link

keyonjie commented Mar 6, 2019

I planed to use this for the non-0 offset stream(including trace) resuming, #248

I prefer to keep this offset which might be more generic definition for the buffer start.

@plbossart
Copy link
Member Author

@keyonjie with all due respect you haven't touched this in 3.5 months and no one has any plans to make this happen in the next 4 months. If this feature is needed we do it well, or we don't do it at all.

@keyonjie
Copy link

keyonjie commented Mar 6, 2019

@keyonjie with all due respect you haven't touched this in 3.5 months and no one has any plans to make this happen in the next 4 months. If this feature is needed we do it well, or we don't do it at all.

@plbossart As the implementation of the feature may vary for different platforms(e.g. SKL- and SKL+), so we need to consider more and align on how to implement it.

There is an enhancement(thesofproject/sof#544) in FW side(only needed for SKL-), but nobody is assigned to work on that yet, so...

@ranj063
Copy link
Collaborator

ranj063 commented Mar 6, 2019

LGTM. @keyonjie may have a point but I think its better to add it when needed to avoid unnecessary confusion

@keyonjie
Copy link

keyonjie commented Mar 6, 2019

I am fine to remove it. This is actually already there in the original code, my try to use it in #248 is only a thought about using it. If it don't introduce extra ABI bump, feel free to remove it or add it back when needed.

@plbossart
Copy link
Member Author

What this thread shows is that we don't have a good control of what we work on, there are enhancements that are parked without any plans, we need to collectively do a better job at
a) reviewing the issue/enhancement and suggested code
b) map its integration into a release
c) assign it to developer(s)
d) test it.

@plbossart
Copy link
Member Author

@keyonjie thinking a bit more on this, even if we have a field that is already present in the IPC structure, if it's not used it cannot be considered as part of the existing ABI. Conversely, the moment we start using such a field with a non-zero value that has a functional impact, we need to make sure that the firmware actually supports this capability by checking the ABI version.
In other words, let's not have any forward-looking fields that are set-but-not-used, it's a sure way to break backwards compatibility later on.
If there are other cases in the IPC you are aware of, please let me know and I'll also remove them to better handle backwards compatibility later.
Thanks!

@plbossart
Copy link
Member Author

let's merge this and revisit later if we need an actual ABI change/functionality.

@plbossart plbossart merged commit 19fe5aa into thesofproject:topic/sof-dev Mar 7, 2019
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.

3 participants