Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification

[Keiichi Watanabe <keiichiw@chromium.org>]

Hi,

On Wed, Jan 15, 2020 at 8:00 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Mon, Jan 13, 2020 at 10:27 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > Well, no.  Tomasz Figa had splitted the devices into three groups:
> > > >
> > > >   (1) requires single buffer.
> > > >   (2) allows any layout (including the one (1) devices want).
> > > >   (3) requires per-plane buffers.
> > > >
> > > > Category (3) devices are apparently rare and old.  Both category (1)+(2)
> > > > devices can handle single buffers just fine.  So maybe support only
> > > > that?
> > >
> > > From the guest implementation point of view, Linux V4L2 currently
> > > supports 2 cases, if used in allocation-mode (i.e. the buffers are
> > > allocated locally by V4L2):
> > >
> > > i) single buffer with plane offsets predetermined by the format, (can
> > > be handled by devices from category 1) and 2))
> > > ii) per-plane buffers with planes at the beginning of their own
> > > buffers. (can be handled by devices from category 2) and 3))
> > >
> > > Support for ii) is required if one wants to be able to import buffers
> > > with arbitrary plane offsets, so I'd consider it unavoidable.
> >
> > If you have (1) hardware you simply can't import buffers with arbitrary
> > plane offsets, so I'd expect software would prefer the single buffer
> > layout (i) over (ii), even when using another driver + dmabuf
> > export/import, to be able to support as much hardware as possible.
> > So (ii) might end up being unused in practice.
> >
> > But maybe not, was just an idea, feel free to scratch it.
>
> That's true, simple user space would often do that. However, if more
> devices are in the game, often some extra alignment or padding between
> planes is needed and that is not allowed by (1), even though all the
> planes are in the same buffer.
>
> My suggestion, based on the latest V4L2 discussion on unifying the
> UAPI of i) and ii), is that we may want to instead always specify
> buffers on a per-plane basis. Any additional requirements would be
> then validated by the host, which could check if the planes end up in
> the same buffer (or different buffers for (3)) and/or at the right
> offsets.
>

It sounds reasonable. Even in the protocol design we discussed so far,
the driver sends an array of struct virtio_video_mem_entry in the
RESOURE_CREATE command:
struct virtio_video_mem_entry {
  le64 addr;
  le32 length;
  u8 padding[4];
};

struct virtio_video_resource_create {
  struct virtio_video_cmd_hdr hdr;
  le32 queue_type;
  le32 resource_id;
  le32 num_planes;
  le32 num_entries[VIRTIO_VIDEO_MAX_PLANES];
  u8 padding[4];
  /* Followed by struct virtio_video_mem_entry entries[] */
};

Does it match with your idea? We may need an |offset| in virtio_video_mem_entry?

Also, we should redesign "enum virtio_video_planes_layout" that we
originally discussed.
How about changing it like this?

enum virtio_video_planes_layout {
        VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER = 1 << 0,
        VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE = 1 << 1,
};

So, the device can combine flags to tell which (1), (2) or (3) it is.
Then, the device check if an incoming RESOURE_CREATE request violates
the requirement.
Does it make sense?

Best regards,
Keiichi

> WDYT?
>
> Best regards,
> Tomasz