[0/5] ASoC: Intel: IPC framework updates
mbox series

Message ID 20190720194532.23682-1-cezary.rojewski@intel.com
Headers show
Series
  • ASoC: Intel: IPC framework updates
Related show

Message

Cezary Rojewski July 20, 2019, 7:45 p.m. UTC
Existing IPC framework omits crucial part of the entire communication:
reply header. Some IPCs cannot function at all without the access to
said header. Update the sst-ipc with new sst_ipc_message structure to
represent both request and reply allowing reply-processing handlers to
save received responses.

Despite the range of changes required for model to be updated, no
functional changes are made for core hanswell, baytrail and skylake
message handlers. Reply-processing handlers now save received response
header yet no usage is added by default.

To allow for future changes, righful kings of IPC kingdom need to be put
back on the throne. This update addresses one of them: LARGE_CONFIG_GET.

Cezary Rojewski (5):
  ASoC: Intel: Update request-reply IPC model
  ASoC: Intel: Haswell: Align with updated request-reply model
  ASoC: Intel: Baytrail: Align with updated request-reply model
  ASoC: Intel: Skylake: Align with updated request-reply model
  ASoC: Intel: Skylake: large_config_get overhaul

 sound/soc/intel/baytrail/sst-baytrail-ipc.c |  65 ++++----
 sound/soc/intel/common/sst-ipc.c            |  68 ++++----
 sound/soc/intel/common/sst-ipc.h            |  27 ++--
 sound/soc/intel/haswell/sst-haswell-ipc.c   | 164 +++++++++++---------
 sound/soc/intel/skylake/cnl-sst.c           |   6 +-
 sound/soc/intel/skylake/skl-messages.c      |   3 +-
 sound/soc/intel/skylake/skl-sst-ipc.c       | 152 ++++++++++--------
 sound/soc/intel/skylake/skl-sst-ipc.h       |   3 +-
 8 files changed, 259 insertions(+), 229 deletions(-)

Comments

Pierre-Louis Bossart July 22, 2019, 4:02 p.m. UTC | #1
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
> Existing IPC framework omits crucial part of the entire communication:
> reply header. Some IPCs cannot function at all without the access to
> said header. Update the sst-ipc with new sst_ipc_message structure to
> represent both request and reply allowing reply-processing handlers to
> save received responses.

I don't get the idea at all.

the DSP provides rx_data and a size. If there is a header at the start 
of the IPC data in some cases, why can't you cast and extract the header 
when it's needed for your SKL usages? Why does adding a header make the 
IPC work better?

I have the feeling this is blurring layers between receiving data and 
processing it in platform-specific ways, and I am nervous about touching 
Baytrail and Broadwell legacy, which are way past maintenance mode and 
should be deprecated really.

And last this patchset does not apply on top of Mark's tree?

> 
> Despite the range of changes required for model to be updated, no
> functional changes are made for core hanswell, baytrail and skylake
> message handlers. Reply-processing handlers now save received response
> header yet no usage is added by default.
> 
> To allow for future changes, righful kings of IPC kingdom need to be put
> back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
> 
> Cezary Rojewski (5):
>    ASoC: Intel: Update request-reply IPC model
>    ASoC: Intel: Haswell: Align with updated request-reply model
>    ASoC: Intel: Baytrail: Align with updated request-reply model
>    ASoC: Intel: Skylake: Align with updated request-reply model
>    ASoC: Intel: Skylake: large_config_get overhaul
> 
>   sound/soc/intel/baytrail/sst-baytrail-ipc.c |  65 ++++----
>   sound/soc/intel/common/sst-ipc.c            |  68 ++++----
>   sound/soc/intel/common/sst-ipc.h            |  27 ++--
>   sound/soc/intel/haswell/sst-haswell-ipc.c   | 164 +++++++++++---------
>   sound/soc/intel/skylake/cnl-sst.c           |   6 +-
>   sound/soc/intel/skylake/skl-messages.c      |   3 +-
>   sound/soc/intel/skylake/skl-sst-ipc.c       | 152 ++++++++++--------
>   sound/soc/intel/skylake/skl-sst-ipc.h       |   3 +-
>   8 files changed, 259 insertions(+), 229 deletions(-)
>
Cezary Rojewski July 22, 2019, 6:32 p.m. UTC | #2
On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
> 
> 
> On 7/20/19 2:45 PM, Cezary Rojewski wrote:
>> Existing IPC framework omits crucial part of the entire communication:
>> reply header. Some IPCs cannot function at all without the access to
>> said header. Update the sst-ipc with new sst_ipc_message structure to
>> represent both request and reply allowing reply-processing handlers to
>> save received responses.
> 
> I don't get the idea at all.

It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, 
LARGE_CONFIG_GET and such define portion of their essential _payload_ 
within header instead of actual SRAM window. Some of these contain 
entire _payload_ within header instead - as already stated.

> the DSP provides rx_data and a size. If there is a header at the start 
> of the IPC data in some cases, why can't you cast and extract the header 
> when it's needed for your SKL usages? Why does adding a header make the 
> IPC work better?

This is cAVS solution we speak of, not an aDSP one. Not all IPCs have 
constant size defined here.
Moreover, define "size" - from architectural point of view this is 
*only* size an application is prepared to handle, it's usually 
overestimated. The actual size of _get_ is known only after reply is 
received. If it exceeds frame's window, then it gets more complicated..

"If there is a header at the start of the IPC data in some cases, why 
can't you cast and extract the header (...)"
??
An IPC Header - primary and extension - is found within Host HW 
registers e.g.: HICPI & HIPCIE what differs from data - payload - which 
is located in SRAM windows: downlink and uplink - depending on the 
direction. Saving header does not "make the IPC work better" - it allows 
them to function in the first place. Again, this is not the case for all 
IPCs.

Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) 
makes no sense if you do not process them correctly. I know not why 
dysfunctional code has been upstreamed or why does it not follow cAVS 
design pattern. All the series being posted currently aim to correct the 
very fundations of Skylake which are/ were broken, clearly.

> 
> I have the feeling this is blurring layers between receiving data and 
> processing it in platform-specific ways, and I am nervous about touching 
> Baytrail and Broadwell legacy, which are way past maintenance mode and 
> should be deprecated really.

I'm not nervous at all. Baytrail IPC is deprecated in favor of /atom/sst 
- God knows why yet another _common_ folder has been created there..
Haswell with DSP does not really exist.
Broadwell is the only one left and my changes only _update_ the 
framework - these do not change its behavior.

Truth be told, it did not age well at all. 99% of Skylake+ IPC 
communication is done in synchronized fashion yet during initialization 
there are always 8 messages allocated (times two: tx & rx). In total we 
are allocating 8 x 2 x PAGE_SIZE memory. These days memory might not be 
a problem, though does not change the fact it could have been done 
better. Lot of wasted memory..
There's certainly more to do, that's the message.

> 
> And last this patchset does not apply on top of Mark's tree?

I can recheck this, been a week since I did a snapshot of 5.3. Thanks 
for heads up.

> 
>>
>> Despite the range of changes required for model to be updated, no
>> functional changes are made for core hanswell, baytrail and skylake
>> message handlers. Reply-processing handlers now save received response
>> header yet no usage is added by default.
>>
>> To allow for future changes, righful kings of IPC kingdom need to be put
>> back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
Pierre-Louis Bossart July 22, 2019, 7:05 p.m. UTC | #3
On 7/22/19 1:32 PM, Cezary Rojewski wrote:
> On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
>>
>>
>> On 7/20/19 2:45 PM, Cezary Rojewski wrote:
>>> Existing IPC framework omits crucial part of the entire communication:
>>> reply header. Some IPCs cannot function at all without the access to
>>> said header. Update the sst-ipc with new sst_ipc_message structure to
>>> represent both request and reply allowing reply-processing handlers to
>>> save received responses.
>>
>> I don't get the idea at all.
> 
> It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, 
> LARGE_CONFIG_GET and such define portion of their essential _payload_ 
> within header instead of actual SRAM window. Some of these contain 
> entire _payload_ within header instead - as already stated.

No one outside of your team has a clue what cAVS design means, so we'll 
have to agree on what this really means, see below. seems to me like 
'header' refers to a hardware capability, not the data format.

> 
>> the DSP provides rx_data and a size. If there is a header at the start 
>> of the IPC data in some cases, why can't you cast and extract the 
>> header when it's needed for your SKL usages? Why does adding a header 
>> make the IPC work better?
> 
> This is cAVS solution we speak of, not an aDSP one. Not all IPCs have 
> constant size defined here.
> Moreover, define "size" - from architectural point of view this is 
> *only* size an application is prepared to handle, it's usually 
> overestimated. The actual size of _get_ is known only after reply is 
> received. If it exceeds frame's window, then it gets more complicated..
> 
> "If there is a header at the start of the IPC data in some cases, why 
> can't you cast and extract the header (...)"
> ??
> An IPC Header - primary and extension - is found within Host HW 
> registers e.g.: HICPI & HIPCIE what differs from data - payload - which 
> is located in SRAM windows: downlink and uplink - depending on the 
> direction. Saving header does not "make the IPC work better" - it allows 
> them to function in the first place. Again, this is not the case for all 
> IPCs.

The limited understanding I have of the architecture is that the IPC can 
be done either by transmitting compressed information in an IPC 
register, using up to 60 bits of information, or a full-blown version in 
SRAM windows - with the caveat that the SRAM windows may not be 
available in all power states. All four combinations between TX and RX 
are possible, e.g. you can transmit a command over the IPC register and 
get a reply in the SRAM window.
I am not aware of a case where the IPC register and SRAM window contain 
unrelated information, it's either self-contained in the IPC register or 
the IPC register has a qualifier to help interpret the SRAM window 
contents. Please correct me here if these explanations are incorrect.

This solution works for SKL+ only, so I am really wondering why you'd 
want to align legacy platforms with newer stuff. Not to mention that 
there are two versions of the IPC registers for cAVS.

> 
> Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) 
> makes no sense if you do not process them correctly. I know not why 
> dysfunctional code has been upstreamed or why does it not follow cAVS 
> design pattern. All the series being posted currently aim to correct the 
> very fundations of Skylake which are/ were broken, clearly.

Since Skylake+ has a fundamentally different way of doing things, and 
likely more changes required down the road, why not create your own IPC 
layer when you can knock yourself out, rather than changing stuff that 
doesn't need any of this?

>>
>> I have the feeling this is blurring layers between receiving data and 
>> processing it in platform-specific ways, and I am nervous about 
>> touching Baytrail and Broadwell legacy, which are way past maintenance 
>> mode and should be deprecated really.
> 
> I'm not nervous at all. Baytrail IPC is deprecated in favor of /atom/sst 
> - God knows why yet another _common_ folder has been created there..

deprecated doesn't mean removed, there are still users and we don't 
break their solution.

> Haswell with DSP does not really exist.
> Broadwell is the only one left and my changes only _update_ the 
> framework - these do not change its behavior.

When you change something that impacts multiple platforms, the 
expectation is that it's been tested on other platforms. Precisely when 
something is deprecated you either don't touch it or make sure the code 
changes are tested. And with the difficulty getting access to platforms 
- we have exactly two Broadwell devices with I2S - I am inclined to 
avoid any changes.

> Truth be told, it did not age well at all. 99% of Skylake+ IPC 
> communication is done in synchronized fashion yet during initialization 
> there are always 8 messages allocated (times two: tx & rx). In total we 
> are allocating 8 x 2 x PAGE_SIZE memory. These days memory might not be 
> a problem, though does not change the fact it could have been done 
> better. Lot of wasted memory..

I don't know what this has to do with this patchset, but it's pretty 
common to allocate multiple messages to queue them up?
Cezary Rojewski July 22, 2019, 8:21 p.m. UTC | #4
On 2019-07-22 21:05, Pierre-Louis Bossart wrote:
> 
> 
> On 7/22/19 1:32 PM, Cezary Rojewski wrote:
>> On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 7/20/19 2:45 PM, Cezary Rojewski wrote:
>>>> Existing IPC framework omits crucial part of the entire communication:
>>>> reply header. Some IPCs cannot function at all without the access to
>>>> said header. Update the sst-ipc with new sst_ipc_message structure to
>>>> represent both request and reply allowing reply-processing handlers to
>>>> save received responses.
>>>
>>> I don't get the idea at all.
>>
>> It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, 
>> LARGE_CONFIG_GET and such define portion of their essential _payload_ 
>> within header instead of actual SRAM window. Some of these contain 
>> entire _payload_ within header instead - as already stated.
> 
> No one outside of your team has a clue what cAVS design means, so we'll 
> have to agree on what this really means, see below. seems to me like 
> 'header' refers to a hardware capability, not the data format.
> 
>>
>>> the DSP provides rx_data and a size. If there is a header at the 
>>> start of the IPC data in some cases, why can't you cast and extract 
>>> the header when it's needed for your SKL usages? Why does adding a 
>>> header make the IPC work better?
>>
>> This is cAVS solution we speak of, not an aDSP one. Not all IPCs have 
>> constant size defined here.
>> Moreover, define "size" - from architectural point of view this is 
>> *only* size an application is prepared to handle, it's usually 
>> overestimated. The actual size of _get_ is known only after reply is 
>> received. If it exceeds frame's window, then it gets more complicated..
>>
>> "If there is a header at the start of the IPC data in some cases, why 
>> can't you cast and extract the header (...)"
>> ??
>> An IPC Header - primary and extension - is found within Host HW 
>> registers e.g.: HICPI & HIPCIE what differs from data - payload - 
>> which is located in SRAM windows: downlink and uplink - depending on 
>> the direction. Saving header does not "make the IPC work better" - it 
>> allows them to function in the first place. Again, this is not the 
>> case for all IPCs.
> 
> The limited understanding I have of the architecture is that the IPC can 
> be done either by transmitting compressed information in an IPC 
> register, using up to 60 bits of information, or a full-blown version in 
> SRAM windows - with the caveat that the SRAM windows may not be 
> available in all power states. All four combinations between TX and RX 
> are possible, e.g. you can transmit a command over the IPC register and 
> get a reply in the SRAM window.
> I am not aware of a case where the IPC register and SRAM window contain 
> unrelated information, it's either self-contained in the IPC register or 
> the IPC register has a qualifier to help interpret the SRAM window 
> contents. Please correct me here if these explanations are incorrect.
> 
> This solution works for SKL+ only, so I am really wondering why you'd 
> want to align legacy platforms with newer stuff. Not to mention that 
> there are two versions of the IPC registers for cAVS.

Two versions of IPC registers is just a mapping thingy, IPC framework 
does not care about that, leaves that to the caller.
Legacy platforms won't notice the difference at all. Status quo is achieved.

If current _get_large_config is not enough for you, see 
GET_PIPELINE_STATE - first 5 bits of reply.header.extension contain 
actual data. Now, how do I get that info using current framework 
capabilities?

About the architecture. SOF's design is different, cAVS (/skylake) 
always makes use of host registers (IPC header is ALWAYS found there and 
is never located within SRAM window) with SRAM being optional and 
context dependent - data that cannot be contained with the very 60 
available bits is found there.

>>
>> Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) 
>> makes no sense if you do not process them correctly. I know not why 
>> dysfunctional code has been upstreamed or why does it not follow cAVS 
>> design pattern. All the series being posted currently aim to correct 
>> the very fundations of Skylake which are/ were broken, clearly.
> 
> Since Skylake+ has a fundamentally different way of doing things, and 
> likely more changes required down the road, why not create your own IPC 
> layer when you can knock yourself out, rather than changing stuff that 
> doesn't need any of this?

Does it really differ that much when it comes to handling IPCs?
Me re-implementing - or let's call it what it would really be: copying - 
IPC framework into /skylake is a lazy option. I prefer doing _simple_ 
changes to an existing /common stuff and thus reducing burden of 
maintenance in months to come.

