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

[Oleksandr Andrushchenko]

On 03/11/2018 10:15 AM, Takashi Iwai wrote:
> Hi,
>
> sorry for the long latency.
Hi, no problem, thank you
>
> On Wed, 07 Mar 2018 09:49:24 +0100,
> Oleksandr Andrushchenko wrote:
>>> Suppose that we negotiate from the frontend to the backend like
>>>
>>> 	int query_hw_param(int parm, int *min_p, int *max_p);
>>>
>>> so that you can call like
>>> 	err = query_hw_param(PARM_RATE, &min_rate, &max_rate);
>>>
>>> This assumes that min_rate and max_rate were already filled by the
>>> values requested from frontend user-space.  In query_hw_parm, the
>>> backend receives this range, checks it, and fills again the actually
>>> applicable range that satisfies the given range in return.
>>>
>>> In that way, user-space will reduce the configuration space
>>> repeatedly.  And at the last step, the configurator chooses the
>>> optimal values that fit in the given configuration space.
>>>
>>> As mentioned in the previous post, in your driver at open, you'd need
>>> to add the hw constraint for each parameter.  That would be like:
>>>
>>> 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
>>> 				  hw_rule_rate, NULL, -1);
>>>
>>> and hw_rule_rate() would look like:
>>>
>>> static int hw_rule_rate(struct snd_pcm_hw_params *params,
>>> 			struct snd_pcm_hw_rule *rule)
>>> {
>>> 	struct snd_interval *p =
>>> 		hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
>>> 	int min_rate = p->min;
>>> 	int max_rate = p->max;
>>> 	struct snd_interval t;
>>> 	int err;
>>>
>>> 	err = query_hw_param(PARM_RATE, &min_rate, &max_rate);
>>> 	if (err < 0)
>>> 		return err;
>>>
>>> 	t.min = min_rate;
>>> 	t.max = max_rate;
>>> 	t.openmin = t.openmax = 0;
>>> 	t.integer = 1;
>>>
>>> 	return snd_interval_refine(p, &t);
>>> }
>>>
>>> The above is simplified not to allow the open min/max and assume only
>>> integer, which should be enough for your cases, I suppose.
>>>
>>> And the above function can be generalized like
>>>
>>> static int hw_rule_interval(struct snd_pcm_hw_params *params,
>>> 			    struct snd_pcm_hw_rule *rule)
>>> {
>>> 	struct snd_interval *p =
>>> 		hw_param_interval(params, rule->var);
>>> 	int min_val = p->min;
>>> 	int max_val = p->max;
>>> 	struct snd_interval t;
>>> 	int err;
>>>
>>> 	err = query_hw_param(alsa_parm_to_xen_parm(rule->var),
>>> 			&min_val, &max_val);
>>> 	if (err < 0)
>>> 		return err;
>>>
>>> 	t.min = min_val;
>>> 	t.max = max_val;
>>> 	t.openmin = t.openmax = 0;
>>> 	t.integer = 1;
>>>
>>> 	return snd_interval_refine(p, &t);
>>> }
>>>
>>> and registering this via
>>>
>>> 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
>>> 				  hw_rule_interval, NULL, -1);
>>>
>>> In the above NULL can be referred in the callback via rule->private,
>>> if you need some closure in the function, too.
>> Thank you so much for that detailed explanation and code sample!!!
>> This is really great to see such a comprehensive response.
>> Meanwhile, I did a yet another change to the protocol (please find
>> attached) which will be added to those two found in this patch set
>> already:
>> In order to provide explicit stream parameter negotiation between
>>      backend and frontend the following changes are introduced in the
>> protocol:
>>       - add XENSND_OP_HW_PARAM_SET request to set one of the stream
>>         parameters: frame rate, sample rate, number of channels,
>>         buffer and period sizes
>>       - add XENSND_OP_HW_PARAM_QUERY request to read a reduced
>>         configuration space for the parameter given: in the response
>>         to this request return min/max interval for the parameter
>>         given
>>       - add minimum buffer size to XenStore configuration
>>
>> With this change:
>> 1. Frontend sends XENSND_OP_HW_PARAM_SET to the backend in response
>> to user space's snd_pcm_hw_params_set_XXX calls, using XenStore entries
>> as initial configuration space (this is what returned on
>> snd_pcm_hw_params_any)
>> 2. Frontend uses snd_pcm_hw_rule_add to set the rules (for sample rate,
>> format, number of channels, buffer and period sizes) as you described
>> above: querying is done with XENSND_OP_HW_PARAM_QUERY request
>> 3. Finally, frontend issues XENSND_OP_OPEN request with all the negotiated
>> configuration values
>>
>> Questions:
>>
>> 1. For XENSND_OP_HW_PARAM_SET I will need a hook in the frontend driver
>> so I can intercept snd_pcm_hw_params_set_XXX calls - is this available
>> in ALSA?
> This is exactly the purpose of hw constraint rule you'd need to add.
> The callback function gets called at each time the corresponding
> parameter is changed (or the change is asked) by applications.
>
> The final parameter setup is done in hw_params PCM callback, but each
> fine-tuning / adjustment beforehand is done via hw constraints.
Excellent
>> 2. From backend side, if it runs as ALSA client, it is almost 1:1
>> mapping for XENSND_OP_HW_PARAM_SET/snd_pcm_hw_params_set_XXX, so I can
>> imagine
>> how to do that. But what do I do if I run the backend as PulseAudio client?
> This pretty depends on your implementation :)
> I can imagine that the backend assumes a limited configuration
> depending on the backend application, e.g. PA can't handle the too
> short period.
Ok, makes sense
>> 3. Period size rules will not allow the check you mentioned before, e.g.
>> require that buffer_size % period_size == 0). Can frontend driver assume
>> that on its own? So, I simply add the rule regardless of what backend can?
> Again it's up to your implementation of the backend side.  If the
> backend can support such configuration (periods not aligned with
> buffer size), it's fine, of course.
>
> I'd say it's safer to add this always, though.  It makes often things
> easier.
Yes, probably I will put it by default
>> 4. Do you think the attached change together with the previous one (
>> which adds sync event) makes the protocol look good? Do we need any
>> other change?
> I guess that'd be enough, but at best, give a rough version of your
> frontend driver code for checking.  It's very hard to judge without
> the actual code.
Great, I will try to model these (hopefully late this week)
and come back: maybe I won't need some of the protocol
operations at all. I will update ASAP
>
> thanks,
>
> Takashi
Thank you,
Oleksandr