sound: Simplify getchns()

[christos]

Summary

Remove all special handling for SIMPLEX, since we can just fetch the
channel directly.

While here:

• Get rid of a no-op getchns() call in dsp_ioctl().
• Rename getchns() to dsp_lock_chans(), and relchns() to dsp_unlock_chans().
• Simplify DSP_FIXUP_ERROR(), as we do not longer assign SD_F_PRIO* flags to the softc.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[christos]

sys/dev/sound/pcm/dsp.c

185

I know this patch deletes this section as well, but things seem to work fine in SIMPLEX mode. @dev_submerge.ch Am I missing something here?

[dev_submerge.ch]

See the inline comment. I found only one hardware driver that sets the SD_F_SIMPLEX, the snd_solo(8) which is quite "retro", and the driver is compiled in duplex mode by default. Other uses of SD_F_SIMPLEX do not require any tie-breaking I think, but please double-check.

This suggests that we can remove the tie-breaking part. About the release of the opposite direction channel, I'd expect pcm_chnalloc() or chn_reset() to fail anyway in the fixed direction simplex cases. But we may want to handle this more explicitly in dsp_open() for safety and robustness, e.g. not try to allocate a channel at all in the wrong direction.

sys/dev/sound/pcm/dsp.c

185

I know this patch deletes this section as well, but things seem to work fine in SIMPLEX mode. @dev_submerge.ch Am I missing something here?

Ok, let's do some guesswork. I think the code in question is relevant in the following situation:

• Simplex hardware, as in "supports both playback and recording, but only one at a time".
• The application opens the pcm device with both read | write flags set.

What getchns() and DSP_FIXUP_ERROR() implement is some sort of lazy tie-breaking in favor of playback or recording, given that we may not know the direction at the time of dsp_open(). Depending on further actions of the application, we may get some information on the desired direction (dsp_io_ops()) or not, but we have to decide anyway (thus the "no-op" in dsp_ioctl()).

The decision manifests in the pcm_setflags(), and prevents further opening of the device in the opposite direction. It also releases the now superfluous channel in the opposite direction that was opened before, in the code above.

In principle that's the right thing to do if we want to support simplex in this fashion. But the implementation seems incomplete to me, I don't see a way how to reset the direction when the channels are closed, and there may be conflicts due to races between different applications. Also I would expect the rdch or wrch pointer to be nullified when released.

Do you agree with this analysis?

[christos]

In D45775#1044473, @dev_submerge.ch wrote:

See the inline comment. I found only one hardware driver that sets the SD_F_SIMPLEX, the snd_solo(8) which is quite "retro", and the driver is compiled in duplex mode by default. Other uses of SD_F_SIMPLEX do not require any tie-breaking I think, but please double-check.

This suggests that we can remove the tie-breaking part. About the release of the opposite direction channel, I'd expect pcm_chnalloc() or chn_reset() to fail anyway in the fixed direction simplex cases. But we may want to handle this more explicitly in dsp_open() for safety and robustness, e.g. not try to allocate a channel at all in the wrong direction.

Doesn't the second case in DSP_FIXUP_ERROR check this already? For example, if the device supports either only playback or recording, dsp_open() will fail with ENOTSUP if we try to open it in the opposite direction. If the device supports both modes but not at the same time, then I guess we will most likely have created at least 1 (or 2 if VCHANs are enabled) channels for each direction during attach, which could be useful at some other point in time when the user wants to change direction, so is there a good reason to handle this?

[dev_submerge.ch]

In D45775#1044931, @christos wrote:

In D45775#1044473, @dev_submerge.ch wrote:

See the inline comment. I found only one hardware driver that sets the SD_F_SIMPLEX, the snd_solo(8) which is quite "retro", and the driver is compiled in duplex mode by default. Other uses of SD_F_SIMPLEX do not require any tie-breaking I think, but please double-check.

This suggests that we can remove the tie-breaking part. About the release of the opposite direction channel, I'd expect pcm_chnalloc() or chn_reset() to fail anyway in the fixed direction simplex cases. But we may want to handle this more explicitly in dsp_open() for safety and robustness, e.g. not try to allocate a channel at all in the wrong direction.

Doesn't the second case in DSP_FIXUP_ERROR check this already? For example, if the device supports either only playback or recording, dsp_open() will fail with ENOTSUP if we try to open it in the opposite direction.

The !DSP_F_DUPLEX(flags) makes this check skip the case when the application opens the pcm with FREAD | FWRITE flags, no? And that's a good thing I suppose, because generic applications will probably check the direction after opening the pcm device, not before.

If the device supports both modes but not at the same time, then I guess we will most likely have created at least 1 (or 2 if VCHANs are enabled) channels for each direction during attach, which could be useful at some other point in time when the user wants to change direction, so is there a good reason to handle this?

Most of the checks in dsp.c rely on the availability of rdch and wrch. The API offers many different ways to start a channel, and we don't have the facility to decide later on which direction is currently free to use. Therefore I'm a bit uneasy to allocate channels which may eventually turn out to be unusable, given the checks you remove in this patch. I'd rather decide upfront in dsp_open() for one direction (e.g. playback by default) if the device supports both modes but not at the same time. Even though we don't seem to get that situation with any driver currently.