On Thu, Jul 25, 2019 at 06:40:19PM -0500, Pierre-Louis Bossart wrote:
This algorithm computes bus parameters like clock frequency, frame shape and port transport parameters based on active stream(s) running on the bus.
This implementation is optimal for Intel platforms. Developers can also implement their own .compute_params() callback for specific resource management algorithm.
Credits: this patch is based on an earlier internal contribution by Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded values were removed from the initial contribution to use BIOS information instead.
FIXME: remove checkpatch report WARNING: Reusing the krealloc arg is almost always a bug
group->rates = krealloc(group->rates,Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/Makefile | 2 +- drivers/soundwire/algo_dynamic_allocation.c | 403 ++++++++++++++++++++ drivers/soundwire/bus.c | 3 + drivers/soundwire/bus.h | 46 ++- drivers/soundwire/stream.c | 20 + include/linux/soundwire/sdw.h | 5 + 6 files changed, 476 insertions(+), 3 deletions(-) create mode 100644 drivers/soundwire/algo_dynamic_allocation.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index 88990cac48a7..f59a9d4a28fd 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -5,7 +5,7 @@
#Bus Objs soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
debugfs.o
debugfs.o algo_dynamic_allocation.oobj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
diff --git a/drivers/soundwire/algo_dynamic_allocation.c b/drivers/soundwire/algo_dynamic_allocation.c new file mode 100644 index 000000000000..89edb39162b8 --- /dev/null +++ b/drivers/soundwire/algo_dynamic_allocation.c @@ -0,0 +1,403 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2015-18 Intel Corporation.
+/*
- Bandwidth management algorithm based on 2^n gears
- */
+#include <linux/device.h> +#include <linux/mod_devicetable.h> +#include <linux/slab.h> +#include <linux/soundwire/sdw.h> +#include "bus.h"
+#define SDW_STRM_RATE_GROUPING 1
+struct sdw_group_params {
- unsigned int rate;
- int full_bw;
- int payload_bw;
- int hwidth;
+};
+struct sdw_group {
- unsigned int count;
- unsigned int max_size;
- unsigned int *rates;
+};
+struct sdw_transport_data {
- int hstart;
- int hstop;
- int block_offset;
- int sub_block_offset;
+};
+static void sdw_compute_slave_ports(struct sdw_master_runtime *m_rt,
struct sdw_transport_data *t_data)+{
- struct sdw_slave_runtime *s_rt = NULL;
Superfluous initialisation.
- struct sdw_port_runtime *p_rt;
- int port_bo, sample_int;
- unsigned int rate, bps, ch = 0;
ditto for ch
- port_bo = t_data->block_offset;
- list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
rate = m_rt->stream->params.rate;bps = m_rt->stream->params.bps;sample_int = (m_rt->bus->params.curr_dr_freq / rate);list_for_each_entry(p_rt, &s_rt->port_list, port_node) {ch = sdw_ch_mask_to_ch(p_rt->ch_mask);sdw_fill_xport_params(&p_rt->transport_params,p_rt->num, true,SDW_BLK_GRP_CNT_1,sample_int, port_bo, port_bo >> 8,t_data->hstart,t_data->hstop,
I think the above two lines could fit in one
(SDW_BLK_GRP_CNT_1 * ch), 0x0);
superfluous parentheses
sdw_fill_port_params(&p_rt->port_params,p_rt->num, bps,SDW_PORT_FLOW_MODE_ISOCH,SDW_PORT_DATA_MODE_NORMAL);port_bo += bps * ch;}- }
+}
+static void sdw_compute_master_ports(struct sdw_master_runtime *m_rt,
struct sdw_group_params *params,int port_bo, int hstop)+{
- struct sdw_transport_data t_data = {0};
- struct sdw_port_runtime *p_rt;
- struct sdw_bus *bus = m_rt->bus;
- int sample_int, hstart = 0;
superfluous initialisation
- unsigned int rate, bps, ch, no_ch;
- rate = m_rt->stream->params.rate;
- bps = m_rt->stream->params.bps;
- ch = m_rt->ch_count;
- sample_int = (bus->params.curr_dr_freq / rate);
superfluous parentheses
- if (rate != params->rate)
return;- t_data.hstop = hstop;
- hstart = hstop - params->hwidth + 1;
- t_data.hstart = hstart;
- list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
no_ch = sdw_ch_mask_to_ch(p_rt->ch_mask);sdw_fill_xport_params(&p_rt->transport_params, p_rt->num,true, SDW_BLK_GRP_CNT_1, sample_int,port_bo, port_bo >> 8, hstart, hstop,(SDW_BLK_GRP_CNT_1 * no_ch), 0x0);
superfluous parentheses
sdw_fill_port_params(&p_rt->port_params,p_rt->num, bps,SDW_PORT_FLOW_MODE_ISOCH,SDW_PORT_DATA_MODE_NORMAL);/* Check for first entry */if (!(p_rt == list_first_entry(&m_rt->port_list,struct sdw_port_runtime,port_node))) {
you wanted to write "if (p_rt != ...)"
port_bo += bps * ch;continue;}t_data.hstart = hstart;t_data.hstop = hstop;
You already set these two above
t_data.block_offset = port_bo;t_data.sub_block_offset = 0;port_bo += bps * ch;- }
- sdw_compute_slave_ports(m_rt, &t_data);
+}
+static void _sdw_compute_port_params(struct sdw_bus *bus,
struct sdw_group_params *params, int count)+{
- struct sdw_master_runtime *m_rt = NULL;
superfluous initialisation
- int hstop = bus->params.col - 1;
- int block_offset, port_bo, i;
- /* Run loop for all groups to compute transport parameters */
- for (i = 0; i < count; i++) {
port_bo = 1;block_offset = 1;list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {sdw_compute_master_ports(m_rt, ¶ms[i],port_bo, hstop);block_offset += m_rt->ch_count *m_rt->stream->params.bps;port_bo = block_offset;}hstop = hstop - params[i].hwidth;
hstop -= ...
- }
+}
+static int sdw_compute_group_params(struct sdw_bus *bus,
struct sdw_group_params *params,int *rates, int count)+{
- struct sdw_master_runtime *m_rt = NULL;
ditto
- int sel_col = bus->params.col;
- unsigned int rate, bps, ch;
- int i, column_needed = 0;
- /* Calculate bandwidth per group */
- for (i = 0; i < count; i++) {
params[i].rate = rates[i];params[i].full_bw = bus->params.curr_dr_freq / params[i].rate;- }
- list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
rate = m_rt->stream->params.rate;bps = m_rt->stream->params.bps;ch = m_rt->ch_count;for (i = 0; i < count; i++) {if (rate == params[i].rate)params[i].payload_bw += bps * ch;
I don't know about the algorithm, rates can repeat, right? So you cannot break out of the loop here once you found one match?
}- }
- for (i = 0; i < count; i++) {
params[i].hwidth = (sel_col *params[i].payload_bw + params[i].full_bw - 1) /params[i].full_bw;column_needed += params[i].hwidth;- }
- if (column_needed > sel_col - 1)
return -EINVAL;- return 0;
+}
+static int sdw_add_element_group_count(struct sdw_group *group,
unsigned int rate)+{
- int num = group->count;
- int i;
- for (i = 0; i <= num; i++) {
if (rate == group->rates[i])
Are you sure this is correct? You actually check count + 1 rates - from 0 to count inclusively. I think this isn't what you wanted to do, so my proposal below only checks count rates.
break;if (i != num)continue;if (group->count >= group->max_size) {group->max_size += 1;group->rates = krealloc(group->rates,(sizeof(int) * group->max_size),GFP_KERNEL);if (!group->rates)return -ENOMEM;}group->rates[group->count++] = rate;- }
How about this:
for (i = 0; i < num; i++) if (rate == group->rates[i]) return 0;
if (group->count >= group->max_size) { group->max_size += 1; group->rates = krealloc(group->rates, (sizeof(int) * group->max_size), GFP_KERNEL); if (!group->rates) return -ENOMEM; }
group->rates[group->count++] = rate;
return 0;
- return 0;
+}
+static int sdw_get_group_count(struct sdw_bus *bus,
struct sdw_group *group)+{
- struct sdw_master_runtime *m_rt;
- unsigned int rate;
- int ret = 0;
- group->count = 0;
- group->max_size = SDW_STRM_RATE_GROUPING;
- group->rates = kcalloc(group->max_size, sizeof(int), GFP_KERNEL);
- if (!group->rates)
return -ENOMEM;- list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
rate = m_rt->stream->params.rate;if (m_rt == list_first_entry(&bus->m_rt_list,struct sdw_master_runtime,bus_node)) {group->rates[group->count++] = rate;} else {ret = sdw_add_element_group_count(group, rate);if (ret < 0)
Actually it looks like you should free rates here? I see that not doing this makes the caller function below easier, but I'd say this is more error- prone... Up to you but I'd go the "safe" way - if it fails, it frees itself, if it succeeds - it's freed elsewhere.
return ret;}- }
- return ret;
I think this will always return 0 here, so you don't need the "ret" variable in the function scope, you only need it in the "else" scope above.
+}
+/**
- sdw_compute_port_params: Compute transport and port parameters
- @bus: SDW Bus instance
- */
+static int sdw_compute_port_params(struct sdw_bus *bus) +{
- struct sdw_group_params *params = NULL;
- struct sdw_group group;
- int ret;
- ret = sdw_get_group_count(bus, &group);
- if (ret < 0)
goto out;- if (group.count == 0)
goto out;- params = kcalloc(group.count, sizeof(*params), GFP_KERNEL);
- if (!params) {
ret = -ENOMEM;goto out;- }
- /* Compute transport parameters for grouped streams */
- ret = sdw_compute_group_params(bus, params,
&group.rates[0], group.count);- if (ret < 0)
goto out;- _sdw_compute_port_params(bus, params, group.count);
+out:
- kfree(params);
- kfree(group.rates);
Depending whether or not you change the code above, this might change too.
- return ret;
+}
+static int sdw_select_row_col(struct sdw_bus *bus, int clk_freq) +{
- struct sdw_master_prop *prop = &bus->prop;
- int frame_int, frame_freq;
- int r, c;
- for (c = 0; c < SDW_FRAME_COLS; c++) {
for (r = 0; r < SDW_FRAME_ROWS; r++) {if (sdw_rows[r] != prop->default_row ||sdw_cols[c] != prop->default_col)continue;frame_int = sdw_rows[r] * sdw_cols[c];frame_freq = clk_freq / frame_int;if ((clk_freq - (frame_freq * SDW_FRAME_CTRL_BITS)) <bus->params.bandwidth)continue;bus->params.row = sdw_rows[r];bus->params.col = sdw_cols[c];return 0;}- }
- return -EINVAL;
+}
+/**
- sdw_compute_bus_params: Compute bus parameters
- @bus: SDW Bus instance
- */
+static int sdw_compute_bus_params(struct sdw_bus *bus) +{
- unsigned int max_dr_freq, curr_dr_freq = 0;
- struct sdw_master_prop *mstr_prop = NULL;
superfluous initialisation
- int i, clk_values, ret;
- bool is_gear = false;
- u32 *clk_buf;
- mstr_prop = &bus->prop;
- if (!mstr_prop)
this is impossible, it's an address of bus->prop...
return -EINVAL;- if (mstr_prop->num_clk_gears) {
clk_values = mstr_prop->num_clk_gears;clk_buf = mstr_prop->clk_gears;is_gear = true;- } else if (mstr_prop->num_clk_freq) {
clk_values = mstr_prop->num_clk_freq;clk_buf = mstr_prop->clk_freq;- } else {
clk_values = 1;clk_buf = NULL;- }
- max_dr_freq = mstr_prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
- for (i = 0; i < clk_values; i++) {
if (!clk_buf)curr_dr_freq = max_dr_freq;elsecurr_dr_freq = (is_gear) ?
superfluous parentheses
(max_dr_freq >> clk_buf[i]) :
ditto
clk_buf[i] * SDW_DOUBLE_RATE_FACTOR;if (curr_dr_freq <= bus->params.bandwidth)continue;break;
I think this is raw code, you'd actually want to write this as
if (curr_dr_freq > bus->params.bandwidth) break;
/** TODO: Check all the Slave(s) port(s) audio modes and find* whether given clock rate is supported with glitchless* transition.*/- }
- if (i == clk_values)
return -EINVAL;- ret = sdw_select_row_col(bus, curr_dr_freq);
- if (ret < 0)
return -EINVAL;- bus->params.curr_dr_freq = curr_dr_freq;
- return 0;
+}
+/**
- sdw_compute_params: Compute bus, transport and port parameters
- @bus: SDW Bus instance
- */
+int sdw_compute_params(struct sdw_bus *bus) +{
- int ret;
- /* Computes clock frequency, frame shape and frame frequency */
- ret = sdw_compute_bus_params(bus);
- if (ret < 0) {
dev_err(bus->dev, "Compute bus params failed: %d", ret);return ret;- }
- /* Compute transport and port params */
- ret = sdw_compute_port_params(bus);
- if (ret < 0) {
dev_err(bus->dev, "Compute transport params failed: %d", ret);return ret;- }
- return 0;
+} +EXPORT_SYMBOL(sdw_compute_params); diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 2354675ef104..76a180578712 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -51,6 +51,9 @@ int sdw_add_bus_master(struct sdw_bus *bus)
bus->debugfs = sdw_bus_debugfs_init(bus);
- if (!bus->compute_params)
bus->compute_params = &sdw_compute_params;
I think it is more usual to not use "&" with functions, but it's valid too
- /*
- Device numbers in SoundWire are 0 through 15. Enumeration device
- number (0), Broadcast device number (15), Group numbers (12 and
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index c57c9c23f6ca..fdb7ce034fdf 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -72,6 +72,7 @@ struct sdw_msg { };
#define SDW_DOUBLE_RATE_FACTOR 2 +#define SDW_STRM_RATE_GROUPING 1
extern int sdw_rows[SDW_FRAME_ROWS]; extern int sdw_cols[SDW_FRAME_COLS]; @@ -157,9 +158,50 @@ int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave, u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
+/* Retrieve and return channel count from channel mask */ +static inline int sdw_ch_mask_to_ch(int ch_mask) +{
- int c = 0;
superfluous initialisation
- for (c = 0; ch_mask; ch_mask >>= 1)
c += ch_mask & 1;
isn't there a built-in or something to count set bits... You could use ffs() to at least not loop 31 times for 0x80000000
- return c;
+}
+/* Fill transport parameter data structure */ +static inline void sdw_fill_xport_params(struct sdw_transport_params *params,
int port_num, bool grp_ctrl_valid,int grp_ctrl, int sample_int,int off1, int off2,int hstart, int hstop,int pack_mode, int lane_ctrl)+{
- params->port_num = port_num;
- params->blk_grp_ctrl_valid = grp_ctrl_valid;
- params->blk_grp_ctrl = grp_ctrl;
- params->sample_interval = sample_int;
- params->offset1 = off1;
- params->offset2 = off2;
- params->hstart = hstart;
- params->hstop = hstop;
- params->blk_pkg_mode = pack_mode;
- params->lane_ctrl = lane_ctrl;
+}
+/* Fill port parameter data structure */ +static inline void sdw_fill_port_params(struct sdw_port_params *params,
int port_num, int bps,int flow_mode, int data_mode)+{
- params->num = port_num;
- params->bps = bps;
- params->flow_mode = flow_mode;
- params->data_mode = data_mode;
+}
/* Read-Modify-Write Slave register */ -static inline int -sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) +static inline int sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) { int tmp;
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 75b9ad1fb1a6..add7c121d084 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1485,6 +1485,16 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) bus->params.bandwidth += m_rt->stream->params.rate * m_rt->ch_count * m_rt->stream->params.bps;
/* Compute params */if (bus->compute_params) {ret = bus->compute_params(bus);if (ret < 0) {dev_err(bus->dev,"Compute params failed: %d", ret);return ret;}}- /* Program params */ ret = sdw_program_params(bus); if (ret < 0) {
@@ -1704,6 +1714,16 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream) bus->params.bandwidth -= m_rt->stream->params.rate * m_rt->ch_count * m_rt->stream->params.bps;
/* Compute params */if (bus->compute_params) {ret = bus->compute_params(bus);if (ret < 0) {dev_err(bus->dev,"Compute params failed: %d", ret);return ret;}}- /* Program params */ ret = sdw_program_params(bus); if (ret < 0) {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index b6acc436ac80..c7dfc824be80 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -730,6 +730,7 @@ struct sdw_master_ops {
- Bit set implies used number, bit clear implies unused number.
- @bus_lock: bus lock
- @msg_lock: message lock
- @compute_params: points to Bus resource management implementation
- @ops: Master callback ops
- @port_ops: Master port callback ops
- @params: Current bus parameters
@@ -752,6 +753,7 @@ struct sdw_bus { DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); struct mutex bus_lock; struct mutex msg_lock;
- int (*compute_params)(struct sdw_bus *bus); const struct sdw_master_ops *ops; const struct sdw_master_port_ops *port_ops; struct sdw_bus_params params;
@@ -852,6 +854,9 @@ struct sdw_stream_runtime {
struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name); void sdw_release_stream(struct sdw_stream_runtime *stream);
+int sdw_compute_params(struct sdw_bus *bus);
int sdw_stream_add_master(struct sdw_bus *bus, struct sdw_stream_config *stream_config, struct sdw_port_config *port_config, -- 2.20.1
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel