[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [RFC PATCH v8 1/1] virtio-video: Add virtio video device specification
On 26.07.23 16:32, Albert Esteve wrote:
On Mon, Jul 10, 2023 at 10:52âAM Alexander Gordeev <alexander.gordeev@opensynergy.com <mailto:alexander.gordeev@opensynergy.com>> 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@opensynergy.com <mailto:Alexander.Gordeev@opensynergy.com> > <mailto:Alexander.Gordeev@opensynergy.com <mailto:Alexander.Gordeev@opensynergy.com>>> 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 guestmemory. 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 obtainedfrom 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@opensynergy.com www.opensynergy.com Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B GeschÃftsfÃhrer/Managing Director: RÃgis Adjamah
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]