OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: Re: [PATCH v4] virtio-snd: add virtio sound device specification


On 08.01.2020 11:34, Gerd Hoffmann wrote:
On Tue, Jan 07, 2020 at 01:50:49PM +0100, Anton Yakovlev wrote:
This patch proposes virtio specification for a new virtio sound device,
that may be useful in case when having audio is required but a device
passthrough or emulation is not an option.

Looks pretty good overall.  Some small nits:

+/* a common header */
+struct virtio_snd_hdr {
+    le32 code;
+};
+
+/* an event notification */
+struct virtio_snd_event {
+    le32 type;

Use "struct virtio_snd_hdr hdr" instead of "type", for consistency with
the other structs?

Yes, good idea.


+\begin{description}
+\item[\field{code}] (device-readable) specifies a device request type
+(VIRTIO_SND_R_*).
+\end{description}
+
+A response part has, or consists of, a common header containing the following
+field:
+
+\begin{description}
+\item[\field{code}] (device-writable) specifies a device request status
+(VIRTIO_SND_S_*).
+\end{description}

Repeating "device-readable" or "device-writable" for each struct field
looks a bit odd because this applies to the whole struct.  Not so much
for these structs with a single field only, but there are structs with
more fields further down the spec ...

Well, I'm not sure here. Previously, I was told to mark explicitly every
field.


+\begin{description}
+\item[\field{type}] (device-writable) specifies an event type (VIRTIO_SND_EVT_*).
+\item[\field{data}] (device-writable) specifies an optional event data.
+\end{description}

... here for example.

+/* a response containing PCM stream capabilities */
+struct virtio_snd_pcm_caps {
+    struct virtio_snd_hdr hdr; /* VIRTIO_SND_S_XXX */
+    u8 directions; /* 1 << VIRTIO_SND_PCM_T_XXX */
+    u8 channels_min;
+    u8 channels_max;
+    u8 features; /* 1 << VIRTIO_SND_PCM_F_XXX */
+    le64 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
+    le32 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */

Use le64 for rates too, just in case?

For features a few more bits don't hurt too, we already have 5 defined,
so having only 8 total available isn't that much for possible
extensions ...

Okey, then I will rearrange the structure.


+\paragraph{VIRTIO_SND_R_PCM_PAUSE}
+\paragraph{VIRTIO_SND_R_PCM_UNPAUSE}

Still here.

So there are good reasons to keep them instead of just using stop/start?

We can get rid of pause/unpause at the moment. Later, they can be added as a stream feature, if necessary.

Also, regarding requirement for populating tx/rx queues with period size buffers. Maybe it's better to replace MUST with SHOULD? It would give more flexibility for the driver. What do you think?


--
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]