Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4890-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
To: luiz.dentz@gmail.com
Cc: andrei.istodorescu@nxp.com,
	iulia.tanasescu@nxp.com,
	linux-bluetooth@vger.kernel.org,
	mihai-octavian.urzica@nxp.com,
	silviu.barbulescu@nxp.com,
	vlad.pruteanu@nxp.com
Subject: Re: [PATCH BlueZ 1/2] shared/bap: Update BAP Broadcast Source state machine
Date: Thu, 23 May 2024 13:46:43 +0300
Message-Id: <20240523104643.18999-1-silviu.barbulescu@nxp.com>
In-Reply-To: <CABBYNZLzSW1VBxZE_f=Qya31OerUxHuwHFOxQJtzNoNt29ia7g@mail.gmail.com>
References: <CABBYNZLzSW1VBxZE_f=Qya31OerUxHuwHFOxQJtzNoNt29ia7g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?SU9lS2F6NmVzR2VjNzBkQU5iQThGTjEzRDFYQk9OcFpRYVVLdmxzWTJ1bmJz?=
 =?utf-8?B?RjhjWHRyMGpxMHBKZkpCTHIyTTVkb2ZPVEhTSkNMMWZPSnhwYW1UejFCVUdU?=
 =?utf-8?B?VjZEN3c4UjVDOEVibG9FK21GTzJvUmZ5VWNZbDdwUUNCVzFSQjhvL2FSeHN4?=
 =?utf-8?B?VzhaMnNTa0tVOXBxcVF0T0xPTjltSmpPOWFuWnVtSGFJK2F5NUErdnozRk1J?=
 =?utf-8?B?dmNkRVFVd3ZNeFR2cGJaQ1RzaXM1YjBrUVRFS1VuYmpnTUsxZWtRMmE3RDd5?=
 =?utf-8?B?clZuenBMUThSRTNTOForWS9IYnh1cmU1ZFZlVklHRS9JMUV6V0ZtcTZYN09V?=
 =?utf-8?B?bm93cjFJZHZsWW15RXpMK0dlc0ZjdFVsUmFVTW1ianljRC9SV2RYbi8zMytJ?=
 =?utf-8?B?T3hIcDlLNzZqb3pOSkFWS0hvRDEzWTBoNWVaMmpjUTRCS3NzRTVVM2JWQWdR?=
 =?utf-8?B?SWxsVnpKM0pMeWNWdkJrU1ExR1d5USswSlNVOHhkQVBQYWVMMDZjZzd3TUVO?=
 =?utf-8?B?K1lqeFJEN256aWRhOVVFVEcwZ1lQRVVGSnk5R1BwZm9qR2c1aUFFek8zNHpS?=
 =?utf-8?B?Tm1YYm9iN0puU0IxTHB5SWo4TTFMd042SllYWHhaaE9Nekc2UmZZNU9UVVdl?=
 =?utf-8?B?SExmYmFZN28xTkNpcVY5b0hFNjFjOXJnVUt3aDZBdDlIRVFjYTNKN2ZUZHFK?=
 =?utf-8?B?Tzg4dEF1bFl5bUNFdjBVNWFwUmZkS3VDSmhsZDU4dUE0T3lmbVlRWXJMTW9x?=
 =?utf-8?B?TXREZ3dHYWU0MEh2dEZidERUSTBuYlM1UnZTQ3Y1QWhMSUdvV1h6S0tDOGI2?=
 =?utf-8?B?UDFtWFVxc3g4UzU3VittRWRlUDI3amkzU1FwK1VlSHVBRG1IRmNBVmxaajNW?=
 =?utf-8?B?dExNVng2alczS0pobWU2WDdnbUxRcDdhVGN5MTZZMTJpNzNIMGJxWU9Uem4v?=
 =?utf-8?B?RlhQaWxaU1VIUy9JMFJGSGtKWmJEbksvbThMRi85Z3haRTFEUm8vQW5FTG5a?=
 =?utf-8?B?U0h5NmQxaTVad1IzbkFmVUVzMlJQZkZ4RXYwK09PV0FUa2NoaFZ5YlNZb3da?=
 =?utf-8?B?YkJicm12enREdGVITUxPMG9UcGlJMGtnVlY1ZlVpcWN5SG1WY1FDb3Nhdy9R?=
 =?utf-8?B?UG9DWGFIZzR1UEFnenMyS0N1d2VoMGRBb2JGbDd5VFJQdnVPampzUWNwWGdu?=
 =?utf-8?B?R1UycFRnVEN4QlFXQ1FXQWd0dTI3dTR5eDlZeTdRYVVJS1pUcTJiM0padWJw?=
 =?utf-8?B?ZUlJTVJlejRiQWNVZGtYdWhrY05KRHZZMkZYUjJQMVVSWkRVeS9wUUp6WGJR?=
 =?utf-8?B?T1lOdkpjTUpoeit6bkN3QnEvTW5JMTNiTFFUNjhsRHZPVnJBd3BKR1JwSVE0?=
 =?utf-8?B?L1EzallHd2VIbDFuK2k4OEhyMjVtWVVodDVEcy8ycUVaQU9Ma3ZCcXZ2TUFx?=
 =?utf-8?B?Z0FxLys5eEJRSmoybzVGbzZJMjF1UFo4WFI3TXFNeFJXM0tmVTRHeWovMXpk?=
 =?utf-8?B?eElhS0g0Mk9kYm55THQvTlBGVUZVN3crYUZCT0lHWGxtQURWcW5UMllwUXMw?=
 =?utf-8?B?S3J6VXJDMnhzUnRzRWNVdGlsVFovMHBTS0oydjN0ZThpVTlwL2NCbTI2MUt1?=
 =?utf-8?B?L0xpaGx3VE9lSmRZd0diNURoNng1Vmc3YVpnVVI4ZWdSUXdhUFdnV3F0NHQy?=
 =?utf-8?B?aDhqRis3MUZaelBIbXpJbUU0VTRRV0srQW1ZaGRhVkYxakJNWkF4NWFNd3BV?=
 =?utf-8?B?UnBkVVEwMEFwWE80U1JmRll2UEJEa0I4NjdKRVZMNHJVa2Q5YUlXUnBRM0F4?=
 =?utf-8?B?VnVlbmlFK3JRSnhCckdubW5qNEt2bUtPbFBRY25wak13ZmtjWmF6QTNEcmFi?=
 =?utf-8?B?UVozek43aXpyTXRINS9sUjVsaDl6V2dlaWdLM1NYQ0h3NjYwWmFpQ3VoQ09F?=
 =?utf-8?B?aTdmTWRoV0FRbGRzdUtvN0JhSGpIbzd1RVIxY2xOUG5KL3ZheFJETTJaS2dy?=
 =?utf-8?B?aDN6WmxNWEFoeWg5VWc2d01oeDBQVjhLVUg1czNCWFZDa3dtZWU5MlhPRHhv?=
 =?utf-8?B?WXJCSnFjMis4OVhJZWpIblZWc0RBVzhpUmFJWWlzQXRNYisxSlRBeWp6SkRs?=
 =?utf-8?B?VkVUcHROb0ZuaWRXbmVpbjVnS2hkUU8vQjI3Y1NOT041RGJEQXU4UVNpbUk1?=
 =?utf-8?B?K3c9PQ==?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c277cae-eadc-425a-3266-08dc7b15a417
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB10073.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2024 10:46:45.9516
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: U8AeTkRHU6XaAjRKHx/QhjCf04TJhG9ZvhkqDbujSKV7pA1UqaVJ/Q/BaG+ZO0bDMV+WZoODFCI8D0NADnLYw27JwQvllz6ezjUv5KHeapU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR04MB10560

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Monday, May 20, 2024 7:40 PM
> To: Silviu Florian Barbulescu <silviu.barbulescu@nxp.com>
> Cc: linux-bluetooth@vger.kernel.org; Mihai-Octavian Urzica <mihai-
> octavian.urzica@nxp.com>; Vlad Pruteanu <vlad.pruteanu@nxp.com>; Andrei
> Istodorescu <andrei.istodorescu@nxp.com>; Iulia Tanasescu
> <iulia.tanasescu@nxp.com>
> Subject: [EXT] Re: [PATCH BlueZ 1/2] shared/bap: Update BAP Broadcast Source
> state machine
> 
> 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 Silviu,
> 
> On Mon, May 20, 2024 at 12:28 PM Silviu Florian Barbulescu
> <silviu.barbulescu@nxp.com> wrote:
> >
> > Update BAP Broadcast Source state machine states to use BAP define
> > states  for source Idle, Config, Streaming, and an intermediary state enabling.
> 
> Not really following, what is an intermediary state? Or do you mean internal
> state? And more importantly why would we need an internal state like that?
> 

