[RFC,v4,0/1] Virtio Video Device Specification

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
  

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.

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
  

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
>
>
  

Hi Keiichi,

Thanks for the clarification. I believe we should explicitly describe this in 
the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem 
there. If it is a guest allocated resource, we cannot consider it to be free 
until the device really releases it. And it won't happen until we issue the 
next ATTACH call. Also, as we discussed before, it might be not possible to 
free individual buffers, but the whole queue only.

Best regards,
Dmitry.

On Donnerstag, 2. Juli 2020 14:50:58 CEST Keiichi Watanabe wrote:
> 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/videob
> > uf2/ 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?u
> > > sp=
> > > sharing [2] (only video section):
> > > https://drive.google.com/file/d/188uAkIWE0BsXETECez98y5fJKw8rslj3/view?u
> > > sp=
> > > sharing [3]
> > > https://lists.oasis-open.org/archives/virtio-comment/202003/msg00035.htm
> > > l
> > > [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
  

Hi Dmitry,

On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> Thanks for the clarification. I believe we should explicitly describe this in
> the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> there. If it is a guest allocated resource, we cannot consider it to be free
> until the device really releases it. And it won't happen until we issue the
> next ATTACH call. Also, as we discussed before, it might be not possible to
> free individual buffers, but the whole queue only.

In the case of the encoder, a V4L2 driver is not supposed to let
user-space dequeue an input frame while it is used as reference for
the encoding process. So if we add a similar rule in the virtio-video
specification, I suppose this would solve the problem?

For the decoder case, we have a bigger problem though. Since DMABUFs
can be arbitrarily attached to any V4L2 buffer ID, we may end up
re-queueing the DMABUF of a decoded frame that is still used as
reference under a different V4L2 buffer ID. In this case I don't think
the driver has a way to know that the memory resource should not be
overwritten, and it will thus happily use it as a decode target. The
easiest fix is probably to update the V4L2 stateful specification to
require that reused DMABUFs must always be assigned to the same V4L2
buffer ID, and must be kept alive as long as decoding is in progress,
or until a resolution change event is received. We can then apply a
similar rule to the virtio device.

>
> Best regards,
> Dmitry.
>
> On Donnerstag, 2. Juli 2020 14:50:58 CEST Keiichi Watanabe wrote:
> > 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/videob
> > > uf2/ 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?u
> > > > sp=
> > > > sharing [2] (only video section):
> > > > https://drive.google.com/file/d/188uAkIWE0BsXETECez98y5fJKw8rslj3/view?u
> > > > sp=
> > > > sharing [3]
> > > > https://lists.oasis-open.org/archives/virtio-comment/202003/msg00035.htm
> > > > l
> > > > [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
>
>
  

On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> Hi Dmitry,
> 
> On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> >
> > Hi Keiichi,
> >
> > Thanks for the clarification. I believe we should explicitly describe this in
> > the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> > there. If it is a guest allocated resource, we cannot consider it to be free
> > until the device really releases it. And it won't happen until we issue the
> > next ATTACH call. Also, as we discussed before, it might be not possible to
> > free individual buffers, but the whole queue only.
> 
> In the case of the encoder, a V4L2 driver is not supposed to let
> user-space dequeue an input frame while it is used as reference for
> the encoding process. So if we add a similar rule in the virtio-video
> specification, I suppose this would solve the problem?
> 
> For the decoder case, we have a bigger problem though. Since DMABUFs
> can be arbitrarily attached to any V4L2 buffer ID, we may end up
> re-queueing the DMABUF of a decoded frame that is still used as
> reference under a different V4L2 buffer ID. In this case I don't think
> the driver has a way to know that the memory resource should not be
> overwritten, and it will thus happily use it as a decode target. The
> easiest fix is probably to update the V4L2 stateful specification to
> require that reused DMABUFs must always be assigned to the same V4L2
> buffer ID, and must be kept alive as long as decoding is in progress,
> or until a resolution change event is received. We can then apply a
> similar rule to the virtio device.


Sounds like a generic kind of problem - how do other devices solve it?
  

On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> > Hi Dmitry,
> >
> > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> > >
> > > Hi Keiichi,
> > >
> > > Thanks for the clarification. I believe we should explicitly describe this in
> > > the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> > > there. If it is a guest allocated resource, we cannot consider it to be free
> > > until the device really releases it. And it won't happen until we issue the
> > > next ATTACH call. Also, as we discussed before, it might be not possible to
> > > free individual buffers, but the whole queue only.
> >
> > In the case of the encoder, a V4L2 driver is not supposed to let
> > user-space dequeue an input frame while it is used as reference for
> > the encoding process. So if we add a similar rule in the virtio-video
> > specification, I suppose this would solve the problem?
> >
> > For the decoder case, we have a bigger problem though. Since DMABUFs
> > can be arbitrarily attached to any V4L2 buffer ID, we may end up
> > re-queueing the DMABUF of a decoded frame that is still used as
> > reference under a different V4L2 buffer ID. In this case I don't think
> > the driver has a way to know that the memory resource should not be
> > overwritten, and it will thus happily use it as a decode target. The
> > easiest fix is probably to update the V4L2 stateful specification to
> > require that reused DMABUFs must always be assigned to the same V4L2
> > buffer ID, and must be kept alive as long as decoding is in progress,
> > or until a resolution change event is received. We can then apply a
> > similar rule to the virtio device.
>
>
> Sounds like a generic kind of problem - how do other devices solve it?

Most users of V4L2 drivers use MMAP buffers, which don't suffer from
this problem: since MMAP buffers are managed by V4L2 and the same V4L2
buffer ID always corresponds to the same backing memory, the driver
just needs to refrain from decoding into a given V4L2 buffer as long
as it is used as a reference frames. This is something that all
drivers currently do AFAICT.

The problem only occurs if you let userspace map anything to V4L2
buffers (USERPTR or DMABUF buffers). In order to guarantee the same
reliable behavior as with MMAP buffers, you must enforce the same
rule: always the same backing memory for a given V4L2 buffer.

... or you can rotate between a large enough number of buffers to
leave enough space for the reference tag to expire on your frames, but
that's clearly not a good way to address the problem.
  

On Fri, Jul 3, 2020 at 11:27 AM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> > > Hi Dmitry,
> > >
> > > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> > > >
> > > > Hi Keiichi,
> > > >
> > > > Thanks for the clarification. I believe we should explicitly describe this in
> > > > the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> > > > there. If it is a guest allocated resource, we cannot consider it to be free
> > > > until the device really releases it. And it won't happen until we issue the
> > > > next ATTACH call. Also, as we discussed before, it might be not possible to
> > > > free individual buffers, but the whole queue only.
> > >
> > > In the case of the encoder, a V4L2 driver is not supposed to let
> > > user-space dequeue an input frame while it is used as reference for
> > > the encoding process. So if we add a similar rule in the virtio-video
> > > specification, I suppose this would solve the problem?
> > >
> > > For the decoder case, we have a bigger problem though. Since DMABUFs
> > > can be arbitrarily attached to any V4L2 buffer ID, we may end up
> > > re-queueing the DMABUF of a decoded frame that is still used as
> > > reference under a different V4L2 buffer ID. In this case I don't think
> > > the driver has a way to know that the memory resource should not be
> > > overwritten, and it will thus happily use it as a decode target. The
> > > easiest fix is probably to update the V4L2 stateful specification to
> > > require that reused DMABUFs must always be assigned to the same V4L2
> > > buffer ID, and must be kept alive as long as decoding is in progress,
> > > or until a resolution change event is received. We can then apply a
> > > similar rule to the virtio device.
> >
> >
> > Sounds like a generic kind of problem - how do other devices solve it?
>
> Most users of V4L2 drivers use MMAP buffers, which don't suffer from
> this problem: since MMAP buffers are managed by V4L2 and the same V4L2
> buffer ID always corresponds to the same backing memory, the driver
> just needs to refrain from decoding into a given V4L2 buffer as long
> as it is used as a reference frames. This is something that all
> drivers currently do AFAICT.
>
> The problem only occurs if you let userspace map anything to V4L2
> buffers (USERPTR or DMABUF buffers). In order to guarantee the same
> reliable behavior as with MMAP buffers, you must enforce the same
> rule: always the same backing memory for a given V4L2 buffer.
>
> ... or you can rotate between a large enough number of buffers to
> leave enough space for the reference tag to expire on your frames, but
> that's clearly not a good way to address the problem.

FWIW, it's typically solved with regular devices by completely
disallowing the DMABUF/USERPTR modes and only allowing the
V4L2-managed MMAP mode for affected buffer queues.

However, that's quite a severe limitation and with a careful API
extension, DMABUF could be still handled. Namely:
 - pre-registration of buffers at initialization time: that would
likely mean mandating VIDIOC_QBUF for all indexes before any
decoding/encoding can start,
 - enforcement of index-buffer mapping: VIDIOC_QBUF would have to fail
if one attempts to queue another buffer at the same index,
 - ability to explicitly release existing buffers: there is
VIDIOC_RELEASE_BUF in the works which could be used to release a
specific index,
 - ability to explicitly add new buffers: a combination of
VIDIOC_CREATEBUFS + VIDIOC_QBUF could be already used to achieve this.

Best regards,
Tomasz
  

Hi Alexandre, Tomasz,

Thank  you very much for your feedback.

Yes, unfortunately we cannot disable dmabuf mode as it is currently mandatory 
for Android.

AFAIU the work to have this ready in the main v4l2 spec requires time. But in 
the virtio-video spec we indeed can mention something like that the device 
does not expected the backing memory for an existing resource id to change.

Best regards,
Dmitry.

On Freitag, 3. Juli 2020 11:55:29 CEST Tomasz Figa wrote:
> On Fri, Jul 3, 2020 at 11:27 AM Alexandre Courbot <acourbot@chromium.org> 
wrote:
> > On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> > > > Hi Dmitry,
> > > > 
> > > > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp 
<dmitry.sepp@opensynergy.com> wrote:
> > > > > Hi Keiichi,
> > > > > 
> > > > > Thanks for the clarification. I believe we should explicitly
> > > > > describe this in the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And
> > > > > I also still see a problem there. If it is a guest allocated
> > > > > resource, we cannot consider it to be free until the device really
> > > > > releases it. And it won't happen until we issue the next ATTACH
> > > > > call. Also, as we discussed before, it might be not possible to
> > > > > free individual buffers, but the whole queue only.
> > > > 
> > > > In the case of the encoder, a V4L2 driver is not supposed to let
> > > > user-space dequeue an input frame while it is used as reference for
> > > > the encoding process. So if we add a similar rule in the virtio-video
> > > > specification, I suppose this would solve the problem?
> > > > 
> > > > For the decoder case, we have a bigger problem though. Since DMABUFs
> > > > can be arbitrarily attached to any V4L2 buffer ID, we may end up
> > > > re-queueing the DMABUF of a decoded frame that is still used as
> > > > reference under a different V4L2 buffer ID. In this case I don't think
> > > > the driver has a way to know that the memory resource should not be
> > > > overwritten, and it will thus happily use it as a decode target. The
> > > > easiest fix is probably to update the V4L2 stateful specification to
> > > > require that reused DMABUFs must always be assigned to the same V4L2
> > > > buffer ID, and must be kept alive as long as decoding is in progress,
> > > > or until a resolution change event is received. We can then apply a
> > > > similar rule to the virtio device.
> > > 
> > > Sounds like a generic kind of problem - how do other devices solve it?
> > 
> > Most users of V4L2 drivers use MMAP buffers, which don't suffer from
> > this problem: since MMAP buffers are managed by V4L2 and the same V4L2
> > buffer ID always corresponds to the same backing memory, the driver
> > just needs to refrain from decoding into a given V4L2 buffer as long
> > as it is used as a reference frames. This is something that all
> > drivers currently do AFAICT.
> > 
> > The problem only occurs if you let userspace map anything to V4L2
> > buffers (USERPTR or DMABUF buffers). In order to guarantee the same
> > reliable behavior as with MMAP buffers, you must enforce the same
> > rule: always the same backing memory for a given V4L2 buffer.
> > 
> > ... or you can rotate between a large enough number of buffers to
> > leave enough space for the reference tag to expire on your frames, but
> > that's clearly not a good way to address the problem.
> 
> FWIW, it's typically solved with regular devices by completely
> disallowing the DMABUF/USERPTR modes and only allowing the
> V4L2-managed MMAP mode for affected buffer queues.
> 
> However, that's quite a severe limitation and with a careful API
> extension, DMABUF could be still handled. Namely:
>  - pre-registration of buffers at initialization time: that would
> likely mean mandating VIDIOC_QBUF for all indexes before any
> decoding/encoding can start,
>  - enforcement of index-buffer mapping: VIDIOC_QBUF would have to fail
> if one attempts to queue another buffer at the same index,
>  - ability to explicitly release existing buffers: there is
> VIDIOC_RELEASE_BUF in the works which could be used to release a
> specific index,
>  - ability to explicitly add new buffers: a combination of
> VIDIOC_CREATEBUFS + VIDIOC_QBUF could be already used to achieve this.
> 
> Best regards,
> Tomasz
  

On Fri, Jul 3, 2020 at 6:55 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Fri, Jul 3, 2020 at 11:27 AM Alexandre Courbot <acourbot@chromium.org> wrote:
> >
> > On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> > > > >
> > > > > Hi Keiichi,
> > > > >
> > > > > Thanks for the clarification. I believe we should explicitly describe this in
> > > > > the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> > > > > there. If it is a guest allocated resource, we cannot consider it to be free
> > > > > until the device really releases it. And it won't happen until we issue the
> > > > > next ATTACH call. Also, as we discussed before, it might be not possible to
> > > > > free individual buffers, but the whole queue only.
> > > >
> > > > In the case of the encoder, a V4L2 driver is not supposed to let
> > > > user-space dequeue an input frame while it is used as reference for
> > > > the encoding process. So if we add a similar rule in the virtio-video
> > > > specification, I suppose this would solve the problem?
> > > >
> > > > For the decoder case, we have a bigger problem though. Since DMABUFs
> > > > can be arbitrarily attached to any V4L2 buffer ID, we may end up
> > > > re-queueing the DMABUF of a decoded frame that is still used as
> > > > reference under a different V4L2 buffer ID. In this case I don't think
> > > > the driver has a way to know that the memory resource should not be
> > > > overwritten, and it will thus happily use it as a decode target. The
> > > > easiest fix is probably to update the V4L2 stateful specification to
> > > > require that reused DMABUFs must always be assigned to the same V4L2
> > > > buffer ID, and must be kept alive as long as decoding is in progress,
> > > > or until a resolution change event is received. We can then apply a
> > > > similar rule to the virtio device.
> > >
> > >
> > > Sounds like a generic kind of problem - how do other devices solve it?
> >
> > Most users of V4L2 drivers use MMAP buffers, which don't suffer from
> > this problem: since MMAP buffers are managed by V4L2 and the same V4L2
> > buffer ID always corresponds to the same backing memory, the driver
> > just needs to refrain from decoding into a given V4L2 buffer as long
> > as it is used as a reference frames. This is something that all
> > drivers currently do AFAICT.
> >
> > The problem only occurs if you let userspace map anything to V4L2
> > buffers (USERPTR or DMABUF buffers). In order to guarantee the same
> > reliable behavior as with MMAP buffers, you must enforce the same
> > rule: always the same backing memory for a given V4L2 buffer.
> >
> > ... or you can rotate between a large enough number of buffers to
> > leave enough space for the reference tag to expire on your frames, but
> > that's clearly not a good way to address the problem.
>
> FWIW, it's typically solved with regular devices by completely
> disallowing the DMABUF/USERPTR modes and only allowing the
> V4L2-managed MMAP mode for affected buffer queues.
>
> However, that's quite a severe limitation and with a careful API
> extension, DMABUF could be still handled. Namely:
>  - pre-registration of buffers at initialization time: that would
> likely mean mandating VIDIOC_QBUF for all indexes before any
> decoding/encoding can start,

Can't we get around this by just requiring that DMABUFs passed to
VIDIOC_QBUFs are always the same for a given index? Why would it be
necessary to require all buffers to be queued before starting the
codec?

>  - enforcement of index-buffer mapping: VIDIOC_QBUF would have to fail
> if one attempts to queue another buffer at the same index,
>  - ability to explicitly release existing buffers: there is
> VIDIOC_RELEASE_BUF in the works which could be used to release a
> specific index,
>  - ability to explicitly add new buffers: a combination of
> VIDIOC_CREATEBUFS + VIDIOC_QBUF could be already used to achieve this.

Even without these I guess we can probably get something working at
the cost of higher restrictions to clients (i.e. purely static set of
buffers).

>
> Best regards,
> Tomasz
  

On Wed, Jul 8, 2020 at 6:35 AM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Fri, Jul 3, 2020 at 6:55 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Fri, Jul 3, 2020 at 11:27 AM Alexandre Courbot <acourbot@chromium.org> wrote:
> > >
> > > On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> > > > > >
> > > > > > Hi Keiichi,
> > > > > >
> > > > > > Thanks for the clarification. I believe we should explicitly describe this in
> > > > > > the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And I also still see a problem
> > > > > > there. If it is a guest allocated resource, we cannot consider it to be free
> > > > > > until the device really releases it. And it won't happen until we issue the
> > > > > > next ATTACH call. Also, as we discussed before, it might be not possible to
> > > > > > free individual buffers, but the whole queue only.
> > > > >
> > > > > In the case of the encoder, a V4L2 driver is not supposed to let
> > > > > user-space dequeue an input frame while it is used as reference for
> > > > > the encoding process. So if we add a similar rule in the virtio-video
> > > > > specification, I suppose this would solve the problem?
> > > > >
> > > > > For the decoder case, we have a bigger problem though. Since DMABUFs
> > > > > can be arbitrarily attached to any V4L2 buffer ID, we may end up
> > > > > re-queueing the DMABUF of a decoded frame that is still used as
> > > > > reference under a different V4L2 buffer ID. In this case I don't think
> > > > > the driver has a way to know that the memory resource should not be
> > > > > overwritten, and it will thus happily use it as a decode target. The
> > > > > easiest fix is probably to update the V4L2 stateful specification to
> > > > > require that reused DMABUFs must always be assigned to the same V4L2
> > > > > buffer ID, and must be kept alive as long as decoding is in progress,
> > > > > or until a resolution change event is received. We can then apply a
> > > > > similar rule to the virtio device.
> > > >
> > > >
> > > > Sounds like a generic kind of problem - how do other devices solve it?
> > >
> > > Most users of V4L2 drivers use MMAP buffers, which don't suffer from
> > > this problem: since MMAP buffers are managed by V4L2 and the same V4L2
> > > buffer ID always corresponds to the same backing memory, the driver
> > > just needs to refrain from decoding into a given V4L2 buffer as long
> > > as it is used as a reference frames. This is something that all
> > > drivers currently do AFAICT.
> > >
> > > The problem only occurs if you let userspace map anything to V4L2
> > > buffers (USERPTR or DMABUF buffers). In order to guarantee the same
> > > reliable behavior as with MMAP buffers, you must enforce the same
> > > rule: always the same backing memory for a given V4L2 buffer.
> > >
> > > ... or you can rotate between a large enough number of buffers to
> > > leave enough space for the reference tag to expire on your frames, but
> > > that's clearly not a good way to address the problem.
> >
> > FWIW, it's typically solved with regular devices by completely
> > disallowing the DMABUF/USERPTR modes and only allowing the
> > V4L2-managed MMAP mode for affected buffer queues.
> >
> > However, that's quite a severe limitation and with a careful API
> > extension, DMABUF could be still handled. Namely:
> >  - pre-registration of buffers at initialization time: that would
> > likely mean mandating VIDIOC_QBUF for all indexes before any
> > decoding/encoding can start,
>
> Can't we get around this by just requiring that DMABUFs passed to
> VIDIOC_QBUFs are always the same for a given index? Why would it be
> necessary to require all buffers to be queued before starting the
> codec?
>

There are decoders (e.g. s5p-mfc) which require all the framebuffers
to be registered beforehands.

> >  - enforcement of index-buffer mapping: VIDIOC_QBUF would have to fail
> > if one attempts to queue another buffer at the same index,
> >  - ability to explicitly release existing buffers: there is
> > VIDIOC_RELEASE_BUF in the works which could be used to release a
> > specific index,
> >  - ability to explicitly add new buffers: a combination of
> > VIDIOC_CREATEBUFS + VIDIOC_QBUF could be already used to achieve this.
>
> Even without these I guess we can probably get something working at
> the cost of higher restrictions to clients (i.e. purely static set of
> buffers).

I suppose you mean without the last two?

Best regards,
Tomasz
  

Keiichi Watanabe <keiichiw@chromium.org> writes:

> 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

Hi Keiichi,

I wanted to ask what the current status of this spec was. Are you
planning to submit a new revision of the specification due or are things
fairly stable now?

We are starting to think about next steps for virtualised video as part
of Linaro's Stratos work. Specifically we are thinking about
implementing backends and getting a stack up and running which we can
use to experiment with multiple hypervisors and VM deployment
approaches.

Longer term goals included looking at how to integrate virtio-video with
a secure world on ARM (e.g. feed video data to a secure world device for
playback via virtio). As part of that we will also be looking at how to
minimise the memory profile of the backend to do this.

Looking at the virtio-spec repo it looks like the cross-device resource
sharing is now merged:

  87fa6b5 * virtio-gpu: add support for mapping/unmapping blob resources
  89e7eb5 * virtio-gpu: add resource create blob
  162578b * virtio-gpu: add the ability to export resources
  68f66ff * content: define what an exported object is

are there any other prerequisites?

From a backend implementation point of view it makes sense to wait until
there is a working frontend driver up-streamed into the kernel. I guess
that is blocked on the final call for vote on the virtio spec?

I'm sure there is scope for parallelism here but I wanted to get a sense
of the current direction before embarking on work that would require a
big re-write down the line.

Regards,
  

On 15.01.21 15:25, Keiichi Watanabe wrote:
> I think the driver implementation is necessary for the spec to be
> merged, but it's not yet clear when we can spend time implementing
> drivers. It's likely to be after April or so.
> 
> IIRC, OpenSynergy folks, who implemented the v3 driver, also had some
> plan to implement the driver with the v5 spec.
> Matti, do you have any update on it? I'd really appreciate it if we
> could keep working for upstream together.

Hey Keiichi and Alex!

Yeah, I think for us it'll also be in the March/April timeline before we
can start looking at it again, there's quite a few loose ends when going
from v3 to v5 so it'll probably take a while to get it in shape and make
sure that all the comments from the drivers v2 are properly addressed.

I'm not 100% sure how to proceed but perhaps it makes sense to jointly
iterate on the driver sources together once the v5 is mostly agreed. Any
suggestions welcome.

Cheers,

		Matti
  

Hi all,

I think the v5 should be ready for public review. It has considerably
changed compared to v3, however the changes are mostly simplifications
and addressing issues we experienced with v3 on Chrome OS, so
hopefully it's for the better. Let me do a final check before sending
it to the virtio list.

Cheers,
Alex.

On Sat, Jan 16, 2021 at 1:55 AM Matti Moell <Matti.Moell@opensynergy.com> wrote:
>
>
> On 15.01.21 15:25, Keiichi Watanabe wrote:
> > I think the driver implementation is necessary for the spec to be
> > merged, but it's not yet clear when we can spend time implementing
> > drivers. It's likely to be after April or so.
> >
> > IIRC, OpenSynergy folks, who implemented the v3 driver, also had some
> > plan to implement the driver with the v5 spec.
> > Matti, do you have any update on it? I'd really appreciate it if we
> > could keep working for upstream together.
>
> Hey Keiichi and Alex!
>
> Yeah, I think for us it'll also be in the March/April timeline before we
> can start looking at it again, there's quite a few loose ends when going
> from v3 to v5 so it'll probably take a while to get it in shape and make
> sure that all the comments from the drivers v2 are properly addressed.
>
> I'm not 100% sure how to proceed but perhaps it makes sense to jointly
> iterate on the driver sources together once the v5 is mostly agreed. Any
> suggestions welcome.
>
> Cheers,
>
>                 Matti
>