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

[Takashi Iwai]

On Thu, 15 Jun 2017 04:32:28 +0200,
Takashi Sakamoto wrote:
> 
> On 2017年06月14日 23:52, Takashi Iwai wrote:
> > On Wed, 14 Jun 2017 16:34:32 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Jun 13 2017 21:03, Takashi Iwai wrote:
> >>> Thanks for the analysis.  Yes, the cost by the explicit calls is
> >>> known, and it's what was mentioned in the commit log as a further
> >>> optimization.  I bought this cost as good enough for better appl_ptr
> >>> accuracy, but you thought differently on that.
> >>>
> >>> One thing to be noted is that user-space doesn't have to call sync_ptr
> >>> at all, and even no period IRQ is triggered depending on the setup
> >>> (e.g. PA prefers it).  This is the case we want to solve.  That is,
> >>> the situation is worse than you thought -- things don't work as
> >>> expected unless we enforce the sync_ptr notification from user-space.
> >>>
> >>> Then, the question is how to enforce it.  The easiest option is to
> >>> disable status/control mmap.  That's how the patch was developed.
> >>> As an option,
> >>> 1) Selectively disable mmap by a new flag, or
> >>> 2) Selectively disable mmap by the presence of ack callback.
> >>>
> >>> And (2) seems too aggressively applied, from your opinion.
> >>>
> >>> Now you might thing another option:
> >>> 3) Add a new PCM info flag and let alsa-lib behaving differently
> >>>      according to it
> >>>
> >>> But this is no-go, since it doesn't work with the old alsa-lib.
> >>>
> >>> So, IMO, we need to go back to (1), which is my original patch, as a
> >>> start.  It affects only the driver that sets (in this case, it's SKL+
> >>> driver) and it works with the old user-space, at least.
> >>>
> >>> Then we can improve the performance in alsa-lib.  alsa-lib has some
> >>> inefficient implementation regarding the hwptr/appl_ptr update.  In
> >>> may places, it calls hwsync, then do avail_update that again calls
> >>> sync_ptr, for example.  I guess we can halves the amount of sync_ptr
> >>> calls by optimizing that.
> >>>
> >>> Since the sync_ptr is used for all non-x86 architectures (i.e.
> >>> nowadays majority of devices in the market), the optimization is a
> >>> good benefit for all.  Worth to try, in anyway.
> >>>
> >>>
> >>> And yet another obvious optimization would be to allow only the status
> >>> mmap and disallow the control mmap.  Currently, both are paired and
> >>> the control mmap can't fail if the status mmap succeeds.  But, for
> >>> the case like this, we want to suppress only the control mmap.
> >>>
> >>> Unfortunately, changing this behavior requires both alsa-lib and
> >>> kernel codes.  And keeping the behavior compatibility, we'd need to
> >>> introduce something new, e.g. an ioctl to set the compatible protocol
> >>> version from alsa-lib.  For now, alsa-lib inquires the protocol
> >>> version the kernel supports (SNDRV_PCM_IOCTL_PVERSION).  The newly
> >>> required one is the other way round, alsa-lib telling the supported
> >>> protocol version to kernel.  Then the kernel can know whether this
> >>> alsa-lib version accepts the status-only mmap, and gracefully returns
> >>> -ENXIO for the control mmap.
> >>
> >> Hm, I have no objections to the changes of both kernel/userspace, but
> >> from different reasons. Therefore I have different solution for this issue.
> >>
> >> I recognize this issue as a change of programming model for new design
> >> of devices. (Advantages for drivers for audio and music units on IEEE
> >> 1394 bus and the others is its sub-effect, a minor bonus.)
> >>
> >> Current ALSA PCM interface is designed based on an idea for data
> >> transmission; hardware is unaware of how many PCM frames are already
> >> queued or dequeued on PCM buffer in system memory. Hardware can transfer
> >> PCM frames to a part of the buffer with un-dequeued PCM frames (at
> >> capture) or from a part of the buffer without enough queued PCM frames
> >> (at playback). This design doesn't require kernel stuffs to maintain the
> >> application-side position on PCM buffer.
> >>
> >> If my understanding is correct, Intel's recent hardware can aware of
> >> the application-side pointer and maintain the data transmission,
> >> according to relative position of the application-side and the
> >> hardware-side on the PCM buffer. As Pierre-Louis said, this could
> >> satisfy Intel's convenience; e.g. reduce power comsumption. I guess
> >> that it can decrease frequency to drive hardware for the data
> >> transmission, or do the data transmission as better timing.
> >>
> >> This model of data transmission is new in this subsystem. I think it
> >> reasonable to add enough stuffs in both update kernel/userspace and
> >> update version of the interface for this design.
> >>
> >> A several years ago, no-period-wakeup programming model was introduced,
> >> and this subsystem got large changes for both kernel/user land. I
> >> believe this issue also has the similar impact. In my taste, in this
> >> case, it's better to give up to keep full backward compatibility, and
> >> renew related stuffs. 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 changes.
> >
> > Well...  I guess you're a bit overreacting to it.  There is no new
> > model in a big picture.
> 
> I suggested to the idea to change kernel/userspace interface itself,
> not to your raw idea. In short, I'm still against the idea to disable
> mmap(2) for status/control data of PCM runtime.
> 
> As I declare in my former message[1], there's an apparent change about
> programming model, independent on architecture support for cache
> coherency. As I declare in my previous message[2], the way for data
> transmission on the new hardware is not expected in current ALSA
> implementation. This should be described in new version of interface
> (2.0.14).
> 
> > The appl_ptr has been always present, and it
> > should be referred at each moment it's updated.
> 
> Actually it's not maintained in kernel space. Userspace applications
> do it, at least for operations under mmap(2) of PCM buffer. Clearly,
> current application-side position on the buffer is not cared in any
> service routine to handle hardware event.
> 
> As of 2017, we have some userspace implementation as consumers of ALSA
> PCM interface. If we add any change into the interface, we're
> responsible for notification of it via version change.
> 
> > That said, the
> > problem is purely in the kernel side implementation -- or more exactly
> > to say, it's about the kernel / user-space ABI.
> 
> Yes. We need to add changes into the way to use kernel/userspace
> interface for applications in this case.
> 
> > The required change would break the ABI the current alsa-lib expects,
> > thus we need to update and enable the new ABI, conditionally for the
> > newer alsa-lib for an optimized path.  For the older alsa-lib, we keep
> > the old ABI, a bit less optimized, but still works well enough.
> > That's all.
> 
> As I described, I have few interests in your raw idea. It's too early
> in this discussion.

Well, I still don't understand (anyone else can?) what you mean as "a
change of programming model" at all.  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.

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.

- The kernel needs to work with the existing user-space.
  It's a MUST.

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


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.


thanks,

Takashi

> [1] [alsa-devel] [PATCH 1/2] ALSA: firewire: process packets in
> 'struct snd_pcm_ops.ack' callback
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121699.html
> [2] [alsa-devel] [PATCH 1/2] ALSA: firewire: process packets in
> 'struct snd_pcm_ops.ack' callback
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121794.html
> 
> 
> Regards
> 
> Takashi Sakamoto
>