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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel