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
