[pulseaudio-discuss] [PATCH 1/2] sink, source: Add a flag to inhibit suspend

[Arun Raghavan]

On Thu, 2013-08-08 at 09:57 +0300, Tanu Kaskinen wrote:
> On Thu, 2013-08-08 at 12:09 +0530, Arun Raghavan wrote:
> > 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.
> 
> I disagree. ALWAYS_RUNNING is set by hw adaptation code, and the hw
> adaptation shouldn't have any say in the policy. I think the
> ALWAYS_RUNNING flag describes the hardware, it says "if you suspend this
> sink needlessly, bad things will happen". The flag is not part of the
> policy configuration. ALWAYS_RUNNING does sound like a policy flag,
> though, so I think AVOID_UNNECESSARY_SUSPENDING would be a better name.

Too long and unwieldy imo.

> Some policy module asked the sink to suspend with the IDLE cause, and
> that operation was not allowed, so I think it makes sense to tell the
> policy module that its request was denied.

I guess we can signal an error. From the core's perspective, we're not
really going to see anything dealing with the error meaningfully (other
than a debug print).

> > 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>.
> 
> I think we shouldn't prepare for IDLE+<something else>, because we don't
> need to and it would bring some complications as I explained in the
> previous mail.

Is the complication the decision about what to do when there are two
causes? I think it makes sense to mask out IDLE if it's there and
continue in this case but maybe you're thinking of some case that I'm
not.

> > > 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.
> 
> A sink might want to start suspended, in which case pa_sink_suspend()
> wouldn't be called.

A sink that's started suspended would likely also start with the
knowledge of whether it can be suspended or not. If it can, then we
won't trigger this logic. If it can't, someone did something that
doesn't make sense and the assert would catch it.

-- Arun