* [PATCH BlueZ 2/4] shared/bap: move bcast configure finish out from set_user_data
2023-11-12 16:00 [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice Pauli Virtanen
@ 2023-11-12 16:00 ` Pauli Virtanen
2023-11-12 16:00 ` [PATCH BlueZ 3/4] bap: call explicitly bt_bap_stream_bcast_configured when needed Pauli Virtanen
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-11-12 16:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Creating transports for broadcast streams should not be a side effect of
bt_bap_stream_set_user_data, but via a separate function.
Move that to new function bt_bap_stream_bcast_configured.
---
Notes:
This might be something that should be done in bt_bap_stream_config(),
however that is sometimes called when it appears creating transports is
not intended. Possibly these calls should use
bt_bap_stream_config_update instead.
The broadcast setup logic probably can be cleaned up here, but I don't
currently have a setup to test it, so this is minimal cleanup.
src/shared/bap.c | 19 +++++++++++++++----
src/shared/bap.h | 2 ++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/shared/bap.c b/src/shared/bap.c
index d90e39f7c..085a46216 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1522,6 +1522,21 @@ static uint8_t stream_config(struct bt_bap_stream *stream, struct iovec *cc,
return 0;
}
+int bt_bap_stream_bcast_configured(struct bt_bap_stream *stream)
+{
+ if (!stream)
+ return -EINVAL;
+ if (!stream->lpac->ops || !stream->lpac->ops->config)
+ return -EINVAL;
+ if (bt_bap_stream_get_type(stream) != BT_BAP_STREAM_TYPE_BCAST)
+ return -EINVAL;
+
+ stream->lpac->ops->config(stream, stream->cc, &stream->qos,
+ ep_config_cb, stream->lpac->user_data);
+
+ return 0;
+}
+
static uint8_t ep_config(struct bt_bap_endpoint *ep, struct bt_bap *bap,
struct bt_ascs_config *req,
struct iovec *iov, struct iovec *rsp)
@@ -4748,10 +4763,6 @@ bool bt_bap_stream_set_user_data(struct bt_bap_stream *stream, void *user_data)
stream->user_data = user_data;
- if (bt_bap_stream_get_type(stream) == BT_BAP_STREAM_TYPE_BCAST)
- stream->lpac->ops->config(stream, stream->cc, &stream->qos,
- ep_config_cb, stream->lpac->user_data);
-
return true;
}
diff --git a/src/shared/bap.h b/src/shared/bap.h
index 099c2edd0..0f9ae27b7 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -263,6 +263,8 @@ unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
bt_bap_stream_func_t func,
void *user_data);
+int bt_bap_stream_bcast_configured(struct bt_bap_stream *stream);
+
unsigned int bt_bap_stream_enable(struct bt_bap_stream *stream,
bool enable_links,
struct iovec *metadata,
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH BlueZ 3/4] bap: call explicitly bt_bap_stream_bcast_configured when needed
2023-11-12 16:00 [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice Pauli Virtanen
2023-11-12 16:00 ` [PATCH BlueZ 2/4] shared/bap: move bcast configure finish out from set_user_data Pauli Virtanen
@ 2023-11-12 16:00 ` Pauli Virtanen
2023-11-12 16:00 ` [PATCH BlueZ 4/4] bap: skip Config Codec when it is not needed Pauli Virtanen
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-11-12 16:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Finishing bcast configuration is no longer a side effect of
set_user_data.
---
profiles/audio/bap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 780dff412..85532b1af 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -854,6 +854,7 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
}
bt_bap_stream_set_user_data(ep->stream, ep->path);
+ bt_bap_stream_bcast_configured(ep->stream);
if (ep->metadata && ep->metadata->iov_len)
bt_bap_stream_metadata(ep->stream, ep->metadata, NULL, NULL);
@@ -959,6 +960,7 @@ static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void *user_data)
data->listen_io = io;
bt_bap_stream_set_user_data(ep->stream, ep->path);
+ bt_bap_stream_bcast_configured(ep->stream);
fd = g_io_channel_unix_get_fd(io);
@@ -1200,6 +1202,7 @@ static void bap_config(void *data, void *user_data)
}
bt_bap_stream_set_user_data(ep->stream, ep->path);
+ bt_bap_stream_bcast_configured(ep->stream);
}
static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH BlueZ 4/4] bap: skip Config Codec when it is not needed
2023-11-12 16:00 [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice Pauli Virtanen
2023-11-12 16:00 ` [PATCH BlueZ 2/4] shared/bap: move bcast configure finish out from set_user_data Pauli Virtanen
2023-11-12 16:00 ` [PATCH BlueZ 3/4] bap: call explicitly bt_bap_stream_bcast_configured when needed Pauli Virtanen
@ 2023-11-12 16:00 ` Pauli Virtanen
2023-11-12 17:26 ` [BlueZ,1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice bluez.test.bot
2023-11-13 15:39 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz
4 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-11-12 16:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
If the ASE is in Codec Configured state and the configuration matches
what we want, skip Config Codec and proceed directly to Config QoS.
Combine the config setup in set_configuration with the one in
bap_config.
---
profiles/audio/bap.c | 137 +++++++++++++++++++++++++++----------------
1 file changed, 86 insertions(+), 51 deletions(-)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 85532b1af..2c869e33c 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -99,6 +99,10 @@ struct bap_data {
static struct queue *sessions;
+static int ep_bap_config(struct bap_ep *ep);
+static void bap_create_io(struct bap_data *data, struct bap_ep *ep,
+ struct bt_bap_stream *stream, int defer);
+
static bool bap_data_set_user_data(struct bap_data *data, void *user_data)
{
if (!data)
@@ -837,25 +841,11 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
return btd_error_invalid_args(msg);
}
- /* TODO: Check if stream capabilities match add support for Latency
- * and PHY.
- */
- if (!ep->stream)
- ep->stream = bt_bap_stream_new(ep->data->bap, ep->lpac,
- ep->rpac, &ep->qos, ep->caps);
-
- ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
- config_cb, ep);
- if (!ep->id) {
+ if (ep_bap_config(ep) < 0) {
DBG("Unable to config stream");
- free(ep->caps);
- ep->caps = NULL;
return btd_error_invalid_args(msg);
}
- bt_bap_stream_set_user_data(ep->stream, ep->path);
- bt_bap_stream_bcast_configured(ep->stream);
-
if (ep->metadata && ep->metadata->iov_len)
bt_bap_stream_metadata(ep->stream, ep->metadata, NULL, NULL);
@@ -1175,34 +1165,100 @@ static struct bap_ep *ep_register(struct btd_service *service,
return ep;
}
-static void bap_config(void *data, void *user_data)
+static int ep_bap_qos(struct bap_ep *ep)
{
- struct bap_ep *ep = data;
+ struct bap_data *data = ep->data;
+ struct bt_bap_stream *stream = ep->stream;
+
+ if (ep->id)
+ return -EBUSY;
+ if (!stream || !ep->caps)
+ return -EINVAL;
+
+ bap_create_io(data, ep, stream, true);
+ if (!ep->io) {
+ error("Unable to create io");
+ goto fail;
+ }
+
+ switch (bt_bap_stream_get_type(stream)) {
+ case BT_BAP_STREAM_TYPE_UCAST:
+ /* Wait QoS response to respond */
+ ep->id = bt_bap_stream_qos(stream, &ep->qos, qos_cb, ep);
+ if (!ep->id) {
+ error("Failed to Configure QoS");
+ goto fail;
+ }
+ break;
+ }
+
+ return 0;
+
+fail:
+ bt_bap_stream_release(stream, NULL, NULL);
+ return -EIO;
+}
+
+static void ep_clear_config(struct bap_ep *ep)
+{
+ util_iov_free(ep->caps, 1);
+ ep->caps = NULL;
+ util_iov_free(ep->metadata, 1);
+ ep->metadata = NULL;
+ memset(&ep->qos, 0, sizeof(ep->qos));
+}
+
+static int ep_bap_config(struct bap_ep *ep)
+{
+ struct iovec *caps;
DBG("ep %p caps %p metadata %p", ep, ep->caps, ep->metadata);
if (!ep->caps)
- return;
+ return -EINVAL;
+ if (ep->id)
+ return -EBUSY;
/* TODO: Check if stream capabilities match add support for Latency
* and PHY.
*/
- if (!ep->stream)
+ if (!ep->stream) {
ep->stream = bt_bap_stream_new(ep->data->bap, ep->lpac,
- ep->rpac, &ep->qos, ep->caps);
+ ep->rpac, NULL, NULL);
+ if (!ep->stream)
+ goto fail;
+ }
+
+ bt_bap_stream_set_user_data(ep->stream, ep->path);
+
+ /* Skip to QoS if reconfiguration not needed */
+ caps = bt_bap_stream_get_config(ep->stream);
+ if (bt_bap_stream_get_type(ep->stream) == BT_BAP_STREAM_TYPE_UCAST &&
+ bt_bap_stream_get_state(ep->stream) == BT_BAP_STREAM_STATE_CONFIG &&
+ util_iov_memcmp(caps, ep->caps) == 0) {
+ DBG("ep %p stream %p no reconfig needed", ep, ep->stream);
+ bt_bap_stream_config_update(ep->stream, &ep->qos);
+ return ep_bap_qos(ep);
+ }
ep->id = bt_bap_stream_config(ep->stream, &ep->qos, ep->caps,
config_cb, ep);
- if (!ep->id) {
- DBG("Unable to config stream");
- util_iov_free(ep->caps, 1);
- ep->caps = NULL;
- util_iov_free(ep->metadata, 1);
- ep->metadata = NULL;
- }
+ if (!ep->id)
+ goto fail;
- bt_bap_stream_set_user_data(ep->stream, ep->path);
bt_bap_stream_bcast_configured(ep->stream);
+
+ return 0;
+
+fail:
+ DBG("Unable to config stream");
+ ep_clear_config(ep);
+ return -EIO;
+}
+
+static void bap_config(void *data, void *user_data)
+{
+ ep_bap_config(data);
}
static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
@@ -1513,9 +1569,6 @@ static bool is_cig_busy(struct bap_data *data, uint8_t cig)
return queue_find(sessions, cig_busy_session, &info);
}
-static void bap_create_io(struct bap_data *data, struct bap_ep *ep,
- struct bt_bap_stream *stream, int defer);
-
static gboolean bap_io_recreate(void *user_data)
{
struct bap_ep *ep = user_data;
@@ -1909,26 +1962,8 @@ 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 (ep && !ep->id) {
- bap_create_io(data, ep, stream, true);
- if (!ep->io) {
- error("Unable to create io");
- bt_bap_stream_release(stream, NULL, NULL);
- return;
- }
-
- if (bt_bap_stream_get_type(stream) ==
- BT_BAP_STREAM_TYPE_UCAST) {
- /* Wait QoS response to respond */
- ep->id = bt_bap_stream_qos(stream, &ep->qos,
- qos_cb, ep);
- if (!ep->id) {
- error("Failed to Configure QoS");
- bt_bap_stream_release(stream,
- NULL, NULL);
- }
- }
- }
+ if (ep)
+ ep_bap_qos(ep);
break;
case BT_BAP_STREAM_STATE_QOS:
if (bt_bap_stream_get_type(stream) ==
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [BlueZ,1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice
2023-11-12 16:00 [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice Pauli Virtanen
` (2 preceding siblings ...)
2023-11-12 16:00 ` [PATCH BlueZ 4/4] bap: skip Config Codec when it is not needed Pauli Virtanen
@ 2023-11-12 17:26 ` bluez.test.bot
2023-11-13 15:39 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz
4 siblings, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2023-11-12 17:26 UTC (permalink / raw)
To: linux-bluetooth, pav
[-- Attachment #1: Type: text/plain, Size: 1512 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=800489
---Test result---
Test Summary:
CheckPatch PASS 1.97 seconds
GitLint FAIL 1.46 seconds
BuildEll PASS 23.84 seconds
BluezMake PASS 544.63 seconds
MakeCheck PASS 10.81 seconds
MakeDistcheck PASS 146.51 seconds
CheckValgrind PASS 207.82 seconds
CheckSmatch PASS 311.76 seconds
bluezmakeextell PASS 95.34 seconds
IncrementalBuild PASS 1990.39 seconds
ScanBuild PASS 886.44 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,2/4] shared/bap: move bcast configure finish out from set_user_data
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
14: B2 Line has trailing whitespace: " "
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice
2023-11-12 16:00 [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice Pauli Virtanen
` (3 preceding siblings ...)
2023-11-12 17:26 ` [BlueZ,1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice bluez.test.bot
@ 2023-11-13 15:39 ` Luiz Augusto von Dentz
2023-11-13 16:27 ` Pauli Virtanen
4 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2023-11-13 15:39 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
Hi Pauli,
On Sun, Nov 12, 2023 at 11:00 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add bt_bap_stream_config_update for updating the QoS while in Codec
> Configured state.
> ---
> src/shared/bap.c | 18 ++++++++++++++++++
> src/shared/bap.h | 3 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 13bbcf793..d90e39f7c 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -4600,6 +4600,24 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> return 0;
> }
>
> +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> + struct bt_bap_qos *qos)
> +{
> + if (!bap_stream_valid(stream))
> + return -EINVAL;
> +
> + if (stream->ep->state != BT_BAP_STREAM_STATE_CONFIG)
> + return -EINVAL;
> +
> + switch (bt_bap_stream_get_type(stream)) {
> + case BT_BAP_STREAM_TYPE_UCAST:
> + stream->qos = *qos;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> void *user_data)
> {
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index 23edbf4c6..099c2edd0 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -255,6 +255,9 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> bt_bap_stream_func_t func,
> void *user_data);
>
> +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> + struct bt_bap_qos *pqos);
> +
Can't we just make bt_bap_stream_config do update the config in case
it is for a broadcast? At least to me it seems a much cleaner approach
then starting introducing new functions where the user needs to know
when they should be called, etc.
> unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
> struct bt_bap_qos *qos,
> bt_bap_stream_func_t func,
> --
> 2.41.0
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ 1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice
2023-11-13 15:39 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz
@ 2023-11-13 16:27 ` Pauli Virtanen
0 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-11-13 16:27 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
ma, 2023-11-13 kello 10:39 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Sun, Nov 12, 2023 at 11:00 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > Add bt_bap_stream_config_update for updating the QoS while in Codec
> > Configured state.
> > ---
> > src/shared/bap.c | 18 ++++++++++++++++++
> > src/shared/bap.h | 3 +++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 13bbcf793..d90e39f7c 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -4600,6 +4600,24 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> > return 0;
> > }
> >
> > +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> > + struct bt_bap_qos *qos)
> > +{
> > + if (!bap_stream_valid(stream))
> > + return -EINVAL;
> > +
> > + if (stream->ep->state != BT_BAP_STREAM_STATE_CONFIG)
> > + return -EINVAL;
> > +
> > + switch (bt_bap_stream_get_type(stream)) {
> > + case BT_BAP_STREAM_TYPE_UCAST:
> > + stream->qos = *qos;
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> > void *user_data)
> > {
> > diff --git a/src/shared/bap.h b/src/shared/bap.h
> > index 23edbf4c6..099c2edd0 100644
> > --- a/src/shared/bap.h
> > +++ b/src/shared/bap.h
> > @@ -255,6 +255,9 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> > bt_bap_stream_func_t func,
> > void *user_data);
> >
> > +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> > + struct bt_bap_qos *pqos);
> > +
>
> Can't we just make bt_bap_stream_config do update the config in case
> it is for a broadcast? At least to me it seems a much cleaner approach
> then starting introducing new functions where the user needs to know
> when they should be called, etc.
This is used for updating the QoS for unicast, when the stream has
already the right caps, and we are not going to send another Config
Codec.
For unicast we can avoid adding this function, but it makes
bap_create_io more complicated as it then cannot get the stream QoS we
want to use from bt_bap_stream, and we have to explicitly deal with the
linked stream QoS in profiles/media/bap.c.
We also cannot use bt_bap_stream_qos to update the QoS in this case,
because the IO has to be created first (with the right QoS), so we
first get the auto-allocated CIG/CIS from kernel, and only after that
send Config QoS to the ASE.
In principle we can have bt_bap_stream_config only update the QoS for
unicast if the config is already right. This is probably not good,
since whether you get ASE event then depends, and caller has to check
first.
Or we could have bt_bap_stream_config(stream, &qos, NULL, NULL, NULL)
do what bt_bap_stream_config_update does here.
Not sure what you prefer.
(v2 is anyway necessary here, we need to check linked streams first
before proceeding to QoS.)
***
For broadcast some of the cases where bt_bap_stream_config is called is
probably meant to only update the QoS only like here.
But there I think the transport creation gets a bit tangled, as
sometimes it's done in bt_bap_stream_config and sometimes as a side
effect in bt_bap_stream_set_user_data...
>
> > unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
> > struct bt_bap_qos *qos,
> > bt_bap_stream_func_t func,
> > --
> > 2.41.0
> >
> >
>
>
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 7+ messages in thread