* [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 17:12 ` BAP stream reconfiguration bluez.test.bot
2025-03-17 18:10 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Luiz Augusto von Dentz
2025-03-01 15:57 ` [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties Pauli Virtanen
` (7 subsequent siblings)
8 siblings, 2 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: BAP stream reconfiguration
2025-03-01 15:57 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Pauli Virtanen
@ 2025-03-01 17:12 ` bluez.test.bot
2025-03-17 18:10 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Luiz Augusto von Dentz
1 sibling, 0 replies; 19+ messages in thread
From: bluez.test.bot @ 2025-03-01 17:12 UTC (permalink / raw)
To: linux-bluetooth, pav
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration
2025-03-01 15:57 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Pauli Virtanen
2025-03-01 17:12 ` BAP stream reconfiguration bluez.test.bot
@ 2025-03-17 18:10 ` Luiz Augusto von Dentz
2025-03-17 19:23 ` Pauli Virtanen
1 sibling, 1 reply; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-17 18:10 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration
2025-03-17 18:10 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Luiz Augusto von Dentz
@ 2025-03-17 19:23 ` Pauli Virtanen
0 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-17 19:23 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-17 18:20 ` Luiz Augusto von Dentz
2025-03-01 15:57 ` [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction Pauli Virtanen
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties
2025-03-01 15:57 ` [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties Pauli Virtanen
@ 2025-03-17 18:20 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-17 18:20 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 1/9] org.bluez.MediaEndpoint: removing BAP streams with ClearConfiguration Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 2/9] org.bluez.MediaEndpoint: add client role SelectProperties Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-17 18:42 ` Luiz Augusto von Dentz
2025-03-01 15:57 ` [RFC PATCH BlueZ 4/9] bap: use futures for select operation Pauli Virtanen
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction
2025-03-01 15:57 ` [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction Pauli Virtanen
@ 2025-03-17 18:42 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-17 18:42 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH BlueZ 4/9] bap: use futures for select operation
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (2 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 3/9] bap: add and use chainable future abstraction Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 5/9] shared/bap: bap_abort_stream_req() should cancel also current req Pauli Virtanen
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH BlueZ 5/9] shared/bap: bap_abort_stream_req() should cancel also current req
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (3 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 4/9] bap: use futures for select operation Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing Pauli Virtanen
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (4 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 5/9] shared/bap: bap_abort_stream_req() should cancel also current req Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 16:26 ` Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 7/9] bap: support removing streams with ClearConfiguration() Pauli Virtanen
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing
2025-03-01 15:57 ` [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing Pauli Virtanen
@ 2025-03-01 16:26 ` Pauli Virtanen
2025-03-17 18:55 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 16:26 UTC (permalink / raw)
To: linux-bluetooth
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)
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing
2025-03-01 16:26 ` Pauli Virtanen
@ 2025-03-17 18:55 ` Luiz Augusto von Dentz
2025-03-17 19:14 ` Pauli Virtanen
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2025-03-17 18:55 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing
2025-03-17 18:55 ` Luiz Augusto von Dentz
@ 2025-03-17 19:14 ` Pauli Virtanen
0 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-17 19:14 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH BlueZ 7/9] bap: support removing streams with ClearConfiguration()
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (5 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 6/9] shared/bap: make sure ucast client stream is destroyed after releasing Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 8/9] bap: do not try QoS before links are updated & io created Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 9/9] bap: implement client role SelectProperties() Pauli Virtanen
8 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH BlueZ 8/9] bap: do not try QoS before links are updated & io created
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (6 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 7/9] bap: support removing streams with ClearConfiguration() Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
2025-03-01 15:57 ` [RFC PATCH BlueZ 9/9] bap: implement client role SelectProperties() Pauli Virtanen
8 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH BlueZ 9/9] bap: implement client role SelectProperties()
2025-03-01 15:57 [RFC PATCH BlueZ 0/9] BAP stream reconfiguration Pauli Virtanen
` (7 preceding siblings ...)
2025-03-01 15:57 ` [RFC PATCH BlueZ 8/9] bap: do not try QoS before links are updated & io created Pauli Virtanen
@ 2025-03-01 15:57 ` Pauli Virtanen
8 siblings, 0 replies; 19+ messages in thread
From: Pauli Virtanen @ 2025-03-01 15:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread