diff mbox series

[BlueZ] shared/bap: improve channel allocation logic in bt_bap_select()

Message ID 81bcbcb669a6f329b7900fb0f6ff0c469ec7201f.1722082241.git.pav@iki.fi (mailing list archive)
State New, archived
Headers show
Series [BlueZ] shared/bap: improve channel allocation logic in bt_bap_select() | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint fail 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 13: B1 Line exceeds max length (83>80): " src/shared/bap.c:foreach_pacs_char() PAC Supported Context found: handle 0x0053" 71: B2 Line has trailing whitespace: " " 77: B2 Line has trailing whitespace: " " 83: B2 Line has trailing whitespace: " " 87: B2 Line has trailing whitespace: " " 90: B2 Line has trailing whitespace: " " 93: B2 Line has trailing whitespace: " "
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 warning CheckSparse WARNING src/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Pauli Virtanen July 27, 2024, 12:13 p.m. UTC
Currently bt_bap_select() determines the need for multi-stream
configurations in a way that depends on accidental GATT detail (whether
Locations is read before or after the PAC) that shouldn't have any
effect on the configuration.

Whether a device gets configured with 1 (AC 1) or 2 (AC 6i) sink ASEs,
consider two devices: Galaxy Buds2 Pro:

    profiles/audio/bap.c:bap_attached() 0x60e000001c40
    src/shared/bap.c:foreach_pacs_char() PAC Context found: handle 0x0050
    src/shared/bap.c:foreach_pacs_char() PAC Supported Context found: handle 0x0053
    src/shared/bap.c:foreach_pacs_char() Sink PAC Location found: handle 0x0056
    src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0059

    Sink PACS Locations: 0x401
    One local PAC, with Locations: all bits
    rpac bt_bap_chan.location == 0x401 (PACS Locations read before PAC)
    -> bt_select() ChannelAllocation: 0x401

Earfun Air Pro 3:

    profiles/audio/bap.c:bap_attached() 0x60e000001e00
    src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x004b
    src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x004e
    src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0051
    src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0054
    src/shared/bap.c:foreach_pacs_char() Sink PAC Location found: handle 0x0057

    Sink PACS Locations: 0x3
    One local PAC, with Locations: all bits
    rpac bt_bap_chan.location == 0x0 (PACS Locations read after PAC)
    -> bt_select() ChannelAllocation: 0x1 (ASE 1)
    -> bt_select() ChannelAllocation: 0x2 (ASE 2)

Both devices have 2+ Sink ASEs and two bits set in Locations, but
bt_bap_select() configures them differently. This occurs due to the
"if (!rc->location) { ... rc->location &= BIT(i);" logic which gets entered
only because of the ordering in which characteristics was read.  Without
"&= BIT(i)" branch, "map.location &= ~(map.location & rc->location)"
causes only one ASE be configured.  The behavior appears accidental,
and e.g. the BIT(i) logic in general is wrong as the rpac index i doesn't
have a relation to channel locations.

Rewrite the multi-ASE configuration logic, and make it independent of
chr read ordering.

Decide first whether to configure 1 or 2 ASE.  Use 2 ASE if locations
contain left/right channel pair and only 1 channel per ASE is supported.

The "right" result depends on what you want to do so this heuristic
tries to aim for a "common" configuration. See eg the above two devices:
their PAC/ASE data only differs in which two location bits are set, but
they need to be configured differently.

If 1 ASE, leave channel allocation to sound server.

If multiple ASE, multiplex as many as possible on each of them.

Remove bt_bap_chan as it's not needed -- also the PACS Locations
is global to the whole device and there are no per-PAC Locations in the
specification, so probably we shouldn't have something like that in the
internal model.
---

