[alsa-devel] [PATCH 1/2] ALSA: firewire: process packets in 'struct snd_pcm_ops.ack' callback

[Takashi Iwai]

On Fri, 16 Jun 2017 17:00:13 +0200,
Takashi Sakamoto wrote:
> 
> On Jun 16 2017 04:06, Takashi Iwai wrote:
> >> Some devices/drivers request applications to perform this, due to their
> >> design of hardware for data transmission.
> >
> > Yes, and it's the default behavior of all non-x86 platforms, too.
> 
> Here, you have mixture of two items; architecture difference for cache
> coherent functionality, and device feature for data transmission. These
> two items are apparently different things.
> 
> In other words, devices can be supported independently of platform
> architectures. Why should I apply the solution prepared for cache
> coherency issue to add better support for the device with new feature
> for its data transmission? On x86 platform (although including some
> exceptions such as old ATOM processors), status/control data of PCM
> substream can successfully be mapped to process' VMA of applications.
> Why should it be disable it even if works well?

Because it doesn't work well for the new feature :)

> Here, I'll show a table with chang history of relevant codes in kernel
> land, and information resources.
> 
> year | release | pcm   | relevant changes
>      | version | iface |
> ==================================================================================
> 2000 | v0.5.0  | 1.0.0 | status/control/fragments data can be mapped
> into VMA[1]
> 2003 | v0.9.0  | 2.0.5 | status/control data can be mapped independently[2]
> 2004 | v1.0.5  | 2.0.7 | SYNC_PTR command was introduced for PCM ioctl
> operation[3]
> 2004 | v1.0.7  | 2.0.7 | mmap for status/control data is allowed for
> x86/ppc/alpha[4]
> 
> In note for v1.0.5 release, Jaroslav described[5]:
> 
> ```
>  - PCM midlevel
>   - added SYNC_PTR ioctl (for problematic cache coherency archs)
> ```
> 
> The SYNC_PTR and architecture-dependent enabling of the mmap was
> originally introduced to solve cache coherent issue.
>
> >>> It's becoming again like a typical situation you falling often into
> >>> while discussing with others; your new term is too ambiguous and
> >>> unique to parse without a clarification.  PLEASE, clarify your idea
> >>> in a more understandable way, at best with a (pseudo-) code snippet.
> >>
> >> The above.
> >
> > So... what's the "new"?  That is what I don't understand...
> > It's the already existing model deployed without mmap.  Nothing new.
> 
> This subsystem has no drivers with the similar feature, thus it's new
> design for data transmission. The design supports below things:
> 
> 1. Hardware features:
> 1.1. Data transmission is done by direct media access (DMA).
> 1.2. Hardware cares of two points for its data transmission; one is
>      hwptr and another is appl_ptr on PCM buffer dedicated for the
>      data transmission.
> 1.3. (perhaps)The granurarity of data transmission can be differed,
>      not fixed to the size of 'period'.
> 
> 2. Drivers are designed with below items:
> 2.1. Return SNDRV_PCM_ACCESS_MMAP_XXX flag
> 2.2. Implement for 'struct snd_pcm_ops.ack' to tell appl_ptr to
>      hardware.
> 
> 3. Applications should perform according to below items:
> 3.1.  Operate with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] to drive kernel
>       land stuffs with changed appl_ptr.
> 3.2.  Or operate with SYNC_PTR to driver kernel land stuffs when
>       handling any PCM frames.
> 
> As of v4.12-rc5, there's no stuffs to satisfy all of these items.

It works on non-x86 systems as is with 4.12.

> It's
> clear that we're working to support devices with the new design.

Yeah, I sort of understand you're sticking with "new design".  So, no
need to argue about it.  It doesn't matter whether it's really so
shiny new or not.  It's something we need to fix, after all.


> > In other words, such a driver already works fine on non-x86
> > platforms.  It's "broken" only on x86 due to the forced mmap usage.
> 
> It's better to distinguish two issues; architecture's support for
> cache coherency and support for new type of device.

Well, that's not really true.  Most of drivers worked so far with a
luck.  Fortunately, the hardware that doesn't have the mappable
hardware buffer can be woken up well enough via period setup, thus it
could synchronize appl_ptr that user-space already changed in the
past.  This, however, doesn't mean that the current x86 mmap is
perfectly working.

Imagine that you stop the stream at the middle of period chunk.  For
the hardware with the mapped h/w buffer, it can play up to the aborted
position.  OTOH, for the hardware without mapped buffer, it can't
reach at that position because the sync of appl_ptr was missing before
the period elapsed.  If the appl_ptr could have been notified via
sync_ptr, the driver could copy the data, and it could reach to the
aborted point.

So, currently it effectively enforces the BATCH style buffer although
it doesn't have to be so.  It's a known bug by the status / control
mmap, but we've ignored this just because such hardware are minor and
the problem itself is trivial.  But you see that the current code is
even not perfect for the existing hardware.


> >>> And, I think you miss a few points, thus the argument was twisted.
> >>>
> >>> - The primary goal is to achieve the notification of appl_ptr change
> >>>     to kernel.
> >>
> >> It's for the purpose to support devices which have the new design for
> >> data transmission. This is the reason that I agree with upgrading
> >> version of PCM interface. I suggest adding new info flags for the
> >> specific purpose.
> >>
> >> Disabling mmap for status/control data of PCM substream is just to
> >> support architectures to which ALSA PCM core judge non cache coherency.
> >> It's not good to utilize it as a solution of this issue because of
> >> abuse of interfaces.
> >
> > No, it's no abuse.  The sync_ptr is the correct and designed API to
> > notify appl_ptr update from the user space to kernel.  For most of
> > device drivers on x86, this isn't requested strictly, thus it's not
> > done when the PCM control is mmapped.  We've been just lucky, so far.
> 
> I described that it was originally designed to solve architecture's
> support for cache coherency. Don't depend on extra bonus from it.

Oh well...  Please stop such a fundamentalism argument.
The original purpose doesn't mean to limit its usage.  We aren't
arguing history or religion.


> > That is, letting user-space to use the sync_ptr is the right
> > strategy from the API POV, and it can be achieved by the disablement
> > of mmap.  Unfortunately, we can't currently disable only PCM control
> > mmap, but we have to disable both control and status PCM mmaps.
> 
> Usage of SYNC_PTR ioctl command and disabling mmap are not tight-coupled.
> Both of them can be used in the same time if user land stuffs is writte
> so.

Sure, it's a semantic difference.  I don't pursue this way if we are
allowed to extend ABI, either.  This will be the "phase 2".

Remember that my suggestion is for the "phase 1", to fix the stuff for
the existing applications without changing the ABI.


> > It's a trade-off, but still good enough for the driver requesting it
> > (the Intel one, which can achieve the deep sleep by that).
> 
> What's the 'deep sleep'? Please explain about it when you introduce
> new words into this discussion.

I thought Pierre (or other Intel people) already mentioned that.  Or
maybe it's in a different patchset.  In anyway...

The chip (DSP) can prefetch the data on the buffer and go to a deep
sleep mode.  It's the reason why appl_ptr update is needed.  Then we
can know how much data can be prefetched.  This should be the great
reduction of power, thus a slight increase of instructions would be
like a peanut.


> >>> - The kernel needs to work with the existing user-space.
> >>>     It's a MUST.
> >>
> >> I described in a previous message.
> >
> > Let me repeat: the support of old user-space is a must.  This is the
> > fact.  Not faked.
> >
> >
> >> On Jun 14 2017 23:34, Takashi Sakamoto wrote:
> >>> When working with old userspace stuffs, drivers run with old mode
> >>> (namely, run for current interface). The difference of interface
> >>> version is absorbed in alsa-lib as much as possible, then application
> >>> runs without large change
> >
> > ... and how the kernel knows that a newer alsa-lib is running, BTW?
> >
> > There is no such information right now, and it's why I suggested an
> > extension of ABI.
> 
> Enough information is not provided yet. Furthermore, I have few
> interests in it at present.

Right, not yet.  That's the point.  If we want to achieve the fix
without changing the user-space, you can't rely on this
not-yet-present information.


> >>> - appl_ptr or whatever a status *is* maintained in kernel -- or better
> >>>     to say, it's kept in both kernel and user-space.  (Otherwise why it
> >>>     becomes a problem now?)
> >>
> >> This is not achieved in current implementation of ALSA PCM core yet.
> >
> > Sigh, again the misunderstanding due to the ambiguous term usage.
> >
> > The appl_ptr value is tracked in user-space and kernel sides -- so
> > they are "maintained" in both sides.  "Maintain a value" doesn't
> > define who triggers the value change.
> 
> The important thing for this issue is 'how to deliver the position of
> appl_ptr from applications to hardware'. In current implementation,
> when applications operate for mapped PCM buffer, kernel land stuffs
> don't have correct appl_ptr. Hardware is given ways to get appl_ptr
> and drivers don't assist it because appl_ptr is in process's VMA of
> applications.

Yes.  That's what I called synchronization in the below.

