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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

Hi,

On 03/12/2018 08:26 AM, Oleksandr Andrushchenko wrote:
> 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
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?

>> thanks,
>>
>> Takashi
> Thank you,
> Oleksandr

Thank you very much,
Oleksandr

[1] 
https://github.com/andr2000/linux/commit/2098a572f452d5247e538462dd1584369d3d1252
[2] 
https://github.com/andr2000/linux/commit/022163b2c39bf3c8cca099f5b34f599b824a045e
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel