| Message ID | 20230405201220.2197878-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] docs: sound: kernel-api: writing-an-alsa-driver.rst: add FIXMEs | expand |
On Wed, 05 Apr 2023 22:12:20 +0200, Oswald Buddenhagen wrote: > > @@ -1646,11 +1647,11 @@ Typically, you'll have a hardware descriptor as below:: > > The “period” is a term that corresponds to a fragment in the OSS > world. The period defines the point at which a PCM interrupt is > generated. This point strongly depends on the hardware. Generally, > a smaller period size will give you more interrupts, that is, > - more controls. In the case of capture, this size defines the input > + more controls (FIXME: huh? granularity, maybe?). In the case of capture, this size defines the input Well, "controls" might be no good choice of the word, it was meant as "the opportunities returned from kernel back to user-space for sending / receiving the defined size of data". This isn't really granularity, strictly speaking, but it may be a better fit. > DMA Buffer Information > ~~~~~~~~~~~~~~~~~~~~~~ > > +// FIXME: this is outdated; dma_private is available only through dma_buffer_p! It's written so: ``dma_private`` is used for the ALSA DMA allocator. And, this field can be used still freely if you implement the full stack in the driver by itself instead of using the standard helpers. > @@ -1709,10 +1711,11 @@ Running Status > The running status can be referred via ``runtime->status``. This is > a pointer to a struct snd_pcm_mmap_status record. > For example, you can get the current > DMA hardware pointer via ``runtime->status->hw_ptr``. > > +// FIXME: DMA application pointer is not explained. A better description please ;) > @@ -2010,10 +2013,12 @@ is called by the interrupt routine. Then the PCM middle layer updates > the position and calculates the available space, and wakes up the > sleeping poll threads, etc. > > This callback is also atomic by default. > > +FIXME: this does not specifiy whether this is the pre- or post-fifo position. > + Again, a patch to add more description please. > @@ -2384,10 +2389,14 @@ fields. > > The ``name`` is the name identifier string. Since ALSA 0.9.x, the > control name is very important, because its role is classified from > its name. There are pre-defined standard control names. The details > are described in the `Control Names`_ subsection. > +// This is a questionable design, IMO. Why user-space heuristics when > +// the driver could set the roles/capabilities? This would avoid > +// problems like the Tone Control sliders (unlike the switch?!) being > +// misclassified as applying also to capture. Why this has to be discussed here and now...? It's the thing that was *defined* over two decades ago. > The ``index`` field holds the index number of this control. If there > are several different controls with the same name, they can be > distinguished by the index number. This is the case when several > codecs exist on the card. If the index is zero, you can omit the > @@ -2485,10 +2494,11 @@ a control constantly. > When the control may be updated, but currently has no effect on anything, > setting the ``INACTIVE`` flag may be appropriate. For example, PCM > controls should be inactive while no PCM device is open. > > There are ``LOCK`` and ``OWNER`` flags to change the write permissions. > +// FIXME: explain. A patch please. > Control Callbacks > ----------------- > > info callback > @@ -3355,10 +3365,11 @@ Buffer and Memory Management > ============================ > > Buffer Types > ------------ > > +// FIXME: this appears obsolete, i only found one pair of functions. Yes, snd_malloc_pages() and snd_free_pages() have been replaced with the managed buffer. > @@ -3670,10 +3681,11 @@ user (root by default), do as follows:: > entry->mode = S_IFREG | S_IRUGO | S_IWUSR; > > and set the write buffer size and the callback:: > > entry->c.text.write = my_proc_write; > + // FIXME: something's missing here? No, that's fine. Rather the line above it (mentioning the write buffer size) is outdated; there is no size to be specified. thanks, Takashi
On Thu, Apr 06, 2023 at 08:42:27AM +0200, Takashi Iwai wrote: >On Wed, 05 Apr 2023 22:12:20 +0200, Oswald Buddenhagen wrote: >> The ``name`` is the name identifier string. Since ALSA 0.9.x, the >> control name is very important, because its role is classified from >> its name. There are pre-defined standard control names. The details >> are described in the `Control Names`_ subsection. >> +// This is a questionable design, IMO. Why user-space heuristics when >> +// the driver could set the roles/capabilities? This would avoid >> +// problems like the Tone Control sliders (unlike the switch?!) being >> +// misclassified as applying also to capture. > >Why this has to be discussed here and now...? > why not? >It's the thing that was *defined* over two decades ago. > that may be so, but this doesn't explain anything. it's a somewhat surprising choice, and it does in fact sometimes cause problems. so at least it should be thoroughly explained. >> +// FIXME: explain. > >A patch please. > well, if i knew what to write there without doing deeper research first, i'd have already included it into the doc update. if you give me rough drafts (even just somewhat extensive bullet points), i can polish it for you (though i suspect that nowadays you may just dump it into chatgpt and get something reasonable out of it). regards
On Thu, 20 Apr 2023 14:47:11 +0200, Oswald Buddenhagen wrote: > > On Thu, Apr 06, 2023 at 08:42:27AM +0200, Takashi Iwai wrote: > > On Wed, 05 Apr 2023 22:12:20 +0200, Oswald Buddenhagen wrote: > >> The ``name`` is the name identifier string. Since ALSA 0.9.x, the > >> control name is very important, because its role is classified from > >> its name. There are pre-defined standard control names. The details > >> are described in the `Control Names`_ subsection. > >> +// This is a questionable design, IMO. Why user-space heuristics when > >> +// the driver could set the roles/capabilities? This would avoid > >> +// problems like the Tone Control sliders (unlike the switch?!) being > >> +// misclassified as applying also to capture. > > > > Why this has to be discussed here and now...? > > > why not? Because it is the already defined rule, and you're complaining the documentation. You are free to start a new discussion, but not it shouldn't be along with the documentation patch at all. > > It's the thing that was *defined* over two decades ago. > > > that may be so, but this doesn't explain anything. > it's a somewhat surprising choice, and it does in fact sometimes cause > problems. so at least it should be thoroughly explained. Again, you're barking at a wrong place. The whole control name ruling is explained in another document; there is another document covering control name rules. Takashi
On Thu, Apr 20, 2023 at 02:54:19PM +0200, Takashi Iwai wrote: >On Thu, 20 Apr 2023 14:47:11 +0200, Oswald Buddenhagen wrote: >> On Thu, Apr 06, 2023 at 08:42:27AM +0200, Takashi Iwai wrote: >> > On Wed, 05 Apr 2023 22:12:20 +0200, Oswald Buddenhagen wrote: >> >> The ``name`` is the name identifier string. Since ALSA 0.9.x, the >> >> control name is very important, because its role is classified from >> >> its name. >> >> +// This is a questionable design, IMO. Why user-space heuristics when >> >> +// the driver could set the roles/capabilities? This would avoid >> >> +// problems like the Tone Control sliders (unlike the switch?!) being >> >> +// misclassified as applying also to capture. >> > >> > Why this has to be discussed here and now...? >> > >> why not? > >Because it is the already defined rule, and you're complaining the >documentation. You are free to start a new discussion, but not it >shouldn't be along with the documentation patch at all. > this is a "various questions about the documentation" patch/thread. i can't think of a better place to discuss/document design choices. >> > It's the thing that was *defined* over two decades ago. >> > >> that may be so, but this doesn't explain anything. >> it's a somewhat surprising choice, and it does in fact sometimes cause >> problems. so at least it should be thoroughly explained. > >Again, you're barking at a wrong place. The whole control name ruling >is explained in another document; there is another document covering >control name rules. > there is the control-names.rst document. if you agree, i'd actually move the entire "Control Names" section into it, to avoid redundancy. but none of that explains the design choice. two questions require an answer, imo: a) why was is done this way and b) do you still consider it the right choice? regards
On Thu, 20 Apr 2023 15:44:50 +0200, Oswald Buddenhagen wrote: > > On Thu, Apr 20, 2023 at 02:54:19PM +0200, Takashi Iwai wrote: > > On Thu, 20 Apr 2023 14:47:11 +0200, Oswald Buddenhagen wrote: > >> On Thu, Apr 06, 2023 at 08:42:27AM +0200, Takashi Iwai wrote: > >> > On Wed, 05 Apr 2023 22:12:20 +0200, Oswald Buddenhagen wrote: > >> >> The ``name`` is the name identifier string. Since ALSA 0.9.x, the > >> >> control name is very important, because its role is classified from > >> >> its name. > >> >> +// This is a questionable design, IMO. Why user-space heuristics when > >> >> +// the driver could set the roles/capabilities? This would avoid > >> >> +// problems like the Tone Control sliders (unlike the switch?!) being > >> >> +// misclassified as applying also to capture. > >> > > Why this has to be discussed here and now...? > >> > why not? > > > > Because it is the already defined rule, and you're complaining the > > documentation. You are free to start a new discussion, but not it > > shouldn't be along with the documentation patch at all. > > > this is a "various questions about the documentation" patch/thread. i > can't think of a better place to discuss/document design choices. But why this has to be buried in the middle of a patch containing lots of other changes...? Better to split out and start a new thread. > > >> > It's the thing that was *defined* over two decades ago. > >> > that may be so, but this doesn't explain anything. > >> it's a somewhat surprising choice, and it does in fact sometimes cause > >> problems. so at least it should be thoroughly explained. > > > > Again, you're barking at a wrong place. The whole control name ruling > > is explained in another document; there is another document covering > > control name rules. > > > there is the control-names.rst document. > if you agree, i'd actually move the entire "Control Names" section > into it, to avoid redundancy. I don't mind too much, but holding a brief description is always nice, better than just mentioning another reference. You can refer to the other document for details, of course, though. > but none of that explains the design choice. The design choice was a looooong history, ca 25 years ago. > two questions require an answer, imo: a) why was is done this way and > b) do you still consider it the right choice? IIRC, this was a result after struggles with the structured control implementations. It became too complex, and the plain array with string representation can cover all complexity, while it still allows the grouping in user-space side. Again, the choice was done in a quarter century ago, and if you change it, you'll certainly break the whole things badly. We must keep the compatibility. Takashi
On Thu, Apr 20, 2023 at 04:27:35PM +0200, Takashi Iwai wrote: >But why this has to be buried in the middle of a patch containing lots >of other changes...? Better to split out and start a new thread. > this "patch" doesn't contain any changes, only a bunch of questions. given the expected audience, i don't think that "burying" it was detrimental. >IIRC, this was a result after struggles with the structured control >implementations. It became too complex, and the plain array with >string representation can cover all complexity, while it still allows >the grouping in user-space side. > i can see how a keyword based interface description is appealing - it keeps the kernel interface slim and flexible. but of course that comes at a steep cost in user space - i for one got completely lost and was unable to debug the bug described in the OP. maybe a middle way would have been the best option? >Again, the choice was done in a quarter century ago, and if you change >it, you'll certainly break the whole things badly. We must keep the >compatibility. > i don't intend to actually change it. but suppose we did. i suppose we'd have to add SNDRV_CTL_ELEM_ACCESS_{PLAYBACK,CAPTURE}. both could be set for unspecific and actually bidirectional controls. if neither is set, user space would fall back to the keyword based rules (and exceptions ...) - that would be backwards compatible and would enable a gradual migration. regards
On Thu, 20 Apr 2023 18:18:46 +0200, Oswald Buddenhagen wrote: > > On Thu, Apr 20, 2023 at 04:27:35PM +0200, Takashi Iwai wrote: > > But why this has to be buried in the middle of a patch containing lots > > of other changes...? Better to split out and start a new thread. > > > this "patch" doesn't contain any changes, only a bunch of questions. > given the expected audience, i don't think that "burying" it was > detrimental. Well, a patch is basically for patching -- i.e. fixing / correcting the stuff. For the basic API design topic, you could start off a thread, not as a form of a patch. > > IIRC, this was a result after struggles with the structured control > > implementations. It became too complex, and the plain array with > > string representation can cover all complexity, while it still allows > > the grouping in user-space side. > > > i can see how a keyword based interface description is appealing - it > keeps the kernel interface slim and flexible. but of course that comes > at a steep cost in user space - i for one got completely lost and was > unable to debug the bug described in the OP. > maybe a middle way would have been the best option? I don't mean that the current API is the best form, either. OTOH, it's been used for very long time, and the history tells that it's "good enough". > > Again, the choice was done in a quarter century ago, and if you change > > it, you'll certainly break the whole things badly. We must keep the > > compatibility. > > > i don't intend to actually change it. but suppose we did. > > i suppose we'd have to add SNDRV_CTL_ELEM_ACCESS_{PLAYBACK,CAPTURE}. > both could be set for unspecific and actually bidirectional > controls. if neither is set, user space would fall back to the keyword > based rules (and exceptions ...) - that would be backwards compatible > and would enable a gradual migration. The backward compatibility isn't really easy as you wrote, I'm afraid. If you run an old user-space stuff on the new kernel, you must not fill that new information bit but translate it to the string suffix instead; and that has to be done inside the kernel automagically. Takashi
On Fri, Apr 21, 2023 at 10:55:23AM +0200, Takashi Iwai wrote: >> i suppose we'd have to add SNDRV_CTL_ELEM_ACCESS_{PLAYBACK,CAPTURE}. >> both could be set for unspecific and actually bidirectional >> controls. if neither is set, user space would fall back to the keyword >> based rules (and exceptions ...) - that would be backwards compatible >> and would enable a gradual migration. > >The backward compatibility isn't really easy as you wrote, I'm >afraid. If you run an old user-space stuff on the new kernel, you >must not fill that new information bit but translate it to the string >suffix instead; and that has to be done inside the kernel >automagically. > right, i didn't mention it, but i imagined the strings to remain the same, both for backwards compat, and because they serve a "label" function regardless of semantic interpretation. of course that would make them partially redundant with the newly added bits, but that doesn't seem too bad to me. anyway, i think i have enough background info now to write a nice paragraph for the docu. thanks!
On 21. 04. 23 10:55, Takashi Iwai wrote: > On Thu, 20 Apr 2023 18:18:46 +0200, > Oswald Buddenhagen wrote: >> i suppose we'd have to add SNDRV_CTL_ELEM_ACCESS_{PLAYBACK,CAPTURE}. >> both could be set for unspecific and actually bidirectional >> controls. if neither is set, user space would fall back to the keyword >> based rules (and exceptions ...) - that would be backwards compatible >> and would enable a gradual migration. > > The backward compatibility isn't really easy as you wrote, I'm > afraid. If you run an old user-space stuff on the new kernel, you > must not fill that new information bit but translate it to the string > suffix instead; and that has to be done inside the kernel > automagically. Also, I think that playback/capture flags are too limited for the current use. The original naming scheme expected to build something more complex later, but we did not realize that. Given the fact, that the ASoC tree is completely crazy about naming, I would propose to check drivers using the Tone controls (only few is using them) and if all are for playback, the alsa-lib code can set the playback direction in the simple mixer API for them (workaround, fine-tune the specification in control-names.rst). But if someone has a power to design the API extensions, we can talk about it. Picking one minor thing without a complex view is not so ideal. The drivers can mark all associated controls for a PCM stream, for example. So the direction classification can be taken from this information. Jaroslav
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index b58f15f2dc7c..1b605867dbd0 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -82,10 +82,11 @@ core/seq/oss This contains the OSS sequencer emulation code. include directory ----------------- +// FIXME: needs update for uapi This is the place for the public header files of ALSA drivers, which are to be exported to user-space, or included by several files in different directories. Basically, the private header files should not be placed in this directory, but you may still find files there, due to historical reasons :) @@ -1646,11 +1647,11 @@ Typically, you'll have a hardware descriptor as below:: The “period” is a term that corresponds to a fragment in the OSS world. The period defines the point at which a PCM interrupt is generated. This point strongly depends on the hardware. Generally, a smaller period size will give you more interrupts, that is, - more controls. In the case of capture, this size defines the input + more controls (FIXME: huh? granularity, maybe?). In the case of capture, this size defines the input latency. On the other hand, the whole buffer size defines the output latency for the playback direction. - There is also a field ``fifo_size``. This specifies the size of the hardware FIFO, but currently it is neither used by the drivers nor @@ -1682,10 +1683,11 @@ frames as unsigned integer while ``snd_pcm_sframes_t`` is for frames as signed integer. DMA Buffer Information ~~~~~~~~~~~~~~~~~~~~~~ +// FIXME: this is outdated; dma_private is available only through dma_buffer_p! The DMA buffer is defined by the following four fields: ``dma_area``, ``dma_addr``, ``dma_bytes`` and ``dma_private``. ``dma_area`` holds the buffer pointer (the logical address). You can call :c:func:`memcpy()` from/to this pointer. Meanwhile, ``dma_addr`` holds the physical address of the buffer. This field is specified only when @@ -1709,10 +1711,11 @@ Running Status The running status can be referred via ``runtime->status``. This is a pointer to a struct snd_pcm_mmap_status record. For example, you can get the current DMA hardware pointer via ``runtime->status->hw_ptr``. +// FIXME: DMA application pointer is not explained. The DMA application pointer can be referred via ``runtime->control``, which points to a struct snd_pcm_mmap_control record. However, accessing this value directly is not recommended. Private Data @@ -2010,10 +2013,12 @@ is called by the interrupt routine. Then the PCM middle layer updates the position and calculates the available space, and wakes up the sleeping poll threads, etc. This callback is also atomic by default. +FIXME: this does not specifiy whether this is the pre- or post-fifo position. + copy_user, copy_kernel and fill_silence ops ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ These callbacks are not mandatory, and can be omitted in most cases. These callbacks are used when the hardware buffer cannot be in the @@ -2384,10 +2389,14 @@ fields. The ``name`` is the name identifier string. Since ALSA 0.9.x, the control name is very important, because its role is classified from its name. There are pre-defined standard control names. The details are described in the `Control Names`_ subsection. +// This is a questionable design, IMO. Why user-space heuristics when +// the driver could set the roles/capabilities? This would avoid +// problems like the Tone Control sliders (unlike the switch?!) being +// misclassified as applying also to capture. The ``index`` field holds the index number of this control. If there are several different controls with the same name, they can be distinguished by the index number. This is the case when several codecs exist on the card. If the index is zero, you can omit the @@ -2485,10 +2494,11 @@ a control constantly. When the control may be updated, but currently has no effect on anything, setting the ``INACTIVE`` flag may be appropriate. For example, PCM controls should be inactive while no PCM device is open. There are ``LOCK`` and ``OWNER`` flags to change the write permissions. +// FIXME: explain. Control Callbacks ----------------- info callback @@ -3355,10 +3365,11 @@ Buffer and Memory Management ============================ Buffer Types ------------ +// FIXME: this appears obsolete, i only found one pair of functions. ALSA provides several different buffer allocation functions depending on the bus and the architecture. All these have a consistent API. The allocation of physically-contiguous pages is done via the :c:func:`snd_malloc_xxx_pages()` function, where xxx is the bus type. @@ -3670,10 +3681,11 @@ user (root by default), do as follows:: entry->mode = S_IFREG | S_IRUGO | S_IWUSR; and set the write buffer size and the callback:: entry->c.text.write = my_proc_write; + // FIXME: something's missing here? In the write callback, you can use :c:func:`snd_info_get_line()` to get a text line, and :c:func:`snd_info_get_str()` to retrieve a string from the line. Some examples are found in ``core/oss/mixer_oss.c``, core/oss/and ``pcm_oss.c``.