Hi,
On Thu, Dec 5, 2019 at 4:12 AM Enrico Granata <egranata@xxxxxxxxxx> wrote:
>
> On Wed, Dec 4, 2019 at 1:16 AM Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > > 1. Focus on only decoder/encoder functionalities first.
> > >
> > > As Tomasz said earlier in this thread, it'd be too complicated to support camera
> > > usage at the same time. So, I'd suggest to make it just a generic mem-to-mem
> > > video processing device protocol for now.
> > > If we finally decide to support camera in this protocol, we can add it later.
> >
> > Agree.
> >
> > > 2. Only one feature bit can be specified for one device.
> > >
> > > I'd like to have a decoder device and encoder device separately.
> > > It'd be natural to assume it because a decoder and an encoder are provided as
> > > different hardware.
> >
> > Hmm, modern GPUs support both encoding and decoding ...
> >
> > I don't think we should bake that restriction into the specification.
> > It probably makes sense to use one virtqueue per function though, that
> > will simplify dispatching in both host and guest.
> >
It'd be a better idea to have a set of virtqueues for each function.
So, we will have 2 * (# of features provided) virtqueues, as one
function requires controlq
and eventq.
> > > 3. Separate buffer allocation functionalities from virtio-video protocol.
> > >
> > > To support various ways of guest/host buffer sharing, we might want to have a
> > > dedicated buffer sharing device as we're discussing in another thread. Until we
> > > reach consensus there, it'd be good not to have buffer allocation
> > > functionalities in virtio-video.
> >
> > I think virtio-video should be able to work as stand-alone device,
> > so we need some way to allocate buffers ...
> >
> > Buffer sharing with other devices can be added later.
Good point. Then, we should have T_RESOURCE_CREATE and T_RESOURCE_DESTROY
at least.
> >
> > > > +The virtio video device is a virtual video streaming device that supports the
> > > > +following functions: encoder, decoder, capture, output.
> > > > +
> > > > +\subsection{Device ID}\label{sec:Device Types / Video Device / Device ID}
> > > > +
> > > > +TBD.
> > >
> > > I'm wondering how and when we can determine and reserve this ID?
> >
> > Grab the next free, update the spec accordingly, submit the one-line
> > patch.
Thanks. I will do so at the next version of the patch.
> >
> > > > +\begin{lstlisting}
> > > > +enum virtio_video_pixel_format {
> > > > + VIRTIO_VIDEO_PIX_FMT_UNDEFINED = 0,
> > > > +
> > > > + VIRTIO_VIDEO_PIX_FMT_H264 = 0x0100,
> > > > + VIRTIO_VIDEO_PIX_FMT_NV12,
> > > > + VIRTIO_VIDEO_PIX_FMT_NV21,
> > > > + VIRTIO_VIDEO_PIX_FMT_I420,
> > > > + VIRTIO_VIDEO_PIX_FMT_I422,
> > > > + VIRTIO_VIDEO_PIX_FMT_XBGR,
> > > > +};
> > >
> > > I'm wondering if we can use FOURCC instead. So, we can avoid reinventing a
> > > mapping from formats to integers.
> > > Also, I suppose the word "pixel formats" means only raw (decoded) formats.
> > > But, it can be encoded format like H.264. So, I guess "image format" or
> > > "fourcc" is a better word choice.
> >
> > Use separate pixel_format (fourcc) and stream_format (H.264 etc.) enums?
> >
Fourcc is used for both raw and coded formats.
I'm not sure if it makes much sense to define different enums for raw
and coded formats, as
we reuse any other structs for both types of formats.
What I'd suggest is like this:
#define FOURCC(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
enum virtio_video_fourcc {
VIRTIO_VIDEO_FOURCC_UNDEFINED = 0,
/* Coded formats */
VIRTIO_VIDEO_FOURCC_H264 = FOURCC('H', '2', '6', '4'),
...
/* Raw formats */
VIRTIO_VIDEO_FOURCC_NV12 = FOURCC('N', 'V', '1', '2'),
...
}
> > > > +\begin{lstlisting}
> > > > +struct virtio_video_function {
> > > > + struct virtio_video_desc desc;
> > > > + __le32 function_type; /* One of VIRTIO_VIDEO_FUNC_* types */
> > > > + __le32 function_id;
> > > > + struct virtio_video_params in_params;
> > > > + struct virtio_video_params out_params;
> > > > + __le32 num_caps;
> > > > + __u8 padding[4];
> > > > + /* Followed by struct virtio_video_capability video_caps[]; */
> > > > +};
> > > > +\end{lstlisting}
> > >
> > > If one device only has one functionality, virtio_video_function's fields will be
> > > no longer needed except in_params and out_params. So, we'd be able to remove
> > > virtio_video_function and have in_params and out_params in
> > > virtio_video_capability instead.
> >
> > Same goes for per-function virtqueues (used virtqueue implies function).
> >
> > > > +\begin{lstlisting}
> > > > +struct virtio_video_resource_detach_backing {
> > > > + struct virtio_video_ctrl_hdr hdr;
> > > > + __le32 resource_id;
> > > > + __u8 padding[4];
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +\begin{description}
> > > > +\item[\field{resource_id}] internal id of the resource.
> > > > +\end{description}
> > >
> > > I suppose that it'd be better not to have the above series of T_RESOURCE
> > > controls at least until we reach a conclusion in the thread of buffer-sharing
> > > device. If we end up concluding this type of controls is the best way, we'll be
> > > able to revisit here.
> >
>
> As an interim solution, this form of "manual resource backing-store
> management" could be specified as a feature flag.
> Once there is an established solution for buffer sharing, we would
> then go ahead and introduce a new feature flag for "works with the
> buffer sharing mechanism", as an alternative to this manual mechanism.
>
> wdyt?
It'd be a good idea to change buffer management method by a feature flag.
>
> > Well. For buffer management there are a bunch of options.
> >
> > (1) Simply stick the buffers (well, pointers to the buffer pages) into
> > the virtqueue. This is the standard virtio way.
> >
> > (2) Create resources, then put the resource ids into the virtqueue.
> > virtio-gpu uses that model. First, because virtio-gpu needs an id
> > to reference resources in the rendering command stream
> > (virtio-video doesn't need this). Also because (some kinds of)
> > resources are around for a long time and the guest-physical ->
> > host-virtual mapping needs to be done only once that way (which
> > I think would be the case for virtio-video too because v4l2
> > re-uses buffers in robin-round fashion). Drawback is this
> > assumes shared memory between host and guest (which is the case
> > in typical use cases but it is not mandated by the virtio spec).
> >
> > (3) Import external resources (from virtio-gpu for example).
> > Out of scope for now, will probably added as optional feature
> > later.
> >
> > I guess long-term we want support either (1)+(3) or (2)+(3).
> >
In the first version of spec, we might want to support the minimal workable set
of controls. Then, we'll be able to add additional controls with a new feature
flag as Enrico suggested.
So, the problem is what's the simplest scenario and which types of controls are
required there. I guess it's enough for (1) and (2) if we have T_RESOURCE_CREATE
and T_RESOURCE_DESTROY. We might need to add some fields in the structs, though.
If so, we should add only these two controls first.
Best regards,
Keiichi
> > cheers,
> > Gerd
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
> > For additional commands, e-mail: virtio-dev-help@xxxxxxxxxxxxxxxxxxxx
> >
