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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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

Attachment: 0001-sndif-add-explicit-back-and-front-parameter-negotiat.patch
Description: Text Data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel