[alsa-devel] [RESEND PATCH v3] ASoC: Intel: Skylake: large_config_get overhaul

[Cezary Rojewski]

On 2019-08-07 22:00, Pierre-Louis Bossart wrote:
> I find this patch quite convoluted, so I ordered my comments to make 
> them linear.

Sure thing, thanks for feedback!

>> LARGE_CONFIG_GET is among the most crucial IPCs. Host is expected to
>> send single request first to obtain total payload size from received
>> header's data_off_size. From then on, it loops for each frame exceeding
>> inbox size until entire payload is read. If entire payload is contained
>> within the very first frame, no looping is performed.
> 
> so there's got to be a test that defines if looping is required or not 
> but see [1] below.
> 

Precisely! That is also why reply handling was needed (already applied 
by Mark). Without "data_off_size" field retrieved from reply's header 
(extension), you're in the dark when it comes to payload size.

>>
>> LARGE_CONFIG_GET is a flexible IPC, it not only receives payload but may
>> carry one with them to provide list of params DSP module should return
>> info for. This behavior is usually reserved for vendors and IPC handler
>> should not touch that content. To achieve that, simply pass provided
>> payload and bytes to sst_ipc_tx_message_wait as a part of request.
> 
> [2] And this has nothing to do with the loops,  does it?

If I'm to dive into details so be it. _get_ operates in 3 modes:

1) single request : single reply (requests fits into single frame)
	1 config param to retrieve
	fw status/ fw error code will point directly what's going on
	(after all there is just one param here..)
	param_id < 0xFF
	data_off_size has single meaning here: payload size

2) large request, spanning over several frames : as many replies as 
required (dsp reply.header.extension.final_block == 1)
	1 config param to retrieve
	fw status/ fw error code will point directly what's going on
	(after all there is just one param here..)
	param_id < 0xFF
	data_off_size switches context here: 
reply.header.extension.data_off_size for the very first reply carries 
TOTAL payload size. From then on, it marks buffer offset size - host 
makes use of this offset and injects data into right pos within his own 
buffer.

3) vendor request : single reply (request MUST fit into single frame)
	X config params to retrieve, each represented by TLV
	fw status/ fw error code provides generic outcome only
	(on failure error-payload is returned. Can be parsed to get idx of TLV, 
from specified TLV array, which caused the failure to occur)
	param_id == 0xFF
	data_off_size has single meaning here: payload size

Error-payload is by no means restricted to 3rd case-only, it's just 
optional and may not be provided by DSP depending on particular request.

> 
>> In current state, LARGE_CONFIG_GET message handler does nothing of that,
>> in consequence making it dysfunctional. Overhaul said handler allowing
>> rightful king of IPCs to return back on his throne - kingdom he shares
>> with his twin brother: LARGE_CONFIG_SET.
> 
> [3] what is dysfunctional? the unnecessary loops or handling data in the 
> IPC that should only be handled in vendor-specific parts. And btw I 
> don't see vendor-specific parts in the Skylake driver, so not sure how 
> those vendor-specific thingies are handled at all.

a) payload size is skipped, this method runs in the dark
b) provides no vendor-request functionality
c) error-payload is never returned
d) wonky toggling of init_block. Host never touches init_block field 
once initial message has been carried out - a book only ever has a 
single prologue, ain't it?

> 
>> The looping has not been added in this update as payloads of such size
>> do not exist in practice. Will need to create custom module specifically
>> for that very case and test against it before that feature can be added.
> 
> [1] here you are just saying that the looping is really not required so 
> there are no tests at all...
> 
> [4] So shouldn't you split the two parts of this patch and separate 
> looping from not touching the data that's vendor-specific?

So, looping mainly gets used in _sets_, for _gets_ I've not seen a live 
example, really - despite FW supporting such flow. However, I'd like to 
verify before adding any looping, possibly by creating a custom module 
myself. Followup to your point: existing looping was not tested either.

There is only one _get_ I can think of which might replace custom module 
approach.. the MODULES_INFO one. You just need a FW binary with bunch of 
features included : ) I'll try out TGL one.

That's why I prefer adding major _get_ fix first and only then a 
separate patch for looping - once tested.

