Re: [alsa-devel] [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

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.o
>  
>  obj-$(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, &params[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;
> +		else
> +			curr_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