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/03e74fb23cf5baa2e252cd1e62fa9506dec... [2] https://github.com/andr2000/linux/blob/tiwai_sound_for_next_pv_snd_upstream_... [3] https://elixir.bootlin.com/linux/latest/source/include/sound/pcm.h#L53