Re: [RFC PATCH v8 1/1] virtio-video: Add virtio video device specification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux