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

[Takashi Iwai]

On Fri, 09 Jun 2017 05:42:31 +0200,
Vinod Koul wrote:
> 
> 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 at 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 at intel.com>
> Tested-by: Subhransu S. Prusty <subhransu.s.prusty at intel.com>

OK, thanks.

If Sakamato-san is happy with this change, I'm going to merge it for
4.13.


Takashi


> > Signed-off-by: Takashi Iwai <tiwai at 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;
> >  	if (!(area->vm_flags & VM_READ))
> >  		return -EINVAL;
> >  	size = area->vm_end - area->vm_start;
> > @@ -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;
> >  	if (!(area->vm_flags & VM_READ))
> >  		return -EINVAL;
> >  	size = area->vm_end - area->vm_start;
> > -- 
> > 2.13.0
> > 
> 
> -- 
> ~Vinod
>