> >> Even if hardware generates any event to get current application-side
> >> position on PCM buffer, it can't be achieved. There's no way for
> >> hardwares. Therefore, it's unavoided to request userspace to have care
> >> of the above pseudo code, when supporting devices with the new design.
> >
> > Yes, the missing piece is the synchronization between the appl_ptr
> > values maintained in both user-space and kernel sides.  And it's only
> > on x86, and the simplest way to fix it is to disable the PCM control
> > mmap.
> 
> It's not better to depend on bonus from architecture differences.
> Provide enough information to userspace via interface.
>
> >>> So, if you have a better idea for achieving the goal without changing
> >>> the current ABI, please tell us.  It'd be really appreciated!
> >>>
> >>> For the future development with the ABI extension, we may do implement
> >>> in a different level, yes.  Basically we can change everything.  But
> >>> this is not the thing we need to fix right now.
> >>>
> >>> I'm open for any changes with the ABI extension.  It's certainly
> >>> exciting.  But, don't mix up this with the solution for the current
> >>> implementation.
> >>
> >> Your idea to extend current ABI includes some jumps from current
> >> position of discussion. At least, the reason of it is somehow based on
> >> your idea to disable mmap for control/status data of PCM substream for
> >> some devices/drivers. As already stated, I'm against it with several
> >> reasons. On cache coherent architecture, there's no matter to consider
> >> about it. Thus they're not matters to me at present.
> >
> > It's fine that you say against it.  But then please state your idea
> > how to implement the appl_ptr notification for the existing
> > application ABI?  Without it, the argument is simply useless.
> 
> I already prepared patches for my ideas several days ago. However, I
> have hesitation to post it because you have mixtures of two issues,
> which I described. As long as you adhere to the mixtures, my patches
> are not evaluated in a appropriate logic, thus I'm unwilling to post
> it. I'd not like to waste of my private time.

Sakamoto-san, if you have the idea to fix the behavior, PLEASE PLEASE
show it at first!!!  This is greatly appreciated.

I meant here a solution that works with the currently existing
application as is -- i.e. change only in the kernel side.  If this can
be achieved without disabling mmap, it's very interesting.  Please let
me know.

Once we fix this for the existing applications, then we can go
further, a better implementation with the extension of ABI.


BTW, mentioning about waste of your private time doesn't sound so
impressive, honestly speaking.  I noticed it often appearing in your
patch changelogs.  Most of people (including me) spend private time
for developing Linux kernel stuff, too.  My own main job is not
maintaining ALSA, either.


thanks,

Takashi


> > Also, think again which disadvantages would disabling the control mmap
> > (not the status mmap) have, when sync_ptr is mandatory.  Or better in
> > another form: suppose you need to use sync_ptr for appl_ptr update,
> > what merit would you have by keeping the PCM control mmap...?
> >
> > Take a look at the content of snd_pcm_mmap_control.  It's only
> > appl_ptr and avail_min where the latter can be ignored mostly.
> > That is, disabling the PCM control mmap is effectively equivalent with
> > requesting the appl_ptr update via sync_ptr.
> >
> > So, if we can disable only the PCM control mmap, the efficiency won't
> > be lost for kernel -> user-space PCM status update by keeping the PCM
> > status mmap.  It's the plan I mentioned as the ABI extension.
> >
> >
> > To recap:
> > - The disablement of control/status mmap is allowed by request from a
> >    driver to enforce the sync_ptr usage for appl_ptr update.
> 
> Not fair. The feature to disable mmap is just for issues from
> architecture feature. It's not good to utilize it for the other kind of
> issues. It surely puzzle developers in user space. When working for
> x86 architecture, developers just consider about content on mapped
> control/status data. We soulnd not change it.
> 
> > - The extension of ABI for further improvements; then we can keep mmap
> >    of PCM status while disabling the PCM control mmap.
> 
> I'm not interested in issues directly relevant to current issue. I'd
> like to just focus on the current one.
> 
> [1] ftp://ftp.alsa-project.org/pub/driver/alsa-driver-0.5.0.tar.gz
> [2] ftp://ftp.alsa-project.org/pub/driver/alsa-driver-0.9.0rc8.tar.bz2
> [3]
> http://git.alsa-project.org/?p=alsa-driver.git;a=commitdiff;h=60300f540726;hp=a1bed841e5b5b2e6c3d5b4709efbb3aa72235e72
> [4]
> http://git.alsa-project.org/?p=alsa-driver.git;a=commitdiff;h=6cd0240aa173;hp=4bc54bf2643d5563d9489ff3ce3e4dbb0d2d4a99
> [5] https://lwn.net/Articles/87517/
> 
> 
> Regards
> 
> Takashi Sakamoto
>