On Tue, Apr 11, 2023 at 04:48:58PM +0200, Jaroslav Kysela wrote:
You're using a hammer to fix a little issue.
yes, but at the time it seemed like a rather small hammer to me.
if large buffers are actually a thing (what for?), then the fill could be limited to two periods or something. it would make the code uglier, though.
Which code does not fill the last period?
a lot, i imagine - doing that is rather counter-intuitive when using the write() access method.
also, just the last period is not enough, due to the fifo, and possibly delayed/lost irqs.
the silencing is controlled using sw_params, so applications may request the silencing before drain.
yeah, they could, but they don't, and most won't ever.
you're arguing for not doing a very practical and simple change that will fix a lot of user code at once, for the sake of preventing an entirely hypothetical and implausible problem. that is not a good trade-off.
I'm arguing that we should not do anything extra with the buffers until the application requests that.
That's the clear API context.
no, it's not. you cannot assume this to be understood as the central guiding principle which trumps more immediate issues. people use an api to solve a specific problem, and they want to do that with the least effort possible. no-one is going to read the whole docu top to bottom and remember every caveat. if it appears to work, people will just call it a day, and that's exactly what will be the case with the use of DRAIN (one needs a somewhat specific configuration and content to even notice that there is a problem).
If we allow modification of the PCM buffer, I think that we should:
- Do not modify the buffer for drivers already working with the appl_ptr data (end position) only.
i suppose that should be detected by the drain callback being set up?
- Handle the situation with the large buffer; it may make sense to change the "wait" operation from the end-of-period interrupt to time scheduler and stop the drain more early when the end-of-valid data condition is fulfilled.
i don't understand what you're asking for.
- Increase the protocol version.
But as I wrote, I would make those extensions configurable (SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
i have no clue what would be involved in doing that. to me that sounds like overkill (solving a non-issue), and goes waaaay beyond what i expected to invest into this issue (really, i just wanted to verify that the emu10k1 fixes work, and accidentally discovered that there is a mid-layer issue that affects user space, as the pyalsaaudio lib i'm using doesn't handle it).
regards