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

From: Takashi Iwai <tiwai@suse.de>
To: Oleksandr Andrushchenko <andr2000@gmail.com>
Cc: xen-devel@lists.xenproject.org, alsa-devel@alsa-project.org,
	Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Subject: Re: [alsa-devel] [PATCH 0/2] sndif: add explicit back and front synchronization
Date: Tue, 13 Mar 2018 17:31:14 +0100	[thread overview]
Message-ID: <s5hvae09bu5.wl-tiwai__17871.58546872$1520958605$gmane$org@suse.de> (raw)
In-Reply-To: <e3f39c7f-721f-ace9-82b3-d270c6bf0137@gmail.com>

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

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