Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module_adapter: return error if get_conf is not implemented #9045

Merged
merged 1 commit into from
Apr 18, 2024
Merged

module_adapter: return error if get_conf is not implemented #9045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions src/audio/module_adapter/module_adapter_ipc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,21 +193,29 @@ static int module_adapter_get_set_params(struct comp_dev *dev, struct sof_ipc_ct
pos = MODULE_CFG_FRAGMENT_LAST;
}

/*
* The type member in struct sof_abi_hdr is used for component's specific blob type
* for IPC3, just like it is used for component's specific blob param_id for IPC4.
*/
if (set && interface->set_configuration)
return interface->set_configuration(mod, cdata->data[0].type, pos,
data_offset_size, (const uint8_t *)cdata,
cdata->num_elems, NULL, 0);
else if (!set && interface->get_configuration)
if (set) {
/*
* The type member in struct sof_abi_hdr is used for component's specific blob type
* for IPC3, just like it is used for component's specific blob param_id for IPC4.
*/
if (interface->set_configuration)
return interface->set_configuration(mod, cdata->data[0].type, pos,
data_offset_size,
(const uint8_t *)cdata,
cdata->num_elems, NULL, 0);

comp_warn(dev, "module_adapter_get_set_params(): no configuration op set for %d",
dev_comp_id(dev));
return 0;
}

if (interface->get_configuration)
return interface->get_configuration(mod, pos, &data_offset_size,
(uint8_t *)cdata, cdata->num_elems);

comp_warn(dev, "module_adapter_get_set_params(): no configuration op set for %d",
dev_comp_id(dev));
return 0;
comp_err(dev, "module_adapter_get_set_params(): no configuration op get for %d",
dev_comp_id(dev));
return -EIO; /* non-implemented error */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized this leads to a special case that by calling this -EIO will be returned for CMD_SET_DATA if set_configuration is not implemented. It's beyond my purpose that returning -EIO for CMD_GET_DATA only.

Hi @LaurentiuM1234 , I just saw your comment saying that cadence.c doesn't implement both get_configuration and set_configuration, is that correct? If the non-implemented set_configuration is intentional, I think this line should be fixed to keep CMD_SET_DATA behavior as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized this leads to a special case that by calling this -EIO will be returned for CMD_SET_DATA if set_configuration is not implemented. It's beyond my purpose that returning -EIO for CMD_GET_DATA only.

ACK, so now get_configuration() becomes mandatory and set_configuration() remains optional. Might be out of scope for this, but did you consider adding all the interface->set/get_configuration() logic into some wrapper functions?

i.e: something like:

static inline int module_set_configuration(struct processing_module *mod, uint32_t config_id
                                                                   enum module_cfg_fragment_position pos, uint32_t data_offset_size,
                                                                   const uint8_t *fragment, size_t fragment_size, uint8_t *response,
                                                                   size_t response_size)
{
    const struct module_interface *const ops = mod->dev->drv->adapter_ops;

    if (ops->set_configuration) {
       // call set_configuration
    }

    // optional operation
    return 0;
}

static inline int module_get_configuration(struct processing_module *mod, uint32_t config_id
                                                                   enum module_cfg_fragment_position pos, uint32_t data_offset_size,
                                                                   const uint8_t *fragment, size_t fragment_size, uint8_t *response,
                                                                   size_t response_size)
{
    const struct module_interface *const ops = mod->dev->drv->adapter_ops;

    // this is not optional
    if (!ops->get_configuration)
        return -EIO;

    // some more code here
}

Since this is also used by the IPC4 stuff, having a centralized view showing which operation is optional and which isn't might be helpful?

Hi @LaurentiuM1234 , I just saw your comment saying that cadence.c doesn't implement both get_configuration and set_configuration, is that correct? If the non-implemented set_configuration is intentional, I think this line should be fixed to keep CMD_SET_DATA behavior as is.

The cadence module doesn't implement the get_configuration() stuff so if we make it mandatory the issue will still persist. Can't really tell you why it's not implemented but we can probably get around the issue by just implementing a dummy get_configuration() if we really don't need a proper one. Anyways, IMO not a blocker for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Some contexts below for clearer statement:

The commit doesn't mean to make get_configuration mandatory (in a literal manner). Instead, the intention is to fix the use case of SOF build which integrates the instance without get_configuration in module_adapter.

Consider a host running the IPC command to read blob data from the module_adapter bytes control, if module_adapter doesn't return error while the instance doesn't implement get_configuration, the host will consider the blob data is read back successfully, leading to problems on wrong (or illegal) data blob access. As a consequence, it should be better to return error than zero, provided that the host has no clue whether get_configuration is implemented by instance or not.

}

static int module_adapter_ctrl_get_set_data(struct comp_dev *dev, struct sof_ipc_ctrl_data *cdata,
Expand Down Expand Up @@ -272,6 +280,12 @@ int module_adapter_cmd(struct comp_dev *dev, int cmd, void *data, int max_data_s
0);
break;
case COMP_CMD_GET_VALUE:
/*
* Return error if getter is not implemented. Otherwise, the host will suppose
* the GET_VALUE command is successful, but the received cdata is not filled.
*/
ret = -EIO;

/*
* IPC3 does not use config_id, so pass 0 for config ID as it will be ignored
* anyway. Also, pass the 0 as the fragment size and data offset as they are not
Expand Down
6 changes: 5 additions & 1 deletion src/audio/module_adapter/module_adapter_ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,11 @@ int module_get_large_config(struct comp_dev *dev, uint32_t param_id, bool first_
if (interface->get_configuration)
return interface->get_configuration(mod, param_id, data_offset_size,
(uint8_t *)data, fragment_size);
return 0;
/*
* Return error if getter is not implemented. Otherwise, the host will suppose
* the GET_VALUE command is successful, but the received cdata is not filled.
*/
return -EIO;
}
EXPORT_SYMBOL(module_get_large_config);

Expand Down