[alsa-devel] [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Russell King - ARM Linux]

On Tue, Dec 08, 2015 at 03:30:22PM +0100, Moise Gergaud wrote:
> On 12/07/2015 10:51 AM, Takashi Iwai wrote:
> >On Mon, 07 Dec 2015 09:46:02 +0100,
> >Moise Gergaud wrote:
> >>
> >>On 12/03/2015 01:03 PM, Takashi Iwai wrote:
> >>>On Thu, 03 Dec 2015 12:13:04 +0100,
> >>>Russell King - ARM Linux wrote:
> >>>>
> >>>>On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
> >>>>>On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> >>>>>>Again, I'm not happy with this change.  The intention here is that we
> >>>>>>validate all arguments before we change anything.  This breaks that
> >>>>>>principle.
> >>>>>>
> >>>>>We propose this change for compressed mode (IEC61937/non linear PCM).
> >>>>>In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
> >>>>>cannot use snd_pcm_create_iec958_consumer.
> >>>>
> >>>>Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
> >>>>are allowed to modify the values you get afterwards in order to handle
> >>>>this case if you so wish to toggle this bit.
> >>>>
> >>>>However, if you are using data mode, you really should be using the AES
> >>>>bytes passed by the application through alsalib and into the kernel via
> >>>>the IEC958 controls.  There are three controls specified for this:
> >>>>
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
> >>>>changed for consumer mode.
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
> >>>>changed for professional mode.
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
> >>>>bits.
> >>>>
> >>>>When using this, it is userspace's responsibility to set the sample rate
> >>>>appropriately for the stream.
> >>>>
> >>>>The only case to use snd_pcm_create_iec958_consumer() is to generate the
> >>>>status bytes for PCM audio, where the userspace API doesn't facilitiate
> >>>>generating and/or passing the AES status bytes.
> >>>
> >>>That's true.  OTOH, in most drivers, we try to be kind not to be too
> >>>strict for this requirement and adjust the status bits accordingly.
> >>>This comes from the lessons we learned how user-space doesn't care the
> >>>things.
> >>>
> >>>So, if we want to keep that good uncle attitude, this change doesn't
> >>>look so bad to me.
> >>>
> >>shall I deliver a new version of this patch with header correction or
> >>shall I abandon this patch?
> >
> >It's up to you whether you want further convincing Russell :)
> >I'd happily merge this once when we get his ack.
> >
> >
> >thanks,
> >
> >Takashi
> >
> 
> I share Takashi's view; unfortunately, we cannot always trust user space.
> And so, for robustness reason, I think drivers shall ensure that channel
> status params are correct. For that purpose, the update helper function
> could be useful.

I don't, and now that you mention about not trusting userspace, I'm
entrenching into that position.  I have hardware where userspace must get
it correct: I need to use the IEC958 alsalib plugin, and that relies on
userspace getting it right - the kernel plays no part in generating the
channel status, it's all handled in userspace.  I do have kernel-side
channel status generation for 24-bit 4-byte PCM audio though, because
it's easy to merge in the channel status information (basically doing
what the IEC958 alsalib plugin does.)  I'd much rather that was moved
out to userspace though.

What you're suggesting creates two separate classes of driver:
* those which always overwrite the channel status information from
  userspace, which will make userspace lazy and _more_ buggy.
* those which have to use the channel status information from
  userspace.

If the second is a minority, what we end up with are bugs with channel
status information being missed, because programs will work on the
"majority" of drivers which overwrite the userspace specified channel
status.

IMHO, that's really not a sane position.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.