| Message ID | 1475239410-16548-4-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Sep 2016 14:43:26 +0200, Subhransu S. Prusty wrote: > > From: Ramesh Babu <ramesh.babu@intel.com> > > In case of mmap, by default alsa-lib mmaps both control & status data. > > If driver subscribes for .appl_ptr_update, driver needs to get > notification whenever appl ptr changes. So with control & status mmaped, > driver won't get appl ptr notifications. > > In alsa-lib IOCTL_SYNC_PTR can be forced using sync_ptr_ioctl flag in > conf But this makes driver behavior dependent on a flag in the conf. > > This patch conditionally checks for .appl_ptr_update and returns error > when user land asks for mmaping control & status data, thus forcing user > to issue IOCTL_SYNC_PTR. > > One drawback with this approach is, if .appl_ptr_update is subscribed by > driver, the user space looses flexibility to mmap the control & status > data. Yes, and it can be seen as an obvious regression, so I'm not sure whether this condition is the best choice. OTOH, I now understand why you need it in this way -- alsa-lib does mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as a fallback. So, one solution would be to fix alsa-lib side. Or, if we fix the kernel in this way, it would work with old alsa-lib, but it has another penalty. Hmm... Takashi > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > --- > sound/core/pcm_native.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index c56d4ed..1965d83 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -3535,10 +3535,22 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) > case SNDRV_PCM_MMAP_OFFSET_STATUS: > if (pcm_file->no_compat_mmap) > return -ENXIO; > + /* > + * we want to force sync pointer, > + * if driver implements appl_ptr_update > + */ > + if (substream->ops->appl_ptr_update) > + return -ENXIO; > return snd_pcm_mmap_status(substream, file, area); > case SNDRV_PCM_MMAP_OFFSET_CONTROL: > if (pcm_file->no_compat_mmap) > return -ENXIO; > + /* > + * we want to force sync pointer, > + * if driver implements appl_ptr_update > + */ > + if (substream->ops->appl_ptr_update) > + return -ENXIO; > return snd_pcm_mmap_control(substream, file, area); > default: > return snd_pcm_mmap_data(substream, file, area); > -- > 1.9.1 >
On Fri, Sep 30, 2016 at 03:40:45PM +0200, Takashi Iwai wrote: > On Fri, 30 Sep 2016 14:43:26 +0200, > Subhransu S. Prusty wrote: > > > > From: Ramesh Babu <ramesh.babu@intel.com> > > > > In case of mmap, by default alsa-lib mmaps both control & status data. > > > > If driver subscribes for .appl_ptr_update, driver needs to get > > notification whenever appl ptr changes. So with control & status mmaped, > > driver won't get appl ptr notifications. > > > > In alsa-lib IOCTL_SYNC_PTR can be forced using sync_ptr_ioctl flag in > > conf But this makes driver behavior dependent on a flag in the conf. > > > > This patch conditionally checks for .appl_ptr_update and returns error > > when user land asks for mmaping control & status data, thus forcing user > > to issue IOCTL_SYNC_PTR. > > > > One drawback with this approach is, if .appl_ptr_update is subscribed by > > driver, the user space looses flexibility to mmap the control & status > > data. > > Yes, and it can be seen as an obvious regression, so I'm not sure > whether this condition is the best choice. > > OTOH, I now understand why you need it in this way -- alsa-lib does > mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as > a fallback. So, one solution would be to fix alsa-lib side. Or, if > we fix the kernel in this way, it would work with old alsa-lib, but it > has another penalty. Hmm... I will check if it can be fixed in alsa-lib side. Regards, Subhransu > >
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index c56d4ed..1965d83 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3535,10 +3535,22 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) case SNDRV_PCM_MMAP_OFFSET_STATUS: if (pcm_file->no_compat_mmap) return -ENXIO; + /* + * we want to force sync pointer, + * if driver implements appl_ptr_update + */ + if (substream->ops->appl_ptr_update) + return -ENXIO; return snd_pcm_mmap_status(substream, file, area); case SNDRV_PCM_MMAP_OFFSET_CONTROL: if (pcm_file->no_compat_mmap) return -ENXIO; + /* + * we want to force sync pointer, + * if driver implements appl_ptr_update + */ + if (substream->ops->appl_ptr_update) + return -ENXIO; return snd_pcm_mmap_control(substream, file, area); default: return snd_pcm_mmap_data(substream, file, area);