Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2602-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240318163853.68598-1-silviu.barbulescu@nxp.com> <20240318163853.68598-4-silviu.barbulescu@nxp.com>
In-Reply-To: <20240318163853.68598-4-silviu.barbulescu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 18 Mar 2024 17:38:10 +0000
Message-ID: <CABBYNZKaxEL5OZud2CVi+2b6r9Oq16gyqR3YOEdnfjSAHO9Q-w@mail.gmail.com>
Subject: Re: [PATCH BlueZ v4 3/5] bap: Broadcast source reconfiguration
 support added
To: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, mihai-octavian.urzica@nxp.com, 
	vlad.pruteanu@nxp.com, andrei.istodorescu@nxp.com, iulia.tanasescu@nxp.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Silviu,

On Mon, Mar 18, 2024 at 4:39=E2=80=AFPM Silviu Florian Barbulescu
<silviu.barbulescu@nxp.com> wrote:
>
> If a BIS is reconfigured, the metadata and codec capabilities are updated=
.
> Also, the BASE is updated to reflect the update.
>
> ---
>  profiles/audio/bap.c       | 76 ++++++++++++++++++++++++++++++++++++++
>  profiles/audio/transport.c |  6 ++-
>  src/shared/bap.c           | 11 +++++-
>  3 files changed, 91 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 964ba9c21..e508e03ba 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -580,6 +580,11 @@ static int setup_parse_bcast_qos(struct bap_setup *s=
etup, const char *key,
>                         return -EINVAL;
>
>                 dbus_message_iter_get_basic(iter, &qos->bcast.big);
> +       } else if (!strcasecmp(key, "BIS")) {
> +               if (var !=3D DBUS_TYPE_BYTE)
> +                       return -EINVAL;
> +
> +               dbus_message_iter_get_basic(iter, &qos->bcast.bis);
>         } else if (!strcasecmp(key, "Options")) {
>                 if (var !=3D DBUS_TYPE_BYTE)
>                         return -EINVAL;
> @@ -884,6 +889,53 @@ static void setup_free(void *data)
>         free(setup);
>  }
>
> +static void iterate_setups(struct bap_setup *setup)
> +{
> +       const struct queue_entry *entry;
> +       struct bap_setup *ent_setup;
> +       uint8_t bis_cnt =3D 1;
> +
> +       for (entry =3D queue_get_entries(setup->ep->setups);
> +                               entry; entry =3D entry->next) {
> +               ent_setup =3D entry->data;
> +
> +               if (setup->qos.bcast.big !=3D ent_setup->qos.bcast.big)
> +                       continue;
> +
> +               util_iov_free(ent_setup->base, 1);
> +               ent_setup->base =3D NULL;

I do wonder why we need to store the base as part of the setup? Can't
we just generate it later when it is actually about to be configured
into the socket like I did?

> +               if (setup->qos.bcast.bis =3D=3D bis_cnt) {
> +                       bt_bap_stream_config(ent_setup->stream, &setup->q=
os,
> +                                               setup->caps, NULL, NULL);
> +                       bt_bap_stream_metadata(ent_setup->stream,
> +                                       setup->metadata, NULL, NULL);
> +               }
> +
> +               bis_cnt++;
> +       }
> +}
> +
> +static bool verify_state(struct bap_setup *setup)
> +{
> +       const struct queue_entry *entry;
> +       struct bap_setup *ent_setup;
> +
> +       for (entry =3D queue_get_entries(setup->ep->setups);
> +                               entry; entry =3D entry->next) {
> +               ent_setup =3D entry->data;
> +
> +               if (setup->qos.bcast.big !=3D ent_setup->qos.bcast.big)
> +                       continue;
> +
> +               if (bt_bap_stream_get_state(ent_setup->stream) =3D=3D
> +                               BT_BAP_STREAM_STATE_STREAMING)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
>  static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage =
*msg,
>                                                                 void *dat=
a)
>  {
> @@ -925,6 +977,30 @@ static DBusMessage *set_configuration(DBusConnection=
 *conn, DBusMessage *msg,
>                 util_iov_free(setup->metadata, 1);
>                 setup->metadata =3D util_iov_dup(
>                                 bt_bap_pac_get_metadata(ep->rpac), 1);
> +       } else if (bt_bap_pac_get_type(ep->lpac) =3D=3D BT_BAP_BCAST_SOUR=
CE) {
> +               if (setup->qos.bcast.bis !=3D BT_ISO_QOS_BIS_UNSET) {
> +                       if ((setup->qos.bcast.bis > queue_length(ep->setu=
ps)) ||
> +                                       (setup->qos.bcast.bis =3D=3D 0)) =
{
> +                               setup_free(setup);
> +                               return btd_error_invalid_args(msg);
> +                       }
> +
> +                       /* Verify that no BIS in the BIG is in streaming =
state
> +                        */
> +                       if (!verify_state(setup)) {
> +                               setup_free(setup);
> +                               return btd_error_not_permitted(msg,
> +                                               "Broadcast Audio Stream s=
tate is invalid");
> +                       }

I was thinking we could delay the BASE setup until later when the code
actually needs it then the code for stream_get_base can actually
detect what are the streams with the same BIG and generate the BASE
just once.

> +                       /* Find and update the BIS specified in
> +                        * set_configuration command
> +                        */
> +                       iterate_setups(setup);
> +
> +                       setup_free(setup);
> +                       return g_dbus_create_reply(msg, DBUS_TYPE_INVALID=
);
> +               }
>         }
>
>         setup->stream =3D bt_bap_stream_new(ep->data->bap, ep->lpac, ep->=
rpac,
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index 122c3339e..a060f8c61 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -1643,8 +1643,12 @@ static void bap_state_changed(struct bt_bap_stream=
 *stream, uint8_t old_state,
>                 bap_update_links(transport);
>                 if (!media_endpoint_is_broadcast(transport->endpoint))
>                         bap_update_qos(transport);
> -               else if (bt_bap_stream_io_dir(stream) !=3D BT_BAP_BCAST_S=
OURCE)
> +               else if (bt_bap_stream_io_dir(stream) !=3D BT_BAP_BCAST_S=
OURCE) {
>                         bap_update_bcast_qos(transport);
> +                       if (old_state =3D=3D BT_BAP_STREAM_STATE_QOS)
> +                               bap_update_bcast_config(transport);
> +               }
> +
>                 transport_update_playing(transport, FALSE);
>                 return;
>         case BT_BAP_STREAM_STATE_DISABLING:
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index fd99cbbca..603d6d646 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -1701,7 +1701,16 @@ static unsigned int bap_bcast_config(struct bt_bap=
_stream *stream,
>                                      struct bt_bap_qos *qos, struct iovec=
 *data,
>                                      bt_bap_stream_func_t func, void *use=
r_data)
>  {
> -       stream->qos =3D *qos;
> +       if (qos) {
> +               stream->qos =3D *qos;
> +               stream->qos.bcast.bcode =3D util_iov_dup(qos->bcast.bcode=
, 1);
> +       }
> +
> +       if (data) {
> +               util_iov_free(stream->cc, 1);
> +               stream->cc =3D util_iov_dup(data, 1);
> +       }
> +
>         stream->lpac->ops->config(stream, stream->cc, &stream->qos,
>                         ep_config_cb, stream->lpac->user_data);
>
> --
> 2.39.2

These changes to shared/bap shall be submitted in a separate patch
with a proper description.


--=20
Luiz Augusto von Dentz