On Wed, Jun 07, 2017 at 11:20:03PM +0200, Takashi Iwai wrote:
On Wed, 07 Jun 2017 07:59:20 +0200, Takashi Iwai wrote:
On Wed, 07 Jun 2017 02:38:05 +0200, Takashi Sakamoto wrote:
In recent commit for ALSA PCM core, some arrangement is done for 'struct snd_pcm_ops.ack' callback. This is called when appl_ptr is explicitly moved in intermediate buffer for PCM frames, except for some cases described later.
For drivers in ALSA firewire stack, usage of this callback has a merit to reduce latency between time of PCM frame queueing and handling actual packets in recent isochronous cycle, because no need to wait for software IRQ context from isochronous context of OHCI 1394.
If this works well in a case that mapped page frame is used for the intermediate buffer, user process should execute some commands for ioctl(2) to tell the number of handled PCM frames in the intermediate buffer just after handling them.
This is one thing that was raised in the discussion with Intel people, and my suggestion was to add a new flag to suppress the status/control mmap like pcm_file->no_compat_mmap. Then alsa-lib falls back to the sync_ptr ioctl, and the driver can catch each appl_ptr update.
Now I considered this again, and concluded that a simple patch like below should suffice.
Adding Intel people to Cc, who raised the issue originally.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: pcm: Suppress status/control mmap when ack ops is present
The drivers using PCM ack ops require the notification whenever appl_ptr is updated in general. But when the PCM status/control page is mmapped, this notification doesn't happen, per design, thus it's not guaranteed to receive the fine-grained updates.
For improving the situation, this patch simply suppresses the PCM status/control mmap when ack ops is defined. At least, for all existing drivers with ack, this should give more benefit.
Once when we really need the full optimization with status/control mmap even using ack ops, we may reconsider the check, e.g. introducing a new flag. But, so far, this should be good enough.
Yes this makes sense and we tested it for us, looks good
Reveiwed-by: Vinod Koul vinod.koul@intel.com Tested-by: Subhransu S. Prusty subhransu.s.prusty@intel.com
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_native.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2bde07a4a87f..b993b0420411 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3189,6 +3189,10 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file struct vm_area_struct *area) { long size;
- /* suppress status/control mmap when driver requires ack */
- if (substream->ops->ack)
return -ENXIO;@@ -3225,6 +3229,10 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file struct vm_area_struct *area) { long size;
- /* suppress status/control mmap when driver requires ack */
- if (substream->ops->ack)
return -ENXIO;-- 2.13.0