diff mbox

[alsa-lib] pcm: fix wrong document references to PCM APIs which perform direct memory access with frame copying

Message ID 20161211113138.32468-1-o-takashi@sakamocchi.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Sakamoto Dec. 11, 2016, 11:31 a.m. UTC
In a design of ALSA PCM interface, for PCM frame transmission to/from
kernel space, applications can select from two options; direct memory access
or ioctl(2). Available options are decided depending on device capacity and
machine architecture. Applications can get available options by the first
entry of 'struct snd_pcm_hw_params.masks'.

When the mask includes 'SNDRV_PCM_ACCESS_MMAP_xxx', applications can use
direct memory access. For this use case, userspace library has two types
of PCM API. One is to expose a pointer over the memory to start
reading/writing PCM frames. Another is to copy PCM frames between the
memory and a given buffer.

Current documentation includes wrong references to these APIs to describe
their advantages/disadvantages. This confuses application developers
because the references indicate PCM APIs to execute ioctl(2) operation to
read/write PCM frames.

This commit fixes the bug.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Takashi Iwai Dec. 12, 2016, 9:36 p.m. UTC | #1
On Sun, 11 Dec 2016 12:31:38 +0100,
Takashi Sakamoto wrote:
> 
> In a design of ALSA PCM interface, for PCM frame transmission to/from
> kernel space, applications can select from two options; direct memory access
> or ioctl(2). Available options are decided depending on device capacity and
> machine architecture. Applications can get available options by the first
> entry of 'struct snd_pcm_hw_params.masks'.
> 
> When the mask includes 'SNDRV_PCM_ACCESS_MMAP_xxx', applications can use
> direct memory access. For this use case, userspace library has two types
> of PCM API. One is to expose a pointer over the memory to start
> reading/writing PCM frames. Another is to copy PCM frames between the
> memory and a given buffer.
> 
> Current documentation includes wrong references to these APIs to describe
> their advantages/disadvantages. This confuses application developers
> because the references indicate PCM APIs to execute ioctl(2) operation to
> read/write PCM frames.
> 
> This commit fixes the bug.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi
Takashi Sakamoto Dec. 12, 2016, 11:06 p.m. UTC | #2
Hi,

On Dec 13 2016 06:36, Takashi Iwai wrote:
> On Sun, 11 Dec 2016 12:31:38 +0100,
> Takashi Sakamoto wrote:
>>
>> In a design of ALSA PCM interface, for PCM frame transmission to/from
>> kernel space, applications can select from two options; direct memory access
>> or ioctl(2). Available options are decided depending on device capacity and
>> machine architecture. Applications can get available options by the first
>> entry of 'struct snd_pcm_hw_params.masks'.
>>
>> When the mask includes 'SNDRV_PCM_ACCESS_MMAP_xxx', applications can use
>> direct memory access. For this use case, userspace library has two types
>> of PCM API. One is to expose a pointer over the memory to start
>> reading/writing PCM frames. Another is to copy PCM frames between the
>> memory and a given buffer.
>>
>> Current documentation includes wrong references to these APIs to describe
>> their advantages/disadvantages. This confuses application developers
>> because the references indicate PCM APIs to execute ioctl(2) operation to
>> read/write PCM frames.
>>
>> This commit fixes the bug.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>
> Applied, thanks.

Thanks.

Additionally, I'm pleased if you apply below patches to fix some 
compiler warnings which topology features recently got. I guess Intel 
developers are under their holidays.

[alsa-devel] [PATCH][alsa-lib] topology: fix sign-compare warning 
introduced to set_link_hw_config() and tplg_add_link_object()
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115245.html

[alsa-devel] [PATCH][alsa-lib] topology: fix unused-variable warnings 
introduced to build_link()
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115246.html

[alsa-devel] [PATCH][alsa-lib] topology: fix unused-const-variable warning
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115247.html


Regards

