[PATCH] ALSA: aaci: report FIFO size in frames

[PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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(-)

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++) {
--
2.42.0.419.g70bf8a5751

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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.

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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.

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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.

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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.

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ALSA: aaci: report FIFO size in frames

[Oswald Buddenhagen]

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.