[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.