Takashi Sakamoto
Takashi Iwai Dec. 12, 2016, 11:18 p.m. UTC | #3
On Tue, 13 Dec 2016 00:06:56 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Dec 13 2016 06:36, Takashi Iwai wrote:
> > On Sun, 11 Dec 2016 12:31:38 +0100,
> > Takashi Sakamoto wrote:
> >>
> >> In a design of ALSA PCM interface, for PCM frame transmission to/from
> >> kernel space, applications can select from two options; direct memory access
> >> or ioctl(2). Available options are decided depending on device capacity and
> >> machine architecture. Applications can get available options by the first
> >> entry of 'struct snd_pcm_hw_params.masks'.
> >>
> >> When the mask includes 'SNDRV_PCM_ACCESS_MMAP_xxx', applications can use
> >> direct memory access. For this use case, userspace library has two types
> >> of PCM API. One is to expose a pointer over the memory to start
> >> reading/writing PCM frames. Another is to copy PCM frames between the
> >> memory and a given buffer.
> >>
> >> Current documentation includes wrong references to these APIs to describe
> >> their advantages/disadvantages. This confuses application developers
> >> because the references indicate PCM APIs to execute ioctl(2) operation to
> >> read/write PCM frames.
> >>
> >> This commit fixes the bug.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >
> > Applied, thanks.
> 
> Thanks.
> 
> Additionally, I'm pleased if you apply below patches to fix some
> compiler warnings which topology features recently got. I guess Intel
> developers are under their holidays.

I leave these, as they are basically harmless.
I'll take once when I get acks from Intel people.


thanks,

Takashi


> [alsa-devel] [PATCH][alsa-lib] topology: fix sign-compare warning
> introduced to set_link_hw_config() and tplg_add_link_object()
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115245.html
> 
> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-variable warnings
> introduced to build_link()
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115246.html
> 
> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-const-variable warning
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115247.html
> 
> 
> Regards
> 
> Takashi Sakamoto
>
Takashi Iwai Dec. 12, 2016, 11:25 p.m. UTC | #4
On Tue, 13 Dec 2016 00:18:04 +0100,
Takashi Iwai wrote:
> 
> On Tue, 13 Dec 2016 00:06:56 +0100,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Dec 13 2016 06:36, Takashi Iwai wrote:
> > > On Sun, 11 Dec 2016 12:31:38 +0100,
> > > Takashi Sakamoto wrote:
> > >>
> > >> In a design of ALSA PCM interface, for PCM frame transmission to/from
> > >> kernel space, applications can select from two options; direct memory access
> > >> or ioctl(2). Available options are decided depending on device capacity and
> > >> machine architecture. Applications can get available options by the first
> > >> entry of 'struct snd_pcm_hw_params.masks'.
> > >>
> > >> When the mask includes 'SNDRV_PCM_ACCESS_MMAP_xxx', applications can use
> > >> direct memory access. For this use case, userspace library has two types
> > >> of PCM API. One is to expose a pointer over the memory to start
> > >> reading/writing PCM frames. Another is to copy PCM frames between the
> > >> memory and a given buffer.
> > >>
> > >> Current documentation includes wrong references to these APIs to describe
> > >> their advantages/disadvantages. This confuses application developers
> > >> because the references indicate PCM APIs to execute ioctl(2) operation to
> > >> read/write PCM frames.
> > >>
> > >> This commit fixes the bug.
> > >>
> > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > >
> > > Applied, thanks.
> > 
> > Thanks.
> > 
> > Additionally, I'm pleased if you apply below patches to fix some
> > compiler warnings which topology features recently got. I guess Intel
> > developers are under their holidays.
> 
> I leave these, as they are basically harmless.
> I'll take once when I get acks from Intel people.

Actually I took the second one (the removal of unused variables), as
it's a good cleanup.

