Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231016065228.424400-1-kiran.k@intel.com> <20231016065228.424400-2-kiran.k@intel.com>
In-Reply-To: <20231016065228.424400-2-kiran.k@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 16 Oct 2023 10:38:41 -0700
Message-ID: <CABBYNZKOzgP5ordRNK9UX=i3r=BMM4OushpaN2nO=Ntuexe_kg@mail.gmail.com>
Subject: Re: [PATCH 2/2] shared/bap: Add support for Audio Locations
To:     Kiran K <kiran.k@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com,
        chethan.tumkur.narayan@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Kiran,

On Sun, Oct 15, 2023 at 11:40=E2=80=AFPM Kiran K <kiran.k@intel.com> wrote:
>
> This adds support to provide Audio Locations for BAP Sink and Source Endp=
oints
> ---
>  profiles/audio/media.c |  2 +-
>  src/shared/bap.c       | 56 ++++++++++++++++++++++++++++++++----------
>  src/shared/bap.h       |  6 +++--
>  3 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 51e3ab65d12d..d063bbf11cf9 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1250,7 +1250,7 @@ static bool endpoint_init_pac(struct media_endpoint=
 *endpoint, uint8_t type,
>
>         endpoint->pac =3D bt_bap_add_vendor_pac(db, name, type, endpoint-=
>codec,
>                                 endpoint->cid, endpoint->vid, &endpoint->=
qos,
> -                               &data, metadata);
> +                               &data, metadata, endpoint->location);
>         if (!endpoint->pac) {
>                 error("Unable to create PAC");
>                 free(metadata);
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 925501c48d98..bee19039900f 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -190,6 +190,7 @@ struct bt_bap_pac {
>         uint8_t type;
>         struct bt_bap_codec codec;
>         struct bt_bap_pac_qos qos;
> +       uint32_t location;
>         struct iovec *data;
>         struct iovec *metadata;
>         struct bt_bap_pac_ops *ops;
> @@ -368,6 +369,14 @@ static void pac_foreach(void *data, void *user_data)
>                 meta->len =3D 0;
>  }
>
> +static void get_pac_loc(void *data, void *user_data)
> +{
> +       struct bt_bap_pac *pac =3D data;
> +       uint32_t *location =3D user_data;
> +
> +       *location |=3D pac->location;
> +}
> +
>  static void pacs_sink_read(struct gatt_db_attribute *attrib,
>                                 unsigned int id, uint16_t offset,
>                                 uint8_t opcode, struct bt_att *att,
> @@ -395,7 +404,15 @@ static void pacs_sink_loc_read(struct gatt_db_attrib=
ute *attrib,
>                                 void *user_data)
>  {
>         struct bt_pacs *pacs =3D user_data;
> -       uint32_t value =3D cpu_to_le32(pacs->sink_loc_value);
> +       struct bt_bap_db *bdb =3D pacs->bdb;
> +       uint32_t value;
> +
> +       queue_foreach(bdb->sinks, get_pac_loc, &pacs->sink_loc_value);
> +       if (pacs->sink_loc_value)
> +               value =3D cpu_to_le32(pacs->sink_loc_value);
> +       else
> +               /* Set default value */
> +               value =3D cpu_to_le32(PACS_SNK_LOCATION);
>
>         gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
>                                                         sizeof(value));
> @@ -428,7 +445,15 @@ static void pacs_source_loc_read(struct gatt_db_attr=
ibute *attrib,
>                                 void *user_data)
>  {
>         struct bt_pacs *pacs =3D user_data;
> -       uint32_t value =3D cpu_to_le32(pacs->source_loc_value);
> +       struct bt_bap_db *bdb =3D pacs->bdb;
> +       uint32_t value;
> +
> +       queue_foreach(bdb->sources, get_pac_loc, &pacs->source_loc_value)=
;
> +       if (pacs->source_loc_value)
> +               value =3D cpu_to_le32(pacs->source_loc_value);
> +       else
> +               /* Set default value */
> +               value =3D cpu_to_le32(PACS_SRC_LOCATION);
>
>         gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
>                                                         sizeof(value));
> @@ -474,9 +499,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
>
>         pacs =3D new0(struct bt_pacs, 1);
>
> -       /* Set default values */
> -       pacs->sink_loc_value =3D PACS_SNK_LOCATION;
> -       pacs->source_loc_value =3D PACS_SRC_LOCATION;
> +       pacs->sink_loc_value =3D 0;
> +       pacs->source_loc_value =3D 0;
>         pacs->sink_context_value =3D PACS_SNK_CTXT;
>         pacs->source_context_value =3D PACS_SRC_CTXT;
>         pacs->supported_sink_context_value =3D PACS_SUPPORTED_SNK_CTXT;
> @@ -2451,7 +2475,8 @@ static struct bt_bap_pac *bap_pac_new(struct bt_bap=
_db *bdb, const char *name,
>                                         struct bt_bap_codec *codec,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         struct bt_bap_pac *pac;
>
> @@ -2468,6 +2493,8 @@ static struct bt_bap_pac *bap_pac_new(struct bt_bap=
_db *bdb, const char *name,
>         if (qos)
>                 pac->qos =3D *qos;
>
> +       pac->location =3D location;
> +
>         return pac;
>  }
>
> @@ -2679,7 +2706,8 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gat=
t_db *db,
>                                         uint8_t id, uint16_t cid, uint16_=
t vid,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         struct bt_bap_db *bdb;
>         struct bt_bap_pac *pac, *pac_broadcast_sink;
> @@ -2699,7 +2727,8 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gat=
t_db *db,
>         codec.cid =3D cid;
>         codec.vid =3D vid;
>
> -       pac =3D bap_pac_new(bdb, name, type, &codec, qos, data, metadata)=
;
> +       pac =3D bap_pac_new(bdb, name, type, &codec, qos, data, metadata,
> +                               location);
>
>         switch (type) {
>         case BT_BAP_SINK:
> @@ -2716,7 +2745,7 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gat=
t_db *db,
>                          */
>                         pac_broadcast_sink =3D bap_pac_new(bdb, name,
>                                         BT_BAP_BCAST_SINK, &codec, qos,
> -                                       data, metadata);
> +                                       data, metadata, 0);
>                         bap_add_broadcast_sink(pac_broadcast_sink);
>                 }
>                 break;
> @@ -2737,10 +2766,11 @@ struct bt_bap_pac *bt_bap_add_pac(struct gatt_db =
*db, const char *name,
>                                         uint8_t type, uint8_t id,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         return bt_bap_add_vendor_pac(db, name, type, id, 0x0000, 0x0000, =
qos,
> -                                                       data, metadata);
> +                                               data, metadata, location)=
;
>  }
>
>  uint8_t bt_bap_pac_get_type(struct bt_bap_pac *pac)
> @@ -3256,7 +3286,7 @@ static void bap_parse_pacs(struct bt_bap *bap, uint=
8_t type,
>                 }
>
>                 pac =3D bap_pac_new(bap->rdb, NULL, type, &p->codec, NULL=
, &data,
> -                                                               &metadata=
);
> +                                                       &metadata, 0);
>                 if (!pac)
>                         continue;
>
> @@ -5481,7 +5511,7 @@ bool bt_bap_new_bcast_source(struct bt_bap *bap, co=
nst char *name)
>                 return true;
>
>         pac_broadcast_source =3D bap_pac_new(bap->rdb, name, BT_BAP_BCAST=
_SOURCE,
> -                       NULL, NULL, NULL, NULL);
> +                       NULL, NULL, NULL, NULL, 0);
>         queue_push_tail(bap->rdb->broadcast_sources, pac_broadcast_source=
);
>
>         if (!pac_broadcast_source)
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index ebe4dbf7d858..10e82f35e547 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -141,13 +141,15 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gat=
t_db *db,
>                                         uint8_t id, uint16_t cid, uint16_=
t vid,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata);
> +                                       struct iovec *metadata,
> +                                       uint32_t location);
>
>  struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
>                                         uint8_t type, uint8_t id,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata);
> +                                       struct iovec *metadata,
> +                                       uint32_t location);

If you change the API you will need to fix their users as well
otherwise it won't build and CI will fail.

>  struct bt_bap_pac_ops {
>         int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> --
> 2.34.1


--=20
Luiz Augusto von Dentz