Hi Andrei, On Wed, Feb 28, 2024 at 10:12 AM Andrei Istodorescu <andrei.istodorescu@xxxxxxx> wrote: > > Hi Luiz, > > I have fixed all the comments excepting one. Please see inline. > > > -----Original Message----- > > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> > > Sent: Tuesday, February 27, 2024 4:39 PM > > To: Andrei Istodorescu <andrei.istodorescu@xxxxxxx> > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mihai-Octavian Urzica <mihai- > > octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu > > <silviu.barbulescu@xxxxxxx>; Vlad Pruteanu <vlad.pruteanu@xxxxxxx>; > > Iulia Tanasescu <iulia.tanasescu@xxxxxxx> > > Subject: Re: [PATCH BlueZ v4 1/2] shared/bap: Add API to add an > > observed BIS > > > > Caution: This is an external email. Please take care when clicking links or > > opening attachments. When in doubt, report the message using the 'Report > > this email' button > > > > > > Hi Andrei, > > > > On Fri, Feb 23, 2024 at 10:34 AM Andrei Istodorescu > > <andrei.istodorescu@xxxxxxx> wrote: > > > > > > Add API to add a PAC for each observed BIS that is supported by the > > > local PACS data. > > > Each BIS observed capabilities LTV is compared to the local > > > capabilities and when we have a full LTVs match a PAC record is created for > > that BIS. > > > Also a MediaEndpoint is registered over DBUS and the stream can be > > > enabled using the SetConfiguration DBUS call. > > > --- > > > src/shared/bap.c | 304 > > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > src/shared/bap.h | 13 +- > > > 2 files changed, 302 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/shared/bap.c b/src/shared/bap.c index > > > f5fc1402701c..853919111835 100644 > > > --- a/src/shared/bap.c > > > +++ b/src/shared/bap.c > > > @@ -48,6 +48,15 @@ > > > > > > #define BAP_PROCESS_TIMEOUT 10 > > > > > > +#define BAP_FREQ_LTV_TYPE 1 > > > +#define BAP_DURATION_LTV_TYPE 2 > > > +#define BAP_CHANNEL_ALLOCATION_LTV_TYPE 3 #define > > > +BAP_FRAME_LEN_LTV_TYPE 4 #define > > CODEC_SPECIFIC_CONFIGURATION_MASK (\ > > > + (1<<BAP_FREQ_LTV_TYPE)|\ > > > + (1<<BAP_DURATION_LTV_TYPE)|\ > > > + (1<<BAP_FRAME_LEN_LTV_TYPE)) > > > + > > > struct bt_bap_pac_changed { > > > unsigned int id; > > > bt_bap_pac_func_t added; > > > @@ -1692,11 +1701,8 @@ static unsigned int bap_bcast_config(struct > > bt_bap_stream *stream, > > > bt_bap_stream_func_t func, void > > > *user_data) { > > > stream->qos = *qos; > > > - if (stream->lpac->type == BT_BAP_BCAST_SINK) { > > > - if (data) > > > - stream_config(stream, data, NULL); > > > - stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); > > > - } > > > + stream->lpac->ops->config(stream, stream->cc, &stream->qos, > > > + ep_config_cb, stream->lpac->user_data); > > > > > > return 1; > > > } > > > @@ -3302,6 +3308,13 @@ static void bap_add_broadcast_source(struct > > > bt_bap_pac *pac) static void bap_add_broadcast_sink(struct bt_bap_pac > > > *pac) { > > > queue_push_tail(pac->bdb->broadcast_sinks, pac); > > > + > > > + /* Update local PACS for broadcast sink also, when registering an > > > + * endpoint > > > + */ > > > + pacs_add_sink_location(pac->bdb->pacs, pac->qos.location); > > > + pacs_add_sink_supported_context(pac->bdb->pacs, > > > + pac->qos.supported_context); > > > } > > > > > > static void notify_pac_added(void *data, void *user_data) @@ -3453,6 > > > +3466,16 @@ struct bt_bap_pac_qos *bt_bap_pac_get_qos(struct > > bt_bap_pac *pac) > > > return &pac->qos; > > > } > > > > > > +struct iovec *bt_bap_pac_get_data(struct bt_bap_pac *pac) { > > > + return pac->data; > > > +} > > > + > > > +struct iovec *bt_bap_pac_get_metadata(struct bt_bap_pac *pac) { > > > + return pac->metadata; > > > +} > > > + > > > uint8_t bt_bap_stream_get_type(struct bt_bap_stream *stream) { > > > if (!stream) > > > @@ -5253,10 +5276,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; > > > } > > > > > > @@ -5892,8 +5911,9 @@ static void add_new_subgroup(struct bt_base > > > *base, > > > > > > struct bt_ltv_match { > > > uint8_t l; > > > - uint8_t *v; > > > + void *data; > > > bool found; > > > + uint32_t data32; > > > }; > > > > > > struct bt_ltv_search { > > > @@ -5912,7 +5932,7 @@ static void match_ltv(size_t i, uint8_t l, uint8_t t, > > uint8_t *v, > > > if (ltv_match->l != l) > > > return; > > > > > > - if (!memcmp(v, ltv_match->v, l)) > > > + if (!memcmp(v, ltv_match->data, l)) > > > ltv_match->found = true; } > > > > > > @@ -5924,7 +5944,7 @@ static void search_ltv(size_t i, uint8_t l, > > > uint8_t t, uint8_t *v, > > > > > > ltv_match.found = false; > > > ltv_match.l = l; > > > - ltv_match.v = v; > > > + ltv_match.data = v; > > > > > > util_ltv_foreach(ltv_search->iov->iov_base, > > > ltv_search->iov->iov_len, &t, @@ -5965,8 > > > +5985,10 @@ static bool compare_ltv(struct iovec *iov1, } > > > > > > struct bt_ltv_extract { > > > - struct iovec *result; > > > struct iovec *src; > > > + void *value; > > > + uint8_t len; > > > + struct iovec *result; > > > }; > > > > > > static void extract_ltv(size_t i, uint8_t l, uint8_t t, uint8_t *v, > > > @@ -5978,7 +6000,7 @@ static void extract_ltv(size_t i, uint8_t l, > > > uint8_t t, uint8_t *v, > > > > > > ltv_match.found = false; > > > ltv_match.l = l; > > > - ltv_match.v = v; > > > + ltv_match.data = v; > > > > > > /* Search each BIS caps ltv in subgroup caps > > > * to extract the one that are BIS specific @@ -6111,3 > > > +6133,257 @@ struct iovec *bt_bap_stream_get_base(struct > > bt_bap_stream > > > *stream) > > > > > > return base_iov; > > > } > > > + > > > +static void bap_sink_get_allocation(size_t i, uint8_t l, uint8_t t, > > > + uint8_t *v, void *user_data) { > > > + uint32_t location32; > > > + > > > + if (!v) > > > + return; > > > + > > > + memcpy(&location32, v, l); > > > + *((uint32_t *)user_data) = le32_to_cpu(location32); } > > > + > > > +/* > > > + * This function compares PAC Codec Specific Capabilities, with the > > > +Codec > > > + * Specific Configuration LTVs received in the BASE of the BAP > > > +Source. The > > > + * result is accumulated in data32 which is a bitmask of types. > > > + */ > > > +static void check_pac_caps_ltv(size_t i, uint8_t l, uint8_t t, uint8_t *v, > > > + void *user_data) { > > > + struct bt_ltv_match *compare_data = user_data; > > > + uint8_t *bis_v = compare_data->data; > > > + > > > + switch (t) { > > > + case BAP_FREQ_LTV_TYPE: > > > + { > > > + uint16_t mask = *((uint16_t *)v); > > > + > > > + mask = le16_to_cpu(mask); > > > + if (mask & (1 << (bis_v[0] - 1))) > > > + compare_data->data32 |= 1<<t; > > > + } > > > + break; > > > + case BAP_DURATION_LTV_TYPE: > > > + if ((v[0]) & (1 << bis_v[0])) > > > + compare_data->data32 |= 1<<t; > > > + break; > > > + case BAP_FRAME_LEN_LTV_TYPE: > > > + { > > > + uint16_t min = *((uint16_t *)v); > > > + uint16_t max = *((uint16_t *)(&v[2])); > > > + uint16_t frame_len = *((uint16_t *)bis_v); > > > + > > > + min = le16_to_cpu(min); > > > + max = le16_to_cpu(max); > > > + frame_len = le16_to_cpu(frame_len); > > > + if ((frame_len >= min) && > > > + (frame_len <= max)) > > > + compare_data->data32 |= 1<<t; > > > + } > > > > Don't create new scopes inside a switch statement, that makes it hard to > > follow the code, if you really think that would help have a helper function to > > check each field. > > > > > + break; > > > + } > > > +} > > > + > > > +static void check_source_ltv(size_t i, uint8_t l, uint8_t t, uint8_t *v, > > > + void *user_data) { > > > + struct bt_ltv_match *local_data = user_data; > > > + struct iovec *pac_caps = local_data->data; > > > + struct bt_ltv_match compare_data; > > > + > > > + compare_data.data = v; > > > + > > > + /* Search inside local PAC's caps for LTV of type t */ > > > + util_ltv_foreach(pac_caps->iov_base, pac_caps->iov_len, &t, > > > + check_pac_caps_ltv, > > > + &compare_data); > > > + > > > + local_data->data32 |= compare_data.data32; } > > > + > > > +static void bap_sink_check_level3_ltv(size_t i, uint8_t l, uint8_t t, > > > + uint8_t *v, void *user_data) { > > > + struct bt_ltv_extract *merge_data = user_data; > > > + > > > + merge_data->value = v; > > > + merge_data->len = l; > > > +} > > > + > > > +static void bap_push_ltv(struct iovec *output, uint8_t l, uint8_t t, > > > +void *v) { > > > + l++; > > > + iov_append(output, 1, &l); > > > + iov_append(output, 1, &t); > > > + iov_append(output, l - 1, v); > > > +} > > > > Perhaps we should create a helper function to do this sort of thing, maybe > > util_ltv_push? > > > > > +static void bap_sink_check_level2_ltv(size_t i, uint8_t l, uint8_t t, > > > + uint8_t *v, void *user_data) { > > > + struct bt_ltv_extract *merge_data = user_data; > > > + > > > + merge_data->value = NULL; > > > + util_ltv_foreach(merge_data->src->iov_base, > > > + merge_data->src->iov_len, > > > + &t, > > > + bap_sink_check_level3_ltv, user_data); > > > + > > > + /* If the LTV at level 2 was found at level 3 add the one from level 3, > > > + * otherwise add the one at level 2 > > > + */ > > > + if (merge_data->value) > > > + bap_push_ltv(merge_data->result, merge_data->len, > > > + t, merge_data->value); > > > + else > > > + bap_push_ltv(merge_data->result, l, t, v); } > > > + > > > +static void check_local_pac(void *data, void *user_data) { > > > + struct bt_ltv_match *compare_data = user_data; > > > + struct iovec *bis_data = (struct iovec *)compare_data->data; > > > + const struct bt_bap_pac *pac = data; > > > + > > > + /* Keep searching for a matching PAC if one wasn't found > > > + * in previous PAC element > > > + */ > > > + if (compare_data->found == false) { > > > + struct bt_ltv_match bis_compare_data = { > > > + .data = pac->data, > > > + .data32 = 0, /* LTVs bitmask result */ > > > + .found = false > > > + }; > > > + > > > + /* loop each BIS LTV */ > > > + util_ltv_foreach(bis_data->iov_base, bis_data->iov_len, NULL, > > > + check_source_ltv, &bis_compare_data); > > > + > > > + /* We have a match if all selected LTVs have a match */ > > > + if ((bis_compare_data.data32 & > > > + CODEC_SPECIFIC_CONFIGURATION_MASK) == > > > + CODEC_SPECIFIC_CONFIGURATION_MASK) > > > + compare_data->found = true; > > > + } > > > +} > > > + > > > +static void bap_sink_match_allocation(size_t i, uint8_t l, uint8_t t, > > > + uint8_t *v, void *user_data) { > > > + struct bt_ltv_match *data = user_data; > > > + uint32_t location32; > > > + > > > + if (!v) > > > + return; > > > + > > > + memcpy(&location32, v, l); > > > + location32 = le32_to_cpu(location32); > > > + > > > + /* If all the bits in the received bitmask are found in > > > + * the local bitmask then we have a match > > > + */ > > > + if ((location32 & data->data32) == location32) > > > + data->found = true; > > > + else > > > + data->found = false; > > > +} > > > + > > > +static bool bap_check_bis(struct bt_bap_db *ldb, struct iovec > > > +*bis_data) { > > > + struct bt_ltv_match compare_data = {}; > > > + > > > + /* Check channel allocation against the PACS location. > > > + * If we don't have a location set we can accept any BIS location. > > > + * If the BIS doesn't have a location set we also accept it > > > + */ > > > + compare_data.found = true; > > > + > > > + if (ldb->pacs->sink_loc_value) { > > > + uint8_t type = BAP_CHANNEL_ALLOCATION_LTV_TYPE; > > > + > > > + compare_data.data32 = ldb->pacs->sink_loc_value; > > > + util_ltv_foreach(bis_data->iov_base, bis_data->iov_len, &type, > > > + bap_sink_match_allocation, &compare_data); > > > + } > > > + > > > + /* Check remaining LTVs against the PACs list */ > > > + if (compare_data.found) { > > > + compare_data.data = bis_data; > > > + compare_data.found = false; > > > + queue_foreach(ldb->broadcast_sinks, check_local_pac, > > > + &compare_data); > > > + } > > > + > > > + return compare_data.found; > > > +} > > > + > > > +void bt_bap_add_bis(struct bt_bap *bap, uint8_t bis_index, > > > + struct bt_bap_codec *codec, > > > + struct iovec *l2_caps, > > > + struct iovec *l3_caps, > > > + struct iovec *meta) > > > +{ > > > + struct bt_bap_pac *pac_source_bis; > > > + struct bt_bap_endpoint *ep; > > > + int err = 0; > > > + struct bt_bap_pac_qos bis_qos = {0}; > > > + uint8_t type = 0; > > > + struct bt_ltv_extract merge_data = {0}; > > > + > > > + merge_data.src = l3_caps; > > > + merge_data.result = new0(struct iovec, 1); > > > + > > > + /* Create a Codec Specific Configuration with LTVs at level 2 (subgroup) > > > + * overwritten by LTVs at level 3 (BIS) > > > + */ > > > + util_ltv_foreach(l2_caps->iov_base, > > > + l2_caps->iov_len, > > > + NULL, > > > + bap_sink_check_level2_ltv, &merge_data); > > > + > > > + /* Check each BIS Codec Specific Configuration LTVs against our Codec > > > + * Specific Capabilities and if the BIS matches create a PAC with it > > > + */ > > > + if (bap_check_bis(bap->ldb, merge_data.result) == false) > > > + goto cleanup; > > > + > > > + DBG(bap, "Matching BIS %i", bis_index); > > > + > > > + /* Create a QoS structure based on the received BIS information to > > > + * specify the desired channel for this BIS/PAC > > > + */ > > > + type = BAP_CHANNEL_ALLOCATION_LTV_TYPE; > > > + util_ltv_foreach(merge_data.result->iov_base, > > > + merge_data.result->iov_len, &type, > > > + bap_sink_get_allocation, &bis_qos.location); > > > > Isn't it better to parse this inline with the use of util_iov_pull_*? > > If you don't want to change the iovec passed, which shall be made a const if > > that is intention, then just create a dup and parse it. > merge_data.result is the concatenated list for Capabilities LTVs. We need to > extract the value for the Allocation LTV so that we pass it to the stream qos. > I used util_ltv_foreach to easily access the ltv. Do you see another way? > Maybe I didn't understand your comment. My bad, I thought you were iterating over the same iov but the merge_data is different, so please disconsider this comment. > > > > > > + /* Create a remote PAC */ > > > + pac_source_bis = bap_pac_new(bap->rdb, NULL, > > > + BT_BAP_BCAST_SOURCE, codec, &bis_qos, > > > + merge_data.result, meta); > > > + > > > + err = asprintf(&pac_source_bis->name, "%d", bis_index); > > > + > > > + if (err < 0) { > > > + DBG(bap, "error in asprintf"); > > > + goto cleanup; > > > + } > > > + > > > + /* Add remote source endpoint */ > > > + if (!bap->rdb->broadcast_sources) > > > + bap->rdb->broadcast_sources = queue_new(); > > > + queue_push_tail(bap->rdb->broadcast_sources, pac_source_bis); > > > + > > > + queue_foreach(bap->pac_cbs, notify_pac_added, pac_source_bis); > > > + /* Push remote endpoint with direction sink */ > > > + ep = bap_endpoint_new_broadcast(bap->rdb, BT_BAP_BCAST_SINK); > > > + > > > + if (ep) > > > + queue_push_tail(bap->remote_eps, ep); > > > + > > > +cleanup: > > > + util_iov_free(merge_data.result, 1); } > > > diff --git a/src/shared/bap.h b/src/shared/bap.h index > > > 2c3550921f07..b2826719f84f 100644 > > > --- a/src/shared/bap.h > > > +++ b/src/shared/bap.h > > > @@ -4,7 +4,7 @@ > > > * BlueZ - Bluetooth protocol stack for Linux > > > * > > > * Copyright (C) 2022 Intel Corporation. All rights reserved. > > > - * Copyright 2023 NXP > > > + * Copyright 2023-2024 NXP > > > * > > > */ > > > > > > @@ -175,6 +175,10 @@ uint16_t bt_bap_pac_get_context(struct > > bt_bap_pac > > > *pac); > > > > > > struct bt_bap_pac_qos *bt_bap_pac_get_qos(struct bt_bap_pac *pac); > > > > > > +struct iovec *bt_bap_pac_get_data(struct bt_bap_pac *pac); > > > + > > > +struct iovec *bt_bap_pac_get_metadata(struct bt_bap_pac *pac); > > > > Have these 2 functions above in a separate patch. > > > > > uint8_t bt_bap_stream_get_type(struct bt_bap_stream *stream); > > > > > > struct bt_bap_stream *bt_bap_pac_get_stream(struct bt_bap_pac *pac); > > > @@ -323,3 +327,10 @@ void bt_bap_update_bcast_source(struct > > bt_bap_pac > > > *pac, bool bt_bap_pac_bcast_is_local(struct bt_bap *bap, struct > > > bt_bap_pac *pac); > > > > > > struct iovec *bt_bap_stream_get_base(struct bt_bap_stream *stream); > > > + > > > +void bt_bap_add_bis(struct bt_bap *bap, uint8_t bis_index, > > > + struct bt_bap_codec *codec, > > > + struct iovec *l2_caps, > > > + struct iovec *l3_caps, > > > + struct iovec *meta); > > > + > > > -- > > > 2.40.1 > > > > > > > > > -- > > Luiz Augusto von Dentz > > Regards, > Andrei -- Luiz Augusto von Dentz