Skip to content

soundwire: stream: skip m_rt which is not in preparing stage #5246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bardliao merged 2 commits into from
Dec 3, 2024
Merged

soundwire: stream: skip m_rt which is not in preparing stage #5246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1cbc68
SoundWire: pass stream to compute_params()
bardliao Nov 21, 2024
6f676f9
soundwire: generic_bandwidth_allocation: count the bandwidth of activ…
bardliao Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/soundwire/amd_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
return slave_stat;
}

static int amd_sdw_compute_params(struct sdw_bus *bus)
static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
{
struct sdw_transport_data t_data = {0};
struct sdw_master_runtime *m_rt;
Expand Down
24 changes: 20 additions & 4 deletions drivers/soundwire/generic_bandwidth_allocation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ static void _sdw_compute_port_params(struct sdw_bus *bus,
}

static int sdw_compute_group_params(struct sdw_bus *bus,
struct sdw_stream_runtime *stream,
struct sdw_group_params *params,
struct sdw_group *group)
{
Expand All @@ -189,6 +190,19 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
}

list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
if (m_rt->stream == stream) {
/* Only runtime during prepare should be added */
if (stream->state != SDW_STREAM_CONFIGURED)
continue;
} else {
Copy link
Collaborator

@ranj063 ranj063 Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for an else after a continue btw

Copy link
Collaborator Author

@bardliao bardliao Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for an else after a continue btw

We still need an else because it will not continue if m_rt->stream == stream and stream->state == SDW_STREAM_CONFIGURED and it will then continue because m_rt->stream->state != SDW_STREAM_ENABLED && m_rt->stream->state != SDW_STREAM_DISABLED.
We only want to check m_rt->stream->state != SDW_STREAM_ENABLED && m_rt->stream->state != SDW_STREAM_DISABLED when the m_rt->stream != stream.

/*
* Include runtimes with running (ENABLED state) and paused (DISABLED state)
* streams
*/
if (m_rt->stream->state != SDW_STREAM_ENABLED &&
Copy link
Collaborator

@ranj063 ranj063 Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is not intuitive at all. What are you trying to say here? Ignore a runtime thats not associated with the stream if it has already been activated once?

Copy link
Collaborator Author

@bardliao bardliao Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, to the opposite. Only calculate the bandwidth of the active streams, and add the bandwidth of the current stream.

Copy link
Collaborator

@ranj063 ranj063 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then the check for != ENABLED makes sense. but why also check for != DISABLED? Is this the case with open but paused streams? Anyway, I think this needs a comment here with the explanation. And the commit message needs to be modified too based on whatever explanation you add here.

Copy link
Collaborator Author

@bardliao bardliao Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, DISABLED is about paused. Please see https://github.com/thesofproject/linux/blob/topic/sof-dev/Documentation/driver-api/soundwire/stream.rst#soundwire-stream-states and https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sdw_utils/soc_sdw_utils.c#L809

m_rt->stream->state != SDW_STREAM_DISABLED)
continue;
}
list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
rate = m_rt->stream->params.rate;
bps = m_rt->stream->params.bps;
Expand Down Expand Up @@ -321,8 +335,9 @@ static int sdw_get_group_count(struct sdw_bus *bus,
* sdw_compute_port_params: Compute transport and port parameters
*
* @bus: SDW Bus instance
* @stream: Soundwire stream
*/
static int sdw_compute_port_params(struct sdw_bus *bus)
static int sdw_compute_port_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
{
struct sdw_group_params *params = NULL;
struct sdw_group group;
Expand All @@ -342,7 +357,7 @@ static int sdw_compute_port_params(struct sdw_bus *bus)
}

/* Compute transport parameters for grouped streams */
ret = sdw_compute_group_params(bus, params, &group);
ret = sdw_compute_group_params(bus, stream, params, &group);
if (ret < 0)
goto free_params;

Expand Down Expand Up @@ -594,8 +609,9 @@ static int sdw_compute_bus_params(struct sdw_bus *bus)
* sdw_compute_params: Compute bus, transport and port parameters
*
* @bus: SDW Bus instance
* @stream: Soundwire stream
*/
int sdw_compute_params(struct sdw_bus *bus)
int sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
{
int ret;

Expand All @@ -605,7 +621,7 @@ int sdw_compute_params(struct sdw_bus *bus)
return ret;

/* Compute transport and port params */
ret = sdw_compute_port_params(bus);
ret = sdw_compute_port_params(bus, stream);
if (ret < 0) {
dev_err(bus->dev, "Compute transport params failed: %d\n", ret);
return ret;
Expand Down
2 changes: 1 addition & 1 deletion drivers/soundwire/qcom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ static const struct sdw_master_ops qcom_swrm_ops = {
.pre_bank_switch = qcom_swrm_pre_bank_switch,
};

static int qcom_swrm_compute_params(struct sdw_bus *bus)
static int qcom_swrm_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
{
struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
struct sdw_master_runtime *m_rt;
Expand Down
4 changes: 2 additions & 2 deletions drivers/soundwire/stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,7 +1425,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,

/* Compute params */
if (bus->compute_params) {
ret = bus->compute_params(bus);
ret = bus->compute_params(bus, stream);
if (ret < 0) {
dev_err(bus->dev, "Compute params failed: %d\n",
ret);
Expand Down Expand Up @@ -1727,7 +1727,7 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)

/* Compute params */
if (bus->compute_params) {
ret = bus->compute_params(bus);
ret = bus->compute_params(bus, stream);
if (ret < 0) {
dev_err(bus->dev, "Compute params failed: %d\n",
ret);
Expand Down
148 changes: 74 additions & 74 deletions include/linux/soundwire/sdw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,79 +854,6 @@ struct sdw_master_ops {
int dev_num);
};

/**
* struct sdw_bus - SoundWire bus
* @dev: Shortcut to &bus->md->dev to avoid changing the entire code.
* @md: Master device
* @bus_lock_key: bus lock key associated to @bus_lock
* @bus_lock: bus lock
* @slaves: list of Slaves on this bus
* @msg_lock_key: message lock key associated to @msg_lock
* @msg_lock: message lock
* @m_rt_list: List of Master instance of all stream(s) running on Bus. This
* is used to compute and program bus bandwidth, clock, frame shape,
* transport and port parameters
* @defer_msg: Defer message
* @params: Current bus parameters
* @stream_refcount: number of streams currently using this bus
* @ops: Master callback ops
* @port_ops: Master port callback ops
* @prop: Master properties
* @vendor_specific_prop: pointer to non-standard properties
* @hw_sync_min_links: Number of links used by a stream above which
* hardware-based synchronization is required. This value is only
* meaningful if multi_link is set. If set to 1, hardware-based
* synchronization will be used even if a stream only uses a single
* SoundWire segment.
* @controller_id: system-unique controller ID. If set to -1, the bus @id will be used.
* @link_id: Link id number, can be 0 to N, unique for each Controller
* @id: bus system-wide unique id
* @compute_params: points to Bus resource management implementation
* @assigned: Bitmap for Slave device numbers.
* Bit set implies used number, bit clear implies unused number.
* @clk_stop_timeout: Clock stop timeout computed
* @bank_switch_timeout: Bank switch timeout computed
* @domain: IRQ domain
* @irq_chip: IRQ chip
* @debugfs: Bus debugfs (optional)
* @multi_link: Store bus property that indicates if multi links
* are supported. This flag is populated by drivers after reading
* appropriate firmware (ACPI/DT).
* @lane_used_bandwidth: how much bandwidth in bits per second is used by each lane
*/
struct sdw_bus {
struct device *dev;
struct sdw_master_device *md;
struct lock_class_key bus_lock_key;
struct mutex bus_lock;
struct list_head slaves;
struct lock_class_key msg_lock_key;
struct mutex msg_lock;
struct list_head m_rt_list;
struct sdw_defer defer_msg;
struct sdw_bus_params params;
int stream_refcount;
const struct sdw_master_ops *ops;
const struct sdw_master_port_ops *port_ops;
struct sdw_master_prop prop;
void *vendor_specific_prop;
int hw_sync_min_links;
int controller_id;
unsigned int link_id;
int id;
int (*compute_params)(struct sdw_bus *bus);
DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
unsigned int clk_stop_timeout;
u32 bank_switch_timeout;
struct irq_chip irq_chip;
struct irq_domain *domain;
#ifdef CONFIG_DEBUG_FS
struct dentry *debugfs;
#endif
bool multi_link;
unsigned int lane_used_bandwidth[SDW_MAX_LANES];
};

int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
struct fwnode_handle *fwnode);
void sdw_bus_master_delete(struct sdw_bus *bus);
Expand Down Expand Up @@ -1016,10 +943,83 @@ struct sdw_stream_runtime {
struct list_head master_list;
};

/**
* struct sdw_bus - SoundWire bus
* @dev: Shortcut to &bus->md->dev to avoid changing the entire code.
* @md: Master device
* @bus_lock_key: bus lock key associated to @bus_lock
* @bus_lock: bus lock
* @slaves: list of Slaves on this bus
* @msg_lock_key: message lock key associated to @msg_lock
* @msg_lock: message lock
* @m_rt_list: List of Master instance of all stream(s) running on Bus. This
* is used to compute and program bus bandwidth, clock, frame shape,
* transport and port parameters
* @defer_msg: Defer message
* @params: Current bus parameters
* @stream_refcount: number of streams currently using this bus
* @ops: Master callback ops
* @port_ops: Master port callback ops
* @prop: Master properties
* @vendor_specific_prop: pointer to non-standard properties
* @hw_sync_min_links: Number of links used by a stream above which
* hardware-based synchronization is required. This value is only
* meaningful if multi_link is set. If set to 1, hardware-based
* synchronization will be used even if a stream only uses a single
* SoundWire segment.
* @controller_id: system-unique controller ID. If set to -1, the bus @id will be used.
* @link_id: Link id number, can be 0 to N, unique for each Controller
* @id: bus system-wide unique id
* @compute_params: points to Bus resource management implementation
* @assigned: Bitmap for Slave device numbers.
* Bit set implies used number, bit clear implies unused number.
* @clk_stop_timeout: Clock stop timeout computed
* @bank_switch_timeout: Bank switch timeout computed
* @domain: IRQ domain
* @irq_chip: IRQ chip
* @debugfs: Bus debugfs (optional)
* @multi_link: Store bus property that indicates if multi links
* are supported. This flag is populated by drivers after reading
* appropriate firmware (ACPI/DT).
* @lane_used_bandwidth: how much bandwidth in bits per second is used by each lane
*/
struct sdw_bus {
struct device *dev;
struct sdw_master_device *md;
struct lock_class_key bus_lock_key;
struct mutex bus_lock;
struct list_head slaves;
struct lock_class_key msg_lock_key;
struct mutex msg_lock;
struct list_head m_rt_list;
struct sdw_defer defer_msg;
struct sdw_bus_params params;
int stream_refcount;
const struct sdw_master_ops *ops;
const struct sdw_master_port_ops *port_ops;
struct sdw_master_prop prop;
void *vendor_specific_prop;
int hw_sync_min_links;
int controller_id;
unsigned int link_id;
int id;
int (*compute_params)(struct sdw_bus *bus, struct sdw_stream_runtime *stream);
DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
unsigned int clk_stop_timeout;
u32 bank_switch_timeout;
struct irq_chip irq_chip;
struct irq_domain *domain;
#ifdef CONFIG_DEBUG_FS
struct dentry *debugfs;
#endif
bool multi_link;
unsigned int lane_used_bandwidth[SDW_MAX_LANES];
};

struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name);
void sdw_release_stream(struct sdw_stream_runtime *stream);

int sdw_compute_params(struct sdw_bus *bus);
int sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream);

int sdw_stream_add_master(struct sdw_bus *bus,
struct sdw_stream_config *stream_config,
Expand Down
Loading