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.

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

I/O Performance Measurement: Add I/O Performance Monitor #9330

Merged
merged 8 commits into from
Sep 18, 2024
1 change: 1 addition & 0 deletions app/boards/intel_adsp_ace15_mtpm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ CONFIG_AMS=y
CONFIG_COUNTER=y
CONFIG_SOF_TELEMETRY=y
CONFIG_SOF_TELEMETRY_PERFORMANCE_MEASUREMENTS=y
CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS=y

CONFIG_HEAP_MEM_POOL_SIZE=8192
CONFIG_L3_HEAP=y
Expand Down
1 change: 1 addition & 0 deletions app/boards/intel_adsp_ace20_lnl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ CONFIG_AMS=y
CONFIG_COUNTER=y
CONFIG_SOF_TELEMETRY=y
CONFIG_SOF_TELEMETRY_PERFORMANCE_MEASUREMENTS=y
CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS=y

CONFIG_HEAP_MEM_POOL_SIZE=8192
CONFIG_L3_HEAP=y
Expand Down
46 changes: 46 additions & 0 deletions src/audio/base_fw.c
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,45 @@ static int global_perf_data_get(uint32_t *data_off_size, char *data)
#endif
}

static int io_global_perf_state_get(uint32_t *data_off_size, char *data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

size_t? char is also a bit confusing for data, why not use the appropriate enum

{
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
*data = io_perf_monitor_get_state();
*data_off_size = sizeof(enum ipc4_perf_measurements_state_set);

return IPC4_SUCCESS;
#else
return IPC4_UNAVAILABLE;
#endif
}

static int io_global_perf_data_get(uint32_t *data_off_size, char *data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why char? what does it have to do with characters / strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A better question would be: why all the other handlers have this kind of interface

{
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
int ret;
struct io_global_perf_data *perf_data = (struct io_global_perf_data *)data;

ret = io_perf_monitor_get_performance_data(perf_data);
if (ret < 0)
return IPC4_ERROR_INVALID_PARAM;
*data_off_size = sizeof(*perf_data)
+ perf_data->perf_item_count * sizeof(*perf_data->perf_items);

return IPC4_SUCCESS;
#else
return IPC4_UNAVAILABLE;
#endif
}

static int io_perf_monitor_state_set(const char *data)
{
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
return io_perf_monitor_set_state((enum ipc4_perf_measurements_state_set)*data);
#else
return IPC4_UNAVAILABLE;
#endif
}

static int basefw_get_large_config(struct comp_dev *dev,
uint32_t param_id,
bool first_block,
Expand Down Expand Up @@ -503,6 +542,11 @@ static int basefw_get_large_config(struct comp_dev *dev,
return extended_global_perf_data_get(data_offset, data);
case IPC4_GLOBAL_PERF_DATA:
return global_perf_data_get(data_offset, data);
case IPC4_IO_PERF_MEASUREMENTS_STATE:
return io_global_perf_state_get(data_offset, data);
case IPC4_IO_GLOBAL_PERF_DATA:
return io_global_perf_data_get(data_offset, data);

/* TODO: add more support */
case IPC4_DSP_RESOURCE_STATE:
case IPC4_NOTIFICATION_MASK:
Expand Down Expand Up @@ -574,6 +618,8 @@ static int basefw_set_large_config(struct comp_dev *dev,
return basefw_dma_control(first_block, last_block, data_offset, data);
case IPC4_PERF_MEASUREMENTS_STATE:
return set_perf_meas_state(data);
case IPC4_IO_PERF_MEASUREMENTS_STATE:
return io_perf_monitor_state_set(data);
case IPC4_SYSTEM_TIME:
return basefw_set_system_time(param_id, first_block,
last_block, data_offset, data);
Expand Down
52 changes: 52 additions & 0 deletions src/audio/dai-zephyr.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
#include <zephyr/device.h>
#include <zephyr/drivers/dai.h>

#include <sof/debug/telemetry/performance_monitor.h>

/* note: if this macro is not defined
* then that means the HOST and the DSP
* have the same view of the address space.
Expand Down Expand Up @@ -383,6 +385,10 @@ dai_dma_cb(struct dai_data *dd, struct comp_dev *dev, uint32_t bytes,
/* update host position (in bytes offset) for drivers */
dd->total_data_processed += bytes;
}
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
/* Increment performance counters */
io_perf_monitor_update_data(dd->io_perf_bytes_count, bytes);
#endif

return dma_status;
}
Expand Down Expand Up @@ -478,6 +484,48 @@ int dai_common_new(struct dai_data *dd, struct comp_dev *dev,
dd->xrun = 0;
dd->chan = NULL;

/* I/O performance init, keep it last so the function does not reach this in case
* of return on error, so that we do not waste a slot
*/
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
enum io_perf_data_item_id perf_type;
enum io_perf_data_item_dir perf_dir;

switch (dai_cfg->type) {
case SOF_DAI_INTEL_SSP:
perf_type = IO_PERF_I2S_ID;
break;
case SOF_DAI_INTEL_ALH:
perf_type = IO_PERF_SOUND_WIRE_ID;
break;
case SOF_DAI_INTEL_DMIC:
perf_type = IO_PERF_DMIC_ID;
break;
case SOF_DAI_INTEL_HDA:
perf_type = IO_PERF_HDA_ID;
break;

default:
perf_type = IO_PERF_INVALID_ID;
comp_warn(dev, "Unsupported DAI type");
}
if (dai_cfg->direction == SOF_IPC_STREAM_PLAYBACK)
perf_dir = IO_PERF_OUTPUT_DIRECTION;
else
perf_dir = IO_PERF_INPUT_DIRECTION;

/* ignore perf meas init on case of other dai types */
if (perf_type != IO_PERF_INVALID_ID) {
struct io_perf_data_item init_data = {perf_type,
cpu_get_id(),
perf_dir,
IO_PERF_POWERED_UP_ENABLED,
IO_PERF_D0IX_POWER_MODE,
0, 0, 0 };
io_perf_monitor_init_data(&dd->io_perf_bytes_count, &init_data);
}
#endif

return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

also in the INVALID case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should treat performance as something optional. Perf setup failing should not make everything else fail

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, then maybe at least print a warning in the default case at line 509 above - not a blocker, can be a follow up patch

}

Expand Down Expand Up @@ -523,6 +571,10 @@ static struct comp_dev *dai_new(const struct comp_driver *drv,

void dai_common_free(struct dai_data *dd)
{
#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
io_perf_monitor_release_slot(dd->io_perf_bytes_count);
#endif

if (dd->group)
dai_group_put(dd->group);

Expand Down
8 changes: 8 additions & 0 deletions src/debug/telemetry/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,11 @@ config SOF_TELEMETRY_PERFORMANCE_MEASUREMENTS
Performance records are stored in the limited number of slots in Memory Window 3,
so only a certain number (PERFORMANCE_DATA_ENTRIES_COUNT) of components can be measured.

config SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
bool "enable I/O performance measurements"
help
Enables IO performance measurements. Each data interface will have its data throughput
measured (IPC/IDC and GPIO will measure number of messages/state changes).
Disabled by default and enabled with IPC. Measurements can be extracted also by IPC.
Interfaces measured: IPC, IDC, DMIC, I2S, SNDW, HDA, USB, GPIO, I2c, I3C, UART, SPI, CSI_2, DTF.

Loading
Loading