Hi Tanu,
On Sun, Dec 2, 2012 at 1:10 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Thu, 2012-11-29 at 14:28 +0100, Mikel Astiz wrote:
>> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>>
>> When a headset is having a profile switch, we can either leave the
>> SCO state unmodified (as it was before this patch) or we can
>> alternatively request it (as older versions of PA).
>>
>> This patch tries to avoid a potential regression in case a module
>> such as module-suspend-on-idle is not present, due to the provided
>> resume-on-running policy. Without this patch, and without such a policy,
>> the sink and sources would stay suspended until the user manually
>> changed the suspend state.
>
> The last sentence is incorrect, the situation is worse than that. If the
> suspend cause is IDLE, there's no way for the user to unsuspend the
> device, because the user can only control the USER suspend cause.
OK, so this is worse than I thought.
>>
>> A more sophisticated solution to this problem would be to revert this
>> patch and implement the resume-on-running policy inside
>> module-bluetooth-policy.
>> ---
>> src/modules/bluetooth/module-bluetooth-device.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>> index d1c386f..66f00e0 100644
>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> @@ -2010,7 +2010,10 @@ static int setup_transport(struct userdata *u) {
>> u->transport_removed_slot = pa_hook_connect(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_REMOVED], PA_HOOK_NORMAL,
>> (pa_hook_cb_t) transport_removed_cb, u);
>>
>> - bt_transport_acquire(u, FALSE);
>> + if (u->profile == PROFILE_HSP || u->profile == PROFILE_A2DP)
>> + bt_transport_acquire(u, TRUE);
>> + else
>> + bt_transport_acquire(u, FALSE);
>>
>> bt_transport_config(u);
>
> Shouldn't the "data.suspend_cause = PA_SUSPEND_IDLE" assignment be
> removed from add_sink()? The problem that we're fixing is that if the
> device gets into the IDLE state, it's hosed if module-suspend-on-idle
> isn't loaded. Therefore, setting the suspend cause to IDLE makes no
> sense.
>
> I guess the change done in this patch makes sense anyway. What if
> bt_transport_acquire() fails, shouldn't setup_transport() return an
> error in that case?
If bt_transport_acquire() fails during card profile initialization,
setup_transport() will succeed but the sink/source will be created
suspended (with PA_SUSPEND_IDLE). So if module-suspend-on-idle is not
present, it will remain suspended until the user makes some weird
profile switch (hsp->off->hsp). So this is quite unlikely in practice
but still bad.
>From my point of view, the situation is the following:
module-bluetooth-device implements a feature
("suspend-headset-sco-if-idle") that relies on module-suspend-on-idle.
So the three options that I see are:
(a) Drop the feature completely (in this case PA_SUSPEND_IDLE would
disappear from module-bluetooth-device).
(b) Make a workaround by extending module-bluetooth-device such that
it detects if module-suspend-on-idle has been loaded, and if yes
support this feature, but not otherwise.
(c) Move the resume-if-busy policy in module-suspend-on-idle
(basically the call to pa_{source|sink}_suspend(d, FALSE,
PA_SUSPEND_IDLE)) to either module-bluetooth-device,
module-bluetooth-policy or the PA core.
(d) Modify module-suspend-on-idle such that sink/sources that are
created in IDLE state are automatically suspended, instead of waiting
for the timer. Could this be achieved using
PA_CORE_HOOK_{SOURCE|SINK}_PUT? Any side effect?
>From what you're saying about returning an error in setup_transport(),
I think you're proposing alternative (a). This sounds fine to me given
the need to have a stable release asap, but in a longer term I think
it's a pity not to include the feature, specially because of the
battery drain that can be avoided if the user is not listening to any
audio.
So for this longer term, what about option (c) and move the
resume-if-busy policy to the core? I mean, isn't this part of the
definition of PA_SUSPEND_IDLE?
Cheers,
Mikel
