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

[Takashi Iwai]

On Thu, 15 Jun 2017 19:56:18 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 15 2017 17:48, Takashi Iwai wrote:
> > Well, I still don't understand (anyone else can?) what you mean as "a
> > change of programming model" at all.
> 
> Here, I refer to my former message.
> 
> On Jun 13 2017 07:49, Takashi Sakamoto wrote:
> > Let me evaluate this patch in a point of overhead at kernel/userspace
> > interaction.
> >
> > When considering about shape of ALSA PCM applications, I can show it by
> > below simple pseudo code:
> >
> > ```
> > while:
> >    poll(2)
> >    calculate how many PCM frames are available.
> >    memcpy(2)
> > ```
> >
> > To satisfy requirements of some drivers, we need to find a way to take
> > userspace applications to commit the number of handled PCM frames to
> > kernel space after the memcpy(2). For this purpose, in ALSA PCM
> > interface, SNDRV_PCM_IOCTL_SYNC_PTR is available.
> >
> > ```
> > while:
> >    poll(2)
> >    calculate how many PCM frames are available.
> >    memcpy(2)
> >    ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR
> > ```
> 
> 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.

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

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.


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

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


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


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

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

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


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.

- The extension of ABI for further improvements; then we can keep mmap
  of PCM status while disabling the PCM control mmap.


thanks,

Takashi