Notes:
    The BAP specification doesn't say that the ordering of characteristics
    in the PACS service should have a meaning, and the current logic looks
    wrong to me.
    
    From what's written in the spec, I don't see that there is a "right"
    configuration here to pick, so it seems we are forced to use some
    heuristic here.  It is hardcoded here, and we probably should make some
    changes later on to make it possible for the sound server to control
    which locations exactly are selected.
    
    One design could be to have the local PAC provide a list of Locations
    bitmasks, and BlueZ checks *in order* if the remote device has all the
    bits set in its Locations. If a match is found, BlueZ assigns exactly
    those bits to ASEs using multiple ASE, if needed. Otherwise, BlueZ uses
    a single ASE and punts the channel decision to the sound server.
    
    This would more or less be the same as what is done in this patch,
    except that the configs[] array is provided by the sound server in the
    local Client PAC.
    
    Also, if there are multiple codecs, bt_bap_select() probably should try
    to select them in order, and stop at the first successful selection.
    
    An alternative would be to leave all this to the sound server: change
    API so that SelectProperties() can return a multi-ASE configuration.
    
    The sound server already has to handle low-level details such as
    deciding which PAC configuration is best. This choice however depends on
    whether ASE multiplexing is done or not, so trying to do the decision
    partly in BlueZ and partly in sound server probably just makes things
    more complicated than they need to be. Sound server can first decide
    what Locations to connect, then allocate PACs multiplexing as much as
    possible. BAP Sec. 3.5.3 guarantees there are enough ASEs for all
    location bits.

 src/shared/bap.c | 196 ++++++++++++++++++++++-------------------------
 1 file changed, 93 insertions(+), 103 deletions(-)

Comments

bluez.test.bot@gmail.com July 27, 2024, 1:51 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=874247

---Test result---

Test Summary:
CheckPatch                    PASS      0.54 seconds
GitLint                       FAIL      0.55 seconds
BuildEll                      PASS      24.57 seconds
BluezMake                     PASS      1648.54 seconds
MakeCheck                     PASS      13.33 seconds
MakeDistcheck                 PASS      178.24 seconds
CheckValgrind                 PASS      252.49 seconds
CheckSmatch                   WARNING   355.20 seconds
bluezmakeextell               PASS      119.94 seconds
IncrementalBuild              PASS      1421.31 seconds
ScanBuild                     PASS      1003.41 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ] shared/bap: improve channel allocation logic in bt_bap_select()

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
13: B1 Line exceeds max length (83>80): "    src/shared/bap.c:foreach_pacs_char() PAC Supported Context found: handle 0x0053"
71: B2 Line has trailing whitespace: "    "
77: B2 Line has trailing whitespace: "    "
83: B2 Line has trailing whitespace: "    "
87: B2 Line has trailing whitespace: "    "
90: B2 Line has trailing whitespace: "    "
93: B2 Line has trailing whitespace: "    "
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:283:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures


---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz July 29, 2024, 11:38 a.m. UTC | #2
Hi Pauli,

On Sat, Jul 27, 2024 at 1:16 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Currently bt_bap_select() determines the need for multi-stream
> configurations in a way that depends on accidental GATT detail (whether
> Locations is read before or after the PAC) that shouldn't have any
> effect on the configuration.
>
> Whether a device gets configured with 1 (AC 1) or 2 (AC 6i) sink ASEs,
> consider two devices: Galaxy Buds2 Pro:
>
>     profiles/audio/bap.c:bap_attached() 0x60e000001c40
>     src/shared/bap.c:foreach_pacs_char() PAC Context found: handle 0x0050
>     src/shared/bap.c:foreach_pacs_char() PAC Supported Context found: handle 0x0053
>     src/shared/bap.c:foreach_pacs_char() Sink PAC Location found: handle 0x0056
>     src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0059
>
>     Sink PACS Locations: 0x401
>     One local PAC, with Locations: all bits
>     rpac bt_bap_chan.location == 0x401 (PACS Locations read before PAC)
>     -> bt_select() ChannelAllocation: 0x401
>
> Earfun Air Pro 3:
>
>     profiles/audio/bap.c:bap_attached() 0x60e000001e00
>     src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x004b
>     src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x004e
>     src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0051
>     src/shared/bap.c:foreach_pacs_char() Sink PAC found: handle 0x0054
>     src/shared/bap.c:foreach_pacs_char() Sink PAC Location found: handle 0x0057
>
>     Sink PACS Locations: 0x3
>     One local PAC, with Locations: all bits
>     rpac bt_bap_chan.location == 0x0 (PACS Locations read after PAC)
>     -> bt_select() ChannelAllocation: 0x1 (ASE 1)
>     -> bt_select() ChannelAllocation: 0x2 (ASE 2)
>
> Both devices have 2+ Sink ASEs and two bits set in Locations, but
> bt_bap_select() configures them differently. This occurs due to the
> "if (!rc->location) { ... rc->location &= BIT(i);" logic which gets entered
> only because of the ordering in which characteristics was read.  Without
> "&= BIT(i)" branch, "map.location &= ~(map.location & rc->location)"
> causes only one ASE be configured.  The behavior appears accidental,
> and e.g. the BIT(i) logic in general is wrong as the rpac index i doesn't
> have a relation to channel locations.

Yeah, I have a change for this one though so we don't read based on
the order of attributes found but use a fixed order to prevent that
rc->location has not been set kind of problem.

> Rewrite the multi-ASE configuration logic, and make it independent of
> chr read ordering.
>
> Decide first whether to configure 1 or 2 ASE.  Use 2 ASE if locations
> contain left/right channel pair and only 1 channel per ASE is supported.
>
> The "right" result depends on what you want to do so this heuristic
> tries to aim for a "common" configuration. See eg the above two devices:
> their PAC/ASE data only differs in which two location bits are set, but
> they need to be configured differently.
>
> If 1 ASE, leave channel allocation to sound server.
>
> If multiple ASE, multiplex as many as possible on each of them.
>
> Remove bt_bap_chan as it's not needed -- also the PACS Locations
> is global to the whole device and there are no per-PAC Locations in the
> specification, so probably we shouldn't have something like that in the
> internal model.

Well per server, yes they are global, but we do have location per
record as we could have multi entities registering PAC records we do
have an abstration to diferentiate them. Anyway this is a separate
problem which we might want to discuss after we fix the order problem.

> ---
>
> Notes:
>     The BAP specification doesn't say that the ordering of characteristics
>     in the PACS service should have a meaning, and the current logic looks
>     wrong to me.
>
>     From what's written in the spec, I don't see that there is a "right"
>     configuration here to pick, so it seems we are forced to use some
>     heuristic here.  It is hardcoded here, and we probably should make some
>     changes later on to make it possible for the sound server to control
>     which locations exactly are selected.
>
>     One design could be to have the local PAC provide a list of Locations
>     bitmasks, and BlueZ checks *in order* if the remote device has all the
>     bits set in its Locations. If a match is found, BlueZ assigns exactly
>     those bits to ASEs using multiple ASE, if needed. Otherwise, BlueZ uses
>     a single ASE and punts the channel decision to the sound server.
>
>     This would more or less be the same as what is done in this patch,
>     except that the configs[] array is provided by the sound server in the
>     local Client PAC.
>
>     Also, if there are multiple codecs, bt_bap_select() probably should try
>     to select them in order, and stop at the first successful selection.
>
>     An alternative would be to leave all this to the sound server: change
>     API so that SelectProperties() can return a multi-ASE configuration.
>
>     The sound server already has to handle low-level details such as
>     deciding which PAC configuration is best. This choice however depends on
>     whether ASE multiplexing is done or not, so trying to do the decision
>     partly in BlueZ and partly in sound server probably just makes things
>     more complicated than they need to be. Sound server can first decide
>     what Locations to connect, then allocate PACs multiplexing as much as
>     possible. BAP Sec. 3.5.3 guarantees there are enough ASEs for all
>     location bits.

Yeah, maybe we need a flag or something to indicate if the
ChannelAllocation shall be attempted by bluetoothd or not, for
bluetoothctl Id prefer it that bluetoothd deals with that just to make
it simpler for thing like testing with PTS, etc, anyway Im traveling
right so we need to get back to this once Im back.

