diff mbox series

[BlueZ,1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice

Message ID b48261aeab5a4f5927c9da5296b2ffb079bee375.1699802164.git.pav@iki.fi (mailing list archive)
State New, archived
Headers show
Series [BlueZ,1/4] shared/bap: add bt_bap_stream_config_update for updating QoS choice | expand

Checks

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 success Scan Build PASS

Commit Message

Pauli Virtanen Nov. 12, 2023, 4 p.m. UTC 
Add bt_bap_stream_config_update for updating the QoS while in Codec
Configured state.
---
 src/shared/bap.c | 18 ++++++++++++++++++
 src/shared/bap.h |  3 +++
 2 files changed, 21 insertions(+)

Comments

bluez.test.bot@gmail.com Nov. 12, 2023, 5:26 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=800489

---Test result---

Test Summary:
CheckPatch                    PASS      1.97 seconds
GitLint                       FAIL      1.46 seconds
BuildEll                      PASS      23.84 seconds
BluezMake                     PASS      544.63 seconds
MakeCheck                     PASS      10.81 seconds
MakeDistcheck                 PASS      146.51 seconds
CheckValgrind                 PASS      207.82 seconds
CheckSmatch                   PASS      311.76 seconds
bluezmakeextell               PASS      95.34 seconds
IncrementalBuild              PASS      1990.39 seconds
ScanBuild                     PASS      886.44 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,2/4] shared/bap: move bcast configure finish out from set_user_data

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
14: B2 Line has trailing whitespace: "    "


---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Nov. 13, 2023, 3:39 p.m. UTC | #2 
Hi Pauli,

On Sun, Nov 12, 2023 at 11:00 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add bt_bap_stream_config_update for updating the QoS while in Codec
> Configured state.
> ---
>  src/shared/bap.c | 18 ++++++++++++++++++
>  src/shared/bap.h |  3 +++
>  2 files changed, 21 insertions(+)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 13bbcf793..d90e39f7c 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -4600,6 +4600,24 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
>         return 0;
>  }
>
> +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> +                                       struct bt_bap_qos *qos)
> +{
> +       if (!bap_stream_valid(stream))
> +               return -EINVAL;
> +
> +       if (stream->ep->state != BT_BAP_STREAM_STATE_CONFIG)
> +               return -EINVAL;
> +
> +       switch (bt_bap_stream_get_type(stream)) {
> +       case BT_BAP_STREAM_TYPE_UCAST:
> +               stream->qos = *qos;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>                                                         void *user_data)
>  {
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index 23edbf4c6..099c2edd0 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -255,6 +255,9 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
>                                         bt_bap_stream_func_t func,
>                                         void *user_data);
>
> +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> +                                       struct bt_bap_qos *pqos);
> +

Can't we just make bt_bap_stream_config do update the config in case
it is for a broadcast? At least to me it seems a much cleaner approach
then starting introducing new functions where the user needs to know
when they should be called, etc.

>  unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
>                                         struct bt_bap_qos *qos,
>                                         bt_bap_stream_func_t func,
> --
> 2.41.0
>
>
Pauli Virtanen Nov. 13, 2023, 4:27 p.m. UTC | #3 
Hi Luiz,

ma, 2023-11-13 kello 10:39 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Nov 12, 2023 at 11:00 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Add bt_bap_stream_config_update for updating the QoS while in Codec
> > Configured state.
> > ---
> >  src/shared/bap.c | 18 ++++++++++++++++++
> >  src/shared/bap.h |  3 +++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 13bbcf793..d90e39f7c 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -4600,6 +4600,24 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> >         return 0;
> >  }
> > 
> > +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> > +                                       struct bt_bap_qos *qos)
> > +{
> > +       if (!bap_stream_valid(stream))
> > +               return -EINVAL;
> > +
> > +       if (stream->ep->state != BT_BAP_STREAM_STATE_CONFIG)
> > +               return -EINVAL;
> > +
> > +       switch (bt_bap_stream_get_type(stream)) {
> > +       case BT_BAP_STREAM_TYPE_UCAST:
> > +               stream->qos = *qos;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> >  static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> >                                                         void *user_data)
> >  {
> > diff --git a/src/shared/bap.h b/src/shared/bap.h
> > index 23edbf4c6..099c2edd0 100644
> > --- a/src/shared/bap.h
> > +++ b/src/shared/bap.h
> > @@ -255,6 +255,9 @@ unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
> >                                         bt_bap_stream_func_t func,
> >                                         void *user_data);
> > 
> > +int bt_bap_stream_config_update(struct bt_bap_stream *stream,
> > +                                       struct bt_bap_qos *pqos);
> > +
> 
> Can't we just make bt_bap_stream_config do update the config in case
> it is for a broadcast? At least to me it seems a much cleaner approach
> then starting introducing new functions where the user needs to know
> when they should be called, etc.

This is used for updating the QoS for unicast, when the stream has
already the right caps, and we are not going to send another Config
Codec.

For unicast we can avoid adding this function, but it makes
bap_create_io more complicated as it then cannot get the stream QoS we
want to use from bt_bap_stream, and we have to explicitly deal with the
linked stream QoS in profiles/media/bap.c.

We also cannot use bt_bap_stream_qos to update the QoS in this case,
because the IO has to be created first (with the right QoS), so we
first get the auto-allocated CIG/CIS from kernel, and only after that
send Config QoS to the ASE.

In principle we can have bt_bap_stream_config only update the QoS for
unicast if the config is already right. This is probably not good,
since whether you get ASE event then depends, and caller has to check
first.

Or we could have bt_bap_stream_config(stream, &qos, NULL, NULL, NULL)
do what bt_bap_stream_config_update does here.

Not sure what you prefer.

(v2 is anyway necessary here, we need to check linked streams first
before proceeding to QoS.)

***

For broadcast some of the cases where bt_bap_stream_config is called is
probably meant to only update the QoS only like here.

But there I think the transport creation gets a bit tangled, as
sometimes it's done in bt_bap_stream_config and sometimes as a side
effect in bt_bap_stream_set_user_data...

> 
> >  unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
> >                                         struct bt_bap_qos *qos,
> >                                         bt_bap_stream_func_t func,
> > --
> > 2.41.0
> > 
> > 
> 
>
diff mbox series

Patch

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 13bbcf793..d90e39f7c 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -4600,6 +4600,24 @@  unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
 	return 0;
 }
 
+int bt_bap_stream_config_update(struct bt_bap_stream *stream,
+					struct bt_bap_qos *qos)
+{
+	if (!bap_stream_valid(stream))
+		return -EINVAL;
+
+	if (stream->ep->state != BT_BAP_STREAM_STATE_CONFIG)
+		return -EINVAL;
+
+	switch (bt_bap_stream_get_type(stream)) {
+	case BT_BAP_STREAM_TYPE_UCAST:
+		stream->qos = *qos;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 							void *user_data)
 {
diff --git a/src/shared/bap.h b/src/shared/bap.h
index 23edbf4c6..099c2edd0 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -255,6 +255,9 @@  unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
 					bt_bap_stream_func_t func,
 					void *user_data);
 
+int bt_bap_stream_config_update(struct bt_bap_stream *stream,
+					struct bt_bap_qos *pqos);
+
 unsigned int bt_bap_stream_qos(struct bt_bap_stream *stream,
 					struct bt_bap_qos *qos,
 					bt_bap_stream_func_t func,