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

[Takashi Iwai]

On Sun, 18 Jun 2017 12:13:54 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 17 2017 00:45, Takashi Iwai wrote:
> > 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 :)
> 
> In this point, I cannot understand your insistence.
> 
> The page frame for status/control data of PCM substream is mapped into
> process' VMA of application with _read-only_ attributes.

No.  The basic ides is that control mmap is write (user -> kernel)
while the status mmap is read (kernel -> user).  It's why there are
two distinct mmaps.

> In a point to
> deliver the status/control data from kernel space to user space, it
> works well. On x86 platforms, this works fine exactly as the aim.
> 
> On the other hand, what we should achieve for current issue is to
> deliver information from applications to hardware. This is not
> relevant to the page frame mapping.

There was an implicit assumption by the control mmap that the hardware
buffer management doesn't need the kernel notification.  It is a
problem even for some already existing drivers.  However, usually this
is a small matter, so we've ignored it.  That is, the problem has been
always there from the beginning.  Now it becomes clearer, no longer
negligible with a large buffer without interrupts, thus it requires
a resolution.

> These two are apparently different issues. You intend to apply a
> solution for the former as a solution for the latter, however it's
> against original aim of the latter. To me this is in a gray zone to
> agree with it.
> 
> >>> 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 a sub-effect, like a bonus, from a solution for issues of cache
> coherency dependently of architecture, as I said.

It's not sub-effect.  Even x86 requires sync_ptr in the 32bit
compatible mode.  That said, the sync_ptr API is not only for
non-coherent architecture, but it's a general purpose API.


> >>> 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.
> 
> I'm a developer for drivers which use PCM buffer as intermediate buffer
> for packet buffer. I understand what you explained correctly because
> I've considered about it for recent several years.
> 
> But this is our of my concern about your patch.
> 
> >>>>> 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.
> 
> If an issue were encapsulated only in kernel land or user land,
> I would have no objection to your patch. I'm willing to review it.
> 
> However, in a current case, it relates to interaction between
> kernel/user. In my opinion, it's better to avoid changing fundamental
> meaning of features which are already exposed to one side. Therefore
> on x86/ppc/alpha architectures, userspace applications (not only
> alsa-lib API applications but also applications with any I/O library)
> can use status/control data on own VMA. Else, not. This is independent
> on peripheral devices.
> 
> My intention to continue this discussion is the above. The scope of
> issues is different; one is architecture-dependent, another is
> device-dependent, thus solution should be different. Your patch has a
> potential to puzzle developers for user land, like 'why the page frame
> is not mapped for my application even if on the same architecture? why
> it's device dependent?'
 
It's an optimized path, and the user-space *must* support the codes
with sync_ptr as fallback mandatorily.  The code path using the mmap
is rather optional, so to say.  It should be documented clearly,
indeed, for avoiding misunderstanding.


I hope the situation is clearer for you now.  If you have the
alternative fix for the issue with keeping the current ABI, please
please show up.  We need the fix, not endless discussions.


thanks,

Takashi