[BlueZ,v4,1/2] shared/bap: Add API to add an observed BIS

Message ID	20240223153450.86694-2-andrei.istodorescu@nxp.com (mailing list archive)
State	Superseded
Headers	show Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-db3eur04on2076.outbound.protection.outlook.com [40.107.6.76]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0FBBF8287A for <linux-bluetooth@vger.kernel.org>; Fri, 23 Feb 2024 15:35:00 +0000 (UTC) From: Andrei Istodorescu <andrei.istodorescu@nxp.com> To: linux-bluetooth@vger.kernel.org Cc: luiz.dentz@gmail.com, mihai-octavian.urzica@nxp.com, silviu.barbulescu@nxp.com, vlad.pruteanu@nxp.com, iulia.tanasescu@nxp.com, Andrei Istodorescu <andrei.istodorescu@nxp.com> Subject: [PATCH BlueZ v4 1/2] shared/bap: Add API to add an observed BIS Date: Fri, 23 Feb 2024 17:34:49 +0200 Message-Id: <20240223153450.86694-2-andrei.istodorescu@nxp.com> In-Reply-To: <20240223153450.86694-1-andrei.istodorescu@nxp.com> References: <20240223153450.86694-1-andrei.istodorescu@nxp.com> Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk MIME-Version: 1.0
Series	Update Sink BASE management \| expand [BlueZ,v4,0/2] Update Sink BASE management [BlueZ,v4,1/2] shared/bap: Add API to add an observed BIS [BlueZ,v4,2/2] bap: Do PA Sync for each BAP Broadcast source discovered

Context	Check	Description
tedd_an/pre-ci_am	success	Success
tedd_an/CheckPatch	success	CheckPatch PASS
tedd_an/GitLint	success	Gitlint PASS
tedd_an/BuildEll	success	Build ELL PASS
tedd_an/BluezMake	success	Bluez Make PASS
tedd_an/MakeCheck	success	Bluez Make Check PASS
tedd_an/MakeDistcheck	success	Make Distcheck PASS
tedd_an/CheckValgrind	success	Check Valgrind PASS
tedd_an/CheckSmatch	success	CheckSparse PASS
tedd_an/bluezmakeextell	success	Make External ELL PASS
tedd_an/IncrementalBuild	success	Incremental Build PASS
tedd_an/ScanBuild	warning	ScanBuild: src/shared/bap.c:1154:2: warning: Use of memory after it is freed DBG(stream->bap, "stream %p", stream); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/shared/bap.c:40:2: note: expanded from macro 'DBG' bap_debug(_bap, "%s:%s() " fmt, __FILE__, __func__, ## arg) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated.

Andrei Istodorescu Feb. 23, 2024, 3:34 p.m. UTC

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(-)

bluez.test.bot@gmail.com Feb. 23, 2024, 5:21 p.m. UTC | #1

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=829146

---Test result---

Test Summary:
CheckPatch                    PASS      1.43 seconds
GitLint                       PASS      0.63 seconds
BuildEll                      PASS      24.47 seconds
BluezMake                     PASS      715.60 seconds
MakeCheck                     PASS      11.57 seconds
MakeDistcheck                 PASS      165.54 seconds
CheckValgrind                 PASS      229.04 seconds
CheckSmatch                   PASS      333.01 seconds
bluezmakeextell               PASS      108.83 seconds
IncrementalBuild              PASS      1352.76 seconds
ScanBuild                     WARNING   968.20 seconds

Details
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
src/shared/bap.c:1154:2: warning: Use of memory after it is freed
        DBG(stream->bap, "stream %p", stream);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/bap.c:40:2: note: expanded from macro 'DBG'
        bap_debug(_bap, "%s:%s() " fmt, __FILE__, __func__, ## arg)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.



---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz Feb. 27, 2024, 2:38 p.m. UTC | #2

Hi Andrei,

On Fri, Feb 23, 2024 at 10:34 AM Andrei Istodorescu
<andrei.istodorescu@nxp.com> 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.

> +       /* 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
>

Andrei Istodorescu Feb. 28, 2024, 3:12 p.m. UTC | #3

Hi Luiz,

I have fixed all the comments excepting one. Please see inline.

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Tuesday, February 27, 2024 4:39 PM
> To: Andrei Istodorescu <andrei.istodorescu@nxp.com>
> Cc: linux-bluetooth@vger.kernel.org; Mihai-Octavian Urzica <mihai-
> octavian.urzica@nxp.com>; Silviu Florian Barbulescu
> <silviu.barbulescu@nxp.com>; Vlad Pruteanu <vlad.pruteanu@nxp.com>;
> Iulia Tanasescu <iulia.tanasescu@nxp.com>
> 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@nxp.com> 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.

> 
> > +       /* 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 Feb. 28, 2024, 3:18 p.m. UTC | #4

Hi Andrei,

On Wed, Feb 28, 2024 at 10:12 AM Andrei Istodorescu
<andrei.istodorescu@nxp.com> wrote:
>
> Hi Luiz,
>
> I have fixed all the comments excepting one. Please see inline.
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> > Sent: Tuesday, February 27, 2024 4:39 PM
> > To: Andrei Istodorescu <andrei.istodorescu@nxp.com>
> > Cc: linux-bluetooth@vger.kernel.org; Mihai-Octavian Urzica <mihai-
> > octavian.urzica@nxp.com>; Silviu Florian Barbulescu
> > <silviu.barbulescu@nxp.com>; Vlad Pruteanu <vlad.pruteanu@nxp.com>;
> > Iulia Tanasescu <iulia.tanasescu@nxp.com>
> > 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@nxp.com> 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

[BlueZ,v4,1/2] shared/bap: Add API to add an observed BIS

Checks

Commit Message

Comments

Patch