On 26.07.23 16:32, Albert Esteve wrote:
On Mon, Jul 10, 2023 at 10:52 AM Alexander Gordeev
<alexander.gordeev@xxxxxxxxxxxxxxx
<mailto:alexander.gordeev@xxxxxxxxxxxxxxx>> wrote:
Hi Albert,
On 06.07.23 16:59, Albert Esteve wrote:
> Hi Alexander,
>
> Thanks for the patch! It is a long document, so I skimmed a bit
in the
> first read. Find some comments/questions inlined.
> I will give it a second deeper read soon, but overall I think is in
> quite good shape. It feels really matured.
Great! Thank you for taking the time to review it.
> On Thu, Jun 29, 2023 at 4:49 PM Alexander Gordeev
> <Alexander.Gordeev@xxxxxxxxxxxxxxx
<mailto:Alexander.Gordeev@xxxxxxxxxxxxxxx>
> <mailto:Alexander.Gordeev@xxxxxxxxxxxxxxx
<mailto:Alexander.Gordeev@xxxxxxxxxxxxxxx>>> wrote:
...snip...
> +
> +\subsubsection{Device Operation: Device Commands}
> +\label{sec:Device Types / Video Device / Device Operation /
Device
> Operation: Device Commands}
> +
> +This command allows retrieving the device capabilities.
> +
> +\paragraph{VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS}
> +\label{sec:Device Types / Video Device / Device Operation /
Device
> Operation: Device Commands / VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS}
> +
> +Retrieve device capabilities for all available stream
parameters.
> +
> +The driver sends this command with
> +\field{struct virtio_video_device_query_caps}:
> +
> +\begin{lstlisting}
> +struct virtio_video_device_query_caps {
> + le32 cmd_type; /* VIRTIO_VIDEO_CMD_DEVICE_QUERY_CAPS */
> +};
> +\end{lstlisting}
> +
> +The device responds with
> +\field{struct virtio_video_device_query_caps_resp}:
> +
> +\begin{lstlisting}
> +#define MASK(x) (1 << (x))
> +
> +struct virtio_video_device_query_caps_resp {
> + le32 result; /* VIRTIO_VIDEO_RESULT_* */
> + le32 stream_params_bitmask; /* Bitmask of
> MASK(VIRTIO_VIDEO_PARAM_*) */
> + le32 coded_formats_bitmask; /* Bitmaks of
> MASK(VIRTIO_VIDEO_CODED_FORMAT_*) */
> + le32 raw_formats_bitmask; /* Bitmask of
> MASK(VIRTIO_VIDEO_RAW_FORMAT_*) */
> + le32 num_format_deps;
> + le32 num_raw_format_caps;
> + le32 num_coded_resources_caps;
> + le32 num_raw_resources_caps;
> + le32 num_bitrate_caps; /* If
> MASK(VIRTIO_VIDEO_PARAM_BITRATE) is set. */
> + u8 padding[4];
> + /* If corresponding
MASK(VIRTIO_VIDEO_PARAM_GROUP_CODEC_*)
> is set. */
> + struct virtio_video_mpeg2_caps mpeg2_caps;
> + struct virtio_video_mpeg4_caps mpeg4_caps;
> + struct virtio_video_h264_caps h264_caps;
> + struct virtio_video_hevc_caps hevc_caps;
> + struct virtio_video_vp8_caps vp8_caps;
> + struct virtio_video_vp9_caps vp9_caps;
> + /**
> + * Followed by
> + * struct virtio_video_format_dependency
> format_deps[num_format_deps];
> + */
> + /**
> + * Followed by
> + * struct virtio_video_raw_format_caps
> raw_format_caps[num_raw_format_caps];
> + */
> + /**
> + * Followed by
> + * struct virtio_video_coded_resources_caps
> + * coded_resources_caps[num_coded_resources_caps];
> + */
> + /**
> + * Followed by
> + * struct virtio_video_raw_resources_caps
> raw_resources_caps[num_raw_resources_caps];
> + */
> + /**
> + * Followed by if MASK(VIRTIO_VIDEO_PARAM_BITRATE)
is set
> + * struct virtio_video_bitrate_caps
> bitrate_caps[num_bitrate_caps];
> + */
> +};
>
>
> Maybe nitpicking, but some of the member structs are inside a
comment
> and some are not.
> Does not seem to correlate with them being conditional.
> I think is nice to have conditional fields in comment blocks to
> highlight it, but then the
> VIRTIO_VIDEO_PARAM_GROUP_CODEC_* structs need to be in their own
comment
> block.
Yeah, this style comes from draft v5, then I added the conditional
statementson top, so now it is harder to understand. I also would like
to do this in a different way. I was thinking recently about
extendability of this construct, it doesn't look good. If a new
codec or
a new codec-specific parameters is added, it has to be guarded by a new
feature flag, say VIRTIO_VIDEO_F_CODECS_2024. Then the device will have
to provide different structures depending on the negotiated flags and
the driver will have to parse it. This looks quite painful and
error-prone. My current idea is to replace this with something like FDT
to make it much more flexible. The resulting blob with all the
capabilities can even be mapped directly to the guest memory. I'm still
exploring this idea. WDYT?
Yes, the struct looks a bit cumbersome and difficult to expand, as you
mention.
But I am not sure what do you mean by FDT, or how you plan to map it to
the guest
memory. Could you expand the idea?
I mean maybe it is better to use something like a flattened device tree,
(which is a serialization of a device tree) as a way to share the device
capabilities. I think this would be well compatible with the V4L2
capability discovery process, as it is tree-like. Unfortunately, FDT
seems to be too specific to device trees. The ideal candidate IMO would
be a well known standard for a flat tree, that we can easily reference,
with an existing implementation in the kernel. That's why we thought
about FDT.
The other thing that comes to mind is type-length-value (TLV). It is a
relatively well known thing, it is easy, but I'm not sure there is a
standard to reference. Still using a number of TLV descriptors organized
in a tree-like fashion seems like a good way to solve the extensibility
problem because it is easy to add new types, obsolete old types and
overall the approach is very flexible. TLV is used in many parts of the
kernel. For example, ALSA. Still thinking about this.
About mapping: there are concerns, that the size of the resulting flat
tree blob would be unpredictable. There might be some limits on the
driver side. One of ideas was to replace copying and sending the
capabilities to every guest with read only mappings. This can be done
using virtio shared memory. Still exploring this idea as well.
But from what I see, structures for most formats have similar fields for
capabilities.
Couldn't this be unified into a single capabilities struct and fill it
with the raw data obtained
from the host device?
I'm still trying to have a single structure that represents all of the
device capabilities. I think there is value in this. We have many listed
codecs, so the structure has to describe device capabilities for every
supported codec. I.e. this is not a union. So we should either have a
lot of reserved space for future generic/codec parameters, or use more
dynamic structures like TLV. Or give up on trying to fit all of this
into a single command. Then we may end up with many commands like in
V4L2. I'd prefer TLV.
--
Alexander Gordeev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Phone: +49 30 60 98 54 0 - 88
Fax: +49 (30) 60 98 54 0 - 99
EMail: alexander.gordeev@xxxxxxxxxxxxxxx
www.opensynergy.com
Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Régis Adjamah
