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.
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.
thanks,
Takashi