But the first one, the change of the type, is in a gray zone, and I'd
like to leave such a thing.  Changing the loop variable like i to
unsigned int is rather confusing, and it may be a source of bugs if
anyone reuses i for another loop.  The warning is merely an annoyance,
and IMO, we just should suppress the compile warning like the kernel
does.

The third patch, the removal of the unused table, is postponed because
I don't know whether it's intentional or overlook unless confirmation
from Intel people.


Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> 
> > [alsa-devel] [PATCH][alsa-lib] topology: fix sign-compare warning
> > introduced to set_link_hw_config() and tplg_add_link_object()
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115245.html
> > 
> > [alsa-devel] [PATCH][alsa-lib] topology: fix unused-variable warnings
> > introduced to build_link()
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115246.html
> > 
> > [alsa-devel] [PATCH][alsa-lib] topology: fix unused-const-variable warning
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115247.html
> > 
> > 
> > Regards
> > 
> > Takashi Sakamoto
> >
Takashi Sakamoto Dec. 12, 2016, 11:42 p.m. UTC | #5
On Dec 13 2016 08:18, Takashi Iwai wrote:
>> Additionally, I'm pleased if you apply below patches to fix some
>> compiler warnings which topology features recently got. I guess Intel
>> developers are under their holidays.
>
> I leave these, as they are basically harmless.
> I'll take once when I get acks from Intel people.

Hm.

In my opinion, it's time to release next version of user space stuffs, 
because some significant patches are already applied since the latest 
release. Especially, alsa-tools should be released because of 'gcc6 
narrowing error'. It's nice to apply these for the preparation even if 
they're harmless.

On the other hand, it's mostly on the end of this year. Most developers 
can have relaxing time and it might be difficult to ask the release work 
to Jaroslav, I think.

>> [alsa-devel] [PATCH][alsa-lib] topology: fix sign-compare warning
>> introduced to set_link_hw_config() and tplg_add_link_object()
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115245.html
>>
>> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-variable warnings
>> introduced to build_link()
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115246.html
>>
>> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-const-variable warning
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115247.html


Regards

Takashi Sakamoto
Takashi Sakamoto Dec. 13, 2016, 12:03 a.m. UTC | #6
On Dec 13 2016 08:25, Takashi Iwai wrote:
> Actually I took the second one (the removal of unused variables), as
> it's a good cleanup.
>
> But the first one, the change of the type, is in a gray zone, and I'd
> like to leave such a thing.  Changing the loop variable like i to
> unsigned int is rather confusing, and it may be a source of bugs if
> anyone reuses i for another loop.  The warning is merely an annoyance,
> and IMO, we just should suppress the compile warning like the kernel
> does.
>
> The third patch, the removal of the unused table, is postponed because
> I don't know whether it's intentional or overlook unless confirmation
> from Intel people.

OK. I wait for their ack about these two patches. Thanks for your reviewing!

>>> [alsa-devel] [PATCH][alsa-lib] topology: fix sign-compare warning
>>> introduced to set_link_hw_config() and tplg_add_link_object()
>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115245.html
>>>
>>> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-variable warnings
>>> introduced to build_link()
>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115246.html
>>>
>>> [alsa-devel] [PATCH][alsa-lib] topology: fix unused-const-variable warning
>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115247.html


Gute Nacht.

Takashi Sakamoto
diff mbox

Patch

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index f2ca02b..0cf740f 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -260,8 +260,9 @@  If you like to use the compatibility functions in mmap mode, there are
 read / write routines equaling to standard read / write transfers. Using
 these functions discards the benefits of direct access to memory region.
 See the #snd_pcm_mmap_readi(),
-#snd_pcm_writei(), #snd_pcm_readn()
-and #snd_pcm_writen() functions.
+#snd_pcm_mmap_writei(), #snd_pcm_mmap_readn()
+and #snd_pcm_mmap_writen() functions. These functions use
+#snd_pcm_areas_copy() internally.
 
 \section pcm_errors Error codes