| Message ID | 20240401101339.506625-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: aaci: report FIFO size in frames | expand |
On Mon, Apr 01, 2024 at 12:13:39PM +0200, Oswald Buddenhagen wrote: > We already have frames, so don't convert them to bytes - the mid-layer > would convert them to frames again anyway. ... > - /* > - * ALSA wants the byte-size of the FIFOs. As we only support > - * 16-bit samples, this is twice the FIFO depth irrespective > - * of whether it's in compact mode or not. > - */ > - runtime->hw.fifo_size = aaci->fifo_depth * 2; > + runtime->hw.fifo_size = aaci->fifo_depth; When did ALSA change to wanting frames in "fifo_size" rather than bytes?
On Mon, Apr 01, 2024 at 11:31:50AM +0100, Russell King (Oracle) wrote: > On Mon, Apr 01, 2024 at 12:13:39PM +0200, Oswald Buddenhagen wrote: > > We already have frames, so don't convert them to bytes - the mid-layer > > would convert them to frames again anyway. > ... > > - /* > > - * ALSA wants the byte-size of the FIFOs. As we only support > > - * 16-bit samples, this is twice the FIFO depth irrespective > > - * of whether it's in compact mode or not. > > - */ > > - runtime->hw.fifo_size = aaci->fifo_depth * 2; > > + runtime->hw.fifo_size = aaci->fifo_depth; > > When did ALSA change to wanting frames in "fifo_size" rather than bytes? In fact, I think your patch is wrong. See snd_pcm_lib_ioctl_fifo_size(). runtime->hw.fifo_size is only measured in _frames_ if SNDRV_PCM_INFO_FIFO_IN_FRAMES is set. AACI doesn't set this flag, so runtime->hw.fifo_size is in bytes. So now I have to ask what caused you to generate this patch. I don't think you've actually run this driver, so presumably it's by flawed code inspection... if so, and if you've made changes similar to this to other drivers, that probably means that those changes are also wrong. Also, I have to wonder whether this patch is an April Fools joke.
On Mon, Apr 01, 2024 at 11:37:27AM +0100, Russell King (Oracle) wrote: >runtime->hw.fifo_size is only measured in _frames_ if >SNDRV_PCM_INFO_FIFO_IN_FRAMES is set. > yes, which is exactly why the other hunk in the patch sets it. >So now I have to ask what caused you to generate this patch. I don't >think you've actually run this driver, so presumably it's by [] >code inspection... > yes, it was a random find while trying to make sense of this parameter.
On Mon, Apr 01, 2024 at 12:53:18PM +0200, Oswald Buddenhagen wrote: > On Mon, Apr 01, 2024 at 11:37:27AM +0100, Russell King (Oracle) wrote: > > runtime->hw.fifo_size is only measured in _frames_ if > > SNDRV_PCM_INFO_FIFO_IN_FRAMES is set. > > > yes, which is exactly why the other hunk in the patch sets it. > > > So now I have to ask what caused you to generate this patch. I don't > > think you've actually run this driver, so presumably it's by [] > > code inspection... > > > yes, it was a random find while trying to make sense of this parameter. The driver worked when I wrote it. The fifo_size contents was correct when I wrote it. The choice for using bytes here and not setting SNDRV_PCM_INFO_FIFO_IN_FRAMES means that ALSA correctly takes the real FIFO size (which is in bytes) and correctly translates that itself into frames. I fail to see how this is any better - in fact, I think it's a lot worse because, as you've pointed out, it doesn't deal with stereo. In fact, it only supports 16-bit mono, whereas the driver supports lots more than that. I think where the confusion comes from is that fifo_depth is the depth of the hardware FIFO in units of 16-bit quantities, which is why we multiply by two to get to bytes. 16-bit quantities does not necessarily equate to ALSA frames - it can be in specific cases but not always. As far as I'm concerned, the code is correct as it stands and your patch will introduce regressions.
On Mon, Apr 01, 2024 at 12:04:51PM +0100, Russell King (Oracle) wrote: >I think where the confusion comes from is that fifo_depth is the >depth of the hardware FIFO in units of 16-bit quantities, [...] > ... irrespective of mono/stereo? well, with that explanation it makes sense. i recommend that you adjust the comment to make it more helpful/less misleading. maybe something like "We configure ALSA to expect the FIFO size in bytes. This works best for us, because [...]". the patch is retracted.
On Mon, Apr 01, 2024 at 01:45:27PM +0200, Oswald Buddenhagen wrote: > On Mon, Apr 01, 2024 at 12:04:51PM +0100, Russell King (Oracle) wrote: > > I think where the confusion comes from is that fifo_depth is the > > depth of the hardware FIFO in units of 16-bit quantities, [...] > > > ... irrespective of mono/stereo? > well, with that explanation it makes sense. > i recommend that you adjust the comment to make it more helpful/less > misleading. maybe something like "We configure ALSA to expect the FIFO > size in bytes. This works best for us, because [...]". Oh FFS. So you generated a patch based on the contents of a mere comment? That is NOT how kernel development should be done. Do not do this. Comments are not always correct. I guess you don't even have the hardware, which makes your approach to "kernel development" even more absurd. NAK to this patch. NAK to *all* your patches whether I've seen them or not if this is how you behave.
On Mon, Apr 01, 2024 at 02:34:20PM +0100, Russell King (Oracle) wrote: >Oh FFS. So you generated a patch based on the contents of a mere >comment? That is NOT how kernel development should be done. Do not do >this. > i think that speculative/rfc patches are a perfectly fine way to get things clarified, and the linux kernel is no exception to that. >Comments are not always correct. > so how about taking the opportunity to fix this one? >NAK to *all* your patches whether I've seen them or >not if this is how you behave. > it was a pleasure to work with you.
On Mon, Apr 01, 2024 at 04:17:53PM +0200, Oswald Buddenhagen wrote: > On Mon, Apr 01, 2024 at 02:34:20PM +0100, Russell King (Oracle) wrote: > > Oh FFS. So you generated a patch based on the contents of a mere > > comment? That is NOT how kernel development should be done. Do not do > > this. > > > i think that speculative/rfc patches are a perfectly fine way to get > things clarified, and the linux kernel is no exception to that. This wasn't a "speculative/rfc" patch. Such patches get marked with "RFC" in the tag. > > Comments are not always correct. > > > so how about taking the opportunity to fix this one? I don't think this comment is incorrect. "ALSA wants the byte-size of the FIFOs." That is a fact when the flag you refer to is not set. "As we only support 16-bit samples" That is also a fact - the driver doesn't support anything but 16-bit samples. "this is twice the FIFO depth irrespective of whether it's in compact mode or not." The only ambiguity there is what "compact" mode is, and one can find that out by reading the documentation referenced at the top of the file which is a public document. Why should the comment go into all the nitty gritty that is described in the _public_ document, like "the FIFO is shared between all channels" and "the FIFO has a fixed width". Maybe it should also state that compact mode is reading 32-bits per transfer and thus takes up two FIFO entries. Maybe it should describe that each 32-bit transfer contains two samples. Maybe it should describe the bit order of those samples. Maybe it should describe what a computer is as well? At some point, knowledge has to be assumed. I assume that, because the public document is referenced at the top of the file, anyone who wishes to patch this driver should refer to the public documentation to get an understanding of the hardware first.
On Mon, Apr 01, 2024 at 09:59:10PM +0100, Russell King (Oracle) wrote: >On Mon, Apr 01, 2024 at 04:17:53PM +0200, Oswald Buddenhagen wrote: >> i think that speculative/rfc patches are a perfectly fine way to get >> things clarified, and the linux kernel is no exception to that. > >This wasn't a "speculative/rfc" patch. Such patches get marked with >"RFC" in the tag. > putting an obvious disclaimer/question section after a three-dash line is a perfectly sufficient way to mark such a patch. at least if the receiver is actually interested in cooperation rather than harping on formalities. >> > Comments are not always correct. >> > >> so how about taking the opportunity to fix this one? > >I don't think this comment is incorrect. > >"ALSA wants the byte-size of the FIFOs." > >That is a fact when the flag you refer to is not set. > yet the formulation also suggests that this is something that just is, rather than something that the code has control over. >[...] >At some point, knowledge has to be assumed. > the problem is the omission of specific information that is in this context at least as pertinent as the information that _was_ supplied. the code is also somewhat special in that it implements an interface, which makes it more likely to be "visited" by outsiders than some implementation details. it makes sense to take that into account in related comments.
On Tue, Apr 02, 2024 at 01:01:24AM +0200, Oswald Buddenhagen wrote: > On Mon, Apr 01, 2024 at 09:59:10PM +0100, Russell King (Oracle) wrote: > > On Mon, Apr 01, 2024 at 04:17:53PM +0200, Oswald Buddenhagen wrote: > > > i think that speculative/rfc patches are a perfectly fine way to get > > > things clarified, and the linux kernel is no exception to that. > > > > This wasn't a "speculative/rfc" patch. Such patches get marked with > > "RFC" in the tag. > > > putting an obvious disclaimer/question section after a three-dash line > is a perfectly sufficient way to mark such a patch. at least if the > receiver is actually interested in cooperation rather than harping on > formalities. Convention is it goes in the subject line, so patch automation such as patchwork can identify the patches that aren't to be applied. It's not just a formality as you suggest. > > > > Comments are not always correct. > > > > > > > so how about taking the opportunity to fix this one? > > > > I don't think this comment is incorrect. > > > > "ALSA wants the byte-size of the FIFOs." > > > > That is a fact when the flag you refer to is not set. > > > yet the formulation also suggests that this is something that just is, > rather than something that the code has control over. Eh what are you going on about. The fact that SNDRV_PCM_INFO_FIFO_IN_FRAMES is not set means fifo_size is in bytes. That's a fact. This flag was introduced in commit 8bea869c5e56 ("ALSA: PCM midlevel: improve fifo_size handling") which was part of v2.6.31-rc1. The driver you are modifying was introduced in v2.6.13-rc1 *before* this flag was available, and thus from a time when fifo_size was _only_ _ever_ specifyable in bytes. Maybe the author of the patch introducing the ability to provide fifo_size in frames should have gone through every driver checking to see whether there were any comments to be updated? > > [...] > > At some point, knowledge has to be assumed. > > > the problem is the omission of specific information that is in this > context at least as pertinent as the information that _was_ supplied. > > the code is also somewhat special in that it implements an interface, > which makes it more likely to be "visited" by outsiders than some > implementation details. it makes sense to take that into account in > related comments. The code is not "somewhat special". The code does what it does and has done so since before specifying fifo_size in frames was possible. I think it is your understanding of ALSA that is the problem, and you've decided that passing fifo_size as bytes is now "somewhat special". It isn't. I am still left wondering if this is some perverse April Fool's joke on your part, designed to provoke a negative reaction. You clearly don't believe in doing any research. You don't follow the process described in submitting-patches.rst. You just create broken patches and send them in a form where they could well be picked up and merged into mainline causing breakage. This makes you dangerous, which is why I'm calling you out for it.
On Tue, Apr 02, 2024 at 12:25:57AM +0100, Russell King (Oracle) wrote: >On Tue, Apr 02, 2024 at 01:01:24AM +0200, Oswald Buddenhagen wrote: >> putting an obvious disclaimer/question section after a three-dash >> line >> is a perfectly sufficient way to mark such a patch. > >Convention is it goes in the subject line, so patch automation such as >patchwork can identify the patches that aren't to be applied. > that's a good point, but things aren't quite as black-and-white. while i didn't _expect_ the patch to be correct, it seemed possible. >The driver you are modifying was introduced >in v2.6.13-rc1 *before* this flag was available, and thus from a time >when fifo_size was _only_ _ever_ specifyable in bytes. > well, that's nice to know, but totally irrelevant. you're clearly more interested in proving that you didn't do anything wrong more than a decade ago, rather than judging whether there is room for improvement *now*. there is no shame in acknowledging that things aren't perfect, and then just moving on, because it isn't important enough. >You clearly don't believe in doing any research. > or maybe i just didn't want to spend hours on investigating something mildly suspicious i coincidentally stumbled upon when someone in the know could make a call in seconds. if you truly believe that this is an unacceptable approach, then you apparently think that your time is worth hundreds of times more than mine. you should reflect upon that attitude. >You just create broken patches and send them in a form where they could >well be picked up and merged into mainline causing breakage. > you seem to have a remarkably low opinion of the people and processes involved in safeguarding that this doesn't happen. which is kinda funny, because it includes yourself.
diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c index c3340b8ff3da..3655e88f3fca 100644 --- a/sound/arm/aaci.c +++ b/sound/arm/aaci.c @@ -350,6 +350,7 @@ static const struct snd_pcm_hardware aaci_hw_info = { SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | + SNDRV_PCM_INFO_FIFO_IN_FRAMES | SNDRV_PCM_INFO_RESUME, /* @@ -430,12 +431,7 @@ static int aaci_pcm_open(struct snd_pcm_substream *substream) snd_ac97_pcm_double_rate_rules(runtime); } - /* - * ALSA wants the byte-size of the FIFOs. As we only support - * 16-bit samples, this is twice the FIFO depth irrespective - * of whether it's in compact mode or not. - */ - runtime->hw.fifo_size = aaci->fifo_depth * 2; + runtime->hw.fifo_size = aaci->fifo_depth; mutex_lock(&aaci->irq_lock); if (!aaci->users++) {
We already have frames, so don't convert them to bytes - the mid-layer would convert them to frames again anyway. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- Is this actually right? What about stereo? cf. 5d350cba486de34eff99d0394d8fb436af54522e Cc: Russell King <rmk@arm.linux.org.uk> --- sound/arm/aaci.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) -- 2.42.0.419.g70bf8a5751