[RFC PATCH BlueZ 0/9] BAP stream reconfiguration

[RFC PATCH BlueZ 0/9] BAP stream reconfiguration

[Pauli Virtanen]

Proposed DBus API extension is:

- org.bluez.MediaEndpoint.ClearConfiguration(transport_path):

  Destroy stream associated with transport

- org.bluez.MediaEndpoint.ClearConfiguration(remote_endpoint_path):

  Destroy all streams associated with endpoint

- org.bluez.MediaEndpoint.SelectProperties(remote_endpoint_path):

  Destroy streams of endpoint, and re-run same unicast configuration as
  on initial connect, which invokes SelectProperties() callbacks on
  sound server and it can return new configuration it wants.

For future:

- org.bluez.MediaEndpoint.SetConfiguration(...):

  I think how this already works for broadcast is good: one call creates
  one new stream. Unicast should be changed to work exactly the same
  way.

  This allows for more detailed control of the configuration than
  SelectProperties(), and sound server can use it if it wants to deal
  with the different ASE configurations by itself.

- We will need to figure out how to handle devices rejecting ASE
  configurations on Config Codec or Enable.

  Whose responsibility is it to try a different configuration, if the
  current configuration is rejected by device?

  Example: Sony Linkbuds S has 48kHz sink & 48kHz source PACs.  However,
  it does not support the duplx configuration with both 48kHz sink and
  48kHz source ASE -- it rejects that in Enable.  It does support 32 kHz
  sink + 32kHz source duplex configuration.  AFAICS it is not possible
  to know which combinations of sinks & sources are possible, except
  trying them one by one. How do we handle this?

Unicast works with this Pipewire branch (can reconfigure to
sink/source/duplex):
https://gitlab.freedesktop.org/pvir/pipewire/-/tree/bap-codec-switch-select

(With device sets, each device needs to be switched separately.  Some
more work is needed to make reconfiguration while CIG is active to work
correctly, sound server must release transports before reconfiguring.)

Broadcast has not been tested at all.

Pauli Virtanen (9):
  org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration
  org.bluez.MediaEndpoint: add client role SelectProperties
  bap: add and use chainable future abstraction
  bap: use futures for select operation
  shared/bap: bap_abort_stream_req() should cancel also current req
  shared/bap: make sure ucast client stream is destroyed after releasing
  bap: support removing streams with ClearConfiguration()
  bap: do not try QoS before links are updated & io created
  bap: implement client role SelectProperties()

 doc/org.bluez.MediaEndpoint.rst |  27 ++
 profiles/audio/bap.c            | 533 ++++++++++++++++++++++++++------
 profiles/audio/transport.c      |  17 +
 profiles/audio/transport.h      |   1 +
 src/shared/bap.c                |  39 ++-
 5 files changed, 522 insertions(+), 95 deletions(-)

-- 
2.48.1

[RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration

[Pauli Virtanen]

Allow user to remove specific streams by calling
ClearConfiguration(transport_path) on the endpoint.  If the path is the
endpoint path instead, clear all streams associated with the endpoint.
---
 doc/org.bluez.MediaEndpoint.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
index f2b830ab0..b81106f0b 100644
--- a/doc/org.bluez.MediaEndpoint.rst
+++ b/doc/org.bluez.MediaEndpoint.rst
@@ -109,6 +109,12 @@ void ClearConfiguration(object transport)
 
 	Clear transport configuration.
 
+	**Server role:** [ISO only]
+
+	Close the stream associated with the given transport. If the
+	path given is the path of this endpoint, all its streams are
+	closed.
+
 void Release()
 ``````````````
 
-- 
2.48.1

[RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties

[Pauli Virtanen]

Add a simple way for the sound server to reconfigure a BAP unicast
endpoint, by calling org.bluez.MediaEndpoint.SelectProperties().

This shall destroy all streams of the endpoint, and restart the
SelectProperties() configuration flow from the beginning.

Since it may be necessary to reconfigure multiple endpoints at once to
correctly make bidirectional CIS, add Defer argument to just mark eps
for configuration.

In future, org.bluez.MediaEndpoint.SetConfiguration() could be changed
to handle unicast in the same way as broadcast: only create streams.
This allows sound server to have full control over stream configuration
itself, and not rely on bt_bap_select().
---
 doc/org.bluez.MediaEndpoint.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
index b81106f0b..5c42b878c 100644
--- a/doc/org.bluez.MediaEndpoint.rst
+++ b/doc/org.bluez.MediaEndpoint.rst
@@ -69,6 +69,8 @@ array{byte} SelectConfiguration(array{byte} capabilities)
 dict SelectProperties(dict capabilities)
 ````````````````````````````````````````
 
+	**Server Role**
+
 	Select BAP unicast configuration from the supported capabilities:
 
 	:object Endpoint:
@@ -104,6 +106,25 @@ dict SelectProperties(dict capabilities)
 	Note: There is no need to cache the selected properties since on
 	success the configuration is send back as parameter of SetConfiguration.
 
+	**Client Role**
+
+	Reconfigure a BAP unicast endpoint. This closes all existing
+	streams of the endpoint, and restarts the configuration
+	selection flow which e.g. triggers calls to *SelectProperties*
+	allowing the sound server to modify the configuration.
+
+	The following arguments are taken in *capabilities*:
+
+	:boolean Defer [optional]:
+
+		If True, mark endpoint for reconfiguration, but
+		postpone it until a non-deferred *SelectProperties()*
+		operation is made on an endpoint of the same device.
+
+		This is necessary to use when reconfiguring source and
+		sink streams with the intention that they be combined
+		into the same CIG, possibly forming bidirectional CIS.
+
 void ClearConfiguration(object transport)
 `````````````````````````````````````````
 
-- 
2.48.1

[RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction

[Pauli Virtanen]

Multi-part operations will need to postpone things like DBus replies
until all parts are complete. Make this a bit simpler with a chainable
future.
---
 profiles/audio/bap.c | 136 +++++++++++++++++++++++++++++++++----------
 1 file changed, 105 insertions(+), 31 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 37168e58c..8b9b89c70 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -80,7 +80,7 @@ struct bap_setup {
 	struct iovec *metadata;
 	unsigned int id;
 	struct iovec *base;
-	DBusMessage *msg;
+	struct future *qos_done;
 	void (*destroy)(struct bap_setup *setup);
 };
 
@@ -114,6 +114,17 @@ struct bap_data {
 	void *user_data;
 };
 
+typedef void (*future_func_t)(int err, const char *err_msg, void *data);
+
+struct future {
+	unsigned int step, steps;
+	const char *name;
+	future_func_t func;
+	void *data;
+	int err;
+	const char *err_msg;
+};
+
 static struct queue *sessions;
 
 /* Structure holding the parameters for Periodic Advertisement create sync.
@@ -155,6 +166,94 @@ struct bt_iso_qos bap_sink_pa_qos = {
 	}
 };
 
+static void future_clear(struct future **p, int err, const char *err_msg)
+{
+	struct future *h = *p;
+
+	if (!h)
+		return;
+
+	DBG("future %p (%s) 0x%02x (%s) step %u/%u", h, h->name ? h->name : "",
+		err, (err && err_msg) ? err_msg : "", h->step + 1, h->steps);
+
+	*p = NULL;
+
+	if (err && !h->err) {
+		h->err = err;
+		h->err_msg = err_msg;
+	}
+
+	if (++h->step < h->steps)
+		return;
+
+	h->func(h->err, h->err_msg, h->data);
+	free(h);
+}
+
+static void future_dbus_callback_func(int err, const char *err_msg, void *data)
+{
+	DBusMessage *msg = data;
+	DBusMessage *reply;
+
+	if (err && !err_msg)
+		err_msg = strerror(err);
+
+	switch (err) {
+	case 0:
+		reply = dbus_message_new_method_return(msg);
+		break;
+	case EINVAL:
+		reply = btd_error_invalid_args_str(msg, err_msg);
+		break;
+	default:
+		reply = btd_error_failed(msg, err_msg);
+		break;
+	}
+
+	g_dbus_send_message(btd_get_dbus_connection(), reply);
+
+	dbus_message_unref(msg);
+}
+
+static void future_init(struct future **p, const char *name, future_func_t func,
+								void *data)
+{
+	struct future *h;
+
+	future_clear(p, ECANCELED, NULL);
+
+	h = new0(struct future, 1);
+	h->steps = 1;
+	h->name = name;
+	h->func = func;
+	h->data = data;
+
+	DBG("future %p (%s) init", h, h->name ? h->name : "");
+
+	*p = h;
+}
+
+static void future_init_dbus_reply(struct future **p, const char *name,
+							DBusMessage *msg)
+{
+	future_init(p, name, future_dbus_callback_func, dbus_message_ref(msg));
+}
+
+__attribute__((unused))
+static void future_init_chain(struct future **p, struct future *h)
+{
+	future_clear(p, ECANCELED, NULL);
+
+	if (h) {
+		h->steps++;
+
+		DBG("future %p (%s) init step %u", h, h->name ? h->name : "",
+								h->steps);
+	}
+
+	*p = h;
+}
+
 static bool bap_data_set_user_data(struct bap_data *data, void *user_data)
 {
 	if (!data)
@@ -740,24 +839,12 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
 					void *user_data)
 {
 	struct bap_setup *setup = user_data;
-	DBusMessage *reply;
 
 	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
 
 	setup->id = 0;
 
-	if (!setup->msg)
-		return;
-
-	if (!code)
-		reply = dbus_message_new_method_return(setup->msg);
-	else
-		reply = btd_error_failed(setup->msg, "Unable to configure");
-
-	g_dbus_send_message(btd_get_dbus_connection(), reply);
-
-	dbus_message_unref(setup->msg);
-	setup->msg = NULL;
+	future_clear(&setup->qos_done, code ? EIO : 0, "Unable to configure");
 }
 
 static void config_cb(struct bt_bap_stream *stream,
@@ -765,7 +852,6 @@ static void config_cb(struct bt_bap_stream *stream,
 					void *user_data)
 {
 	struct bap_setup *setup = user_data;
-	DBusMessage *reply;
 
 	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
 
@@ -786,14 +872,7 @@ static void config_cb(struct bt_bap_stream *stream,
 		return;
 	}
 
-	if (!setup->msg)
-		return;
-
-	reply = btd_error_failed(setup->msg, "Unable to configure");
-	g_dbus_send_message(btd_get_dbus_connection(), reply);
-
-	dbus_message_unref(setup->msg);
-	setup->msg = NULL;
+	future_clear(&setup->qos_done, EIO, "Unable to configure");
 }
 
 static void setup_io_close(void *data, void *user_data)
@@ -872,7 +951,6 @@ static struct bap_setup *setup_new(struct bap_ep *ep)
 static void setup_free(void *data)
 {
 	struct bap_setup *setup = data;
-	DBusMessage *reply;
 
 	DBG("%p", setup);
 
@@ -881,12 +959,7 @@ static void setup_free(void *data)
 		setup->id = 0;
 	}
 
-	if (setup->msg) {
-		reply = btd_error_failed(setup->msg, "Canceled");
-		g_dbus_send_message(btd_get_dbus_connection(), reply);
-		dbus_message_unref(setup->msg);
-		setup->msg = NULL;
-	}
+	future_clear(&setup->qos_done, ECANCELED, NULL);
 
 	if (setup->ep)
 		queue_remove(setup->ep->setups, setup);
@@ -1038,7 +1111,8 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 
 	switch (bt_bap_stream_get_type(setup->stream)) {
 	case BT_BAP_STREAM_TYPE_UCAST:
-		setup->msg = dbus_message_ref(msg);
+		future_init_dbus_reply(&setup->qos_done, "set_configuration",
+									msg);
 		break;
 	case BT_BAP_STREAM_TYPE_BCAST:
 		/* No message sent over the air for broadcast */
-- 
2.48.1

[RFC PATCH BlueZ 4/9] bap: use futures for select operation

[Pauli Virtanen]

Change select operation to use futures.

Remove spurious bap_config for bcast streams, it does not even run if
there are only bcast PACs, and bcast streams are created differently.
---
 profiles/audio/bap.c | 74 ++++++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 19 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 8b9b89c70..39484c74a 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -93,6 +93,8 @@ struct bap_ep {
 	uint16_t supported_context;
 	uint16_t context;
 	struct queue *setups;
+	struct future *select_done;
+	int selecting;
 };
 
 struct bap_data {
@@ -110,7 +112,6 @@ struct bap_data {
 	struct queue *streams;
 	GIOChannel *listen_io;
 	unsigned int io_id;
-	int selecting;
 	void *user_data;
 };
 
@@ -166,6 +167,8 @@ struct bt_iso_qos bap_sink_pa_qos = {
 	}
 };
 
+static void pac_select_all(struct bap_data *data);
+
 static void future_clear(struct future **p, int err, const char *err_msg)
 {
 	struct future *h = *p;
@@ -239,7 +242,6 @@ static void future_init_dbus_reply(struct future **p, const char *name,
 	future_init(p, name, future_dbus_callback_func, dbus_message_ref(msg));
 }
 
-__attribute__((unused))
 static void future_init_chain(struct future **p, struct future *h)
 {
 	future_clear(p, ECANCELED, NULL);
@@ -1595,7 +1597,6 @@ static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
 
 	if (err) {
 		error("err %d", err);
-		ep->data->selecting--;
 		goto done;
 	}
 
@@ -1604,16 +1605,12 @@ static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
 	setup->metadata = util_iov_dup(metadata, 1);
 	setup->qos = *qos;
 
-	DBG("selecting %d", ep->data->selecting);
-	ep->data->selecting--;
-
 done:
-	if (ep->data->selecting)
-		return;
+	DBG("ep %p selecting %d", ep, ep->selecting);
 
-	queue_foreach(ep->data->srcs, bap_config, NULL);
-	queue_foreach(ep->data->snks, bap_config, NULL);
-	queue_foreach(ep->data->bcast, bap_config, NULL);
+	ep->selecting--;
+	if (!ep->selecting)
+		future_clear(&ep->select_done, 0, NULL);
 }
 
 static bool pac_register(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
@@ -1631,11 +1628,17 @@ static bool pac_register(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return true;
 }
 
+struct pac_select_data {
+	struct bap_data *data;
+	struct future *select_done;
+};
+
 static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 							void *user_data)
 {
-	struct btd_service *service = user_data;
-	struct bap_data *data = btd_service_get_user_data(service);
+	struct pac_select_data *select_data = user_data;
+	struct bap_data *data = select_data->data;
+	struct btd_service *service = data->service;
 	struct match_ep match = { lpac, rpac };
 	struct queue *queue;
 	struct bap_ep *ep;
@@ -1658,8 +1661,12 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	}
 
 	/* TODO: Cache LRU? */
-	if (btd_service_is_initiator(service))
-		bt_bap_select(lpac, rpac, &ep->data->selecting, select_cb, ep);
+	if (btd_service_is_initiator(service)) {
+		bt_bap_select(lpac, rpac, &ep->selecting, select_cb, ep);
+		if (ep->selecting)
+			future_init_chain(&ep->select_done,
+						select_data->select_done);
+	}
 
 	return true;
 }
@@ -1678,8 +1685,12 @@ static void ep_cancel_select(struct bap_ep *ep)
 {
 	struct bt_bap *bap = ep->data->bap;
 
+	future_clear(&ep->select_done, ECANCELED, NULL);
+
 	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
 	bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_cancel_select, ep);
+
+	ep->selecting = 0;
 }
 
 static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
@@ -1703,9 +1714,36 @@ static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return true;
 }
 
+static void select_complete_cb(int err, const char *err_msg, void *user_data)
+{
+	struct bap_data *data = user_data;
+
+	if (err == ECANCELED)
+		return;
+
+	queue_foreach(data->srcs, bap_config, NULL);
+	queue_foreach(data->snks, bap_config, NULL);
+}
+
+static void pac_select_all(struct bap_data *data)
+{
+	struct pac_select_data select_data = {
+		.data = data,
+	};
+
+	future_init(&select_data.select_done, "pac_select_all",
+						select_complete_cb, data);
+
+	bt_bap_foreach_pac(data->bap, BT_BAP_SOURCE, pac_select, &select_data);
+	bt_bap_foreach_pac(data->bap, BT_BAP_SINK, pac_select, &select_data);
+
+	future_clear(&select_data.select_done, 0, NULL);
+}
+
 static void bap_ready(struct bt_bap *bap, void *user_data)
 {
 	struct btd_service *service = user_data;
+	struct bap_data *data = btd_service_get_user_data(service);
 
 	DBG("bap %p", bap);
 
@@ -1715,8 +1753,7 @@ static void bap_ready(struct bt_bap *bap, void *user_data)
 	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_register, service);
 	bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_register, service);
 
-	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_select, service);
-	bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_select, service);
+	pac_select_all(data);
 }
 
 static bool match_setup_stream(const void *data, const void *user_data)
@@ -2684,8 +2721,7 @@ static void pac_added(struct bt_bap_pac *pac, void *user_data)
 	bt_bap_foreach_pac(data->bap, BT_BAP_SOURCE, pac_register, service);
 	bt_bap_foreach_pac(data->bap, BT_BAP_SINK, pac_register, service);
 
-	bt_bap_foreach_pac(data->bap, BT_BAP_SOURCE, pac_select, service);
-	bt_bap_foreach_pac(data->bap, BT_BAP_SINK, pac_select, service);
+	pac_select_all(data);
 }
 
 static void pac_added_broadcast(struct bt_bap_pac *pac, void *user_data)
-- 
2.48.1

[RFC PATCH BlueZ 5/9] shared/bap: bap_abort_stream_req() should cancel also current req

[Pauli Virtanen]

After bap_abort_stream_req() no req callbacks for stream shall be
called, so it has to fail also the currently in-flight request.
---
 src/shared/bap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 208fc1bf2..54c6e8629 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1229,6 +1229,13 @@ static void bap_abort_stream_req(struct bt_bap *bap,
 						struct bt_bap_stream *stream)
 {
 	queue_remove_all(bap->reqs, match_req_stream, stream, bap_req_abort);
+
+	if (bap->req && bap->req->stream == stream) {
+		struct bt_bap_req *req = bap->req;
+
+		bap->req = NULL;
+		bap_req_complete(req, NULL);
+	}
 }
 
 static void bt_bap_stream_unref(struct bt_bap_stream *stream)
-- 
2.48.1

[RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing

[Pauli Virtanen]

Upper layer as Unicast Client needs to be able to destroy streams when
it wants to reconfigure endpoints.

This does not currently work right, because of Server issued
Releasing->Config (caching) state transitions, which currently cause
streams never enter Idle (so they are never destroyed).

Fix this by considering Releasing->Config as Releasing->Idle->Config.
Also do not make new streams from cached config data as Unicast Client,
and leave all stream configuration to upper layer.
---
 src/shared/bap.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 54c6e8629..4f44db07a 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1363,6 +1363,31 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
 	struct bt_bap *bap = stream->bap;
 	const struct queue_entry *entry;
 
+	switch (stream->ep->old_state) {
+	case BT_ASCS_ASE_STATE_RELEASING:
+		/* After Releasing, Server may either transition to Config or
+		 * Idle. Our Unicast Client streams shall be considered
+		 * destroyed after Releasing, so that upper layer can control
+		 * stream creation. Make the lifecycle management simpler by
+		 * making sure the streams are destroyed by always emitting Idle
+		 * to upper layer after Releasing, even if the remote ASE did
+		 * not go through that state.
+		 */
+		if (stream->client &&
+				stream->ep->state != BT_ASCS_ASE_STATE_IDLE &&
+				(stream->lpac->type & (BT_BAP_SINK |
+							BT_BAP_SOURCE))) {
+			struct bt_bap_endpoint *ep = stream->ep;
+			uint8_t state = ep->state;
+
+			ep->state = BT_ASCS_ASE_STATE_IDLE;
+			bap_stream_state_changed(stream);
+			ep->state = state;
+			return;
+		}
+		break;
+	}
+
 	/* Pre notification updates */
 	switch (stream->ep->state) {
 	case BT_ASCS_ASE_STATE_IDLE:
@@ -4851,7 +4876,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
 	}
 
 	/* Any previously applied codec configuration may be cached by the
-	 * server.
+	 * server. However, all Unicast Client stream creation shall be left to
+	 * the upper layer.
 	 */
 	if (!ep->stream) {
 		struct match_pac match;
@@ -4866,7 +4892,9 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
 		if (!match.lpac || !match.rpac)
 			return;
 
-		bap_stream_new(bap, ep, match.lpac, match.rpac, NULL, true);
+		if (!(match.lpac->type & (BT_BAP_SINK | BT_BAP_SOURCE)))
+			bap_stream_new(bap, ep, match.lpac, match.rpac,
+								NULL, true);
 	}
 
 	if (!ep->stream)
-- 
2.48.1

[RFC PATCH BlueZ 7/9] bap: support removing streams with ClearConfiguration()

[Pauli Virtanen]

Implement removing streams via ClearConfiguration().
---
 profiles/audio/bap.c       | 87 ++++++++++++++++++++++++++++++++++++--
 profiles/audio/transport.c | 17 ++++++++
 profiles/audio/transport.h |  1 +
 3 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 39484c74a..46512a7f3 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -58,6 +58,7 @@
 
 #include "bap.h"
 #include "bass.h"
+#include "transport.h"
 
 #define ISO_SOCKET_UUID "6fbaf188-05e0-496a-9885-d6ddfdb4e03e"
 #define PACS_UUID_STR "00001850-0000-1000-8000-00805f9b34fb"
@@ -81,6 +82,7 @@ struct bap_setup {
 	unsigned int id;
 	struct iovec *base;
 	struct future *qos_done;
+	struct future *close_done;
 	void (*destroy)(struct bap_setup *setup);
 };
 
@@ -903,12 +905,30 @@ static void setup_io_close(void *data, void *user_data)
 	bt_bap_stream_io_connecting(setup->stream, -1);
 }
 
-static void ep_close(struct bap_ep *ep)
+static void setup_close(void *data, void *user_data)
+{
+	struct bap_setup *setup = data;
+	struct future *close_done = user_data;
+	struct bt_bap_stream *stream = setup->stream;
+
+	DBG("%p", setup);
+
+	future_init_chain(&setup->close_done, close_done);
+
+	setup_io_close(setup, NULL);
+
+	if (bt_bap_stream_get_state(stream) != BT_BAP_STREAM_STATE_RELEASING)
+		bt_bap_stream_release(stream, NULL, NULL);
+	else
+		setup_free(setup);
+}
+
+static void ep_close(struct bap_ep *ep, struct future *close_done)
 {
 	if (!ep)
 		return;
 
-	queue_foreach(ep->setups, setup_io_close, NULL);
+	queue_foreach(ep->setups, setup_close, close_done);
 }
 
 static struct bap_setup *setup_new(struct bap_ep *ep)
@@ -962,6 +982,7 @@ static void setup_free(void *data)
 	}
 
 	future_clear(&setup->qos_done, ECANCELED, NULL);
+	future_clear(&setup->close_done, 0, NULL);
 
 	if (setup->ep)
 		queue_remove(setup->ep->setups, setup);
@@ -1077,7 +1098,7 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 	 * TO DO reconfiguration of a BIS.
 	 */
 	if (bt_bap_pac_get_type(ep->lpac) != BT_BAP_BCAST_SOURCE)
-		ep_close(ep);
+		ep_close(ep, NULL);
 
 	setup = setup_new(ep);
 
@@ -1129,6 +1150,63 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 	return NULL;
 }
 
+struct close_stream_data {
+	const char *path;
+	struct future *close_done;
+	unsigned int count;
+};
+
+static void close_stream(void *data, void *user_data)
+{
+	struct bap_setup *setup = data;
+	struct close_stream_data *info = user_data;
+	struct bt_bap_stream *stream = setup->stream;
+	const char *path = media_transport_stream_path(stream);
+
+	if (info->path && (!path || strcmp(info->path, path)))
+		return;
+
+	setup_close(setup, info->close_done);
+	info->count++;
+}
+
+static unsigned int ep_close_stream(struct bap_ep *ep,
+						struct future *close_done,
+						const char *transport_path)
+{
+	struct close_stream_data info = { transport_path, close_done, 0 };
+
+	queue_foreach(ep->setups, close_stream, &info);
+	return info.count;
+}
+
+
+static DBusMessage *clear_configuration(DBusConnection *conn, DBusMessage *msg,
+								void *data)
+{
+	struct bap_ep *ep = data;
+	const char *path;
+	DBusMessageIter args;
+	struct future *done = NULL;
+
+	dbus_message_iter_init(msg, &args);
+	dbus_message_iter_get_basic(&args, &path);
+
+	DBG("%s: %s", ep->path, path ? path : "NULL");
+
+	future_init_dbus_reply(&done, "clear_configuration", msg);
+
+	if (strcmp(path, ep->path) == 0)
+		path = NULL;
+
+	if (ep_close_stream(ep, done, path))
+		future_clear(&done, 0, NULL);
+	else
+		future_clear(&done, path ? EINVAL : 0, NULL);
+
+	return NULL;
+}
+
 static bool stream_io_unset(const void *data, const void *user_data)
 {
 	struct bt_bap_stream *stream = (struct bt_bap_stream *)data;
@@ -1350,6 +1428,9 @@ static const GDBusMethodTable ep_methods[] = {
 					GDBUS_ARGS({ "endpoint", "o" },
 						{ "Configuration", "a{sv}" } ),
 					NULL, set_configuration) },
+	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("ClearConfiguration",
+					GDBUS_ARGS({ "transport", "o" }),
+					NULL, clear_configuration) },
 	{ },
 };
 
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index f3ac1a251..e81ef6e9a 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -2671,3 +2671,20 @@ void media_transport_update_device_volume(struct btd_device *dev,
 
 	btd_device_set_volume(dev, volume);
 }
+
+const char *media_transport_stream_path(void *stream)
+{
+	GSList *l;
+
+	if (!stream)
+		return NULL;
+
+	for (l = transports; l; l = l->next) {
+		struct media_transport *transport = l->data;
+
+		if (media_transport_get_stream(transport) == stream)
+			return transport->path;
+	}
+
+	return NULL;
+}
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index 808e1a193..7c107281a 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -33,3 +33,4 @@ void transport_get_properties(struct media_transport *transport,
 int media_transport_get_device_volume(struct btd_device *dev);
 void media_transport_update_device_volume(struct btd_device *dev,
 								int volume);
+const char *media_transport_stream_path(void *stream);
-- 
2.48.1

[RFC PATCH BlueZ 8/9] bap: do not try QoS before links are updated & io created

[Pauli Virtanen]

It's not possible to QoS in config_cb, even if stream is in Config
state, because stream->io may not exist and stream links are not yet
updated. The stream links are updated only before bap_state(), so the
completion of config has to be handled only there.

Wait for both events to complete before proceeding to QoS.
---
 profiles/audio/bap.c | 80 +++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 38 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 46512a7f3..da1f9feb1 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -83,6 +83,7 @@ struct bap_setup {
 	struct iovec *base;
 	struct future *qos_done;
 	struct future *close_done;
+	bool config_done;
 	void (*destroy)(struct bap_setup *setup);
 };
 
@@ -851,32 +852,49 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
 	future_clear(&setup->qos_done, code ? EIO : 0, "Unable to configure");
 }
 
+static void setup_create_io(struct bap_data *data, struct bap_setup *setup,
+				struct bt_bap_stream *stream, int defer);
+
+static void setup_qos(struct bap_setup *setup)
+{
+	struct bap_data *data = setup->ep->data;
+	struct bt_bap_stream *stream = setup->stream;
+
+	if (setup->id || !stream)
+		return;
+
+	setup_create_io(data, setup, stream, true);
+	if (!setup->io) {
+		error("Unable to create io");
+		if (bt_bap_stream_get_state(stream) !=
+						BT_BAP_STREAM_STATE_RELEASING)
+			bt_bap_stream_release(stream, NULL, NULL);
+		return;
+	}
+
+	/* Wait QoS response to respond */
+	setup->id = bt_bap_stream_qos(stream, &setup->qos, qos_cb, setup);
+	if (!setup->id) {
+		error("Failed to Configure QoS");
+		bt_bap_stream_release(stream, NULL, NULL);
+	}
+}
+
 static void config_cb(struct bt_bap_stream *stream,
 					uint8_t code, uint8_t reason,
 					void *user_data)
 {
 	struct bap_setup *setup = user_data;
+	const char *err_msg = "Unable to configure";
 
 	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
 
 	setup->id = 0;
 
-	if (!code) {
-		/* Check state is already set to config then proceed to qos */
-		if (bt_bap_stream_get_state(stream) ==
-					BT_BAP_STREAM_STATE_CONFIG) {
-			setup->id = bt_bap_stream_qos(stream, &setup->qos,
-							qos_cb, setup);
-			if (!setup->id) {
-				error("Failed to Configure QoS");
-				bt_bap_stream_release(stream, NULL, NULL);
-			}
-		}
-
-		return;
-	}
-
-	future_clear(&setup->qos_done, EIO, "Unable to configure");
+	if (code)
+		future_clear(&setup->qos_done, EIO, err_msg);
+	else if (setup->config_done)
+		setup_qos(setup);
 }
 
 static void setup_io_close(void *data, void *user_data)
@@ -1128,6 +1146,8 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 		return btd_error_invalid_args(msg);
 	}
 
+	setup->config_done = false;
+
 	if (setup->metadata && setup->metadata->iov_len)
 		bt_bap_stream_metadata(setup->stream, setup->metadata, NULL,
 								NULL);
@@ -1655,6 +1675,8 @@ static void setup_config(void *data, void *user_data)
 		return;
 	}
 
+	setup->config_done = false;
+
 	if (setup->metadata && setup->metadata->iov_len)
 		bt_bap_stream_metadata(setup->stream, setup->metadata, NULL,
 								NULL);
@@ -2086,9 +2108,6 @@ static bool is_cig_busy(struct bap_data *data, uint8_t cig)
 	return queue_find(sessions, cig_busy_session, &info);
 }
 
-static void setup_create_io(struct bap_data *data, struct bap_setup *setup,
-				struct bt_bap_stream *stream, int defer);
-
 static gboolean setup_io_recreate(void *user_data)
 {
 	struct bap_setup *setup = user_data;
@@ -2473,25 +2492,10 @@ static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
 			queue_remove(data->streams, stream);
 		break;
 	case BT_BAP_STREAM_STATE_CONFIG:
-		if (setup && !setup->id) {
-			setup_create_io(data, setup, stream, true);
-			if (!setup->io) {
-				error("Unable to create io");
-				if (old_state != BT_BAP_STREAM_STATE_RELEASING)
-					bt_bap_stream_release(stream, NULL,
-								NULL);
-				return;
-			}
-
-			/* Wait QoS response to respond */
-			setup->id = bt_bap_stream_qos(stream,
-							&setup->qos,
-							qos_cb,	setup);
-			if (!setup->id) {
-				error("Failed to Configure QoS");
-				bt_bap_stream_release(stream,
-							NULL, NULL);
-			}
+		if (setup) {
+			setup->config_done = true;
+			if (!setup->id)
+				setup_qos(setup);
 		}
 		break;
 	case BT_BAP_STREAM_STATE_QOS:
-- 
2.48.1

[RFC PATCH BlueZ 9/9] bap: implement client role SelectProperties()

[Pauli Virtanen]

Add SelectProperties() on a BAP unicast endpoint, which triggers its
reconfiguration or marks it for reconfiguration.

First, all associated streams are closed. After that, endpoints marked
for reconfiguration are reconfigured using the same flow as in the
initial configuration.
---
 profiles/audio/bap.c | 180 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 166 insertions(+), 14 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index da1f9feb1..0590067eb 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -96,8 +96,10 @@ struct bap_ep {
 	uint16_t supported_context;
 	uint16_t context;
 	struct queue *setups;
+	struct future *setup_done;
 	struct future *select_done;
 	int selecting;
+	bool reconfigure;
 };
 
 struct bap_data {
@@ -170,7 +172,8 @@ struct bt_iso_qos bap_sink_pa_qos = {
 	}
 };
 
-static void pac_select_all(struct bap_data *data);
+static void pac_select_all(struct bap_data *data, bool reconfigure,
+							struct future *done);
 
 static void future_clear(struct future **p, int err, const char *err_msg)
 {
@@ -849,7 +852,8 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
 
 	setup->id = 0;
 
-	future_clear(&setup->qos_done, code ? EIO : 0, "Unable to configure");
+	if (code)
+		future_clear(&setup->qos_done, EIO, "Unable to configure");
 }
 
 static void setup_create_io(struct bap_data *data, struct bap_setup *setup,
@@ -1227,6 +1231,120 @@ static DBusMessage *clear_configuration(DBusConnection *conn, DBusMessage *msg,
 	return NULL;
 }
 
+static void ep_close_if_reconfigure(void *obj, void *user_data)
+{
+	struct bap_ep *ep = obj;
+	struct future *done = user_data;
+
+	if (ep->reconfigure)
+		ep_close(ep, done);
+}
+
+static int select_properties_parse(DBusMessageIter *props, bool *defer)
+{
+	const char *key;
+
+	if (dbus_message_iter_get_arg_type(props) != DBUS_TYPE_DICT_ENTRY)
+		return -EINVAL;
+
+	while (dbus_message_iter_get_arg_type(props) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter value, entry;
+		int var;
+
+		dbus_message_iter_recurse(props, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		var = dbus_message_iter_get_arg_type(&value);
+
+		if (!strcasecmp(key, "Defer")) {
+			dbus_bool_t flag;
+
+			if (var != DBUS_TYPE_BOOLEAN)
+				goto fail;
+
+			dbus_message_iter_get_basic(&value, &flag);
+			*defer = flag;
+		}
+
+		dbus_message_iter_next(props);
+	}
+
+	return 0;
+
+fail:
+	DBG("Failed parsing %s", key);
+
+	return -EINVAL;
+}
+
+struct select_properties_data {
+	struct bap_data *data;
+	DBusMessage *msg;
+};
+
+static void select_properties_reconfig(int err, const char *err_msg, void *data)
+{
+	struct select_properties_data *info = data;
+	struct future *done = NULL;
+
+	future_init_dbus_reply(&done, "select_properties_reconfig", info->msg);
+	dbus_message_unref(info->msg);
+
+	/* Reconfigure endpoints using the same flow as for initial config */
+	if (!err)
+		pac_select_all(info->data, true, done);
+
+	future_clear(&done, err, err_msg);
+
+	free(info);
+}
+
+static DBusMessage *select_properties(DBusConnection *conn, DBusMessage *msg,
+								void *user_data)
+{
+	struct bap_ep *ep = user_data;
+	struct bap_data *data = ep->data;
+	struct select_properties_data *info;
+	struct future *done = NULL;
+	bool defer = false;
+	DBusMessageIter args, props;
+
+	switch (bt_bap_pac_get_type(ep->lpac)) {
+	case BT_BAP_SOURCE:
+	case BT_BAP_SINK:
+		break;
+	default:
+		return btd_error_invalid_args(msg);
+	}
+
+	dbus_message_iter_init(msg, &args);
+	dbus_message_iter_recurse(&args, &props);
+	if (select_properties_parse(&props, &defer))
+		return btd_error_invalid_args(msg);
+
+	DBG("%s defer %d", ep->path, (int)defer);
+
+	ep->reconfigure = true;
+	if (defer)
+		return dbus_message_new_method_return(msg);
+
+	info = new0(struct select_properties_data, 1);
+	info->data = ep->data;
+	info->msg = dbus_message_ref(msg);
+
+	future_init(&done, "select_properties", select_properties_reconfig,
+									info);
+
+	queue_foreach(data->snks, ep_close_if_reconfigure, done);
+	queue_foreach(data->srcs, ep_close_if_reconfigure, done);
+
+	future_clear(&done, 0, NULL);
+	return NULL;
+}
+
 static bool stream_io_unset(const void *data, const void *user_data)
 {
 	struct bt_bap_stream *stream = (struct bt_bap_stream *)data;
@@ -1451,6 +1569,10 @@ static const GDBusMethodTable ep_methods[] = {
 	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("ClearConfiguration",
 					GDBUS_ARGS({ "transport", "o" }),
 					NULL, clear_configuration) },
+	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("SelectProperties",
+					GDBUS_ARGS(
+						{ "Properties", "a{sv}" }),
+					NULL, select_properties) },
 	{ },
 };
 
@@ -1684,13 +1806,6 @@ static void setup_config(void *data, void *user_data)
 	bt_bap_stream_set_user_data(setup->stream, ep->path);
 }
 
-static void bap_config(void *data, void *user_data)
-{
-	struct bap_ep *ep = data;
-
-	queue_foreach(ep->setups, setup_config, NULL);
-}
-
 static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
 				struct iovec *metadata, struct bt_bap_qos *qos,
 				void *user_data)
@@ -1707,13 +1822,16 @@ static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
 	setup->caps = util_iov_dup(caps, 1);
 	setup->metadata = util_iov_dup(metadata, 1);
 	setup->qos = *qos;
+	future_init_chain(&setup->qos_done, ep->setup_done);
 
 done:
 	DBG("ep %p selecting %d", ep, ep->selecting);
 
 	ep->selecting--;
-	if (!ep->selecting)
+	if (!ep->selecting) {
+		future_clear(&ep->setup_done, 0, NULL);
 		future_clear(&ep->select_done, 0, NULL);
+	}
 }
 
 static bool pac_register(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
@@ -1734,6 +1852,8 @@ static bool pac_register(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 struct pac_select_data {
 	struct bap_data *data;
 	struct future *select_done;
+	struct future *setup_done;
+	bool reconfigure;
 };
 
 static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
@@ -1763,12 +1883,21 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 		return true;
 	}
 
+	if (select_data->reconfigure) {
+		if (!ep->reconfigure)
+			return true;
+		ep->reconfigure = false;
+	}
+
 	/* TODO: Cache LRU? */
 	if (btd_service_is_initiator(service)) {
 		bt_bap_select(lpac, rpac, &ep->selecting, select_cb, ep);
-		if (ep->selecting)
+		if (ep->selecting) {
 			future_init_chain(&ep->select_done,
 						select_data->select_done);
+			future_init_chain(&ep->setup_done,
+						select_data->setup_done);
+		}
 	}
 
 	return true;
@@ -1788,6 +1917,7 @@ static void ep_cancel_select(struct bap_ep *ep)
 {
 	struct bt_bap *bap = ep->data->bap;
 
+	future_clear(&ep->setup_done, ECANCELED, NULL);
 	future_clear(&ep->select_done, ECANCELED, NULL);
 
 	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
@@ -1817,6 +1947,21 @@ static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return true;
 }
 
+static void setup_ensure_stream(void *data, void *user_data)
+{
+	struct bap_setup *setup = data;
+
+	if (!setup->stream)
+		setup_config(setup, user_data);
+}
+
+static void bap_config(void *data, void *user_data)
+{
+	struct bap_ep *ep = data;
+
+	queue_foreach(ep->setups, setup_ensure_stream, NULL);
+}
+
 static void select_complete_cb(int err, const char *err_msg, void *user_data)
 {
 	struct bap_data *data = user_data;
@@ -1828,10 +1973,13 @@ static void select_complete_cb(int err, const char *err_msg, void *user_data)
 	queue_foreach(data->snks, bap_config, NULL);
 }
 
-static void pac_select_all(struct bap_data *data)
+static void pac_select_all(struct bap_data *data, bool reconfigure,
+							struct future *done)
 {
 	struct pac_select_data select_data = {
 		.data = data,
+		.reconfigure = reconfigure,
+		.setup_done = done,
 	};
 
 	future_init(&select_data.select_done, "pac_select_all",
@@ -1856,7 +2004,7 @@ static void bap_ready(struct bt_bap *bap, void *user_data)
 	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_register, service);
 	bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_register, service);
 
-	pac_select_all(data);
+	pac_select_all(data, false, NULL);
 }
 
 static bool match_setup_stream(const void *data, const void *user_data)
@@ -2499,7 +2647,11 @@ static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
 		}
 		break;
 	case BT_BAP_STREAM_STATE_QOS:
+		if (setup) {
 			setup_create_io(data, setup, stream, true);
+			future_clear(&setup->qos_done, setup->io ? 0 : EIO,
+							"Unable to configure");
+		}
 		break;
 	case BT_BAP_STREAM_STATE_ENABLING:
 		if (setup)
@@ -2806,7 +2958,7 @@ static void pac_added(struct bt_bap_pac *pac, void *user_data)
 	bt_bap_foreach_pac(data->bap, BT_BAP_SOURCE, pac_register, service);
 	bt_bap_foreach_pac(data->bap, BT_BAP_SINK, pac_register, service);
 
-	pac_select_all(data);
+	pac_select_all(data, false, NULL);
 }
 
 static void pac_added_broadcast(struct bt_bap_pac *pac, void *user_data)
-- 
2.48.1

Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing

[Pauli Virtanen]

la, 2025-03-01 kello 17:57 +0200, Pauli Virtanen kirjoitti:
> Upper layer as Unicast Client needs to be able to destroy streams when
> it wants to reconfigure endpoints.
> 
> This does not currently work right, because of Server issued
> Releasing->Config (caching) state transitions, which currently cause
> streams never enter Idle (so they are never destroyed).
> 
> Fix this by considering Releasing->Config as Releasing->Idle->Config.
> Also do not make new streams from cached config data as Unicast Client,
> and leave all stream configuration to upper layer.

This change also implies the following, so that reconfiguring multi-ASE
configurations works right:

    shared/bap: ucast client streams always use a free ASE
    
    Because upper layer controls Unicast Client streams, bt_bap_stream_new()
    should for unicast always allocate an unused ASE.

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 4f44db07a..a85169009 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -5836,29 +5836,6 @@ static bool find_ep_unused(const void *data, const void *user_data)
 		return true;
 }
 
-static bool find_ep_pacs(const void *data, const void *user_data)
-{
-	const struct bt_bap_endpoint *ep = data;
-	const struct match_pac *match = user_data;
-
-	if (!ep->stream)
-		return false;
-
-	if (ep->stream->lpac != match->lpac)
-		return false;
-
-	if (ep->stream->rpac != match->rpac)
-		return false;
-
-	switch (ep->state) {
-	case BT_BAP_STREAM_STATE_CONFIG:
-	case BT_BAP_STREAM_STATE_QOS:
-		return true;
-	}
-
-	return false;
-}
-
 static bool find_ep_source(const void *data, const void *user_data)
 {
 	const struct bt_bap_endpoint *ep = data;
@@ -6053,15 +6030,11 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
 	match.lpac = lpac;
 	match.rpac = rpac;
 
-	/* Check for existing stream */
-	ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
+	/* Find unused ASE */
+	ep = queue_find(bap->remote_eps, find_ep_unused, &match);
 	if (!ep) {
-		/* Check for unused ASE */
-		ep = queue_find(bap->remote_eps, find_ep_unused, &match);
-		if (!ep) {
-			DBG(bap, "Unable to find unused ASE");
-			return NULL;
-		}
+		DBG(bap, "Unable to find unused ASE");
+		return NULL;
 	}
 
 	stream = ep->stream;



> ---
>  src/shared/bap.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 54c6e8629..4f44db07a 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -1363,6 +1363,31 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
>  	struct bt_bap *bap = stream->bap;
>  	const struct queue_entry *entry;
>  
> +	switch (stream->ep->old_state) {
> +	case BT_ASCS_ASE_STATE_RELEASING:
> +		/* After Releasing, Server may either transition to Config or
> +		 * Idle. Our Unicast Client streams shall be considered
> +		 * destroyed after Releasing, so that upper layer can control
> +		 * stream creation. Make the lifecycle management simpler by
> +		 * making sure the streams are destroyed by always emitting Idle
> +		 * to upper layer after Releasing, even if the remote ASE did
> +		 * not go through that state.
> +		 */
> +		if (stream->client &&
> +				stream->ep->state != BT_ASCS_ASE_STATE_IDLE &&
> +				(stream->lpac->type & (BT_BAP_SINK |
> +							BT_BAP_SOURCE))) {
> +			struct bt_bap_endpoint *ep = stream->ep;
> +			uint8_t state = ep->state;
> +
> +			ep->state = BT_ASCS_ASE_STATE_IDLE;
> +			bap_stream_state_changed(stream);
> +			ep->state = state;
> +			return;
> +		}
> +		break;
> +	}
> +
>  	/* Pre notification updates */
>  	switch (stream->ep->state) {
>  	case BT_ASCS_ASE_STATE_IDLE:
> @@ -4851,7 +4876,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
>  	}
>  
>  	/* Any previously applied codec configuration may be cached by the
> -	 * server.
> +	 * server. However, all Unicast Client stream creation shall be left to
> +	 * the upper layer.
>  	 */
>  	if (!ep->stream) {
>  		struct match_pac match;
> @@ -4866,7 +4892,9 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
>  		if (!match.lpac || !match.rpac)
>  			return;
>  
> -		bap_stream_new(bap, ep, match.lpac, match.rpac, NULL, true);
> +		if (!(match.lpac->type & (BT_BAP_SINK | BT_BAP_SOURCE)))
> +			bap_stream_new(bap, ep, match.lpac, match.rpac,
> +								NULL, true);
>  	}
>  
>  	if (!ep->stream)

RE: BAP stream reconfiguration

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=939281

---Test result---

Test Summary:
CheckPatch                    PENDING   0.24 seconds
GitLint                       PENDING   0.24 seconds
BuildEll                      PASS      20.51 seconds
BluezMake                     PASS      1476.20 seconds
MakeCheck                     PASS      13.02 seconds
MakeDistcheck                 PASS      160.25 seconds
CheckValgrind                 PASS      215.99 seconds
CheckSmatch                   WARNING   299.79 seconds
bluezmakeextell               PASS      98.83 seconds
IncrementalBuild              PENDING   0.30 seconds
ScanBuild                     PASS      883.73 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:305:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Mar 1, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Allow user to remove specific streams by calling
> ClearConfiguration(transport_path) on the endpoint.  If the path is the
> endpoint path instead, clear all streams associated with the endpoint.
> ---
>  doc/org.bluez.MediaEndpoint.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
> index f2b830ab0..b81106f0b 100644
> --- a/doc/org.bluez.MediaEndpoint.rst
> +++ b/doc/org.bluez.MediaEndpoint.rst
> @@ -109,6 +109,12 @@ void ClearConfiguration(object transport)
>
>         Clear transport configuration.
>
> +       **Server role:** [ISO only]
> +
> +       Close the stream associated with the given transport. If the
> +       path given is the path of this endpoint, all its streams are
> +       closed.

This seems sort of trivial, that said we can't really guarantee the
MediaTransports will be closed even if we send an ASCS_Release
operation the server may still cache the codec configuration.

>  void Release()
>  ``````````````
>
> --
> 2.48.1
>
>


-- 
Luiz Augusto von Dentz

Re: [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Mar 1, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add a simple way for the sound server to reconfigure a BAP unicast
> endpoint, by calling org.bluez.MediaEndpoint.SelectProperties().
>
> This shall destroy all streams of the endpoint, and restart the
> SelectProperties() configuration flow from the beginning.
>
> Since it may be necessary to reconfigure multiple endpoints at once to
> correctly make bidirectional CIS, add Defer argument to just mark eps
> for configuration.
>
> In future, org.bluez.MediaEndpoint.SetConfiguration() could be changed
> to handle unicast in the same way as broadcast: only create streams.
> This allows sound server to have full control over stream configuration
> itself, and not rely on bt_bap_select().

This one Im not too sure, perhaps it would be better to add something
like org.bluez.MediaTransport.Reconfigure to avoid having to add role
dependent documentation like bellow

> ---
>  doc/org.bluez.MediaEndpoint.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
> index b81106f0b..5c42b878c 100644
> --- a/doc/org.bluez.MediaEndpoint.rst
> +++ b/doc/org.bluez.MediaEndpoint.rst
> @@ -69,6 +69,8 @@ array{byte} SelectConfiguration(array{byte} capabilities)
>  dict SelectProperties(dict capabilities)
>  ````````````````````````````````````````
>
> +       **Server Role**
> +
>         Select BAP unicast configuration from the supported capabilities:
>
>         :object Endpoint:
> @@ -104,6 +106,25 @@ dict SelectProperties(dict capabilities)
>         Note: There is no need to cache the selected properties since on
>         success the configuration is send back as parameter of SetConfiguration.
>
> +       **Client Role**
> +
> +       Reconfigure a BAP unicast endpoint. This closes all existing
> +       streams of the endpoint, and restarts the configuration
> +       selection flow which e.g. triggers calls to *SelectProperties*
> +       allowing the sound server to modify the configuration.
> +
> +       The following arguments are taken in *capabilities*:
> +
> +       :boolean Defer [optional]:
> +
> +               If True, mark endpoint for reconfiguration, but
> +               postpone it until a non-deferred *SelectProperties()*
> +               operation is made on an endpoint of the same device.
> +
> +               This is necessary to use when reconfiguring source and
> +               sink streams with the intention that they be combined
> +               into the same CIG, possibly forming bidirectional CIS.

We could perhaps adds Defer option to MediaTransport.Reconfigure and
then use MediaTransport.Select to clear it, that said for broadcast we
did set a dedicated state when it is not active so perhaps we should
do something similar here.

>  void ClearConfiguration(object transport)
>  `````````````````````````````````````````
>
> --
> 2.48.1
>
>


-- 
Luiz Augusto von Dentz

Re: [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Mar 1, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Multi-part operations will need to postpone things like DBus replies
> until all parts are complete. Make this a bit simpler with a chainable
> future.
> ---
>  profiles/audio/bap.c | 136 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 105 insertions(+), 31 deletions(-)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 37168e58c..8b9b89c70 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -80,7 +80,7 @@ struct bap_setup {
>         struct iovec *metadata;
>         unsigned int id;
>         struct iovec *base;
> -       DBusMessage *msg;
> +       struct future *qos_done;
>         void (*destroy)(struct bap_setup *setup);
>  };
>
> @@ -114,6 +114,17 @@ struct bap_data {
>         void *user_data;
>  };
>
> +typedef void (*future_func_t)(int err, const char *err_msg, void *data);
> +
> +struct future {
> +       unsigned int step, steps;
> +       const char *name;
> +       future_func_t func;
> +       void *data;
> +       int err;
> +       const char *err_msg;
> +};

This I'm not convinced is the right direction, it will be sort of hard
to make it generic enough besides I think this should actually be
handled directly by bt_bap instead, it is actually weird that the
testing spec doesn't capture stream reconfiguration from streaming
state for example, in any case we can come up with our on tests for it
to ensure the stream is properly released and CIS is closed before we
attempt reconfigure it for example.

>  static struct queue *sessions;
>
>  /* Structure holding the parameters for Periodic Advertisement create sync.
> @@ -155,6 +166,94 @@ struct bt_iso_qos bap_sink_pa_qos = {
>         }
>  };
>
> +static void future_clear(struct future **p, int err, const char *err_msg)
> +{
> +       struct future *h = *p;
> +
> +       if (!h)
> +               return;
> +
> +       DBG("future %p (%s) 0x%02x (%s) step %u/%u", h, h->name ? h->name : "",
> +               err, (err && err_msg) ? err_msg : "", h->step + 1, h->steps);
> +
> +       *p = NULL;
> +
> +       if (err && !h->err) {
> +               h->err = err;
> +               h->err_msg = err_msg;
> +       }
> +
> +       if (++h->step < h->steps)
> +               return;
> +
> +       h->func(h->err, h->err_msg, h->data);
> +       free(h);
> +}
> +
> +static void future_dbus_callback_func(int err, const char *err_msg, void *data)
> +{
> +       DBusMessage *msg = data;
> +       DBusMessage *reply;
> +
> +       if (err && !err_msg)
> +               err_msg = strerror(err);
> +
> +       switch (err) {
> +       case 0:
> +               reply = dbus_message_new_method_return(msg);
> +               break;
> +       case EINVAL:
> +               reply = btd_error_invalid_args_str(msg, err_msg);
> +               break;
> +       default:
> +               reply = btd_error_failed(msg, err_msg);
> +               break;
> +       }
> +
> +       g_dbus_send_message(btd_get_dbus_connection(), reply);
> +
> +       dbus_message_unref(msg);
> +}
> +
> +static void future_init(struct future **p, const char *name, future_func_t func,
> +                                                               void *data)
> +{
> +       struct future *h;
> +
> +       future_clear(p, ECANCELED, NULL);
> +
> +       h = new0(struct future, 1);
> +       h->steps = 1;
> +       h->name = name;
> +       h->func = func;
> +       h->data = data;
> +
> +       DBG("future %p (%s) init", h, h->name ? h->name : "");
> +
> +       *p = h;
> +}
> +
> +static void future_init_dbus_reply(struct future **p, const char *name,
> +                                                       DBusMessage *msg)
> +{
> +       future_init(p, name, future_dbus_callback_func, dbus_message_ref(msg));
> +}
> +
> +__attribute__((unused))
> +static void future_init_chain(struct future **p, struct future *h)
> +{
> +       future_clear(p, ECANCELED, NULL);
> +
> +       if (h) {
> +               h->steps++;
> +
> +               DBG("future %p (%s) init step %u", h, h->name ? h->name : "",
> +                                                               h->steps);
> +       }
> +
> +       *p = h;
> +}
> +
>  static bool bap_data_set_user_data(struct bap_data *data, void *user_data)
>  {
>         if (!data)
> @@ -740,24 +839,12 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
>                                         void *user_data)
>  {
>         struct bap_setup *setup = user_data;
> -       DBusMessage *reply;
>
>         DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
>
>         setup->id = 0;
>
> -       if (!setup->msg)
> -               return;
> -
> -       if (!code)
> -               reply = dbus_message_new_method_return(setup->msg);
> -       else
> -               reply = btd_error_failed(setup->msg, "Unable to configure");
> -
> -       g_dbus_send_message(btd_get_dbus_connection(), reply);
> -
> -       dbus_message_unref(setup->msg);
> -       setup->msg = NULL;
> +       future_clear(&setup->qos_done, code ? EIO : 0, "Unable to configure");
>  }
>
>  static void config_cb(struct bt_bap_stream *stream,
> @@ -765,7 +852,6 @@ static void config_cb(struct bt_bap_stream *stream,
>                                         void *user_data)
>  {
>         struct bap_setup *setup = user_data;
> -       DBusMessage *reply;
>
>         DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
>
> @@ -786,14 +872,7 @@ static void config_cb(struct bt_bap_stream *stream,
>                 return;
>         }
>
> -       if (!setup->msg)
> -               return;
> -
> -       reply = btd_error_failed(setup->msg, "Unable to configure");
> -       g_dbus_send_message(btd_get_dbus_connection(), reply);
> -
> -       dbus_message_unref(setup->msg);
> -       setup->msg = NULL;
> +       future_clear(&setup->qos_done, EIO, "Unable to configure");
>  }
>
>  static void setup_io_close(void *data, void *user_data)
> @@ -872,7 +951,6 @@ static struct bap_setup *setup_new(struct bap_ep *ep)
>  static void setup_free(void *data)
>  {
>         struct bap_setup *setup = data;
> -       DBusMessage *reply;
>
>         DBG("%p", setup);
>
> @@ -881,12 +959,7 @@ static void setup_free(void *data)
>                 setup->id = 0;
>         }
>
> -       if (setup->msg) {
> -               reply = btd_error_failed(setup->msg, "Canceled");
> -               g_dbus_send_message(btd_get_dbus_connection(), reply);
> -               dbus_message_unref(setup->msg);
> -               setup->msg = NULL;
> -       }
> +       future_clear(&setup->qos_done, ECANCELED, NULL);
>
>         if (setup->ep)
>                 queue_remove(setup->ep->setups, setup);
> @@ -1038,7 +1111,8 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
>
>         switch (bt_bap_stream_get_type(setup->stream)) {
>         case BT_BAP_STREAM_TYPE_UCAST:
> -               setup->msg = dbus_message_ref(msg);
> +               future_init_dbus_reply(&setup->qos_done, "set_configuration",
> +                                                                       msg);
>                 break;
>         case BT_BAP_STREAM_TYPE_BCAST:
>                 /* No message sent over the air for broadcast */
> --
> 2.48.1
>
>


-- 
Luiz Augusto von Dentz

Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Mar 1, 2025 at 11:27 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> la, 2025-03-01 kello 17:57 +0200, Pauli Virtanen kirjoitti:
> > Upper layer as Unicast Client needs to be able to destroy streams when
> > it wants to reconfigure endpoints.
> >
> > This does not currently work right, because of Server issued
> > Releasing->Config (caching) state transitions, which currently cause
> > streams never enter Idle (so they are never destroyed).
> >
> > Fix this by considering Releasing->Config as Releasing->Idle->Config.
> > Also do not make new streams from cached config data as Unicast Client,
> > and leave all stream configuration to upper layer.

Not sure I follow you here, how can we consider it idle even for
cached config data? If we don't keep the stream there won't be a
MediaTransport representing it either so it won't even be possible to
know from the upper layer there is something already configured, so I
really think we should have an option to reconfigure at MediaTransport
layer rather than trying to hide it somehow.

> This change also implies the following, so that reconfiguring multi-ASE
> configurations works right:
>
>     shared/bap: ucast client streams always use a free ASE
>
>     Because upper layer controls Unicast Client streams, bt_bap_stream_new()
>     should for unicast always allocate an unused ASE.
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 4f44db07a..a85169009 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -5836,29 +5836,6 @@ static bool find_ep_unused(const void *data, const void *user_data)
>                 return true;
>  }
>
> -static bool find_ep_pacs(const void *data, const void *user_data)
> -{
> -       const struct bt_bap_endpoint *ep = data;
> -       const struct match_pac *match = user_data;
> -
> -       if (!ep->stream)
> -               return false;
> -
> -       if (ep->stream->lpac != match->lpac)
> -               return false;
> -
> -       if (ep->stream->rpac != match->rpac)
> -               return false;
> -
> -       switch (ep->state) {
> -       case BT_BAP_STREAM_STATE_CONFIG:
> -       case BT_BAP_STREAM_STATE_QOS:
> -               return true;
> -       }
> -
> -       return false;
> -}
> -
>  static bool find_ep_source(const void *data, const void *user_data)
>  {
>         const struct bt_bap_endpoint *ep = data;
> @@ -6053,15 +6030,11 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
>         match.lpac = lpac;
>         match.rpac = rpac;
>
> -       /* Check for existing stream */
> -       ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
> +       /* Find unused ASE */
> +       ep = queue_find(bap->remote_eps, find_ep_unused, &match);

And if there aren't any ASE left then what? I'd go with the design the
spec suggests that stream can be reconfigured for QoS Configuration
state down to Idle, the only hard part is figuring out if a stream
state affects another, etc.

>         if (!ep) {
> -               /* Check for unused ASE */
> -               ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> -               if (!ep) {
> -                       DBG(bap, "Unable to find unused ASE");
> -                       return NULL;
> -               }
> +               DBG(bap, "Unable to find unused ASE");
> +               return NULL;
>         }
>
>         stream = ep->stream;
>
>
>
> > ---
> >  src/shared/bap.c | 32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 54c6e8629..4f44db07a 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -1363,6 +1363,31 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
> >       struct bt_bap *bap = stream->bap;
> >       const struct queue_entry *entry;
> >
> > +     switch (stream->ep->old_state) {
> > +     case BT_ASCS_ASE_STATE_RELEASING:
> > +             /* After Releasing, Server may either transition to Config or
> > +              * Idle. Our Unicast Client streams shall be considered
> > +              * destroyed after Releasing, so that upper layer can control
> > +              * stream creation. Make the lifecycle management simpler by
> > +              * making sure the streams are destroyed by always emitting Idle
> > +              * to upper layer after Releasing, even if the remote ASE did
> > +              * not go through that state.
> > +              */
> > +             if (stream->client &&
> > +                             stream->ep->state != BT_ASCS_ASE_STATE_IDLE &&
> > +                             (stream->lpac->type & (BT_BAP_SINK |
> > +                                                     BT_BAP_SOURCE))) {
> > +                     struct bt_bap_endpoint *ep = stream->ep;
> > +                     uint8_t state = ep->state;
> > +
> > +                     ep->state = BT_ASCS_ASE_STATE_IDLE;
> > +                     bap_stream_state_changed(stream);
> > +                     ep->state = state;
> > +                     return;
> > +             }
> > +             break;
> > +     }
> > +
> >       /* Pre notification updates */
> >       switch (stream->ep->state) {
> >       case BT_ASCS_ASE_STATE_IDLE:
> > @@ -4851,7 +4876,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> >       }
> >
> >       /* Any previously applied codec configuration may be cached by the
> > -      * server.
> > +      * server. However, all Unicast Client stream creation shall be left to
> > +      * the upper layer.
> >        */
> >       if (!ep->stream) {
> >               struct match_pac match;
> > @@ -4866,7 +4892,9 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> >               if (!match.lpac || !match.rpac)
> >                       return;
> >
> > -             bap_stream_new(bap, ep, match.lpac, match.rpac, NULL, true);
> > +             if (!(match.lpac->type & (BT_BAP_SINK | BT_BAP_SOURCE)))
> > +                     bap_stream_new(bap, ep, match.lpac, match.rpac,
> > +                                                             NULL, true);
> >       }
> >
> >       if (!ep->stream)
>
>


-- 
Luiz Augusto von Dentz

Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing

[Pauli Virtanen]

Hi Luiz,

ma, 2025-03-17 kello 14:55 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sat, Mar 1, 2025 at 11:27 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > la, 2025-03-01 kello 17:57 +0200, Pauli Virtanen kirjoitti:
> > > Upper layer as Unicast Client needs to be able to destroy streams when
> > > it wants to reconfigure endpoints.
> > > 
> > > This does not currently work right, because of Server issued
> > > Releasing->Config (caching) state transitions, which currently cause
> > > streams never enter Idle (so they are never destroyed).
> > > 
> > > Fix this by considering Releasing->Config as Releasing->Idle->Config.
> > > Also do not make new streams from cached config data as Unicast Client,
> > > and leave all stream configuration to upper layer.
> 
> Not sure I follow you here, how can we consider it idle even for
> cached config data? If we don't keep the stream there won't be a
> MediaTransport representing it either so it won't even be possible to
> know from the upper layer there is something already configured, so I
> really think we should have an option to reconfigure at MediaTransport
> layer rather than trying to hide it somehow.

The idea is that in Client role, the upper layer does not care whether
an ASE is configured or idle. It has to (re)configure it before it
wants to use it. The cost of this is that you are sometimes doing some
Config Codec that wouldn't be strictly needed, but it is harmless.

The problem here was that that linking of CIS gets broken by the
configured streams. That said, now I see it might be addressed just by
clearing the stream QoS and linking state when Releasing (QoS is then
invalid anyway so it needs to be cleared).


> > This change also implies the following, so that reconfiguring multi-ASE
> > configurations works right:
> > 
> >     shared/bap: ucast client streams always use a free ASE
> > 
> >     Because upper layer controls Unicast Client streams, bt_bap_stream_new()
> >     should for unicast always allocate an unused ASE.
> > 
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 4f44db07a..a85169009 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -5836,29 +5836,6 @@ static bool find_ep_unused(const void *data, const void *user_data)
> >                 return true;
> >  }
> > 
> > -static bool find_ep_pacs(const void *data, const void *user_data)
> > -{
> > -       const struct bt_bap_endpoint *ep = data;
> > -       const struct match_pac *match = user_data;
> > -
> > -       if (!ep->stream)
> > -               return false;
> > -
> > -       if (ep->stream->lpac != match->lpac)
> > -               return false;
> > -
> > -       if (ep->stream->rpac != match->rpac)
> > -               return false;
> > -
> > -       switch (ep->state) {
> > -       case BT_BAP_STREAM_STATE_CONFIG:
> > -       case BT_BAP_STREAM_STATE_QOS:
> > -               return true;
> > -       }
> > -
> > -       return false;
> > -}
> > -
> >  static bool find_ep_source(const void *data, const void *user_data)
> >  {
> >         const struct bt_bap_endpoint *ep = data;
> > @@ -6053,15 +6030,11 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
> >         match.lpac = lpac;
> >         match.rpac = rpac;
> > 
> > -       /* Check for existing stream */
> > -       ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
> > +       /* Find unused ASE */
> > +       ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> 
> And if there aren't any ASE left then what? I'd go with the design the
> spec suggests that stream can be reconfigured for QoS Configuration
> state down to Idle, the only hard part is figuring out if a stream
> state affects another, etc.
> 
> >         if (!ep) {
> > -               /* Check for unused ASE */
> > -               ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> > -               if (!ep) {
> > -                       DBG(bap, "Unable to find unused ASE");
> > -                       return NULL;
> > -               }
> > +               DBG(bap, "Unable to find unused ASE");
> > +               return NULL;
> >         }
> > 
> >         stream = ep->stream;
> > 
> > 
> > 
> > > ---
> > >  src/shared/bap.c | 32 ++++++++++++++++++++++++++++++--
> > >  1 file changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index 54c6e8629..4f44db07a 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -1363,6 +1363,31 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
> > >       struct bt_bap *bap = stream->bap;
> > >       const struct queue_entry *entry;
> > > 
> > > +     switch (stream->ep->old_state) {
> > > +     case BT_ASCS_ASE_STATE_RELEASING:
> > > +             /* After Releasing, Server may either transition to Config or
> > > +              * Idle. Our Unicast Client streams shall be considered
> > > +              * destroyed after Releasing, so that upper layer can control
> > > +              * stream creation. Make the lifecycle management simpler by
> > > +              * making sure the streams are destroyed by always emitting Idle
> > > +              * to upper layer after Releasing, even if the remote ASE did
> > > +              * not go through that state.
> > > +              */
> > > +             if (stream->client &&
> > > +                             stream->ep->state != BT_ASCS_ASE_STATE_IDLE &&
> > > +                             (stream->lpac->type & (BT_BAP_SINK |
> > > +                                                     BT_BAP_SOURCE))) {
> > > +                     struct bt_bap_endpoint *ep = stream->ep;
> > > +                     uint8_t state = ep->state;
> > > +
> > > +                     ep->state = BT_ASCS_ASE_STATE_IDLE;
> > > +                     bap_stream_state_changed(stream);
> > > +                     ep->state = state;
> > > +                     return;
> > > +             }
> > > +             break;
> > > +     }
> > > +
> > >       /* Pre notification updates */
> > >       switch (stream->ep->state) {
> > >       case BT_ASCS_ASE_STATE_IDLE:
> > > @@ -4851,7 +4876,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> > >       }
> > > 
> > >       /* Any previously applied codec configuration may be cached by the
> > > -      * server.
> > > +      * server. However, all Unicast Client stream creation shall be left to
> > > +      * the upper layer.
> > >        */
> > >       if (!ep->stream) {
> > >               struct match_pac match;
> > > @@ -4866,7 +4892,9 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> > >               if (!match.lpac || !match.rpac)
> > >                       return;
> > > 
> > > -             bap_stream_new(bap, ep, match.lpac, match.rpac, NULL, true);
> > > +             if (!(match.lpac->type & (BT_BAP_SINK | BT_BAP_SOURCE)))
> > > +                     bap_stream_new(bap, ep, match.lpac, match.rpac,
> > > +                                                             NULL, true);
> > >       }
> > > 
> > >       if (!ep->stream)
> > 
> > 
> 
> 

-- 
Pauli Virtanen

Re: [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration

[Pauli Virtanen]

Hi,

ma, 2025-03-17 kello 14:10 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sat, Mar 1, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Allow user to remove specific streams by calling
> > ClearConfiguration(transport_path) on the endpoint.  If the path is the
> > endpoint path instead, clear all streams associated with the endpoint.
> > ---
> >  doc/org.bluez.MediaEndpoint.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
> > index f2b830ab0..b81106f0b 100644
> > --- a/doc/org.bluez.MediaEndpoint.rst
> > +++ b/doc/org.bluez.MediaEndpoint.rst
> > @@ -109,6 +109,12 @@ void ClearConfiguration(object transport)
> > 
> >         Clear transport configuration.
> > 
> > +       **Server role:** [ISO only]
> > +
> > +       Close the stream associated with the given transport. If the
> > +       path given is the path of this endpoint, all its streams are
> > +       closed.
> 
> This seems sort of trivial, that said we can't really guarantee the
> MediaTransports will be closed even if we send an ASCS_Release
> operation the server may still cache the codec configuration.

It does not work if we consider every ASE in Config Codec state is
associated with a transport. BAP Client should be able to fully control
what it transitions to QoS state, so transport = QoS state makes more
sense. (See also BAP §2.2.1.1 / 2.2.1.2)

Client must be able to transition from state where it e.g. uses sources
& sinks to a state where it only uses sink.

Some devices have restrictions that they cannot use sources with eg.
48kHz rate sinks together. The same devices may also always transition
all ASEs to Config Codec. As Client, we must be able to ignore some of
the configured ASEs, and transition to QoS only the ones we want to
use.

-- 
Pauli Virtanen

RE: BAP stream reconfiguration

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=969627

---Test result---

Test Summary:
CheckPatch                    PENDING   0.26 seconds
GitLint                       PENDING   0.23 seconds
BuildEll                      PASS      20.03 seconds
BluezMake                     PASS      2603.71 seconds
MakeCheck                     PASS      20.48 seconds
MakeDistcheck                 PASS      197.56 seconds
CheckValgrind                 PASS      273.63 seconds
CheckSmatch                   WARNING   301.96 seconds
bluezmakeextell               PASS      130.84 seconds
IncrementalBuild              PENDING   0.26 seconds
ScanBuild                     PASS      901.38 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth