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

[Oleksandr Andrushchenko]

On 03/02/2018 12:11 AM, Konrad Rzeszutek Wilk wrote:
>>    * +----------------+----------------+----------------+----------------+
>>    * |                           gref_directory                          | 24
>>    * +----------------+----------------+----------------+----------------+
>> - * |                             reserved                              | 28
>> - * +----------------+----------------+----------------+----------------+
>> - * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * |                             period_sz                             | 28
>>    * +----------------+----------------+----------------+----------------+
>>    * |                             reserved                              | 32
>>    * +----------------+----------------+----------------+----------------+
>> @@ -578,6 +616,14 @@
>>    * pcm_channels - uint8_t, number of channels of this stream,
>>    *   [channels-min; channels-max]
>>    * buffer_sz - uint32_t, buffer size to be allocated, octets
>> + * period_sz - uint32_t, recommended event period size, octets
>> + *   This is the recommended (hint) value of the period at which frontend would
>> + *   like to receive XENSND_EVT_CUR_POS notifications from the backend when
>> + *   stream position advances during playback/capture.
>> + *   It shows how many octets are expected to be played/captured before
>> + *   sending such an event.
>> + *   If set to 0 no XENSND_EVT_CUR_POS events are sent by the backend.
>> + *
> I would gate this based on the version. That is if version 0 then this
> field does not exist.
Well, by default we have all unused fields set to 0: [1]
And for version 1 (or 0, initial) of the protocol it means
that period_sz falls into reserved region [2].
With the comment above for version 2 it means that old
behavior is in use, e.g. no XENSND_EVT_CUR_POS events
are sent by the backend, so we are backward compatible
in this case
>>    * gref_directory - grant_ref_t, a reference to the first shared page
>>    *   describing shared buffer references. At least one page exists. If shared
>>    *   buffer size  (buffer_sz) exceeds what can be addressed by this single page,
>> @@ -592,6 +638,7 @@ struct xensnd_open_req {
>>       uint16_t reserved;
>>       uint32_t buffer_sz;
>>       grant_ref_t gref_directory;
>> +    uint32_t period_sz;
> The same here. Just put a comment mentioning the version part.
So, if you still want to gate it, how would you like it?
XENSND_PROTOCOL_VERSION was defined as a string "1"/"2"
and preprocessor won't allow comparing strings, e.g.
you can't do #if XENSND_PROTOCOL_VERSION == "1"
So, we might want re-defining XENSND_PROTOCOL_VERSION as
an integer in the first patch, so it can be used here.
Then, if it is an integer:
1.
#if XENSND_PROTOCOL_VERSION != 1
     uint32_t period_sz;
#endif
2.
#if XENSND_PROTOCOL_VERSION > 1
     uint32_t period_sz;
#endif
3.
#if XENSND_PROTOCOL_VERSION == 2
     uint32_t period_sz;
#endif

Please let me know whether we still want gating and if so
in which way.

Thank you,
Oleksandr
[1] 
https://elixir.bootlin.com/linux/v4.16-rc3/source/include/xen/interface/io/sndif.h#L525
[2] 
https://elixir.bootlin.com/linux/v4.16-rc3/source/include/xen/interface/io/sndif.h#L562