On 13. 01. 22 13:56, Takashi Iwai wrote:
On Thu, 13 Jan 2022 12:31:30 +0100,
Jaroslav Kysela wrote:
It may be useful to completely invalidate streaming under some
circumstances like the USB gadget detach. This case is a bit different
than the complete disconnection. The applications should be notified
that the PCM streaming is no longer available, but the recovery may be
expected.
This patch adds support for SNDRV_PCM_STATE_OPEN passed
to snd_pcm_stop() function. Only the hw_free ioctl is allowed to free
the buffers in this state for a full recovery operation or the PCM file
handle must be closed.
In the OPEN state, ioctls return EBADFD error (with the added hw_free
exception above). The applications which are not aware about this new
state transition (and recovery) will fail with an error. This operation
is expected.
Link: https://lore.kernel.org/alsa-devel/4e17c99b-1d8a-8ebe-972c-9b1299952ff8@xxxxxxxxxxx/
Cc: Pavel Hofman <pavel.hofman@xxxxxxxxxxx>
Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx>
I find the idea neat, but I'm afraid that it's a bit confusing from
the user POV as is. Namely, the state is in OPEN, but you'd have to
perform hw_free manually at first for moving forward; how can user
know it?
Thanks for the feedback.
The state ioctls are also not blocked, so the PCM state can be checked when
EBADFD is returned for the updated applications. But as noted in the comment,
it's expected that current applications will fail (like for the disconnect).
The OPEN state can be set only when hw_params fails in the current code. So
the applications can distinguish the hw_params error / invalidate stream
cases. We may also add this info (flag) to the PCM status structure.
This extension can be also implemented using a new PCM state, but in the
regard of our discussion a few months ago, it's probably not a way.
Maybe PCM core should do hw_free by itself when hw_params is
called with hw_free_queued.
Yes, it's a possible optimization, too. I had same idea when I post the patch.
But the mmap is an issue here - applications must do unmap before hw_params,
so I'm not convinced to add this auto-free to hw_params, because hw_free has
the straight semantics (munmap before). It would be probably clever to keep
those steps separated.
Also ideally, there may be a check in hw_params, if parameters (buffers) are
changed, but the implementation is not so easy. Maybe we can allow OPEN ->
PREPARE transition for this case, so the applications may just restart the
streaming in the most light way.
This extension can be also implemented using a new PCM state, but in the
regard of our discussion a few months ago, it's probably not a way.
Also, do_hw_free() will skip the actual free because it's in OPEN
state, no?
Yes, I forgot to add it. I'll sent v2 when we settle those little details.
Jaroslav
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
