[alsa-devel] [PATCH v2 4/6] soundwire: Handle multiple master instances in a stream
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Tue Jun 12 21:12:46 CEST 2018
On 06/12/2018 05:53 AM, Shreyas NC wrote:
> From: Vinod Koul <vkoul at kernel.org>
>
> For each SoundWire stream operation, we need to parse master
> list and operate upon all master runtime.
>
> This patch does the boilerplate conversion of stream handling
> from single master runtime to handle a list of master runtime.
>
> Signed-off-by: Sanyog Kale <sanyog.r.kale at intel.com>
> Signed-off-by: Vinod Koul <vkoul at kernel.org>
> Signed-off-by: Shreyas NC <shreyas.nc at intel.com>
> ---
> drivers/soundwire/stream.c | 308 +++++++++++++++++++++++++-----------------
> include/linux/soundwire/sdw.h | 2 -
> 2 files changed, 185 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index eb942c6..36506a7 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -681,35 +681,44 @@ static int sdw_bank_switch(struct sdw_bus *bus)
>
> static int do_bank_switch(struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> + struct sdw_master_runtime *m_rt = NULL;
> const struct sdw_master_ops *ops;
> - struct sdw_bus *bus = m_rt->bus;
> + struct sdw_bus *bus = NULL;
> int ret = 0;
>
> - ops = bus->ops;
>
> - /* Pre-bank switch */
> - if (ops->pre_bank_switch) {
> - ret = ops->pre_bank_switch(bus);
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + ops = bus->ops;
> +
> + /* Pre-bank switch */
> + if (ops->pre_bank_switch) {
> + ret = ops->pre_bank_switch(bus);
> + if (ret < 0) {
> + dev_err(bus->dev,
> + "Pre bank switch op failed: %d", ret);
> + return ret;
> + }
> + }
> +
> + /* Bank switch */
> + ret = sdw_bank_switch(bus);
> if (ret < 0) {
> - dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
> + dev_err(bus->dev, "Bank switch failed: %d", ret);
> return ret;
> }
You probably want to add a comment that in multi-link operation the
actual bank_switch happens later and is done in a synchronized manner.
This is lost if you just move code around and move the bank_switch into
a loop.
I am actually no longer clear just looking at this code on when the
bank_switch happens (i know we've discussed this before but I am trying
to find out just based on this v2 instead of projecting how I think it
should work):
In Patch 6/6 it's pretty obvious that the bank switch happens when the
SyncGO bit is set, but there is no comment or explanation on how we
reach the intel_post_bank_switch() routine once for all masters handling
a stream when everything is based on loops. Clearly the
intel_pre_bank_switch is called multiple times (once per master), I
guess I am missing what the trigger is for the intel_post_bank_switch()
routine to be invoked?
> }
>
> - /* Bank switch */
> - ret = sdw_bank_switch(bus);
> - if (ret < 0) {
> - dev_err(bus->dev, "Bank switch failed: %d", ret);
> - return ret;
> - }
> -
> /* Post-bank switch */
> - if (ops->post_bank_switch) {
> - ret = ops->post_bank_switch(bus);
> - if (ret < 0) {
> - dev_err(bus->dev,
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + ops = bus->ops;
> + if (ops->post_bank_switch) {
> + ret = ops->post_bank_switch(bus);
> + if (ret < 0) {
> + dev_err(bus->dev,
> "Post bank switch op failed: %d", ret);
> + }
> }
> }
>
> @@ -754,6 +763,21 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
> }
> EXPORT_SYMBOL(sdw_alloc_stream);
>
> +static struct sdw_master_runtime
> +*sdw_find_master_rt(struct sdw_bus *bus,
> + struct sdw_stream_runtime *stream)
> +{
> + struct sdw_master_runtime *m_rt = NULL;
> +
> + /* Retrieve Bus handle if already available */
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + if (m_rt->bus == bus)
> + return m_rt;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * sdw_alloc_master_rt() - Allocates and initialize Master runtime handle
> *
> @@ -770,12 +794,11 @@ static struct sdw_master_runtime
> {
> struct sdw_master_runtime *m_rt;
>
> - m_rt = stream->m_rt;
> -
> /*
> * check if Master is already allocated (as a result of Slave adding
> * it first), if so skip allocation and go to configure
> */
> + m_rt = sdw_find_master_rt(bus, stream);
> if (m_rt)
> goto stream_config;
>
> @@ -786,7 +809,7 @@ static struct sdw_master_runtime
> /* Initialization of Master runtime handle */
> INIT_LIST_HEAD(&m_rt->port_list);
> INIT_LIST_HEAD(&m_rt->slave_rt_list);
> - stream->m_rt = m_rt;
> + list_add_tail(&m_rt->stream_node, &stream->master_list);
>
> list_add_tail(&m_rt->bus_node, &bus->m_rt_list);
>
> @@ -844,17 +867,21 @@ static void sdw_slave_port_release(struct sdw_bus *bus,
> struct sdw_stream_runtime *stream)
> {
> struct sdw_port_runtime *p_rt, *_p_rt;
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> + struct sdw_master_runtime *m_rt;
> struct sdw_slave_runtime *s_rt;
>
> - list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> - if (s_rt->slave != slave)
> - continue;
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>
> - list_for_each_entry_safe(p_rt, _p_rt,
> - &s_rt->port_list, port_node) {
> - list_del(&p_rt->port_node);
> - kfree(p_rt);
> + if (s_rt->slave != slave)
> + continue;
> +
> + list_for_each_entry_safe(p_rt, _p_rt,
> + &s_rt->port_list, port_node) {
> +
> + list_del(&p_rt->port_node);
> + kfree(p_rt);
> + }
> }
> }
> }
> @@ -871,16 +898,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
> struct sdw_stream_runtime *stream)
> {
> struct sdw_slave_runtime *s_rt, *_s_rt;
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> -
> - /* Retrieve Slave runtime handle */
> - list_for_each_entry_safe(s_rt, _s_rt,
> - &m_rt->slave_rt_list, m_rt_node) {
> + struct sdw_master_runtime *m_rt;
>
> - if (s_rt->slave == slave) {
> - list_del(&s_rt->m_rt_node);
> - kfree(s_rt);
> - return;
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + /* Retrieve Slave runtime handle */
> + list_for_each_entry_safe(s_rt, _s_rt,
> + &m_rt->slave_rt_list, m_rt_node) {
> +
> + if (s_rt->slave == slave) {
> + list_del(&s_rt->m_rt_node);
> + kfree(s_rt);
> + return;
> + }
> }
> }
> }
> @@ -888,6 +917,7 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
> /**
> * sdw_release_master_stream() - Free Master runtime handle
> *
> + * @m_rt: Master runtime node
> * @stream: Stream runtime handle.
> *
> * This function is to be called with bus_lock held
> @@ -895,16 +925,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
> * handle. If this is called first then sdw_release_slave_stream() will have
> * no effect as Slave(s) runtime handle would already be freed up.
> */
> -static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> +static void sdw_release_master_stream(struct sdw_master_runtime *m_rt,
> + struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> struct sdw_slave_runtime *s_rt, *_s_rt;
>
> list_for_each_entry_safe(s_rt, _s_rt,
> &m_rt->slave_rt_list, m_rt_node)
> sdw_stream_remove_slave(s_rt->slave, stream);
>
> + list_del(&m_rt->stream_node);
> list_del(&m_rt->bus_node);
> + kfree(m_rt);
> }
>
> /**
> @@ -918,13 +950,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> int sdw_stream_remove_master(struct sdw_bus *bus,
> struct sdw_stream_runtime *stream)
> {
> + struct sdw_master_runtime *m_rt, *_m_rt;
> +
> mutex_lock(&bus->bus_lock);
>
> - sdw_release_master_stream(stream);
> - sdw_master_port_release(bus, stream->m_rt);
> - stream->state = SDW_STREAM_RELEASED;
> - kfree(stream->m_rt);
> - stream->m_rt = NULL;
> + list_for_each_entry_safe(m_rt, _m_rt,
> + &stream->master_list, stream_node) {
> +
> + if (m_rt->bus != bus)
> + continue;
> +
> + sdw_master_port_release(bus, m_rt);
> + sdw_release_master_stream(m_rt, stream);
> + }
> +
> + if (list_empty(&stream->master_list))
> + stream->state = SDW_STREAM_RELEASED;
>
> mutex_unlock(&bus->bus_lock);
>
> @@ -1127,7 +1168,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
> stream->state = SDW_STREAM_CONFIGURED;
>
> stream_error:
> - sdw_release_master_stream(stream);
> + sdw_release_master_stream(m_rt, stream);
> error:
> mutex_unlock(&bus->bus_lock);
> return ret;
> @@ -1195,7 +1236,7 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
> * we hit error so cleanup the stream, release all Slave(s) and
> * Master runtime
> */
> - sdw_release_master_stream(stream);
> + sdw_release_master_stream(m_rt, stream);
> error:
> mutex_unlock(&slave->bus->bus_lock);
> return ret;
> @@ -1277,31 +1318,36 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
>
> static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> - struct sdw_bus *bus = m_rt->bus;
> + struct sdw_master_runtime *m_rt = NULL;
> + struct sdw_bus *bus = NULL;
> struct sdw_master_prop *prop = NULL;
> struct sdw_bus_params params;
> int ret;
>
> - prop = &bus->prop;
> - memcpy(¶ms, &bus->params, sizeof(params));
> + /* Prepare Master(s) and Slave(s) port(s) associated with stream */
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + prop = &bus->prop;
> + memcpy(¶ms, &bus->params, sizeof(params));
>
> - /* TODO: Support Asynchronous mode */
> - if ((prop->max_freq % stream->params.rate) != 0) {
> - dev_err(bus->dev, "Async mode not supported");
> - return -EINVAL;
> - }
> + /* TODO: Support Asynchronous mode */
> + if ((prop->max_freq % stream->params.rate) != 0) {
> + dev_err(bus->dev, "Async mode not supported");
> + return -EINVAL;
> + }
>
> - /* Increment cumulative bus bandwidth */
> - /* TODO: Update this during Device-Device support */
> - bus->params.bandwidth += m_rt->stream->params.rate *
> - m_rt->ch_count * m_rt->stream->params.bps;
> + /* Increment cumulative bus bandwidth */
> + /* TODO: Update this during Device-Device support */
> + bus->params.bandwidth += m_rt->stream->params.rate *
> + m_rt->ch_count * m_rt->stream->params.bps;
> +
> + /* Program params */
> + ret = sdw_program_params(bus);
> + if (ret < 0) {
> + dev_err(bus->dev, "Program params failed: %d", ret);
> + goto restore_params;
> + }
>
> - /* Program params */
> - ret = sdw_program_params(bus);
> - if (ret < 0) {
> - dev_err(bus->dev, "Program params failed: %d", ret);
> - goto restore_params;
> }
>
> ret = do_bank_switch(stream);
> @@ -1310,12 +1356,16 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
> goto restore_params;
> }
>
> - /* Prepare port(s) on the new clock configuration */
> - ret = sdw_prep_deprep_ports(m_rt, true);
> - if (ret < 0) {
> - dev_err(bus->dev, "Prepare port(s) failed ret = %d",
> - ret);
> - return ret;
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> +
> + /* Prepare port(s) on the new clock configuration */
> + ret = sdw_prep_deprep_ports(m_rt, true);
> + if (ret < 0) {
> + dev_err(bus->dev, "Prepare port(s) failed ret = %d",
> + ret);
> + return ret;
> + }
> }
>
> stream->state = SDW_STREAM_PREPARED;
> @@ -1343,35 +1393,40 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
> return -EINVAL;
> }
>
> - mutex_lock(&stream->m_rt->bus->bus_lock);
> + sdw_acquire_bus_lock(stream);
>
> ret = _sdw_prepare_stream(stream);
> if (ret < 0)
> pr_err("Prepare for stream:%s failed: %d", stream->name, ret);
>
> - mutex_unlock(&stream->m_rt->bus->bus_lock);
> + sdw_release_bus_lock(stream);
> return ret;
> }
> EXPORT_SYMBOL(sdw_prepare_stream);
>
> static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> - struct sdw_bus *bus = m_rt->bus;
> + struct sdw_master_runtime *m_rt = NULL;
> + struct sdw_bus *bus = NULL;
> int ret;
>
> - /* Program params */
> - ret = sdw_program_params(bus);
> - if (ret < 0) {
> - dev_err(bus->dev, "Program params failed: %d", ret);
> - return ret;
> - }
> + /* Enable Master(s) and Slave(s) port(s) associated with stream */
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
>
> - /* Enable port(s) */
> - ret = sdw_enable_disable_ports(m_rt, true);
> - if (ret < 0) {
> - dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
> - return ret;
> + /* Program params */
> + ret = sdw_program_params(bus);
> + if (ret < 0) {
> + dev_err(bus->dev, "Program params failed: %d", ret);
> + return ret;
> + }
> +
> + /* Enable port(s) */
> + ret = sdw_enable_disable_ports(m_rt, true);
> + if (ret < 0) {
> + dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
> + return ret;
> + }
> }
>
> ret = do_bank_switch(stream);
> @@ -1400,37 +1455,42 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
> return -EINVAL;
> }
>
> - mutex_lock(&stream->m_rt->bus->bus_lock);
> + sdw_acquire_bus_lock(stream);
>
> ret = _sdw_enable_stream(stream);
> if (ret < 0)
> pr_err("Enable for stream:%s failed: %d", stream->name, ret);
>
> - mutex_unlock(&stream->m_rt->bus->bus_lock);
> + sdw_release_bus_lock(stream);
> return ret;
> }
> EXPORT_SYMBOL(sdw_enable_stream);
>
> static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> - struct sdw_bus *bus = m_rt->bus;
> + struct sdw_master_runtime *m_rt = NULL;
> + struct sdw_bus *bus = NULL;
> int ret;
>
> - /* Disable port(s) */
> - ret = sdw_enable_disable_ports(m_rt, false);
> - if (ret < 0) {
> - dev_err(bus->dev, "Disable port(s) failed: %d", ret);
> - return ret;
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + /* Disable port(s) */
> + ret = sdw_enable_disable_ports(m_rt, false);
> + if (ret < 0) {
> + dev_err(bus->dev, "Disable port(s) failed: %d", ret);
> + return ret;
> + }
> }
> -
> stream->state = SDW_STREAM_DISABLED;
>
> - /* Program params */
> - ret = sdw_program_params(bus);
> - if (ret < 0) {
> - dev_err(bus->dev, "Program params failed: %d", ret);
> - return ret;
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + /* Program params */
> + ret = sdw_program_params(bus);
> + if (ret < 0) {
> + dev_err(bus->dev, "Program params failed: %d", ret);
> + return ret;
> + }
> }
>
> return do_bank_switch(stream);
> @@ -1452,43 +1512,46 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
> return -EINVAL;
> }
>
> - mutex_lock(&stream->m_rt->bus->bus_lock);
> + sdw_acquire_bus_lock(stream);
>
> ret = _sdw_disable_stream(stream);
> if (ret < 0)
> pr_err("Disable for stream:%s failed: %d", stream->name, ret);
>
> - mutex_unlock(&stream->m_rt->bus->bus_lock);
> + sdw_release_bus_lock(stream);
> return ret;
> }
> EXPORT_SYMBOL(sdw_disable_stream);
>
> static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> {
> - struct sdw_master_runtime *m_rt = stream->m_rt;
> - struct sdw_bus *bus = m_rt->bus;
> + struct sdw_master_runtime *m_rt = NULL;
> + struct sdw_bus *bus = NULL;
> int ret = 0;
>
> - /* De-prepare port(s) */
> - ret = sdw_prep_deprep_ports(m_rt, false);
> - if (ret < 0) {
> - dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> - return ret;
> - }
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + /* De-prepare port(s) */
> + ret = sdw_prep_deprep_ports(m_rt, false);
> + if (ret < 0) {
> + dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> + return ret;
> + }
>
> - stream->state = SDW_STREAM_DEPREPARED;
> + /* TODO: Update this during Device-Device support */
> + bus->params.bandwidth -= m_rt->stream->params.rate *
> + m_rt->ch_count * m_rt->stream->params.bps;
>
> - /* TODO: Update this during Device-Device support */
> - bus->params.bandwidth -= m_rt->stream->params.rate *
> - m_rt->ch_count * m_rt->stream->params.bps;
> + /* Program params */
> + ret = sdw_program_params(bus);
> + if (ret < 0) {
> + dev_err(bus->dev, "Program params failed: %d", ret);
> + return ret;
> + }
>
> - /* Program params */
> - ret = sdw_program_params(bus);
> - if (ret < 0) {
> - dev_err(bus->dev, "Program params failed: %d", ret);
> - return ret;
> }
>
> + stream->state = SDW_STREAM_DEPREPARED;
> return do_bank_switch(stream);
> }
>
> @@ -1508,13 +1571,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> return -EINVAL;
> }
>
> - mutex_lock(&stream->m_rt->bus->bus_lock);
> -
> + sdw_acquire_bus_lock(stream);
> ret = _sdw_deprepare_stream(stream);
> if (ret < 0)
> pr_err("De-prepare for stream:%d failed: %d", ret, ret);
>
> - mutex_unlock(&stream->m_rt->bus->bus_lock);
> + sdw_release_bus_lock(stream);
> return ret;
> }
> EXPORT_SYMBOL(sdw_deprepare_stream);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index ccd8dcdf..03df709 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -768,7 +768,6 @@ struct sdw_stream_params {
> * @params: Stream parameters
> * @state: Current state of the stream
> * @type: Stream type PCM or PDM
> - * @m_rt: Master runtime
> * @master_list: List of Master runtime(s) in this stream.
> * master_list can contain only one m_rt per Master instance
> * for a stream
> @@ -778,7 +777,6 @@ struct sdw_stream_runtime {
> struct sdw_stream_params params;
> enum sdw_stream_state state;
> enum sdw_stream_type type;
> - struct sdw_master_runtime *m_rt;
> struct list_head master_list;
> };
>
More information about the Alsa-devel
mailing list