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

[Pierre-Louis Bossart]

On 8/7/19 3:51 PM, Cezary Rojewski wrote:
> 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.

So how about removing this looping first in the existing code and add 
the needed changes in a second patch? wouldn't that help make the 
changes more self-contained? A large part of your patch below has 
indentation differences which make it hard to figure out what the new 
approach is.

> 
> 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);
>>>