Re: [PATCH RFC v4 0/1] Virtio Video Device Specification

[Keiichi Watanabe <keiichiw@chromium.org>]

Hi Dmitry,

On Thu, Jul 2, 2020 at 4:32 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> Thank you very much for the hard work to update the spec and to summarize all
> of the recent proposals!
>
> I want to again raise a topic that was discussed earlier and unfortunately the
> latest proposal cannot resolve the problem. I hope together with upstream
> people we'll be able to find a neat solution.
>
> Please consider the following case:
> 1. Encoder case
> 2. User app does reqbufs with DMABUF flag.
> 3. User app submits frames to encode, each frame has a different fd, might be a
> completely new buffer.
> 4. Driver receives this buffer via queue() and does this check to verify
> whether it is a known dmabuf:
> https://elixir.bootlin.com/linux/v5.7.6/source/drivers/media/common/videobuf2/
> videobuf2-core.c#L1163
> 5. When the check fails, it does cleanup.
> 6. BUG: As we got rid of the flexible resource detach/destroy calls, host side
> has no way to know the resource has a new memory region. The new sgt is never
> propagated to the host.
>
> The mentioned earlier CMD_RESOURE_REASSIGN_ENTRIES/CMD_RESOURE_REASSIGN_OBJECT
> are not included in the spec.

This shouldn't be a problem. Though we don't have a per-resource
detach command, we can _replace_ attached sg-list or virtio-object by
calling the attach command.
In your scenario, if the driver notices a new dmabuf is given at 4 and
5, the driver should send RESOURCE_ATTACH command with the new dmabuf.
Then, the old dmabuf was regarded as "detached". Please see the
"Buffer life cycle" section.

I renamed RESOURCE_REASSIGN_* commands to RESOURCE_ATTACH as it's also
used at the first time to attach a buffer.

Best regards,
Keiichi

>
> Thanks in advance.
>
> Best regards,
> Dmitry.
>
> On Dienstag, 23. Juni 2020 13:13:24 CEST Keiichi Watanabe wrote:
> >
> > This is the v4 RFC of virtio video device specification.
> > PDF versions are available at [1, 2].
> >
> > Note that this patch depends on a recent patchset "Cross-device resource
> > sharing" [3].
> >
> > Here is a list of major changes from v3:
> > * Redesingned struct definitions for each command and request based on
> >   discussions at [4].
> > * Renamed commands/structs/enums to more descriptive names.
> > * Had different structs and fields for image formats and bitstream formats.
> > * Added more detailed specification for supported video codecs.
> > * Made stream_id be allocated by the device.
> > * Had a single parameter struct per-stream instead of per-queue parameters
> > and controls.
> > * Allowed the driver to specify the number of buffers to use via
> >   "cur_{image,bitstream}_buffers".
> > * Renamed RESOURCE_CREATE command to RESOURCE_ATTACH command and allow the
> >   driver to use this command when replacing backing memories as well.
> >
> > [5] is the diff of the header file from v3. Note that it only contains
> > changes in the header. We haven't updated the driver nor device
> > implementation to focus on protocol design discussion first.
> >
> > While it may appear that many parts have been changed since the previous
> > revision, these changes are to address the issues raised in previous
> > discussions or/and to make the protocol simpler and easier to prevent
> > misuse.
> > I'd appreciate any types of feedback.
> >
> > Best regards,
> > Keiichi
> >
> > [1] (full):
> > https://drive.google.com/file/d/1DiOJZfUJ5wvFtnNFQicxt0zkp4Ob1o9C/view?usp=
> > sharing [2] (only video section):
> > https://drive.google.com/file/d/188uAkIWE0BsXETECez98y5fJKw8rslj3/view?usp=
> > sharing [3]
> > https://lists.oasis-open.org/archives/virtio-comment/202003/msg00035.html
> > [4] https://markmail.org/thread/c6h3e3zn647qli3w
> > [5]
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/
> > 2164411
> >
> > Keiichi Watanabe (1):
> >   virtio-video: Add virtio video device specification
> >
> >  .gitignore                        |    1 +
> >  content.tex                       |    1 +
> >  images/video-buffer-lifecycle.dot |   18 +
> >  make-setup-generated.sh           |    8 +
> >  virtio-video.tex                  | 1163 +++++++++++++++++++++++++++++
> >  5 files changed, 1191 insertions(+)
> >  create mode 100644 .gitignore
> >  create mode 100644 images/video-buffer-lifecycle.dot
> >  create mode 100644 virtio-video.tex
> >
> > --
> > 2.27.0.111.gc72c7da667-goog
>
>