| Message ID | 20210222153444.348390-7-anton.yakovlev@opensynergy.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/9] uapi: virtio_ids: add a sound device type ID from OASIS spec | expand |
On Mon, 22 Feb 2021 16:34:41 +0100, Anton Yakovlev wrote: > +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); > + struct virtio_pcm_substream *vss = NULL; > + > + if (vpcm) { > + switch (substream->stream) { > + case SNDRV_PCM_STREAM_PLAYBACK: > + case SNDRV_PCM_STREAM_CAPTURE: { The switch() here looks superfluous. The substream->stream must be a good value in the callback. If any, you can put WARN_ON() there, but I don't think it worth. > +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *hw_params) > +{ .... > + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); We have the allocation, but... > +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) > +{ > + return 0; ... no release at hw_free()? I know that the free is present in the allocator, but it's only for re-allocation case, I suppose. thanks, Takashi
On 25.02.2021 11:55, Takashi Iwai wrote: > On Mon, 22 Feb 2021 16:34:41 +0100, > Anton Yakovlev wrote: >> +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) >> +{ >> + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); >> + struct virtio_pcm_substream *vss = NULL; >> + >> + if (vpcm) { >> + switch (substream->stream) { >> + case SNDRV_PCM_STREAM_PLAYBACK: >> + case SNDRV_PCM_STREAM_CAPTURE: { > > The switch() here looks superfluous. The substream->stream must be a > good value in the callback. If any, you can put WARN_ON() there, but > I don't think it worth. At least it doesn't do any harm. If something really went wrong, we can check it right in the open callback, which is called the very first. >> +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *hw_params) >> +{ > .... >> + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); > > We have the allocation, but... > >> +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) >> +{ >> + return 0; > > ... no release at hw_free()? > I know that the free is present in the allocator, but it's only for > re-allocation case, I suppose. When the substream stops, sync_ptr waits until the device has completed all pending messages. This wait can be interrupted either by a signal or due to a timeout. In this case, the device can still access messages even after calling hw_free(). It can also issue an interrupt, and the interrupt handler will also try to access message structures. Therefore, freeing of already allocated messages occurs either in hw_params() or in dev->release(), since there it is 100% safe. > thanks, > > Takashi >
On Thu, 25 Feb 2021 13:14:37 +0100, Anton Yakovlev wrote: > > On 25.02.2021 11:55, Takashi Iwai wrote: > > On Mon, 22 Feb 2021 16:34:41 +0100, > > Anton Yakovlev wrote: > >> +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) > >> +{ > >> + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); > >> + struct virtio_pcm_substream *vss = NULL; > >> + > >> + if (vpcm) { > >> + switch (substream->stream) { > >> + case SNDRV_PCM_STREAM_PLAYBACK: > >> + case SNDRV_PCM_STREAM_CAPTURE: { > > > > The switch() here looks superfluous. The substream->stream must be a > > good value in the callback. If any, you can put WARN_ON() there, but > > I don't think it worth. > > At least it doesn't do any harm. It does -- it makes the readability worse, and that's a very important point. > >> +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, > >> + struct snd_pcm_hw_params *hw_params) > >> +{ > > .... > >> + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); > > > > We have the allocation, but... > > > >> +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) > >> +{ > >> + return 0; > > > > ... no release at hw_free()? > > I know that the free is present in the allocator, but it's only for > > re-allocation case, I suppose. > > When the substream stops, sync_ptr waits until the device has completed > all pending messages. This wait can be interrupted either by a signal or > due to a timeout. In this case, the device can still access messages > even after calling hw_free(). It can also issue an interrupt, and the > interrupt handler will also try to access message structures. Therefore, > freeing of already allocated messages occurs either in hw_params() or in > dev->release(), since there it is 100% safe. OK, then it's worth to document it about this object lifecycle. The buffer management of this driver is fairly unique, so otherwise it confuses readers. thanks, Takashi
On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote: > On Thu, 25 Feb 2021 13:14:37 +0100, > Anton Yakovlev wrote: > > > > On 25.02.2021 11:55, Takashi Iwai wrote: > > > On Mon, 22 Feb 2021 16:34:41 +0100, > > > Anton Yakovlev wrote: > > >> +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) > > >> +{ > > >> + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); > > >> + struct virtio_pcm_substream *vss = NULL; > > >> + > > >> + if (vpcm) { > > >> + switch (substream->stream) { > > >> + case SNDRV_PCM_STREAM_PLAYBACK: > > >> + case SNDRV_PCM_STREAM_CAPTURE: { > > > > > > The switch() here looks superfluous. The substream->stream must be a > > > good value in the callback. If any, you can put WARN_ON() there, but > > > I don't think it worth. > > > > At least it doesn't do any harm. > > It does -- it makes the readability worse, and that's a very important > point. > > > >> +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, > > >> + struct snd_pcm_hw_params *hw_params) > > >> +{ > > > .... > > >> + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); > > > > > > We have the allocation, but... > > > > > >> +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) > > >> +{ > > >> + return 0; > > > > > > ... no release at hw_free()? > > > I know that the free is present in the allocator, but it's only for > > > re-allocation case, I suppose. > > > > When the substream stops, sync_ptr waits until the device has completed > > all pending messages. This wait can be interrupted either by a signal or > > due to a timeout. In this case, the device can still access messages > > even after calling hw_free(). It can also issue an interrupt, and the > > interrupt handler will also try to access message structures. Therefore, > > freeing of already allocated messages occurs either in hw_params() or in > > dev->release(), since there it is 100% safe. > > OK, then it's worth to document it about this object lifecycle. > The buffer management of this driver is fairly unique, so otherwise it > confuses readers. > > > thanks, > > Takashi Takashi given I was in my tree for a while and I planned to merge it this merge window. I can still drop it but there are unrelated patches behind these in the tree so that's a rebase which will invalidate my testing, I'm just concerned about meeting the merge window. Would it be ok to merge this as is and then address readability stuff by patches on top? If yes please send acks! If you want to merge it yourself instead, also please say so.
On Thu, 25 Feb 2021 20:02:50 +0100, Michael S. Tsirkin wrote: > > On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote: > > On Thu, 25 Feb 2021 13:14:37 +0100, > > Anton Yakovlev wrote: > > > > > > On 25.02.2021 11:55, Takashi Iwai wrote: > > > > On Mon, 22 Feb 2021 16:34:41 +0100, > > > > Anton Yakovlev wrote: > > > >> +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) > > > >> +{ > > > >> + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); > > > >> + struct virtio_pcm_substream *vss = NULL; > > > >> + > > > >> + if (vpcm) { > > > >> + switch (substream->stream) { > > > >> + case SNDRV_PCM_STREAM_PLAYBACK: > > > >> + case SNDRV_PCM_STREAM_CAPTURE: { > > > > > > > > The switch() here looks superfluous. The substream->stream must be a > > > > good value in the callback. If any, you can put WARN_ON() there, but > > > > I don't think it worth. > > > > > > At least it doesn't do any harm. > > > > It does -- it makes the readability worse, and that's a very important > > point. > > > > > >> +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, > > > >> + struct snd_pcm_hw_params *hw_params) > > > >> +{ > > > > .... > > > >> + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); > > > > > > > > We have the allocation, but... > > > > > > > >> +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) > > > >> +{ > > > >> + return 0; > > > > > > > > ... no release at hw_free()? > > > > I know that the free is present in the allocator, but it's only for > > > > re-allocation case, I suppose. > > > > > > When the substream stops, sync_ptr waits until the device has completed > > > all pending messages. This wait can be interrupted either by a signal or > > > due to a timeout. In this case, the device can still access messages > > > even after calling hw_free(). It can also issue an interrupt, and the > > > interrupt handler will also try to access message structures. Therefore, > > > freeing of already allocated messages occurs either in hw_params() or in > > > dev->release(), since there it is 100% safe. > > > > OK, then it's worth to document it about this object lifecycle. > > The buffer management of this driver is fairly unique, so otherwise it > > confuses readers. > > > > > > thanks, > > > > Takashi > > Takashi given I was in my tree for a while and I planned to merge > it this merge window. Hmm, that's too quick, I'm afraid. I see still a few rough edges in the code. e.g. the reset work should be canceled at the driver removal, but it's missing right now. And that'll become tricky because the reset work itself unbinds the device, hence it'll get stuck if calling cancel_work_sync() at remove callback. > I can still drop it but there are > unrelated patches behind these in the tree so that's a rebase > which will invalidate my testing, I'm just concerned about > meeting the merge window. > > Would it be ok to merge this as is and then address > readability stuff by patches on top? > If yes please send acks! > If you want to merge it yourself instead, also please say so. I don't mind who take the patches, although it looks more fitting to merge through sound git tree if judging from the changes put in sound/* directory. thanks, Takashi
On 25.02.2021 21:30, Takashi Iwai wrote:> On Thu, 25 Feb 2021 20:02:50 +0100, > Michael S. Tsirkin wrote: >> >> On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote: >>> On Thu, 25 Feb 2021 13:14:37 +0100, >>> Anton Yakovlev wrote: [snip] >> Takashi given I was in my tree for a while and I planned to merge >> it this merge window. > > Hmm, that's too quick, I'm afraid. I see still a few rough edges in > the code. e.g. the reset work should be canceled at the driver > removal, but it's missing right now. And that'll become tricky > because the reset work itself unbinds the device, hence it'll get > stuck if calling cancel_work_sync() at remove callback. Yes, you made a good point here! In this case, we need some external mutex for synchronization. This is just a rough idea, but maybe something like this might work: struct reset_work { struct mutex mutex; struct work_struct work; struct virtio_snd *snd; bool resetting; }; static struct reset_work reset_works[SNDRV_CARDS]; init() // init mutexes and workers virtsnd_probe() snd_card_new(snd->card) reset_works[snd->card->number].snd = snd; virtsnd_remove() mutex_lock(reset_works[snd->card->number].mutex) reset_works[snd->card->number].snd = NULL; resetting = reset_works[snd->card->number].resetting; mutex_unlock(reset_works[snd->card->number].mutex) if (!resetting) // cancel worker reset_works[snd->card->number].work // remove device virtsnd_reset_fn(work) mutex_lock(work->mutex) if (!work->snd) // do nothing and take an exit path work->resetting = true; mutex_unlock(work->mutex) device_reprobe() work->resetting = false; interrupt_handler() schedule_work(reset_works[snd->card->number].work); What do you think?
On Thu, 25 Feb 2021 23:19:31 +0100, Anton Yakovlev wrote: > > On 25.02.2021 21:30, Takashi Iwai wrote:> On Thu, 25 Feb 2021 20:02:50 > +0100, > > Michael S. Tsirkin wrote: > >> > >> On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote: > >>> On Thu, 25 Feb 2021 13:14:37 +0100, > >>> Anton Yakovlev wrote: > > > [snip] > > > >> Takashi given I was in my tree for a while and I planned to merge > >> it this merge window. > > > > Hmm, that's too quick, I'm afraid. I see still a few rough edges in > > the code. e.g. the reset work should be canceled at the driver > > removal, but it's missing right now. And that'll become tricky > > because the reset work itself unbinds the device, hence it'll get > > stuck if calling cancel_work_sync() at remove callback. > > Yes, you made a good point here! In this case, we need some external > mutex for synchronization. This is just a rough idea, but maybe > something like this might work: > > struct reset_work { > struct mutex mutex; > struct work_struct work; > struct virtio_snd *snd; > bool resetting; > }; > > static struct reset_work reset_works[SNDRV_CARDS]; > > init() > // init mutexes and workers > > > virtsnd_probe() > snd_card_new(snd->card) > reset_works[snd->card->number].snd = snd; > > > virtsnd_remove() > mutex_lock(reset_works[snd->card->number].mutex) > reset_works[snd->card->number].snd = NULL; > resetting = reset_works[snd->card->number].resetting; > mutex_unlock(reset_works[snd->card->number].mutex) > > if (!resetting) > // cancel worker reset_works[snd->card->number].work > // remove device > > > virtsnd_reset_fn(work) > mutex_lock(work->mutex) > if (!work->snd) > // do nothing and take an exit path > work->resetting = true; > mutex_unlock(work->mutex) > > device_reprobe() > > work->resetting = false; > > > interrupt_handler() > schedule_work(reset_works[snd->card->number].work); > > > What do you think? I think it's still somehow racy. Suppose that the reset_work is already running right before entering virtsnd_remove(): it sets reset_works[].resetting flag, virtsnd_remove() skips canceling, and both reset work and virtsnd_remove() perform at the very same time. (I don't know whether this may happen, but I assume it's possible.) In that case, maybe a better check is to check current_work(), and perform cancel_work_sync() unless it's &reset_works[].work itself. Then the recursive cancel call can be avoided. After that point, the reset must be completed, and we can (again) process the rest release procedure. (But also snd object itself might have been changed again, so it needs to be re-evaluated.) One remaining concern is that the card number of the sound instance may change after reprobe. That is, we may want to another persistent object instead of accessing via an array index of sound card number. So, we might need reset_works[] associated with virtio_snd object instead. In anyway, this is damn complex. I sincerely hope that we can avoid this kind of things. Wouldn't it be better to shift the reset stuff up to the virtio core layer? Or drop the feature in the first version. Shooting itself (and revival) is a dangerous magic spell, after all. thanks, Takashi
On 26.02.2021 15:23, Takashi Iwai wrote: > On Thu, 25 Feb 2021 23:19:31 +0100, > Anton Yakovlev wrote: >> >> On 25.02.2021 21:30, Takashi Iwai wrote:> On Thu, 25 Feb 2021 20:02:50 >> +0100, >>> Michael S. Tsirkin wrote: >>>> >>>> On Thu, Feb 25, 2021 at 01:51:16PM +0100, Takashi Iwai wrote: >>>>> On Thu, 25 Feb 2021 13:14:37 +0100, >>>>> Anton Yakovlev wrote: >> >> >> [snip] >> >> >>>> Takashi given I was in my tree for a while and I planned to merge >>>> it this merge window. >>> >>> Hmm, that's too quick, I'm afraid. I see still a few rough edges in >>> the code. e.g. the reset work should be canceled at the driver >>> removal, but it's missing right now. And that'll become tricky >>> because the reset work itself unbinds the device, hence it'll get >>> stuck if calling cancel_work_sync() at remove callback. >> >> Yes, you made a good point here! In this case, we need some external >> mutex for synchronization. This is just a rough idea, but maybe >> something like this might work: >> >> struct reset_work { >> struct mutex mutex; >> struct work_struct work; >> struct virtio_snd *snd; >> bool resetting; >> }; >> >> static struct reset_work reset_works[SNDRV_CARDS]; >> >> init() >> // init mutexes and workers >> >> >> virtsnd_probe() >> snd_card_new(snd->card) >> reset_works[snd->card->number].snd = snd; >> >> >> virtsnd_remove() >> mutex_lock(reset_works[snd->card->number].mutex) >> reset_works[snd->card->number].snd = NULL; >> resetting = reset_works[snd->card->number].resetting; >> mutex_unlock(reset_works[snd->card->number].mutex) >> >> if (!resetting) >> // cancel worker reset_works[snd->card->number].work >> // remove device >> >> >> virtsnd_reset_fn(work) >> mutex_lock(work->mutex) >> if (!work->snd) >> // do nothing and take an exit path >> work->resetting = true; >> mutex_unlock(work->mutex) >> >> device_reprobe() >> >> work->resetting = false; >> >> >> interrupt_handler() >> schedule_work(reset_works[snd->card->number].work); >> >> >> What do you think? > > I think it's still somehow racy. Suppose that the reset_work is > already running right before entering virtsnd_remove(): it sets > reset_works[].resetting flag, virtsnd_remove() skips canceling, and > both reset work and virtsnd_remove() perform at the very same time. > (I don't know whether this may happen, but I assume it's possible.) > > In that case, maybe a better check is to check current_work(), and > perform cancel_work_sync() unless it's &reset_works[].work itself. > Then the recursive cancel call can be avoided. > > After that point, the reset must be completed, and we can (again) > process the rest release procedure. (But also snd object itself might > have been changed again, so it needs to be re-evaluated.) > > One remaining concern is that the card number of the sound instance > may change after reprobe. That is, we may want to another persistent > object instead of accessing via an array index of sound card number. > So, we might need reset_works[] associated with virtio_snd object > instead. > > In anyway, this is damn complex. I sincerely hope that we can avoid > this kind of things. Wouldn't it be better to shift the reset stuff > up to the virtio core layer? Or drop the feature in the first > version. Shooting itself (and revival) is a dangerous magic spell, > after all. Yes, I also got an impression, that without some assistance somewhere from the bus it will hardly be possible to find a suitable solution. Ok, then I will postpone this feature at the moment. > > thanks, > > Takashi >
On 25.02.2021 21:30, Takashi Iwai wrote:> On Thu, 25 Feb 2021 20:02:50 +0100, > Michael S. Tsirkin wrote: >> [snip] >> If you want to merge it yourself instead, also please say so. > > I don't mind who take the patches, although it looks more fitting to > merge through sound git tree if judging from the changes put in > sound/* directory. Then should I update the MAINTAINERS and add Takashi instead of Michael, or should I put both of you there?
diff --git a/sound/virtio/Makefile b/sound/virtio/Makefile index 626af3cc3ed7..34493226793f 100644 --- a/sound/virtio/Makefile +++ b/sound/virtio/Makefile @@ -6,5 +6,6 @@ virtio_snd-objs := \ virtio_card.o \ virtio_ctl_msg.o \ virtio_pcm.o \ - virtio_pcm_msg.o + virtio_pcm_msg.o \ + virtio_pcm_ops.o diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c index 3cfd3520a9c0..3605151860f2 100644 --- a/sound/virtio/virtio_pcm.c +++ b/sound/virtio/virtio_pcm.c @@ -454,6 +454,8 @@ int virtsnd_pcm_build_devs(struct virtio_snd *snd) for (kss = ks->substream; kss; kss = kss->next) vs->substreams[kss->number]->substream = kss; + + snd_pcm_set_ops(vpcm->pcm, i, &virtsnd_pcm_ops); } snd_pcm_set_managed_buffer_all(vpcm->pcm, diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h index c70e4fed9044..c48d00acee2e 100644 --- a/sound/virtio/virtio_pcm.h +++ b/sound/virtio/virtio_pcm.h @@ -35,6 +35,7 @@ struct virtio_pcm_msg; * @msg_last_enqueued: Index of the last I/O message added to the virtqueue. * @msg_count: Number of pending I/O messages in the virtqueue. * @msg_empty: Notify when msg_count is zero. + * @msg_flushing: True if the I/O queue is in flushing state. */ struct virtio_pcm_substream { struct virtio_snd *snd; @@ -56,6 +57,7 @@ struct virtio_pcm_substream { int msg_last_enqueued; unsigned int msg_count; wait_queue_head_t msg_empty; + bool msg_flushing; }; /** @@ -82,6 +84,8 @@ struct virtio_pcm { struct virtio_pcm_stream streams[SNDRV_PCM_STREAM_LAST + 1]; }; +extern const struct snd_pcm_ops virtsnd_pcm_ops; + int virtsnd_pcm_validate(struct virtio_device *vdev); int virtsnd_pcm_parse_cfg(struct virtio_snd *snd); diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c new file mode 100644 index 000000000000..07510778b555 --- /dev/null +++ b/sound/virtio/virtio_pcm_ops.c @@ -0,0 +1,469 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * virtio-snd: Virtio sound device + * Copyright (C) 2021 OpenSynergy GmbH + */ +#include <sound/pcm_params.h> + +#include "virtio_card.h" + +/* Map for converting ALSA format to VirtIO format. */ +struct virtsnd_a2v_format { + snd_pcm_format_t alsa_bit; + unsigned int vio_bit; +}; + +static const struct virtsnd_a2v_format g_a2v_format_map[] = { + { SNDRV_PCM_FORMAT_IMA_ADPCM, VIRTIO_SND_PCM_FMT_IMA_ADPCM }, + { SNDRV_PCM_FORMAT_MU_LAW, VIRTIO_SND_PCM_FMT_MU_LAW }, + { SNDRV_PCM_FORMAT_A_LAW, VIRTIO_SND_PCM_FMT_A_LAW }, + { SNDRV_PCM_FORMAT_S8, VIRTIO_SND_PCM_FMT_S8 }, + { SNDRV_PCM_FORMAT_U8, VIRTIO_SND_PCM_FMT_U8 }, + { SNDRV_PCM_FORMAT_S16_LE, VIRTIO_SND_PCM_FMT_S16 }, + { SNDRV_PCM_FORMAT_U16_LE, VIRTIO_SND_PCM_FMT_U16 }, + { SNDRV_PCM_FORMAT_S18_3LE, VIRTIO_SND_PCM_FMT_S18_3 }, + { SNDRV_PCM_FORMAT_U18_3LE, VIRTIO_SND_PCM_FMT_U18_3 }, + { SNDRV_PCM_FORMAT_S20_3LE, VIRTIO_SND_PCM_FMT_S20_3 }, + { SNDRV_PCM_FORMAT_U20_3LE, VIRTIO_SND_PCM_FMT_U20_3 }, + { SNDRV_PCM_FORMAT_S24_3LE, VIRTIO_SND_PCM_FMT_S24_3 }, + { SNDRV_PCM_FORMAT_U24_3LE, VIRTIO_SND_PCM_FMT_U24_3 }, + { SNDRV_PCM_FORMAT_S20_LE, VIRTIO_SND_PCM_FMT_S20 }, + { SNDRV_PCM_FORMAT_U20_LE, VIRTIO_SND_PCM_FMT_U20 }, + { SNDRV_PCM_FORMAT_S24_LE, VIRTIO_SND_PCM_FMT_S24 }, + { SNDRV_PCM_FORMAT_U24_LE, VIRTIO_SND_PCM_FMT_U24 }, + { SNDRV_PCM_FORMAT_S32_LE, VIRTIO_SND_PCM_FMT_S32 }, + { SNDRV_PCM_FORMAT_U32_LE, VIRTIO_SND_PCM_FMT_U32 }, + { SNDRV_PCM_FORMAT_FLOAT_LE, VIRTIO_SND_PCM_FMT_FLOAT }, + { SNDRV_PCM_FORMAT_FLOAT64_LE, VIRTIO_SND_PCM_FMT_FLOAT64 }, + { SNDRV_PCM_FORMAT_DSD_U8, VIRTIO_SND_PCM_FMT_DSD_U8 }, + { SNDRV_PCM_FORMAT_DSD_U16_LE, VIRTIO_SND_PCM_FMT_DSD_U16 }, + { SNDRV_PCM_FORMAT_DSD_U32_LE, VIRTIO_SND_PCM_FMT_DSD_U32 }, + { SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE, + VIRTIO_SND_PCM_FMT_IEC958_SUBFRAME } +}; + +/* Map for converting ALSA frame rate to VirtIO frame rate. */ +struct virtsnd_a2v_rate { + unsigned int rate; + unsigned int vio_bit; +}; + +static const struct virtsnd_a2v_rate g_a2v_rate_map[] = { + { 5512, VIRTIO_SND_PCM_RATE_5512 }, + { 8000, VIRTIO_SND_PCM_RATE_8000 }, + { 11025, VIRTIO_SND_PCM_RATE_11025 }, + { 16000, VIRTIO_SND_PCM_RATE_16000 }, + { 22050, VIRTIO_SND_PCM_RATE_22050 }, + { 32000, VIRTIO_SND_PCM_RATE_32000 }, + { 44100, VIRTIO_SND_PCM_RATE_44100 }, + { 48000, VIRTIO_SND_PCM_RATE_48000 }, + { 64000, VIRTIO_SND_PCM_RATE_64000 }, + { 88200, VIRTIO_SND_PCM_RATE_88200 }, + { 96000, VIRTIO_SND_PCM_RATE_96000 }, + { 176400, VIRTIO_SND_PCM_RATE_176400 }, + { 192000, VIRTIO_SND_PCM_RATE_192000 } +}; + +static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream); + +/** + * virtsnd_pcm_open() - Open the PCM substream. + * @substream: Kernel ALSA substream. + * + * Context: Process context. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_pcm_open(struct snd_pcm_substream *substream) +{ + struct virtio_pcm *vpcm = snd_pcm_substream_chip(substream); + struct virtio_pcm_substream *vss = NULL; + + if (vpcm) { + switch (substream->stream) { + case SNDRV_PCM_STREAM_PLAYBACK: + case SNDRV_PCM_STREAM_CAPTURE: { + struct virtio_pcm_stream *vs = + &vpcm->streams[substream->stream]; + + if (substream->number < vs->nsubstreams) + vss = vs->substreams[substream->number]; + break; + } + } + } + + if (!vss) + return -EBADFD; + + substream->runtime->hw = vss->hw; + substream->private_data = vss; + + snd_pcm_hw_constraint_integer(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + + /* + * If the substream has already been used, then the I/O queue may be in + * an invalid state. Just in case, we do a check and try to return the + * queue to its original state, if necessary. + */ + vss->msg_flushing = true; + + return virtsnd_pcm_sync_stop(substream); +} + +/** + * virtsnd_pcm_close() - Close the PCM substream. + * @substream: Kernel ALSA substream. + * + * Context: Process context. + * Return: 0. + */ +static int virtsnd_pcm_close(struct snd_pcm_substream *substream) +{ + return 0; +} + +/** + * virtsnd_pcm_hw_params() - Set the parameters of the PCM substream. + * @substream: Kernel ALSA substream. + * @hw_params: Hardware parameters (can be NULL). + * + * The function can be called both from the upper level (in this case, + * @hw_params is not NULL) or from the driver itself (in this case, @hw_params + * is NULL, and the parameter values are taken from the runtime structure). + * + * Context: Process context. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *hw_params) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); + struct virtio_device *vdev = vss->snd->vdev; + struct virtio_snd_msg *msg; + struct virtio_snd_pcm_set_params *request; + snd_pcm_format_t format; + unsigned int channels; + unsigned int rate; + unsigned int buffer_bytes; + unsigned int period_bytes; + unsigned int periods; + unsigned int i; + int vformat = -1; + int vrate = -1; + int rc; + + if (vss->msg_flushing) { + dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", + vss->sid); + return -EBADFD; + } + + /* Set hardware parameters in device */ + if (hw_params) { + format = params_format(hw_params); + channels = params_channels(hw_params); + rate = params_rate(hw_params); + buffer_bytes = params_buffer_bytes(hw_params); + period_bytes = params_period_bytes(hw_params); + periods = params_periods(hw_params); + } else { + format = runtime->format; + channels = runtime->channels; + rate = runtime->rate; + buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size); + period_bytes = frames_to_bytes(runtime, runtime->period_size); + periods = runtime->periods; + } + + for (i = 0; i < ARRAY_SIZE(g_a2v_format_map); ++i) + if (g_a2v_format_map[i].alsa_bit == format) { + vformat = g_a2v_format_map[i].vio_bit; + + break; + } + + for (i = 0; i < ARRAY_SIZE(g_a2v_rate_map); ++i) + if (g_a2v_rate_map[i].rate == rate) { + vrate = g_a2v_rate_map[i].vio_bit; + + break; + } + + if (vformat == -1 || vrate == -1) + return -EINVAL; + + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_SET_PARAMS, + GFP_KERNEL); + if (!msg) + return -ENOMEM; + + request = virtsnd_ctl_msg_request(msg); + request->buffer_bytes = cpu_to_le32(buffer_bytes); + request->period_bytes = cpu_to_le32(period_bytes); + request->channels = channels; + request->format = vformat; + request->rate = vrate; + + if (vss->features & (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) + request->features |= + cpu_to_le32(1U << VIRTIO_SND_PCM_F_MSG_POLLING); + + if (vss->features & (1U << VIRTIO_SND_PCM_F_EVT_XRUNS)) + request->features |= + cpu_to_le32(1U << VIRTIO_SND_PCM_F_EVT_XRUNS); + + rc = virtsnd_ctl_msg_send_sync(vss->snd, msg); + if (rc) + return rc; + + return virtsnd_pcm_msg_alloc(vss, periods, period_bytes); +} + +/** + * virtsnd_pcm_hw_free() - Reset the parameters of the PCM substream. + * @substream: Kernel ALSA substream. + * + * Context: Process context. + * Return: 0 + */ +static int virtsnd_pcm_hw_free(struct snd_pcm_substream *substream) +{ + return 0; +} + +/** + * virtsnd_pcm_prepare() - Prepare the PCM substream. + * @substream: Kernel ALSA substream. + * + * The function can be called both from the upper level or from the driver + * itself. + * + * Context: Process context. Takes and releases the VirtIO substream spinlock. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); + struct virtio_device *vdev = vss->snd->vdev; + struct virtio_snd_msg *msg; + unsigned long flags; + + if (vss->msg_flushing) { + dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", + vss->sid); + return -EBADFD; + } + + spin_lock_irqsave(&vss->lock, flags); + /* + * Since I/O messages are asynchronous, they can be completed + * when the runtime structure no longer exists. Since each + * completion implies incrementing the hw_ptr, we cache all the + * current values needed to compute the new hw_ptr value. + */ + vss->frame_bytes = runtime->frame_bits >> 3; + vss->period_size = runtime->period_size; + vss->buffer_size = runtime->buffer_size; + + vss->hw_ptr = 0; + vss->xfer_xrun = false; + vss->msg_last_enqueued = -1; + vss->msg_count = 0; + spin_unlock_irqrestore(&vss->lock, flags); + + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE, + GFP_KERNEL); + if (!msg) + return -ENOMEM; + + return virtsnd_ctl_msg_send_sync(vss->snd, msg); +} + +/** + * virtsnd_pcm_trigger() - Process command for the PCM substream. + * @substream: Kernel ALSA substream. + * @command: Substream command (SNDRV_PCM_TRIGGER_XXX). + * + * Context: Any context. Takes and releases the VirtIO substream spinlock. + * May take and release the tx/rx queue spinlock. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command) +{ + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); + struct virtio_snd *snd = vss->snd; + struct virtio_snd_msg *msg; + unsigned long flags; + int rc; + + switch (command) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: { + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); + + spin_lock_irqsave(&queue->lock, flags); + spin_lock(&vss->lock); + rc = virtsnd_pcm_msg_send(vss); + if (!rc) + vss->xfer_enabled = true; + spin_unlock(&vss->lock); + spin_unlock_irqrestore(&queue->lock, flags); + if (rc) + return rc; + + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START, + GFP_KERNEL); + if (!msg) { + spin_lock_irqsave(&vss->lock, flags); + vss->xfer_enabled = false; + spin_unlock_irqrestore(&vss->lock, flags); + + return -ENOMEM; + } + + return virtsnd_ctl_msg_send_sync(snd, msg); + } + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: { + spin_lock_irqsave(&vss->lock, flags); + vss->xfer_enabled = false; + spin_unlock_irqrestore(&vss->lock, flags); + + /* + * The I/O queue needs to be flushed only when the substream is + * completely stopped. + */ + if (command == SNDRV_PCM_TRIGGER_STOP) + vss->msg_flushing = true; + + /* + * The STOP command can be issued in an atomic context after + * the drain is complete. Therefore, in general, we cannot sleep + * here. + */ + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, + GFP_ATOMIC); + if (!msg) + return -ENOMEM; + + return virtsnd_ctl_msg_send_async(snd, msg); + } + default: { + return -EINVAL; + } + } +} + +/** + * virtsnd_pcm_msg_count() - Returns the number of pending I/O messages. + * @vss: VirtIO substream. + * + * Context: Any context. + * Return: Number of messages. + */ +static inline +unsigned int virtsnd_pcm_msg_count(struct virtio_pcm_substream *vss) +{ + unsigned int msg_count; + unsigned long flags; + + spin_lock_irqsave(&vss->lock, flags); + msg_count = vss->msg_count; + spin_unlock_irqrestore(&vss->lock, flags); + + return msg_count; +} + +/** + * virtsnd_pcm_sync_stop() - Synchronous PCM substream stop. + * @substream: Kernel ALSA substream. + * + * The function can be called both from the upper level or from the driver + * itself. + * + * Context: Process context. Takes and releases the VirtIO substream spinlock. + * Return: 0 on success, -errno on failure. + */ +static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream) +{ + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); + struct virtio_snd *snd = vss->snd; + struct virtio_snd_msg *msg; + unsigned int js = msecs_to_jiffies(msg_timeout_ms); + int rc; + + if (!vss->msg_flushing) + return 0; + + if (!virtsnd_pcm_msg_count(vss)) + goto on_exit; + + msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE, + GFP_KERNEL); + if (!msg) + return -ENOMEM; + + rc = virtsnd_ctl_msg_send_sync(snd, msg); + if (rc) + return rc; + + /* + * The spec states that upon receipt of the RELEASE command "the device + * MUST complete all pending I/O messages for the specified stream ID". + * Thus, we consider the absence of I/O messages in the queue as an + * indication that the substream has been released. + */ + rc = wait_event_interruptible_timeout(vss->msg_empty, + !virtsnd_pcm_msg_count(vss), + js); + if (rc <= 0) { + dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n", + vss->sid); + + return !rc ? -ETIMEDOUT : rc; + } + +on_exit: + vss->msg_flushing = false; + + return 0; +} + +/** + * virtsnd_pcm_pointer() - Get the current hardware position for the PCM + * substream. + * @substream: Kernel ALSA substream. + * + * Context: Any context. Takes and releases the VirtIO substream spinlock. + * Return: Hardware position in frames inside [0 ... buffer_size) range. + */ +static snd_pcm_uframes_t +virtsnd_pcm_pointer(struct snd_pcm_substream *substream) +{ + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); + snd_pcm_uframes_t hw_ptr = SNDRV_PCM_POS_XRUN; + unsigned long flags; + + spin_lock_irqsave(&vss->lock, flags); + if (!vss->xfer_xrun) + hw_ptr = vss->hw_ptr; + spin_unlock_irqrestore(&vss->lock, flags); + + return hw_ptr; +} + +/* PCM substream operators map. */ +const struct snd_pcm_ops virtsnd_pcm_ops = { + .open = virtsnd_pcm_open, + .close = virtsnd_pcm_close, + .ioctl = snd_pcm_lib_ioctl, + .hw_params = virtsnd_pcm_hw_params, + .hw_free = virtsnd_pcm_hw_free, + .prepare = virtsnd_pcm_prepare, + .trigger = virtsnd_pcm_trigger, + .sync_stop = virtsnd_pcm_sync_stop, + .pointer = virtsnd_pcm_pointer, +};
Introduce the operators required for the operation of substreams. Signed-off-by: Anton Yakovlev <anton.yakovlev@opensynergy.com> --- sound/virtio/Makefile | 3 +- sound/virtio/virtio_pcm.c | 2 + sound/virtio/virtio_pcm.h | 4 + sound/virtio/virtio_pcm_ops.c | 469 ++++++++++++++++++++++++++++++++++ 4 files changed, 477 insertions(+), 1 deletion(-) create mode 100644 sound/virtio/virtio_pcm_ops.c