Re: [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'

[Jaroslav Kysela <perex@perex.cz>]

Dne 11. 03. 19 v 9:15 Takashi Sakamoto napsal(a):
> Hi all,
> 
> On Fri, Mar 08, 2019 at 08:54:03PM +0100, Jaroslav Kysela wrote:
>> Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
>>> On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
>>>> On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
>>>
>>>>> In expectation of ALSA PCM interface for hardware for usual device:
>>>>>  - half number of phases of SCK per phase of WC
>>>>>    = physical_width of sample
>>>>>    = bytes per sample
>>>
>>>> They are not the same thing.
>>>
>>>> Let's take SNDRV_PCM_FORMAT_S16_LE.  The in-memory layout of this format
>>>> is two 16-bit samples next to each other in a single 32-bit word.  Their
>>>> width is 16, their physical_width is 16, and bytes per sample is 2.
>>>
>>>> A CPU DAI can do one of two things:
>>>
>>>> 1) it can generate exactly 16 SCK clock cycles per sample before WS
>>>>    changes state, leading to a total of 32 SCK clock cycles per
>>>>    frame.
>>>
>>>> 2) it can generate more than 16 SCK clock cycles per sample.
>>>
>>>> Both are entirely legal and permissable under the I2S specification.
>>>> Both look the same in memory.
>>>
>>>> The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify
>>>> which of these two is used on the wire - it only specifies the in-
>>>> memory format.
>>>
>>> Everything Russell is saying here is correct.  The actual
>>> ABI only affects the in memory format, userspace really shouldn't care
>>> what's going on on the wire.  However we don't have separate
>>> infrastructure for what goes on the wire and 90% of the time you can
>>> just translate the memory layout into a wire layout which works so we're
>>> conflating the two in a lot of places internally which is confusing and
>>> fragile.
>>
>> I agree. We just need a library which will:
>>
>> 1) gather the information from hardware drivers
>>   - a simple description of the bclk constrains
>> 2) create right constraints (hw_params rules) for the ALSA PCM API
>> 3) return the selected bclk when hw_params are installed
>>
>> The description of the bclk constraints from the hardware driver might
>> be min/max or a list of allowed wire format bit width * channels,
>> eventually define the wire formats (bitmask) and use them in this
>> library. I can imagine that all of those bclk contraints descriptions
>> (or any future, if there are such requirement) can be implemented in
>> this library.
>>
>> The library should not extend hw_params (new interval), but it should
>> work as a separate layer - use new structures / functions etc.
> 
> As another option I can suggest; usage of 'subformat' field of 'struct
> snd_pcm_hw_params'.
> 
> At present, ALSA PCM interface has one entry to this field:
>  - SNDRV_PCM_SUBFORMAT_STD
> 
> And no drivers use this entry.
> 
> Let's assume to add new entries to represent bclk/ws for the issued case:
>  - SNDRV_PCM_SUBFORMAT_I2S_16BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_48BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_64BCLK_PER_WS
>  - SNDRV_PCM_SUBFORMAT_I2S_128BCLK_PER_WS
> 
> Drivers can have constraints and rules for the parameter. At least, ALSA
> PCM core should have an rule (if I understand correctly):
>  - BCLK count of these subformats > physical_width * channel
> 
> One week point is it's not 'interval' type of parameter but 'mask'
> parameter, thus we can't represent min/max/integer and so on. However,
> less changes for ALSA PCM interface.
> 
> This is just an idea so it's not enough for me to consider about
> relevant implementation. Any indication is welcome.

I would not use any of the "user space" ioctl API to represent the
hardware bclk requirements. The applications should know just the DMA
memory layout.

Also, think about the multiple simultaneous paths for the audio output
in the sound controller (so one DMA from the user space to the
controller, but the controller can do multiple simultaneous outputs
using different clocks combining different wire buses or so). Yes, it's
the corner case, but it's another reason to have the bclk code totally
separated from the user space ALSA's PCM API.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.