There are fields and methods /skylake does not require too and yet these 
are there only for the sake of legacy platforms. Long time ago someone 
decided to make them all share one framework. Do you think splitting 
these now is a good idea?

>>>
>>> I have the feeling this is blurring layers between receiving data and 
>>> processing it in platform-specific ways, and I am nervous about 
>>> touching Baytrail and Broadwell legacy, which are way past 
>>> maintenance mode and should be deprecated really.
>>
>> I'm not nervous at all. Baytrail IPC is deprecated in favor of 
>> /atom/sst - God knows why yet another _common_ folder has been created 
>> there..
> 
> deprecated doesn't mean removed, there are still users and we don't 
> break their solution.

Sure thing, wasn't my intention - that's why changes maintain status quo.

>> Haswell with DSP does not really exist.
>> Broadwell is the only one left and my changes only _update_ the 
>> framework - these do not change its behavior.
> 
> When you change something that impacts multiple platforms, the 
> expectation is that it's been tested on other platforms. Precisely when 
> something is deprecated you either don't touch it or make sure the code 
> changes are tested. And with the difficulty getting access to platforms 
> - we have exactly two Broadwell devices with I2S - I am inclined to 
> avoid any changes.

Gathered some legacy platforms, but yeah I was not fortunate yet to test 
these out. Validating on several different platforms starting from SKL 
instead.

Here to makes things right, hope we all are. Skylake+ has been 
dysfunctional for long enough. I'd understand the concern if these were 
functional changes to legacy platforms. They aren't really. Would like 
not to delay the resurrection process any longer.

>> Truth be told, it did not age well at all. 99% of Skylake+ IPC 
>> communication is done in synchronized fashion yet during 
>> initialization there are always 8 messages allocated (times two: tx & 
>> rx). In total we are allocating 8 x 2 x PAGE_SIZE memory. These days 
>> memory might not be a problem, though does not change the fact it 
>> could have been done better. Lot of wasted memory..
> 
> I don't know what this has to do with this patchset, but it's pretty 
> common to allocate multiple messages to queue them up?
Pierre-Louis Bossart July 22, 2019, 9:12 p.m. UTC | #5
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
> Existing IPC framework omits crucial part of the entire communication:
> reply header. Some IPCs cannot function at all without the access to
> said header. Update the sst-ipc with new sst_ipc_message structure to
> represent both request and reply allowing reply-processing handlers to
> save received responses.
> 
> Despite the range of changes required for model to be updated, no
> functional changes are made for core hanswell, baytrail and skylake
> message handlers. Reply-processing handlers now save received response
> header yet no usage is added by default.
> 
> To allow for future changes, righful kings of IPC kingdom need to be put
> back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
> 
> Cezary Rojewski (5):
>    ASoC: Intel: Update request-reply IPC model

I had a doubt on the structure of this patchset since you first change 
the structure definitions/prototypes and then use them in follow-up 
patches, and sure enough if I apply the first patch only the compilation 
is broken, see below.

The rule is that we can't break git bisect. And if you squash the 
patches together, then you have a really complicated patch to 
review/test, so like I said earlier such invasive changes in shared 
prototypes are really painful.

>    ASoC: Intel: Haswell: Align with updated request-reply model
>    ASoC: Intel: Baytrail: Align with updated request-reply model
>    ASoC: Intel: Skylake: Align with updated request-reply model
>    ASoC: Intel: Skylake: large_config_get overhaul
> 
>   sound/soc/intel/baytrail/sst-baytrail-ipc.c |  65 ++++----
>   sound/soc/intel/common/sst-ipc.c            |  68 ++++----
>   sound/soc/intel/common/sst-ipc.h            |  27 ++--
>   sound/soc/intel/haswell/sst-haswell-ipc.c   | 164 +++++++++++---------
>   sound/soc/intel/skylake/cnl-sst.c           |   6 +-
>   sound/soc/intel/skylake/skl-messages.c      |   3 +-
>   sound/soc/intel/skylake/skl-sst-ipc.c       | 152 ++++++++++--------
>   sound/soc/intel/skylake/skl-sst-ipc.h       |   3 +-
>   8 files changed, 259 insertions(+), 229 deletions(-)


  CC [M]  sound/soc/intel/baytrail/sst-baytrail-ipc.o
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_update’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:214:18: 
error: ‘struct ipc_message’ has no member named ‘header’
   u64 header = msg->header;
                   ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_process_reply’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:244:6: 
error: ‘struct ipc_message’ has no member named ‘rx_size’
    msg->rx_size = sst_byt_header_data(header);
       ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:245:35: 
error: ‘struct ipc_message’ has no member named ‘rx_data’
    sst_dsp_inbox_read(byt->dsp, msg->rx_data, msg->rx_size);
                                    ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:245:49: 
error: ‘struct ipc_message’ has no member named ‘rx_size’
    sst_dsp_inbox_read(byt->dsp, msg->rx_data, msg->rx_size);
                                                  ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_commit’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:43: 
error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’
   ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req,
                                            ^~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: 
note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ 
{aka ‘long long unsigned int’}
   struct sst_ipc_message request, struct sst_ipc_message *reply);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:51: 
error: passing argument 3 of ‘sst_ipc_tx_message_wait’ from incompatible 
pointer type [-Werror=incompatible-pointer-types]
   ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req,
                                                    ^~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:58: 
note: expected ‘struct sst_ipc_message *’ but argument is of type 
‘struct sst_byt_alloc_params *’
   struct sst_ipc_message request, struct sst_ipc_message *reply);
                                   ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:8: 
error: too many arguments to function ‘sst_ipc_tx_message_wait’
   ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req,
         ^~~~~~~~~~~~~~~~~~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: 
note: declared here
  int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc,
      ^~~~~~~~~~~~~~~~~~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_free’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:442:43: 
error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’
   ret = sst_ipc_tx_message_wait(&byt->ipc, header, NULL, 0, NULL, 0);
                                            ^~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: 
note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ 
{aka ‘long long unsigned int’}
   struct sst_ipc_message request, struct sst_ipc_message *reply);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:442:8: 
error: too many arguments to function ‘sst_ipc_tx_message_wait’
   ret = sst_ipc_tx_message_wait(&byt->ipc, header, NULL, 0, NULL, 0);
         ^~~~~~~~~~~~~~~~~~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: 
note: declared here
  int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc,
      ^~~~~~~~~~~~~~~~~~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_operations’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:466:45: 
error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’
    return sst_ipc_tx_message_wait(&byt->ipc, header, NULL,
                                              ^~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: 
note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ 
{aka ‘long long unsigned int’}
   struct sst_ipc_message request, struct sst_ipc_message *reply);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:466:10: 
error: too many arguments to function ‘sst_ipc_tx_message_wait’
    return sst_ipc_tx_message_wait(&byt->ipc, header, NULL,
           ^~~~~~~~~~~~~~~~~~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: 
note: declared here
  int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc,
      ^~~~~~~~~~~~~~~~~~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:469:47: 
error: incompatible type for argument 2 of ‘sst_ipc_tx_message_nowait’
    return sst_ipc_tx_message_nowait(&byt->ipc, header,
                                                ^~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:73:25: 
note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ 
{aka ‘long long unsigned int’}
   struct sst_ipc_message request);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:469:10: 
error: too many arguments to function ‘sst_ipc_tx_message_nowait’
    return sst_ipc_tx_message_nowait(&byt->ipc, header,
           ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:72:5: 
note: declared here
  int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc,
      ^~~~~~~~~~~~~~~~~~~~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_start’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:490:45: 
error: incompatible type for argument 2 of ‘sst_ipc_tx_message_nowait’
   ret = sst_ipc_tx_message_nowait(&byt->ipc, header, tx_msg, size);
                                              ^~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:73:25: 
note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ 
{aka ‘long long unsigned int’}
   struct sst_ipc_message request);
   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:490:8: 
error: too many arguments to function ‘sst_ipc_tx_message_nowait’
   ret = sst_ipc_tx_message_nowait(&byt->ipc, header, tx_msg, size);
         ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from 
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:72:5: 
note: declared here
  int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc,
      ^~~~~~~~~~~~~~~~~~~~~~~~~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘byt_tx_msg’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:626:9: 
error: ‘struct ipc_message’ has no member named ‘header’
   if (msg->header & IPC_HEADER_LARGE(true))
          ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:627:37: 
error: ‘struct ipc_message’ has no member named ‘tx_data’
    sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size);
                                      ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:627:51: 
