All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: ALSA ML <alsa-devel@alsa-project.org>,
	Vinod Koul <vinod.koul@intel.com>,
	Sriram Periyasamy <sriramx.periyasamy@intel.com>,
	Ramesh Babu <ramesh.babu@intel.com>,
	Takashi Sakamoto <o-takashi@sakamocchi.jp>,
	Liam Girdwood <liam.r.girdwood@linux.intel.com>,
	Patches Audio <patches.audio@intel.com>,
	Mark Brown <broonie@kernel.org>,
	"Subhransu S . Prusty" <subhransu.s.prusty@intel.com>
Subject: Re: [RESEND][PATCH v4 1/3] ALSA: core: let low-level driver or userspace disable rewinds
Date: Thu, 29 Mar 2018 17:42:23 +0200	[thread overview]
Message-ID: <s5h8taakhwg.wl-tiwai@suse.de> (raw)
In-Reply-To: <1714954c-0703-d397-ae08-24f3359f6af4@linux.intel.com>

On Wed, 28 Mar 2018 23:51:46 +0200,
Pierre-Louis Bossart wrote:
> >> Chrome can be benefit immediately since rewinds are never used.
> >> Jack can benefit immediately since rewinds are never used
> >> Android HALs can benefit immediately since rewinds are never used.
> > ... if you patch all these.  That's not immediate.
> It's a single flag to add to the hw_params. It'll take time to ripple
> to distros but it's not really complex.

Yes, but you have to modify *each* of them.  That's flaky.
Sure, it's safer for not enabling it as default, but is it your goal?

> > OTOH, if you do introduce a single switch, it's immediately
> > effective, even with the old user-space without compilation.
> Humm, now you lost me.
> In your replies, you stated that the applications need to tell the
> driver - but disagreed that a hw_params flag was the right way. I am
> not religious here, as long as it remains simple enough we can look at
> other options.
> 
> I don't get however what 'single switch' are you referring to here?

Just a simple module option or a kctl, for example.
That is, if we can forget about the ability for adjustment per stream,
the operation mode can be flipped by a switch on the fly.  This makes
things a lot easier.

> In all cases, I don't see how we can enable this without rebuilding
> the apps.
> Except for ChromeOS, I don't know how a distro would enable a switch
> that would impact all apps using the ALSA API.

So here came the question how it would impact on PA.
If it doesn't work for PA, it means that essentially all traditional
Linux distros will turn it off as default.  It's only for Android and
Chrome OS, and they have own setup.  They can turn it on as default.

If anyone wants to turn it on for JACK, they can do it.  But I don't
think people would mix up JACK and PA on the same system at the very
same time.  (One can hook up as a client, but then it doesn't matter
about this hardware feature.)

> > And, now back to the question: in which situation you'll have to mix
> > both no-rewind and rewind operations on a single machine?  In theory,
> > yes, we may keep rewind working, but from the practical POV, it is
> > only PA who uses the rewind feature.  And, even if we disable rewind,
> > PA will still work as is.  (I know it because I broke it previously :)
> define 'work'. If PulseAudio cannot use the rewinds, then the system
> sounds will be heard with a delay and transitions between use cases
> will only happen when the queued audio is finished playing.

I just tried music players with PA, and disabled the rewind forcibly
(by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
couldn't hear any audible difference in its response for changing the
stream position, etc, interactively.  There was no audible latency
increase by that change.

It was the test weeks ago, so I refreshed testing now.  And the
result is same, I saw no response difference.
Any specific workload to make the clear difference in your mind?

> If an app wants to play a sound immediately due to user interaction
> requirements, rewinds are an attractive solution. I don't know how we
> could determine ahead of time what userspace will do, certainly the
> use of rewinds is on a per-card basis and likely a per-stream
> decision.
> 
> > So, what's wrong with introducing a global switch instead of changing
> > the existing API and requiring the code change in each application and
> > rebuild?
> I totally agree that pretty much all apps don't do any rewinds, and
> rewinds on capture is quite useless. But I stuck to the 'we don't
> break userspace' mantra with the idea of an opt-in flag requiring an
> explicit code change to take effect. we will break userspace if we add
> a kernel-level switch, and the net result is that no one will ever use
> this option.

Sure, I agree with the golden "no regression" rule, too.  So I thought
of some switch that is default off for the systems that may use
rewind, while turning it on as default on the known systems like
Android.  As PA is the sole user of rewind, here we won't see any
regression, in theory.

And, such a system-level switch is preferable from the system
management POV.  It's easier than a hard-coded API extension flag, and
you can toggle it at any time for debugging if a problem happens.

*IFF* the no-rewind feature is required by other multiple systems,
adding some API would make sense.  But, currently, no-rewind is needed
only for enabling some feature that is indirectly involved with the
rewind on Intel chips.  To my eyes, it's too far away from the purpose
of the PCM hw_params API.

(Again, no-irq was in a different situation; it's a flag for a mode
 that didn't exist (zero period) in the hw_params space.  But
 no-rewind is irrelevant with the hw_params configuration itself.)


thanks,

Takashi

  reply	other threads:[~2018-03-29 15:42 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-20 16:01 [RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms Sriram Periyasamy
2018-03-20 16:01 ` [RESEND][PATCH v4 1/3] ALSA: core: let low-level driver or userspace disable rewinds Sriram Periyasamy
2018-03-20 16:17   ` Takashi Iwai
2018-03-25 10:46     ` Sriram Periyasamy
2018-03-25 14:58       ` Takashi Iwai
2018-03-28 14:30         ` Pierre-Louis Bossart
2018-03-28 15:20           ` Takashi Iwai
2018-03-28 17:58             ` Pierre-Louis Bossart
2018-03-28 18:35               ` Takashi Iwai
2018-03-28 19:50                 ` Pierre-Louis Bossart
2018-03-28 21:09                   ` Takashi Iwai
2018-03-28 21:51                     ` Pierre-Louis Bossart
2018-03-29 15:42                       ` Takashi Iwai [this message]
2018-03-29 19:16                         ` Pierre-Louis Bossart
2018-03-29 20:10                           ` Takashi Iwai
2018-03-29 21:40                             ` Pierre-Louis Bossart
2018-03-20 16:01 ` [RESEND][PATCH v4 2/3] ALSA: hda: ext: add spib to stream context Sriram Periyasamy
2018-03-20 16:01 ` [RESEND][PATCH v4 3/3] ASoC: Intel: Skylake: Add support for spib mode Sriram Periyasamy
2018-03-21  1:34 ` [RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5h8taakhwg.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=liam.r.girdwood@linux.intel.com \
    --cc=o-takashi@sakamocchi.jp \
    --cc=patches.audio@intel.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ramesh.babu@intel.com \
    --cc=sriramx.periyasamy@intel.com \
    --cc=subhransu.s.prusty@intel.com \
    --cc=vinod.koul@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.