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

[Oleksandr Andrushchenko]

On 03/13/2018 08:48 PM, Takashi Iwai wrote:
> On Tue, 13 Mar 2018 18:31:55 +0100,
> Oleksandr Andrushchenko wrote:
>> 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
> Actually with the hw_param query mechanism, this setup is moot.
> You can pass a fixed value that should be enough large for all cases
> there.
ok, so only buffer max as it is already defined
>> 2. Can I assume that min buffer size == period size and add such a
>> constraint
>> in the frontend driver?
> The buffer sie == period size is a special case, i.e. periods=1, and
> this won't work most likely.  It's used only for a case like PA
> deployment without the period interrupt.  And it needs a special
> hw_params flag your driver doesn't deal with.
>
> So for the sane setup, you can safely assume min_periods=2.
Thanks, will limit min to 2 periods then
>> 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?
> Yeah, that's better, I suppose.
Excellent
>
> Takashi
Thank you very much for helping with this!!!
Oleksandr Andrushchenko