error: ‘struct ipc_message’ has no member named ‘tx_size’
    sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size);
                                                    ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:629:55: 
error: ‘struct ipc_message’ has no member named ‘header’
   sst_dsp_shim_write64_unlocked(ipc->dsp, SST_IPCX, msg->header);
                                                        ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘byt_tx_data_copy’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:651:13: 
error: ‘struct ipc_message’ has no member named ‘tx_data’
   *(u32 *)msg->tx_data = (u32)(msg->header & (u32)-1);
              ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:651:34: 
error: ‘struct ipc_message’ has no member named ‘header’
   *(u32 *)msg->tx_data = (u32)(msg->header & (u32)-1);
                                   ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:652:12: 
error: ‘struct ipc_message’ has no member named ‘tx_data’
   memcpy(msg->tx_data + sizeof(u32), tx_data, tx_size);
             ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:653:5: 
error: ‘struct ipc_message’ has no member named ‘tx_size’
   msg->tx_size += sizeof(u32);
      ^~
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: 
In function ‘sst_byt_stream_operations’:
/data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:471:1: 
warning: control reaches end of non-void function [-Wreturn-type]
  }
  ^
cc1: some warnings being treated as errors
Cezary Rojewski July 22, 2019, 9:37 p.m. UTC | #6
On 2019-07-22 23:12, Pierre-Louis Bossart wrote:
> 
> 
> On 7/20/19 2:45 PM, Cezary Rojewski wrote:
>> Existing IPC framework omits crucial part of the entire communication:
>> reply header. Some IPCs cannot function at all without the access to
>> said header. Update the sst-ipc with new sst_ipc_message structure to
>> represent both request and reply allowing reply-processing handlers to
>> save received responses.
>>
>> Despite the range of changes required for model to be updated, no
>> functional changes are made for core hanswell, baytrail and skylake
>> message handlers. Reply-processing handlers now save received response
>> header yet no usage is added by default.
>>
>> To allow for future changes, righful kings of IPC kingdom need to be put
>> back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
>>
>> Cezary Rojewski (5):
>>    ASoC: Intel: Update request-reply IPC model
> 
> I had a doubt on the structure of this patchset since you first change 
> the structure definitions/prototypes and then use them in follow-up 
> patches, and sure enough if I apply the first patch only the compilation 
> is broken, see below.
> 
> The rule is that we can't break git bisect. And if you squash the 
> patches together, then you have a really complicated patch to 
> review/test, so like I said earlier such invasive changes in shared 
> prototypes are really painful.

Thanks for your time and input, Pierre!

Agreed on the patchset structure. This wasn't a random mistake, though. 
Knew that meshing them all together immediately (v1) would be very hard 
for readers to review, despite the _simplicity_ of actual solution: 
explicit listing of message parts -> containment within sst_ipc_message.

I'll combine them together - except for the large_config_get one. Had 
these issues been addressed earlier, patches such as this wouldn't have 
been needed at all ;/

Czarek
Mark Brown July 23, 2019, 4:59 p.m. UTC | #7
On Mon, Jul 22, 2019 at 04:12:19PM -0500, Pierre-Louis Bossart wrote:

> The rule is that we can't break git bisect. And if you squash the patches
> together, then you have a really complicated patch to review/test, so like I
> said earlier such invasive changes in shared prototypes are really painful.

Yeah, I build test each commit as they get applied (though not
everything gets covered so things can get missed) and it's generally
helpful for people doing things like bisection.