| Message ID | 20190807134745.1648-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] ASoC: Intel: Skylake: large_config_get overhaul | expand |
I find this patch quite convoluted, so I ordered my comments to make them linear. > 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. > > 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? > 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. > 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? > > Signed-off-by: Cezary Rojewski <cezary.rojewski@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, ¶ms, &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); >
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@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, ¶ms, &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); >>
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@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, ¶ms, &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); >>>
On 2019-08-08 05:30, Pierre-Louis Bossart wrote: >>> >>> [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. Agreed. Must say, didn't get you at first. Update v4 has been sent and should do a better job at guiding the reader through changes. Czarek
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, ¶ms, &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);
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. 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. 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. 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. Signed-off-by: Cezary Rojewski <cezary.rojewski@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(-)