[alsa-devel] [Xen-devel][PATCH 0/2] sndif: add explicit back and front synchronization

[Oleksandr Andrushchenko]

On 03/13/2018 06:31 PM, Takashi Iwai wrote:
> On Tue, 13 Mar 2018 12:49:00 +0100,
> Oleksandr Andrushchenko wrote:
>> So, I tried to make a POC to stress the protocol changes and see
>> what implementation of the HW parameter negotiation would look like.
>>
>> Please find protocol changes at [1]:
>> - add XENSND_OP_HW_PARAM_QUERY request to read/update
>>     configuration space for the parameter given: request passes
>>     desired parameter interval and the response to this request
>>     returns min/max interval for the parameter to be used.
>>     Parameters supported by this request:
>>       - frame rate
>>       - sample rate
>>       - number of channels
>>       - buffer size
>>       - period size
>>   - add minimum buffer size to XenStore configuration
>>
>>  From the previous changes to the protocol which I posted earlier I see
>> that XENSND_OP_HW_PARAM_SET is not really needed - removed.
>>
>> The implementation in the PV frontend driver is at [2].
>>
>> Takashi, could you please take a look at the above if it meets your
>> expectations
>> so I can move forward?
> This looks almost good through a quick glance.
> But the mixture of SNDRV_PCM_HW_PARAM_PERIOD_SIZE and
> SNDRV_PCM_HW_PARAM_BUFFER_BYTES are likely confusing.
> The *_SIZE means in frames unit while *_BYTES means in bytes.
> You should align both PERIOD_ and BUFFER_ to the same units,
> i.e. either use SNDRV_PCM_HW_PARAM_PERIOD_BYTES and *_BUFFER_BYTES,
> or SNDRV_PCM_HW_PARAM_PERIOD_SIZE and *_BUFFER_SIZE.
You are correct, fixed this at [1]
> Also, a slightly remaining concern is the use-case where hw_params is
> called multiple times.  An application may call hw_free and hw_params
> freely, or even hw_params calls multiple times, in order to change the
> parameter.
>
> If the backend needs to resolve some dependency between parameters
> (e.g. the available period size depends on the sample rate), the
> backend has to remember the previously passed parameters.
>
> So, instead of passing a single parameter, you may extend the protocol
> always to pass the full (five) parameters, too.
>
> OTOH, this can be considered to be a minor case, and the backend
> (e.g. PA) can likely support every possible combinations, so maybe a
> simpler code may be a better solution in the end.
Yes, let's have it step by step.
If you are ok with what we have at the moment then, after I implement both
backend and frontend changes and confirm that protocol works,
I will send v3 of the series (protocol changes).

Still there some questions:
1. Do we really need min buffer value as configuration [2]? I see no way 
it can be used,
for instance at [3], we only have snd_pcm_hardware.buffer_bytes_max, but 
not min.
So, I feel I can drop that

2. Can I assume that min buffer size == period size and add such a 
constraint
in the frontend driver?

3. On backend side (ALSA), with current changes in the protocol I will 
call something like
int snd_pcm_hw_params_set_channels_minmax(snd_pcm_t *pcm, 
snd_pcm_hw_params_t *params, unsigned int *min, unsigned int *max)

instead of

int snd_pcm_hw_params_set_channels(snd_pcm_t *pcm, snd_pcm_hw_params_t 
*params, unsigned int val)

while servicing XENSND_OP_HW_PARAM_QUERY.XENSND_OP_HW_PARAM_CHANNELS. 
Does this make sense?

> thanks,
>
> Takashi
Thank you,
Oleksandr
[1] 
https://github.com/andr2000/linux/commit/03e74fb23cf5baa2e252cd1e62fa9506decbca7e
[2] 
https://github.com/andr2000/linux/blob/tiwai_sound_for_next_pv_snd_upstream_v2/include/xen/interface/io/sndif.h#L253
[3] https://elixir.bootlin.com/linux/latest/source/include/sound/pcm.h#L53