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

[Tanu Kaskinen]

On Thu, 2013-08-08 at 12:44 +0530, Arun Raghavan wrote:
> 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.

I don't care much about the name, so feel free to use ALWAYS_RUNNING if
you want.

> > 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.

I think it would be odd to fail if the cause is IDLE, but silently
ignore the IDLE part if it's IDLE+FOO. I'm not saying that completely
failing IDLE+FOO would be any better alternative - both alternatives
seem bad to me, so I'd like to avoid choosing by forbidding multi-cause
suspending altogether.

> > > > 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.

Good point. I'm not against an assertion.

-- 
Tanu