>
>  src/shared/bap.c | 196 ++++++++++++++++++++++-------------------------
>  1 file changed, 93 insertions(+), 103 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 499e740c9..b64399193 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -187,11 +187,6 @@ struct bt_bap {
>         void *user_data;
>  };
>
> -struct bt_bap_chan {
> -       uint8_t count;
> -       uint32_t location;
> -};
> -
>  struct bt_bap_pac {
>         struct bt_bap_db *bdb;
>         char *name;
> @@ -3249,50 +3244,6 @@ static void *ltv_merge(struct iovec *data, struct iovec *cont)
>         return util_iov_append(data, cont->iov_base, cont->iov_len);
>  }
>
> -static void bap_pac_chan_add(struct bt_bap_pac *pac, uint8_t count,
> -                               uint32_t location)
> -{
> -       struct bt_bap_chan *chan;
> -
> -       if (!pac->channels)
> -               pac->channels = queue_new();
> -
> -       chan = new0(struct bt_bap_chan, 1);
> -       chan->count = count;
> -       chan->location = location;
> -
> -       queue_push_tail(pac->channels, chan);
> -}
> -
> -static void bap_pac_foreach_channel(size_t i, uint8_t l, uint8_t t, uint8_t *v,
> -                                       void *user_data)
> -{
> -       struct bt_bap_pac *pac = user_data;
> -
> -       if (!v)
> -               return;
> -
> -       bap_pac_chan_add(pac, *v, bt_bap_pac_get_locations(pac));
> -}
> -
> -static void bap_pac_update_channels(struct bt_bap_pac *pac, struct iovec *data)
> -{
> -       uint8_t type = 0x03;
> -
> -       if (!data)
> -               return;
> -
> -       util_ltv_foreach(data->iov_base, data->iov_len, &type,
> -                               bap_pac_foreach_channel, pac);
> -
> -       /* If record didn't set a channel count but set a location use that as
> -        * channel count.
> -        */
> -       if (queue_isempty(pac->channels) && pac->qos.location)
> -               bap_pac_chan_add(pac, pac->qos.location, pac->qos.location);
> -
> -}
> -
>  static void bap_pac_merge(struct bt_bap_pac *pac, struct iovec *data,
>                                         struct iovec *metadata)
>  {
> @@ -3302,9 +3253,6 @@ static void bap_pac_merge(struct bt_bap_pac *pac, struct iovec *data,
>         else
>                 pac->data = util_iov_dup(data, 1);
>
> -       /* Update channels */
> -       bap_pac_update_channels(pac, data);
> -
>         /* Merge metadata into existing record */
>         if (pac->metadata)
>                 ltv_merge(pac->metadata, metadata);
> @@ -4178,6 +4126,8 @@ static void read_source_pac_loc(bool success, uint8_t att_ecode,
>
>         pacs->source_loc_value = get_le32(value);
>
> +       DBG(bap, "PACS Source Locations: 0x%08x", pacs->source_loc_value);
> +
>         /* Resume reading sinks if supported but for some reason is empty */
>         if (pacs->source && queue_isempty(bap->rdb->sources)) {
>                 uint16_t value_handle;
> @@ -4211,6 +4161,8 @@ static void read_sink_pac_loc(bool success, uint8_t att_ecode,
>
>         pacs->sink_loc_value = get_le32(value);
>
> +       DBG(bap, "PACS Sink Locations: 0x%08x", pacs->sink_loc_value);
> +
>         /* Resume reading sinks if supported but for some reason is empty */
>         if (pacs->sink && queue_isempty(bap->rdb->sinks)) {
>                 uint16_t value_handle;
> @@ -5386,12 +5338,53 @@ static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>         return false;
>  }
>
> +static void bap_pac_ltv_ch_counts(size_t i, uint8_t l, uint8_t t, uint8_t *v,
> +                                                               void *user_data)
> +{
> +       uint8_t *mask = user_data;
> +
> +       if (v)
> +               *mask = *v;
> +}
> +
> +static uint8_t bap_pac_ch_counts(struct bt_bap_pac *pac)
> +{
> +       uint8_t type = 0x03;
> +       uint8_t mask = 0;
> +
> +       if (!pac->data)
> +               return 0;
> +
> +       util_ltv_foreach(pac->data->iov_base, pac->data->iov_len, &type,
> +                                               bap_pac_ltv_ch_counts, &mask);
> +
> +       if (!mask)
> +               mask = 0x01;  /* default (BAP v1.0.1 Sec 4.3.1) */
> +
> +       return mask;
> +}
> +
>  int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>                         int *count, bt_bap_pac_select_t func,
>                         void *user_data)
>  {
> -       const struct queue_entry *lchan, *rchan;
> -       int selected = 0;
> +       uint32_t locations;
> +       uint8_t ch_counts;
> +       unsigned int i, num_ase;
> +
> +       /* Hardcoded supported (multi-ASE) configurations: L/R channel pairs */
> +       static const uint32_t configs[] = {
> +               0x1 | 0x2,
> +               0x10 | 0x20,
> +               0x40 | 0x80,
> +               0x400 | 0x800,
> +               0x1000 | 0x2000,
> +               0x10000 | 0x30000,
> +               0x40000 | 0x80000,
> +               0x400000 | 0x800000,
> +               0x1000000 | 0x2000000,
> +               0x4000000 | 0x8000000,
> +       };
>
>         if (!lpac || !rpac || !func)
>                 return -EINVAL;
> @@ -5399,66 +5392,63 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>         if (!lpac->ops || !lpac->ops->select)
>                 return -EOPNOTSUPP;
>
> -       for (lchan = queue_get_entries(lpac->channels); lchan;
> -                                       lchan = lchan->next) {
> -               struct bt_bap_chan *lc = lchan->data;
> -               struct bt_bap_chan map = *lc;
> -               int i;
> +       ch_counts = bap_pac_ch_counts(lpac) & bap_pac_ch_counts(rpac);
> +       locations = bt_bap_pac_get_locations(rpac) & lpac->qos.location;
> +       num_ase = 1;
>
> -               for (i = 0, rchan = queue_get_entries(rpac->channels); rchan;
> -                                       rchan = rchan->next, i++) {
> -                       struct bt_bap_chan *rc = rchan->data;
> +       /* Check if multi-ASE configuration is needed */
> +       for (i = 0; i < ARRAY_SIZE(configs); ++i) {
> +               unsigned int n = __builtin_popcount(configs[i]);
>
> -                       /* Try matching the channel count */
> -                       if (!(map.count & rc->count))
> -                               break;
> +               if (n == 0 || n > 8 || (ch_counts & BIT(n - 1)))
> +                       continue;
>
> -                       /* Check if location was set otherwise attempt to
> -                        * assign one based on the number of channels it
> -                        * supports.
> -                        */
> -                       if (!rc->location) {
> -                               rc->location = bt_bap_pac_get_locations(rpac);
> -                               /* If channel count is 1 use a single
> -                                * location
> -                                */
> -                               if (rc->count == 0x01)
> -                                       rc->location &= BIT(i);
> -                       }
> +               if ((locations & configs[i]) == configs[i]) {
> +                       num_ase = n;
> +                       locations = configs[i];
> +                       break;
> +               }
> +       }
>
> -                       /* Try matching the channel location */
> -                       if (!(map.location & rc->location))
> -                               continue;
> +       /* Otherwise leave channel allocation to sound server */
> +       if (!locations || !ch_counts || num_ase == 1) {
> +               lpac->ops->select(lpac, rpac, 0, &rpac->qos, func, user_data,
> +                                                       lpac->user_data);
> +               if (count)
> +                       (*count)++;
> +               return 0;
> +       }
>
> -                       lpac->ops->select(lpac, rpac, map.location &
> -                                               rc->location, &rpac->qos,
> -                                               func, user_data,
> -                                               lpac->user_data);
> -                       selected++;
> +       /* Allocate channels */
> +       while (locations && num_ase > 0) {
> +               uint32_t allocation = 0, alloc = 0;
> +               unsigned int n;
>
> -                       /* Check if there are any channels left to select */
> -                       map.count &= ~(map.count & rc->count);
> -                       /* Check if there are any locations left to select */
> -                       map.location &= ~(map.location & rc->location);
> +               /* Multiplex as many as possible */
> +               for (i = 0, n = 0; i < 32 && n < 8; ++i) {
> +                       if (!(locations & BIT(i)))
> +                               continue;
>
> -                       if (!map.count || !map.location)
> -                               break;
> +                       alloc |= BIT(i);
> +                       if (BIT(n) & ch_counts)
> +                               allocation = alloc;
>
> -                       /* Check if device require AC*(i) settings */
> -                       if (rc->count == 0x01)
> -                               map.count = map.count >> 1;
> +                       ++n;
>                 }
> -       }
>
> -       /* Fallback to no channel allocation since none could be matched. */
> -       if (!selected) {
> -               lpac->ops->select(lpac, rpac, 0, &rpac->qos, func, user_data,
> -                                       lpac->user_data);
> -               selected++;
> -       }
> +               if (!allocation)
> +                       break;
> +
> +               /* Select */
> +               lpac->ops->select(lpac, rpac, allocation, &rpac->qos,
> +                                       func, user_data, lpac->user_data);
>
> -       if (count)
> -               *count += selected;
> +               locations &= ~allocation;
> +               --num_ase;
> +
> +               if (count)
> +                       (*count)++;
> +       }
>
>         return 0;
>  }
> --
> 2.45.2
>
>
diff mbox series

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 499e740c9..b64399193 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -187,11 +187,6 @@  struct bt_bap {
 	void *user_data;
 };
 
-struct bt_bap_chan {
-	uint8_t count;
-	uint32_t location;
-};
-
 struct bt_bap_pac {
 	struct bt_bap_db *bdb;
 	char *name;
@@ -3249,50 +3244,6 @@  static void *ltv_merge(struct iovec *data, struct iovec *cont)
 	return util_iov_append(data, cont->iov_base, cont->iov_len);
 }
 
-static void bap_pac_chan_add(struct bt_bap_pac *pac, uint8_t count,
-				uint32_t location)
-{
-	struct bt_bap_chan *chan;
-
-	if (!pac->channels)
-		pac->channels = queue_new();
-
-	chan = new0(struct bt_bap_chan, 1);
-	chan->count = count;
-	chan->location = location;
-
-	queue_push_tail(pac->channels, chan);
-}
-
-static void bap_pac_foreach_channel(size_t i, uint8_t l, uint8_t t, uint8_t *v,
-					void *user_data)
-{
-	struct bt_bap_pac *pac = user_data;
-
-	if (!v)
-		return;
-
-	bap_pac_chan_add(pac, *v, bt_bap_pac_get_locations(pac));
-}
-
-static void bap_pac_update_channels(struct bt_bap_pac *pac, struct iovec *data)
-{
-	uint8_t type = 0x03;
-
-	if (!data)
-		return;
-
-	util_ltv_foreach(data->iov_base, data->iov_len, &type,
-				bap_pac_foreach_channel, pac);
-
-	/* If record didn't set a channel count but set a location use that as
-	 * channel count.
-	 */
-	if (queue_isempty(pac->channels) && pac->qos.location)
-		bap_pac_chan_add(pac, pac->qos.location, pac->qos.location);
-
-}
-
 static void bap_pac_merge(struct bt_bap_pac *pac, struct iovec *data,
 					struct iovec *metadata)
 {
@@ -3302,9 +3253,6 @@  static void bap_pac_merge(struct bt_bap_pac *pac, struct iovec *data,
 	else
 		pac->data = util_iov_dup(data, 1);
 
-	/* Update channels */
-	bap_pac_update_channels(pac, data);
-
 	/* Merge metadata into existing record */
 	if (pac->metadata)
 		ltv_merge(pac->metadata, metadata);
@@ -4178,6 +4126,8 @@  static void read_source_pac_loc(bool success, uint8_t att_ecode,
 
 	pacs->source_loc_value = get_le32(value);
 
+	DBG(bap, "PACS Source Locations: 0x%08x", pacs->source_loc_value);
+
 	/* Resume reading sinks if supported but for some reason is empty */
 	if (pacs->source && queue_isempty(bap->rdb->sources)) {
 		uint16_t value_handle;
@@ -4211,6 +4161,8 @@  static void read_sink_pac_loc(bool success, uint8_t att_ecode,
 
 	pacs->sink_loc_value = get_le32(value);
 
+	DBG(bap, "PACS Sink Locations: 0x%08x", pacs->sink_loc_value);
+
 	/* Resume reading sinks if supported but for some reason is empty */
 	if (pacs->sink && queue_isempty(bap->rdb->sinks)) {
 		uint16_t value_handle;
@@ -5386,12 +5338,53 @@  static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return false;
 }
 
+static void bap_pac_ltv_ch_counts(size_t i, uint8_t l, uint8_t t, uint8_t *v,
+								void *user_data)
+{
+	uint8_t *mask = user_data;
+
+	if (v)
+		*mask = *v;
+}
+
+static uint8_t bap_pac_ch_counts(struct bt_bap_pac *pac)
+{
+	uint8_t type = 0x03;
+	uint8_t mask = 0;
+
+	if (!pac->data)
+		return 0;
+
+	util_ltv_foreach(pac->data->iov_base, pac->data->iov_len, &type,
+						bap_pac_ltv_ch_counts, &mask);
+
+	if (!mask)
+		mask = 0x01;  /* default (BAP v1.0.1 Sec 4.3.1) */
+
+	return mask;
+}
+
 int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 			int *count, bt_bap_pac_select_t func,
 			void *user_data)
 {
-	const struct queue_entry *lchan, *rchan;
-	int selected = 0;
+	uint32_t locations;
+	uint8_t ch_counts;
+	unsigned int i, num_ase;
+
+	/* Hardcoded supported (multi-ASE) configurations: L/R channel pairs */
+	static const uint32_t configs[] = {
+		0x1 | 0x2,
+		0x10 | 0x20,
+		0x40 | 0x80,
+		0x400 | 0x800,
+		0x1000 | 0x2000,
+		0x10000 | 0x30000,
+		0x40000 | 0x80000,
+		0x400000 | 0x800000,
+		0x1000000 | 0x2000000,
+		0x4000000 | 0x8000000,
+	};
 
 	if (!lpac || !rpac || !func)
 		return -EINVAL;
@@ -5399,66 +5392,63 @@  int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	if (!lpac->ops || !lpac->ops->select)
 		return -EOPNOTSUPP;
 
-	for (lchan = queue_get_entries(lpac->channels); lchan;
-					lchan = lchan->next) {
-		struct bt_bap_chan *lc = lchan->data;
-		struct bt_bap_chan map = *lc;
-		int i;
+	ch_counts = bap_pac_ch_counts(lpac) & bap_pac_ch_counts(rpac);
+	locations = bt_bap_pac_get_locations(rpac) & lpac->qos.location;
+	num_ase = 1;
 
-		for (i = 0, rchan = queue_get_entries(rpac->channels); rchan;
-					rchan = rchan->next, i++) {
-			struct bt_bap_chan *rc = rchan->data;
+	/* Check if multi-ASE configuration is needed */
+	for (i = 0; i < ARRAY_SIZE(configs); ++i) {
+		unsigned int n = __builtin_popcount(configs[i]);
 
-			/* Try matching the channel count */
-			if (!(map.count & rc->count))
-				break;
+		if (n == 0 || n > 8 || (ch_counts & BIT(n - 1)))
+			continue;
 
-			/* Check if location was set otherwise attempt to
-			 * assign one based on the number of channels it
-			 * supports.
-			 */
-			if (!rc->location) {
-				rc->location = bt_bap_pac_get_locations(rpac);
-				/* If channel count is 1 use a single
-				 * location
-				 */
-				if (rc->count == 0x01)
-					rc->location &= BIT(i);
-			}
+		if ((locations & configs[i]) == configs[i]) {
+			num_ase = n;
+			locations = configs[i];
+			break;
+		}
+	}
 
-			/* Try matching the channel location */
-			if (!(map.location & rc->location))
-				continue;
+	/* Otherwise leave channel allocation to sound server */
+	if (!locations || !ch_counts || num_ase == 1) {
+		lpac->ops->select(lpac, rpac, 0, &rpac->qos, func, user_data,
+							lpac->user_data);
+		if (count)
+			(*count)++;
+		return 0;
+	}
 
-			lpac->ops->select(lpac, rpac, map.location &
-						rc->location, &rpac->qos,
-						func, user_data,
-						lpac->user_data);
-			selected++;
+	/* Allocate channels */
+	while (locations && num_ase > 0) {
+		uint32_t allocation = 0, alloc = 0;
+		unsigned int n;
 
-			/* Check if there are any channels left to select */
-			map.count &= ~(map.count & rc->count);
-			/* Check if there are any locations left to select */
-			map.location &= ~(map.location & rc->location);
+		/* Multiplex as many as possible */
+		for (i = 0, n = 0; i < 32 && n < 8; ++i) {
+			if (!(locations & BIT(i)))
+				continue;
 
-			if (!map.count || !map.location)
-				break;
+			alloc |= BIT(i);
+			if (BIT(n) & ch_counts)
+				allocation = alloc;
 
-			/* Check if device require AC*(i) settings */
-			if (rc->count == 0x01)
-				map.count = map.count >> 1;
+			++n;
 		}
-	}
 
-	/* Fallback to no channel allocation since none could be matched. */
-	if (!selected) {
-		lpac->ops->select(lpac, rpac, 0, &rpac->qos, func, user_data,
-					lpac->user_data);
-		selected++;
-	}
+		if (!allocation)
+			break;
+
+		/* Select */
+		lpac->ops->select(lpac, rpac, allocation, &rpac->qos,
+					func, user_data, lpac->user_data);
 
-	if (count)
-		*count += selected;
+		locations &= ~allocation;
+		--num_ase;
+
+		if (count)
+			(*count)++;
+	}
 
 	return 0;
 }