On Thu, 2013-08-08 at 09:07 +0300, Tanu Kaskinen wrote:
> On Thu, 2013-08-08 at 10:53 +0530, Arun Raghavan wrote:
> > This adds the ability to inhibit the suspending of a sink/source. Policy
> > modules may override this, but should avoid doing so if they can.
> > ---
> > src/pulse/def.h | 6 ++++++
> > src/pulsecore/sink.c | 3 +++
> > src/pulsecore/source.c | 3 +++
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/src/pulse/def.h b/src/pulse/def.h
> > index 58190cb..11f5cb2 100644
> > --- a/src/pulse/def.h
> > +++ b/src/pulse/def.h
> > @@ -795,6 +795,9 @@ typedef enum pa_sink_flags {
> >
> > PA_SINK_DEFERRED_VOLUME = 0x2000000U,
> > /**< The HW volume changes are syncronized with SW volume. */
> > +
> > + PA_SINK_ALWAYS_RUNNING = 0x4000000U,
> > + /**< The sink should not be automatically suspended. */
> > /** \endcond */
> > #endif
> >
> > @@ -914,6 +917,9 @@ typedef enum pa_source_flags {
> >
> > PA_SOURCE_DEFERRED_VOLUME = 0x2000000U,
> > /**< The HW volume changes are syncronized with SW volume. */
> > +
> > + PA_SOURCE_ALWAYS_RUNNING = 0x4000000U,
> > + /**< The source should not be automatically suspended. */
> > #endif
> > } pa_source_flags_t;
> >
> > diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> > index 00bc29a..9ebf4ce 100644
> > --- a/src/pulsecore/sink.c
> > +++ b/src/pulsecore/sink.c
> > @@ -834,6 +834,9 @@ int pa_sink_suspend(pa_sink *s, bool suspend, pa_suspend_cause_t cause) {
> > pa_assert(PA_SINK_IS_LINKED(s->state));
> > pa_assert(cause != 0);
> >
> > + if ((s->flags & PA_SINK_ALWAYS_RUNNING) && (cause == PA_SUSPEND_IDLE))
> > + return 0;
>
> Shouldn't the return value be -1?
>
> This assumes that if ALWAYS_RUNNING is set, then the IDLE cause can't
> appear together with other causes. I think that's a valid assumption,
> though, so I don't have a problem with that. Avoiding that assumption
> would require deciding what to do when one suspend cause is not allowed
> and one is, and I don't want to form an opinion about that.
My thinking was basically that this is not an error. A policy module
said "go ahead and suspend this sink, I think it's not needed", while
some other policy asserted "nope, don't ever suspend this sink". So it's
not an error but a behaviour override.
As for chained suspend causes, the intention of this patch was to only
ignore idle suspend. If something else wants to force a suspend, I think
it should be honoured. That is, the check should be filter out the IDLE
cause if cause is IDLE+<something else>.
> This also assumes that if ALWAYS_RUNNING is set, then s->suspend_cause
> can't contain IDLE, because this blocks also unsuspending with the IDLE
> cause. I think it would be better to avoid this assumption by not
> failing when unsuspending.
Or we do the thing I stated above and (David's going to hate me) add an
assert for this case. If something's modifying that suspend cause and
it's not pa_sink_suspend(), we probably want to catch it and either make
it not do that or make sure it's handling this flag correctly.
> > +
> > if (suspend) {
> > s->suspend_cause |= cause;
> > s->monitor_source->suspend_cause |= cause;
> > diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
> > index b14b9c8..e141649 100644
> > --- a/src/pulsecore/source.c
> > +++ b/src/pulsecore/source.c
> > @@ -751,6 +751,9 @@ int pa_source_suspend(pa_source *s, bool suspend, pa_suspend_cause_t cause) {
> > pa_assert(PA_SOURCE_IS_LINKED(s->state));
> > pa_assert(cause != 0);
> >
> > + if ((s->flags & PA_SINK_ALWAYS_RUNNING) && (cause == PA_SUSPEND_IDLE))
>
> Copy-paste error. s/SINK/SOURCE/.
Erf, thanks for catching that.
Cheers,
Arun
