From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 279B3C7618B for ; Fri, 26 Jul 2019 15:43:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D945B20838 for ; Fri, 26 Jul 2019 15:43:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727584AbfGZPna (ORCPT ); Fri, 26 Jul 2019 11:43:30 -0400 Received: from mga07.intel.com ([134.134.136.100]:31070 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726001AbfGZPna (ORCPT ); Fri, 26 Jul 2019 11:43:30 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jul 2019 08:43:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,311,1559545200"; d="scan'208";a="254368161" Received: from gliakhov-mobl2.ger.corp.intel.com (HELO ubuntu) ([10.249.35.244]) by orsmga001.jf.intel.com with ESMTP; 26 Jul 2019 08:43:24 -0700 Date: Fri, 26 Jul 2019 17:43:23 +0200 From: Guennadi Liakhovetski To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, tiwai@suse.de, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, vkoul@kernel.org, broonie@kernel.org, srinivas.kandagatla@linaro.org, jank@cadence.com, slawomir.blauciak@intel.com, Sanyog Kale Subject: Re: [alsa-devel] [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm Message-ID: <20190726154322.GJ16003@ubuntu> References: <20190725234032.21152-1-pierre-louis.bossart@linux.intel.com> <20190725234032.21152-28-pierre-louis.bossart@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190725234032.21152-28-pierre-louis.bossart@linux.intel.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > +#include > +#include > +#include > +#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; > + 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