> 
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski at intel.com>
>> ---
>>   sound/soc/intel/skylake/skl-messages.c |  3 +-
>>   sound/soc/intel/skylake/skl-sst-ipc.c  | 54 +++++++++++---------------
>>   sound/soc/intel/skylake/skl-sst-ipc.h  |  3 +-
>>   3 files changed, 27 insertions(+), 33 deletions(-)
>>
>> diff --git a/sound/soc/intel/skylake/skl-messages.c 
>> b/sound/soc/intel/skylake/skl-messages.c
>> index e8cc710f092b..84f0e6f58eb5 100644
>> --- a/sound/soc/intel/skylake/skl-messages.c
>> +++ b/sound/soc/intel/skylake/skl-messages.c
>> @@ -1379,11 +1379,12 @@ int skl_get_module_params(struct skl_dev *skl, 
>> u32 *params, int size,
>>                 u32 param_id, struct skl_module_cfg *mcfg)
>>   {
>>       struct skl_ipc_large_config_msg msg;
>> +    size_t bytes = size;
>>       msg.module_id = mcfg->id.module_id;
>>       msg.instance_id = mcfg->id.pvt_id;
>>       msg.param_data_size = size;
>>       msg.large_param_id = param_id;
>> -    return skl_ipc_get_large_config(&skl->ipc, &msg, params);
>> +    return skl_ipc_get_large_config(&skl->ipc, &msg, &params, &bytes);
>>   }
>> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c 
>> b/sound/soc/intel/skylake/skl-sst-ipc.c
>> index a2b69a02aab2..9d269a5f8bd9 100644
>> --- a/sound/soc/intel/skylake/skl-sst-ipc.c
>> +++ b/sound/soc/intel/skylake/skl-sst-ipc.c
>> @@ -969,12 +969,17 @@ int skl_ipc_set_large_config(struct 
>> sst_generic_ipc *ipc,
>>   EXPORT_SYMBOL_GPL(skl_ipc_set_large_config);
>>   int skl_ipc_get_large_config(struct sst_generic_ipc *ipc,
>> -        struct skl_ipc_large_config_msg *msg, u32 *param)
>> +        struct skl_ipc_large_config_msg *msg,
>> +        unsigned int **payload, size_t *bytes)
>>   {
>>       struct skl_ipc_header header = {0};
>> -    struct sst_ipc_message request = {0}, reply = {0};
>> -    int ret = 0;
>> -    size_t sz_remaining, rx_size, data_offset;
>> +    struct sst_ipc_message request, reply = {0};
>> +    unsigned int *buf;
>> +    int ret;
>> +
>> +    reply.data = kzalloc(SKL_ADSP_W1_SZ, GFP_KERNEL);
>> +    if (!reply.data)
>> +        return -ENOMEM;
>>       header.primary = IPC_MSG_TARGET(IPC_MOD_MSG);
>>       header.primary |= IPC_MSG_DIR(IPC_MSG_REQUEST);
>> @@ -987,34 +992,21 @@ int skl_ipc_get_large_config(struct 
>> sst_generic_ipc *ipc,
>>       header.extension |= IPC_FINAL_BLOCK(1);
>>       header.extension |= IPC_INITIAL_BLOCK(1);
>> -    sz_remaining = msg->param_data_size;
>> -    data_offset = 0;
>> -
>> -    while (sz_remaining != 0) {
>> -        rx_size = sz_remaining > SKL_ADSP_W1_SZ
>> -                ? SKL_ADSP_W1_SZ : sz_remaining;
>> -        if (rx_size == sz_remaining)
>> -            header.extension |= IPC_FINAL_BLOCK(1);
>> -
>> -        request.header = *(u64 *)(&header);
>> -        reply.data = ((char *)param) + data_offset;
>> -        reply.size = msg->param_data_size;
>> -        ret = sst_ipc_tx_message_wait(ipc, request, &reply);
>> -        if (ret < 0) {
>> -            dev_err(ipc->dev,
>> -                "ipc: get large config fail, err: %d\n", ret);
>> -            return ret;
>> -        }
>> -        sz_remaining -= rx_size;
>> -        data_offset = msg->param_data_size - sz_remaining;
>> +    request.header = *(u64 *)&header;
>> +    request.data = *payload;
>> +    request.size = *bytes;
>> +    reply.size = SKL_ADSP_W1_SZ;
>> -        /* clear the fields */
>> -        header.extension &= IPC_INITIAL_BLOCK_CLEAR;
>> -        header.extension &= IPC_DATA_OFFSET_SZ_CLEAR;
>> -        /* fill the fields */
>> -        header.extension |= IPC_INITIAL_BLOCK(1);
>> -        header.extension |= IPC_DATA_OFFSET_SZ(data_offset);
>> -    }
>> +    ret = sst_ipc_tx_message_wait(ipc, request, &reply);
>> +    if (ret < 0)
>> +        dev_err(ipc->dev, "ipc: get large config fail: %d\n", ret);
>> +
>> +    reply.size = (reply.header >> 32) & IPC_DATA_OFFSET_SZ_MASK;
>> +    buf = krealloc(reply.data, reply.size, GFP_KERNEL);
>> +    if (!buf)
>> +        return -ENOMEM;
>> +    *payload = buf;
>> +    *bytes = reply.size;
>>       return ret;
>>   }
>> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h 
>> b/sound/soc/intel/skylake/skl-sst-ipc.h
>> index 93af08cf41d2..a7ab2c589cc5 100644
>> --- a/sound/soc/intel/skylake/skl-sst-ipc.h
>> +++ b/sound/soc/intel/skylake/skl-sst-ipc.h
>> @@ -139,7 +139,8 @@ int skl_ipc_set_large_config(struct 
>> sst_generic_ipc *ipc,
>>           struct skl_ipc_large_config_msg *msg, u32 *param);
>>   int skl_ipc_get_large_config(struct sst_generic_ipc *ipc,
>> -        struct skl_ipc_large_config_msg *msg, u32 *param);
>> +        struct skl_ipc_large_config_msg *msg,
>> +        unsigned int **payload, size_t *bytes);
>>   int skl_sst_ipc_load_library(struct sst_generic_ipc *ipc,
>>               u8 dma_id, u8 table_id, bool wait);
>>