Messages in this thread | | | Subject | Re: [alsa-devel] [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm | From | Pierre-Louis Bossart <> | Date | Fri, 26 Jul 2019 12:55:23 -0500 |
| |
Thanks Guennadi for looking at this code, it's hard to review and figure things out... I replied to each, even trivial ones, to have a trace of all the issues.
>> +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.
ok
> >> + struct sdw_port_runtime *p_rt; >> + int port_bo, sample_int; >> + unsigned int rate, bps, ch = 0; > > ditto for ch
ok
>> + >> + 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
likely yes.
> >> + (SDW_BLK_GRP_CNT_1 * ch), 0x0); > > superfluous parentheses
yep
>> +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
yes
> >> + 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
yes
>> + >> + 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
yes
> >> + >> + 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 != ...)"
bad code indeed, thanks for spotting this. I need to double-check this one, now I don't trust the == it could also be that it was meant to be a NULL check on the first entry.
> >> + port_bo += bps * ch; >> + continue; >> + } >> + >> + t_data.hstart = hstart; >> + t_data.hstop = hstop; > > You already set these two above
need to check this as well.
> >> + 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
yes
> >> + 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 -= ...
yes
> >> + } >> +} >> + >> +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
yes
>> + 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?
This code looks wrong. Need to get an intravenous caffeine injection.
> >> + } >> + } >> + >> + 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.
I'll have to double check. There's already the warning on krealloc that looks suspicious as well.
> >> + 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;
will check offline and see.
>> + >> + 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.
good catch, will look into this.
> >> + 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.
will check. In general I avoid per-scope declarations.
>> +} >> + >> +/** >> + * 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.
will check
>> +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
yes
>> + 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...
Gah, yes.
>> + 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
yes and yes > >> + 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;
I saw this before my Summer break and forgot about it. yes it needs to be fixed.
>> >> 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
yes, will fix
>> +/* Retrieve and return channel count from channel mask */ >> +static inline int sdw_ch_mask_to_ch(int ch_mask) >> +{ >> + int c = 0; > > superfluous initialisation
yes
> >> + >> + 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
will check
| |