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

[Oleksandr Andrushchenko]

On 03/06/2018 06:30 PM, Takashi Iwai wrote:
> On Tue, 06 Mar 2018 17:04:41 +0100,
> Oleksandr Andrushchenko wrote:
>>>>>> If we decide to negotiate the parameters, then it can't be done
>>>>>> at .open stage as well, as at this moment we don't know stream
>>>>>> parameters yet, e.g. we don't know the number of channels, PCM
>>>>>> format etc., so we cannot explain to the backend what we want.
>>>>>> Thus, it seems that we need to move the negotiation to .hw_params
>>>>>> callback where stream properties are known. But this leaves the
>>>>>> only option to ask the backend if it can handle the requested
>>>>>> buffer/period and other parameters or not... This is what I do now :(
>>>>> The additional parameter setup can be done via hw_constraints.  The hw
>>>>> constraint is basically a function call for each parameter change to
>>>>> narrow down the range of the given parameter.
>>>>>
>>>>> snd_pcm_hw_constraint_integer() in the above is just an example.
>>>>> The actual function to adjust values can be freely written.
>>>> Yes, this is clear, the question here mostly was not
>>>> *how* to set the constraints, but *where* to get those
>>> ... and here comes the hw constraint into the play.
>>>
>>> For each parameter change, for example, the frontend just passes the
>>> inquiry to the backend.  The basis of the hw constraint is nothing but
>>> to reduce the range of the given parameter.  It's either interval
>>> (range, used for period/buffer size or sample rate) or the list (for
>>> the format).  When any parameter is changed, ALSA PCM core invokes the
>>> corresponding hw constraint function, and the function reduces the
>>> range.  It's repeated until all parameters are set and settled down.
>>>
>>> So, for your driver, the frontend just passes the hw constraint for
>>> each of basic 5 parameters to the backend.  For example, at beginning,
>>> the hw constraint for the buffer size will pass the range (1,INTMAX).
>>> Then the backend returns the range like (1024,65536).   This already
>>> gives users the min/max buffer size information.  The similar
>>> procedure will be done for all other parameters.
>>>
>>> In addition, you can put the implicit rule like the integer periods,
>>> which makes things easier.
>>>
>> Thank you very much for such a detailed explanation.
>> Could you please give me an example of ALSA driver which
>> code I can read in order to understand how it is supposed
>> to be used, e.g. which meets the expectations we have for
>> Xen PV sound driver?
> This is the most difficult part, apparently :)
> There are lots of codes deploying the own hw constraints, but nothing
> is similar like your case.
>
> 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?

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?

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?

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?
>
> Takashi
Thank you very much for helping with this,
Oleksandr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sndif-add-explicit-back-and-front-parameter-negotiat.patch
Type: text/x-patch
Size: 10198 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20180307/f99a3683/attachment.bin>