Hi Luiz we use the ENABLING state to know when a transport linked to a stream 
has been acquired by a process and in the case of a BIG with one BIS stream goes 
in the ENABLING state waiting for the response from the kernel that the BIG has 
been created so it can go to the streaming state.
For the case of a BIG with multiple BISes, the BIG is created when all BISes 
are acquired. So we use the ENABLING state to  verify that all transports 
attached to that streams form  BIG have been acquired so we can create the BIG.
If we don’t use this internal state we can use a flag in the stream structure 
to be informed that the transport attached to the stream is in the acquiring 
procedure, but this would not look good, as we will have a flag intercalated 
with the stream state machine.

> > Updated test-bap too.
> >
> > ---
> >  src/shared/bap.c | 39 ++++++++++++++++++++-------------------
> >  unit/test-bap.c  |  4 ++--
> >  2 files changed, 22 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/shared/bap.c b/src/shared/bap.c index
> > 6572ef1d1..639149520 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -1361,14 +1361,6 @@ static void ep_config_cb(struct bt_bap_stream
> *stream, int err)
> >         if (err)
> >                 return;
> >
> > -       if (bt_bap_stream_get_type(stream) == BT_BAP_STREAM_TYPE_BCAST)
> {
> > -               if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK)
> > -                       stream_set_state(stream, BT_BAP_STREAM_STATE_QOS);
> > -               else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE)
> > -                       stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> > -               return;
> > -       }
> > -
> >         stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);  }
> >
> > @@ -1759,6 +1751,15 @@ static unsigned int bap_stream_metadata(struct
> bt_bap_stream *stream,
> >         return req->id;
> >  }
> >
> > +static unsigned int bap_bcast_qos(struct bt_bap_stream *stream,
> > +                                       struct bt_bap_qos *data,
> > +                                       bt_bap_stream_func_t func,
> > +                                       void *user_data) {
> > +       stream->qos = *data;
> > +       return 1;
> > +}
> > +
> >  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
> > *user_data) @@ -2071,7 +2072,7 @@ static unsigned int
> bap_bcast_get_state(struct bt_bap_stream *stream)
> >         return stream->state;
> >  }
> >
> > -static unsigned int bap_bcast_enable(struct bt_bap_stream *stream,
> > +static unsigned int bap_bcast_sink_enable(struct bt_bap_stream
> > +*stream,
> >                                         bool enable_links, struct iovec *data,
> >                                         bt_bap_stream_func_t func,
> >                                         void *user_data) @@ -2081,22
> > +2082,21 @@ static unsigned int bap_bcast_enable(struct bt_bap_stream
> *stream,
> >         return 1;
> >  }
> >
> > -static unsigned int bap_bcast_start(struct bt_bap_stream *stream,
> > +static unsigned int bap_bcast_src_enable(struct bt_bap_stream *stream,
> > +                                       bool enable_links, struct
> > +iovec *data,
> >                                         bt_bap_stream_func_t func,
> >                                         void *user_data)  {
> > -       stream_set_state(stream, BT_BAP_STREAM_STATE_STREAMING);
> > +       stream_set_state(stream, BT_BAP_STREAM_STATE_ENABLING);
> >
> >         return 1;
> >  }
> >
> > -static unsigned int bap_bcast_sink_disable(struct bt_bap_stream *stream,
> > -                                       bool disable_links,
> > +static unsigned int bap_bcast_start(struct bt_bap_stream *stream,
> >                                         bt_bap_stream_func_t func,
> >                                         void *user_data)
> >  {
> > -       bap_stream_io_detach(stream);
> > -       stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> > +       stream_set_state(stream, BT_BAP_STREAM_STATE_STREAMING);
> >
> >         return 1;
> >  }
> > @@ -2106,7 +2106,8 @@ static unsigned int bap_bcast_disable(struct
> bt_bap_stream *stream,
> >                                         bt_bap_stream_func_t func,
> >                                         void *user_data)
> >  {
> > -       stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING);
> > +       bap_stream_io_detach(stream);
> > +       stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> >
> >         return 1;
> >  }
> > @@ -2205,14 +2206,14 @@ static const struct bt_bap_stream_ops
> stream_ops[] = {
> >                         bap_ucast_release, bap_ucast_detach),
> >         STREAM_OPS(BT_BAP_BCAST_SINK, bap_bcast_set_state,
> >                         bap_bcast_get_state,
> > -                       bap_bcast_config, NULL, bap_bcast_enable,
> > -                       bap_bcast_start, bap_bcast_sink_disable, NULL,
> > +                       bap_bcast_config, NULL, bap_bcast_sink_enable,
> > +                       bap_bcast_start, bap_bcast_disable, NULL,
> >                         bap_bcast_metadata, bap_bcast_sink_get_dir,
> >                         bap_bcast_get_location,
> >                         bap_bcast_release, bap_bcast_sink_detach),
> >         STREAM_OPS(BT_BAP_BCAST_SOURCE, bap_bcast_set_state,
> >                         bap_bcast_get_state,
> > -                       bap_bcast_config, NULL, bap_bcast_enable,
> > +                       bap_bcast_config, bap_bcast_qos, bap_bcast_src_enable,
> >                         bap_bcast_start, bap_bcast_disable, NULL,
> >                         bap_bcast_metadata, bap_bcast_src_get_dir,
> >                         bap_bcast_get_location,
> > diff --git a/unit/test-bap.c b/unit/test-bap.c
> > index 46ee0e4e5..10f9e348c 100644
> > --- a/unit/test-bap.c
> > +++ b/unit/test-bap.c
> > @@ -547,10 +547,10 @@ static void bsrc_state(struct bt_bap_stream
> *stream, uint8_t old_state,
> >         struct test_data *data = user_data;
> >
> >         switch (new_state) {
> > -       case BT_BAP_STREAM_STATE_QOS:
> > +       case BT_BAP_STREAM_STATE_CONFIG:
> >                 bt_bap_stream_enable(stream, true, NULL, NULL, NULL);
> >                 break;
> > -       case BT_BAP_STREAM_STATE_CONFIG:
> > +       case BT_BAP_STREAM_STATE_ENABLING:
> >                 data->base = bt_bap_stream_get_base(stream);
> >
> >                 g_assert(data->base);
> > --
> > 2.40.1
> >
> 
> 
> --
> Luiz Augusto von Dentz