| Message ID | 397eb20c-096a-f8d1-1e63-3662d79f14cf@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, sorry for the long latency. 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. > 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. > 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. > 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. thanks, Takashi
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
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
On Tue, 13 Mar 2018 12:49:00 +0100, Oleksandr Andrushchenko wrote: > > 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? This looks almost good through a quick glance. But the mixture of SNDRV_PCM_HW_PARAM_PERIOD_SIZE and SNDRV_PCM_HW_PARAM_BUFFER_BYTES are likely confusing. The *_SIZE means in frames unit while *_BYTES means in bytes. You should align both PERIOD_ and BUFFER_ to the same units, i.e. either use SNDRV_PCM_HW_PARAM_PERIOD_BYTES and *_BUFFER_BYTES, or SNDRV_PCM_HW_PARAM_PERIOD_SIZE and *_BUFFER_SIZE. Also, a slightly remaining concern is the use-case where hw_params is called multiple times. An application may call hw_free and hw_params freely, or even hw_params calls multiple times, in order to change the parameter. If the backend needs to resolve some dependency between parameters (e.g. the available period size depends on the sample rate), the backend has to remember the previously passed parameters. So, instead of passing a single parameter, you may extend the protocol always to pass the full (five) parameters, too. OTOH, this can be considered to be a minor case, and the backend (e.g. PA) can likely support every possible combinations, so maybe a simpler code may be a better solution in the end. thanks, Takashi
On 03/13/2018 06:31 PM, Takashi Iwai wrote: > On Tue, 13 Mar 2018 12:49:00 +0100, > Oleksandr Andrushchenko wrote: >> 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? > This looks almost good through a quick glance. > But the mixture of SNDRV_PCM_HW_PARAM_PERIOD_SIZE and > SNDRV_PCM_HW_PARAM_BUFFER_BYTES are likely confusing. > The *_SIZE means in frames unit while *_BYTES means in bytes. > You should align both PERIOD_ and BUFFER_ to the same units, > i.e. either use SNDRV_PCM_HW_PARAM_PERIOD_BYTES and *_BUFFER_BYTES, > or SNDRV_PCM_HW_PARAM_PERIOD_SIZE and *_BUFFER_SIZE. You are correct, fixed this at [1] > Also, a slightly remaining concern is the use-case where hw_params is > called multiple times. An application may call hw_free and hw_params > freely, or even hw_params calls multiple times, in order to change the > parameter. > > If the backend needs to resolve some dependency between parameters > (e.g. the available period size depends on the sample rate), the > backend has to remember the previously passed parameters. > > So, instead of passing a single parameter, you may extend the protocol > always to pass the full (five) parameters, too. > > OTOH, this can be considered to be a minor case, and the backend > (e.g. PA) can likely support every possible combinations, so maybe a > simpler code may be a better solution in the end. Yes, let's have it step by step. If you are ok with what we have at the moment then, after I implement both backend and frontend changes and confirm that protocol works, I will send v3 of the series (protocol changes). Still there some questions: 1. Do we really need min buffer value as configuration [2]? I see no way it can be used, for instance at [3], we only have snd_pcm_hardware.buffer_bytes_max, but not min. So, I feel I can drop that 2. Can I assume that min buffer size == period size and add such a constraint in the frontend driver? 3. On backend side (ALSA), with current changes in the protocol I will call something like int snd_pcm_hw_params_set_channels_minmax(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *min, unsigned int *max) instead of int snd_pcm_hw_params_set_channels(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val) while servicing XENSND_OP_HW_PARAM_QUERY.XENSND_OP_HW_PARAM_CHANNELS. Does this make sense? > thanks, > > Takashi Thank you, Oleksandr [1] https://github.com/andr2000/linux/commit/03e74fb23cf5baa2e252cd1e62fa9506decbca7e [2] https://github.com/andr2000/linux/blob/tiwai_sound_for_next_pv_snd_upstream_v2/include/xen/interface/io/sndif.h#L253 [3] https://elixir.bootlin.com/linux/latest/source/include/sound/pcm.h#L53
On Tue, 13 Mar 2018 18:31:55 +0100, Oleksandr Andrushchenko wrote: > > On 03/13/2018 06:31 PM, Takashi Iwai wrote: > > On Tue, 13 Mar 2018 12:49:00 +0100, > > Oleksandr Andrushchenko wrote: > >> 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? > > This looks almost good through a quick glance. > > But the mixture of SNDRV_PCM_HW_PARAM_PERIOD_SIZE and > > SNDRV_PCM_HW_PARAM_BUFFER_BYTES are likely confusing. > > The *_SIZE means in frames unit while *_BYTES means in bytes. > > You should align both PERIOD_ and BUFFER_ to the same units, > > i.e. either use SNDRV_PCM_HW_PARAM_PERIOD_BYTES and *_BUFFER_BYTES, > > or SNDRV_PCM_HW_PARAM_PERIOD_SIZE and *_BUFFER_SIZE. > You are correct, fixed this at [1] > > Also, a slightly remaining concern is the use-case where hw_params is > > called multiple times. An application may call hw_free and hw_params > > freely, or even hw_params calls multiple times, in order to change the > > parameter. > > > > If the backend needs to resolve some dependency between parameters > > (e.g. the available period size depends on the sample rate), the > > backend has to remember the previously passed parameters. > > > > So, instead of passing a single parameter, you may extend the protocol > > always to pass the full (five) parameters, too. > > > > OTOH, this can be considered to be a minor case, and the backend > > (e.g. PA) can likely support every possible combinations, so maybe a > > simpler code may be a better solution in the end. > Yes, let's have it step by step. > If you are ok with what we have at the moment then, after I implement both > backend and frontend changes and confirm that protocol works, > I will send v3 of the series (protocol changes). > > Still there some questions: > 1. Do we really need min buffer value as configuration [2]? I see no > way it can be used, > for instance at [3], we only have snd_pcm_hardware.buffer_bytes_max, > but not min. > So, I feel I can drop that Actually with the hw_param query mechanism, this setup is moot. You can pass a fixed value that should be enough large for all cases there. > 2. Can I assume that min buffer size == period size and add such a > constraint > in the frontend driver? The buffer sie == period size is a special case, i.e. periods=1, and this won't work most likely. It's used only for a case like PA deployment without the period interrupt. And it needs a special hw_params flag your driver doesn't deal with. So for the sane setup, you can safely assume min_periods=2. > 3. On backend side (ALSA), with current changes in the protocol I will > call something like > int snd_pcm_hw_params_set_channels_minmax(snd_pcm_t *pcm, > snd_pcm_hw_params_t *params, unsigned int *min, unsigned int *max) > > instead of > > int snd_pcm_hw_params_set_channels(snd_pcm_t *pcm, snd_pcm_hw_params_t > *params, unsigned int val) > > while servicing > XENSND_OP_HW_PARAM_QUERY.XENSND_OP_HW_PARAM_CHANNELS. Does this make > sense? Yeah, that's better, I suppose. Takashi
On 03/13/2018 08:48 PM, Takashi Iwai wrote: > On Tue, 13 Mar 2018 18:31:55 +0100, > Oleksandr Andrushchenko wrote: >> On 03/13/2018 06:31 PM, Takashi Iwai wrote: >>> On Tue, 13 Mar 2018 12:49:00 +0100, >>> Oleksandr Andrushchenko wrote: >>>> 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? >>> This looks almost good through a quick glance. >>> But the mixture of SNDRV_PCM_HW_PARAM_PERIOD_SIZE and >>> SNDRV_PCM_HW_PARAM_BUFFER_BYTES are likely confusing. >>> The *_SIZE means in frames unit while *_BYTES means in bytes. >>> You should align both PERIOD_ and BUFFER_ to the same units, >>> i.e. either use SNDRV_PCM_HW_PARAM_PERIOD_BYTES and *_BUFFER_BYTES, >>> or SNDRV_PCM_HW_PARAM_PERIOD_SIZE and *_BUFFER_SIZE. >> You are correct, fixed this at [1] >>> Also, a slightly remaining concern is the use-case where hw_params is >>> called multiple times. An application may call hw_free and hw_params >>> freely, or even hw_params calls multiple times, in order to change the >>> parameter. >>> >>> If the backend needs to resolve some dependency between parameters >>> (e.g. the available period size depends on the sample rate), the >>> backend has to remember the previously passed parameters. >>> >>> So, instead of passing a single parameter, you may extend the protocol >>> always to pass the full (five) parameters, too. >>> >>> OTOH, this can be considered to be a minor case, and the backend >>> (e.g. PA) can likely support every possible combinations, so maybe a >>> simpler code may be a better solution in the end. >> Yes, let's have it step by step. >> If you are ok with what we have at the moment then, after I implement both >> backend and frontend changes and confirm that protocol works, >> I will send v3 of the series (protocol changes). >> >> Still there some questions: >> 1. Do we really need min buffer value as configuration [2]? I see no >> way it can be used, >> for instance at [3], we only have snd_pcm_hardware.buffer_bytes_max, >> but not min. >> So, I feel I can drop that > Actually with the hw_param query mechanism, this setup is moot. > You can pass a fixed value that should be enough large for all cases > there. ok, so only buffer max as it is already defined >> 2. Can I assume that min buffer size == period size and add such a >> constraint >> in the frontend driver? > The buffer sie == period size is a special case, i.e. periods=1, and > this won't work most likely. It's used only for a case like PA > deployment without the period interrupt. And it needs a special > hw_params flag your driver doesn't deal with. > > So for the sane setup, you can safely assume min_periods=2. Thanks, will limit min to 2 periods then >> 3. On backend side (ALSA), with current changes in the protocol I will >> call something like >> int snd_pcm_hw_params_set_channels_minmax(snd_pcm_t *pcm, >> snd_pcm_hw_params_t *params, unsigned int *min, unsigned int *max) >> >> instead of >> >> int snd_pcm_hw_params_set_channels(snd_pcm_t *pcm, snd_pcm_hw_params_t >> *params, unsigned int val) >> >> while servicing >> XENSND_OP_HW_PARAM_QUERY.XENSND_OP_HW_PARAM_CHANNELS. Does this make >> sense? > Yeah, that's better, I suppose. Excellent > > Takashi Thank you very much for helping with this!!! Oleksandr Andrushchenko
From 267fb5f74026b8313a54c47fcecdeb8c644f3257 Mon Sep 17 00:00:00 2001 From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> Date: Wed, 7 Mar 2018 10:21:20 +0200 Subject: [PATCH] sndif: add explicit back and front parameter negotiation 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 Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Takashi Iwai <tiwai@suse.de> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> --- xen/include/public/io/sndif.h | 127 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 120 insertions(+), 7 deletions(-) diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h index e38644423c0a..8036c5c2f212 100644 --- a/xen/include/public/io/sndif.h +++ b/xen/include/public/io/sndif.h @@ -250,6 +250,11 @@ * * The maximum size in octets of the buffer to allocate per stream. * + * buffer-size-min + * Values: <uint32_t> + * + * The minimum size in octets of the buffer to allocate per stream. + * *----------------------- Virtual sound card settings ------------------------- * short-name * Values: <char[32]> @@ -465,12 +470,20 @@ #define XENSND_OP_MUTE 6 #define XENSND_OP_UNMUTE 7 #define XENSND_OP_TRIGGER 8 +#define XENSND_OP_HW_PARAM_SET 9 +#define XENSND_OP_HW_PARAM_QUERY 10 #define XENSND_OP_TRIGGER_START 0 #define XENSND_OP_TRIGGER_PAUSE 1 #define XENSND_OP_TRIGGER_STOP 2 #define XENSND_OP_TRIGGER_RESUME 3 +#define XENSND_OP_HW_PARAM_FORMAT 0 +#define XENSND_OP_HW_PARAM_RATE 1 +#define XENSND_OP_HW_PARAM_BUFFER 2 +#define XENSND_OP_HW_PARAM_PERIOD 3 +#define XENSND_OP_HW_PARAM_CHANNELS 4 + /* ****************************************************************************** * EVENT CODES @@ -503,6 +516,7 @@ #define XENSND_FIELD_SAMPLE_RATES "sample-rates" #define XENSND_FIELD_SAMPLE_FORMATS "sample-formats" #define XENSND_FIELD_BUFFER_SIZE "buffer-size" +#define XENSND_FIELD_BUFFER_SIZE_MIN "buffer-size-min" /* Stream type field values. */ #define XENSND_STREAM_TYPE_PLAYBACK "p" @@ -828,28 +842,122 @@ struct xensnd_trigger_req { }; /* + * Request stream configuration parameter selection: + * Sound device configuration for a particular stream is a limited subset + * of the multidimensional configuration available on XenStore, e.g. + * once the frame rate has been selected there is a limited supported range + * for sample rates becomes available (which might be the same set configured + * on XenStore or less). For example, selecting 96kHz sample rate may limit + * number of channels available for such configuration from 4 to 2 etc. + * Thus, each call to XENSND_OP_SET_HW_PARAM will reduce configuration space + * making it possible to iteratively get the final stream configuration, + * used in XENSND_OP_OPEN request. + * + * 0 1 2 3 octet + * +----------------+----------------+----------------+----------------+ + * | id |_OP_SET_HW_PARAM| reserved | 4 + * +----------------+----------------+----------------+----------------+ + * | reserved | 8 + * +----------------+----------------+----------------+----------------+ + * | value | 12 + * +----------------+----------------+----------------+----------------+ + * | type | reserved | 16 + * +----------------+----------------+----------------+----------------+ + * | reserved | 20 + * +----------------+----------------+----------------+----------------+ + * | reserved | 24 + * +----------------+----------------+----------------+----------------+ + * | reserved | 28 + * +----------------+----------------+----------------+----------------+ + * | reserved | 32 + * +----------------+----------------+----------------+----------------+ + * + * value - uint32_t, requested value of the parameter + * type - uint8_t, XENSND_OP_HW_PARAM_XXX value + */ + +struct xensnd_set_hw_param_req { + uint32_t value; + uint8_t type; +}; + +/* + * Request stream configuration parameter interval: request interval of + * supported values for the parameter given. See response format for this + * request. + * + * 0 1 2 3 octet + * +----------------+----------------+----------------+----------------+ + * | id | _HW_PARAM_QUERY| reserved | 4 + * +----------------+----------------+----------------+----------------+ + * | reserved | 8 + * +----------------+----------------+----------------+----------------+ + * | type | reserved | 16 + * +----------------+----------------+----------------+----------------+ + * | reserved | 16 + * +----------------+----------------+----------------+----------------+ + * | reserved | 20 + * +----------------+----------------+----------------+----------------+ + * | reserved | 24 + * +----------------+----------------+----------------+----------------+ + * | reserved | 28 + * +----------------+----------------+----------------+----------------+ + * | reserved | 32 + * +----------------+----------------+----------------+----------------+ + * + * type - uint8_t, XENSND_OP_HW_PARAM_XXX value + */ + +struct xensnd_query_hw_param_req { + uint8_t type; +}; + +/* *---------------------------------- Responses -------------------------------- * * All response packets have the same length (32 octets) * - * Response for all requests: + * All response packets have common header: * 0 1 2 3 octet * +----------------+----------------+----------------+----------------+ * | id | operation | reserved | 4 * +----------------+----------------+----------------+----------------+ * | status | 8 * +----------------+----------------+----------------+----------------+ - * | reserved | 12 + * + * id - uint16_t, copied from the request + * operation - uint8_t, XENSND_OP_* - copied from request + * status - int32_t, response status, zero on success and -XEN_EXX on failure + * + * + * HW parameter query response - response for XENSND_OP_HW_PARAM_QUERY: + * 0 1 2 3 octet + * +----------------+----------------+----------------+----------------+ + * | id | operation | reserved | 4 + * +----------------+----------------+----------------+----------------+ + * | status | 8 + * +----------------+----------------+----------------+----------------+ + * | min | 12 + * +----------------+----------------+----------------+----------------+ + * | max | 16 + * +----------------+----------------+----------------+----------------+ + * | reserved | 20 * +----------------+----------------+----------------+----------------+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| * +----------------+----------------+----------------+----------------+ * | reserved | 32 * +----------------+----------------+----------------+----------------+ * - * id - uint16_t, copied from the request - * operation - uint8_t, XENSND_OP_* - copied from request - * status - int32_t, response status, zero on success and -XEN_EXX on failure - * + * min - uint32_t, minimum value of the parameter that can be used + * max - uint32_t, maximum value of the parameter that can be used + */ + +struct xensnd_query_hw_param_resp { + uint32_t min; + uint32_t max; +}; + +/* *----------------------------------- Events ---------------------------------- * * Events are sent via a shared page allocated by the front and propagated by @@ -902,6 +1010,8 @@ struct xensnd_req { struct xensnd_open_req open; struct xensnd_rw_req rw; struct xensnd_trigger_req trigger; + struct xensnd_set_hw_param_req set_hw_param; + struct xensnd_query_hw_param_req query_hw_param; uint8_t reserved[24]; } op; }; @@ -911,7 +1021,10 @@ struct xensnd_resp { uint8_t operation; uint8_t reserved; int32_t status; - uint8_t reserved1[24]; + union { + struct xensnd_query_hw_param_resp hw_param; + uint8_t reserved1[24]; + } resp; }; struct xensnd_evt { -- 2.7.4