Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Tue, Jan 14, 2020 at 7:35 PM Dmitry Sepp wrote:
>
> Hi Keiichi,
>
> thank you for the update.
>
> On Dienstag, 14. Januar 2020 08:18:50 CET Keiichi Watanabe wrote:
> > Hi,
> >
> > On Thu, Jan 9, 2020 at 11:21 PM Tomasz Figa wrote:
> > > On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe
> wrote:
> > > > Hi Gerd,
> > > >
> > > > On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann
> > > > wrote:
> > > > > Hi,
> > > > >
> > > > > > > However that still doesn't let the driver know which buffers will
> > > > > > > be
> > > > > > > dequeued when. A simple example of this scenario is when the guest
> > > > > > > is
> > > > > > > done displaying a frame and requeues the buffer back to the
> > > > > > > decoder.
> > > > > > > Then the decoder will not choose it for decoding next frames into
> > > > > > > as
> > > > > > > long as the frame in that buffer is still used as a reference
> > > > > > > frame,
> > > > > > > even if one sends the drain request.
> > > > > >
> > > > > > It might be that I'm getting your point wrong, but do you mean some
> > > > > > hardware can mark a buffer as ready to be displayed yet still using
> > > > > > the underlying memory to decode other frames?
> > > > >
> > > > > Yes, this is how I understand Tomasz Figa.
> > > > >
> > > > > > This means, if you occasionally/intentionally
> > > > > > write to the buffer you mess up the whole decoding pipeline.
> > > > >
> > > > > And to avoid this the buffer handling aspect must be clarified in the
> > > > > specification. Is the device allowed to continue using the buffer
> > > > > after
> > > > > finishing decoding and completing the queue request? If so, how do we
> > > > > hand over buffer ownership back to the driver so it can free the
> > > > > pages?
> > > > > drain request? How do we handle re-using buffers? Can the driver
> > > > > simply re-queue them and expect the device figures by itself whenever
> > > > > it
> > > > > can use the buffer or whenever it is still needed as reference frame?
> > > >
> > > > The device shouldn't be able to access buffers after it completes a
> > > > queue request.
> > > > The device can touch buffer contents from when a queue request is sent
> > > > until the device responds it.
> > > > In contrast, the driver must not modify buffer contents in that period.
> > > >
> > > > Regarding re-using, the driver can simply re-queue buffers returned by
> > > > the device. If the device needs a buffer as reference frame, it must
> > > > not return the buffer.
> > >
> > > I think that might not be what we expect. We want the decoder to
> > > return a decoded frame as soon as possible, but that decoded frame may
> > > be also needed for decoding next frames. In V4L2 stateful decoder, the
> > > API is defined that the client must not modify the decoded
> > > framebuffer, otherwise decoding next frames may not be correct.
> >
> > Thanks Tomasz! I didn't know the V4L2's convention.
> > So, the host should be able to read a frame buffer after it is
> > returned by responding RESOURCE_QUEUE command.
> >
> > > We may
> > > need something similar, with an explicit operation that makes the
> > > buffers not accessible to the host anymore. I think the queue flush
> > > operation could work as such.
> >
> > After offline discussion with Tomasz, I suspect the queue flush
> > operation (= VIRTIO_VIDEO_CMD_QUEUE_CLEAR) shouldn't work so, as it
> > just forces pending QUEUE requests to be backed for video seek.
> > So, a buffer can be readable from the device once it's queued until
> > STREAM_DESTROY or RESOURCE_DESTROY is called.
>
> Speaking of v4l2, drivers usually get all buffers back on stop_streaming
> (this
> means seek(decoder), reset (encoder)). As per my understanding, this means
> that the device should not read the buffers anymore after
> stop_streaming(STREAMOFF) has been called. We can mention that after
> VIRTIO_VIDEO_CMD_QUEUE_CLEAR no device access is allowed.
>
> So:
> stop_streaming() = VIRTIO_VIDEO_CMD_QUEUE_CLEAR
> REQBUFS(0) = RESOURCE_DESTROY
>
I'm afraid this may not be enough. "drivers usually get all buffers
back on stop_streaming" is more like executing DQBUF implicitly on all
queued buffers. Unfortunately it doesn't necessarily mean releasing
the buffers. AFAICT currently the only way to guarantee that the
stateful V4L2 decoder on the host wouldn't use the buffers anymore is
to call REQBUFS(CAPTURE, 0).
Best regards,
Tomasz
> >
> > In my understanding, the life cycle of video buffers is defined as
> > this state machine.
> > https://drive.google.com/file/d/1c6oY5S_9bhpJlrOt4UfoQex0CanpG-kZ/view?usp=s
> > haring
> >
> > ```
> > # The definition of the state machine in DOT language
> > digraph G {
> > graph [ rankdir = LR, layout = dot ];
> >
> > init [shape=point]
> > created [label="created", shape=circle]
> > dequeued [label="dequeued", shape=circle]
> > queued [label="queued", shape=circle]
> >
> > init -> created [label="RESOURCE_CREATE"]
> >
> > created -> queued [label="RESO
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, On Tue, Jan 14, 2020 at 7:35 PM Dmitry Sepp wrote: > > Hi Keiichi, > > thank you for the update. > > On Dienstag, 14. Januar 2020 08:18:50 CET Keiichi Watanabe wrote: > > Hi, > > > > On Thu, Jan 9, 2020 at 11:21 PM Tomasz Figa wrote: > > > On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe > wrote: > > > > Hi Gerd, > > > > > > > > On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann > > > > wrote: > > > > > Hi, > > > > > > > > > > > > However that still doesn't let the driver know which buffers will > > > > > > > be > > > > > > > dequeued when. A simple example of this scenario is when the guest > > > > > > > is > > > > > > > done displaying a frame and requeues the buffer back to the > > > > > > > decoder. > > > > > > > Then the decoder will not choose it for decoding next frames into > > > > > > > as > > > > > > > long as the frame in that buffer is still used as a reference > > > > > > > frame, > > > > > > > even if one sends the drain request. > > > > > > > > > > > > It might be that I'm getting your point wrong, but do you mean some > > > > > > hardware can mark a buffer as ready to be displayed yet still using > > > > > > the underlying memory to decode other frames? > > > > > > > > > > Yes, this is how I understand Tomasz Figa. > > > > > > > > > > > This means, if you occasionally/intentionally > > > > > > write to the buffer you mess up the whole decoding pipeline. > > > > > > > > > > And to avoid this the buffer handling aspect must be clarified in the > > > > > specification. Is the device allowed to continue using the buffer > > > > > after > > > > > finishing decoding and completing the queue request? If so, how do we > > > > > hand over buffer ownership back to the driver so it can free the > > > > > pages? > > > > > drain request? How do we handle re-using buffers? Can the driver > > > > > simply re-queue them and expect the device figures by itself whenever > > > > > it > > > > > can use the buffer or whenever it is still needed as reference frame? > > > > > > > > The device shouldn't be able to access buffers after it completes a > > > > queue request. > > > > The device can touch buffer contents from when a queue request is sent > > > > until the device responds it. > > > > In contrast, the driver must not modify buffer contents in that period. > > > > > > > > Regarding re-using, the driver can simply re-queue buffers returned by > > > > the device. If the device needs a buffer as reference frame, it must > > > > not return the buffer. > > > > > > I think that might not be what we expect. We want the decoder to > > > return a decoded frame as soon as possible, but that decoded frame may > > > be also needed for decoding next frames. In V4L2 stateful decoder, the > > > API is defined that the client must not modify the decoded > > > framebuffer, otherwise decoding next frames may not be correct. > > > > Thanks Tomasz! I didn't know the V4L2's convention. > > So, the host should be able to read a frame buffer after it is > > returned by responding RESOURCE_QUEUE command. > > > > > We may > > > need something similar, with an explicit operation that makes the > > > buffers not accessible to the host anymore. I think the queue flush > > > operation could work as such. > > > > After offline discussion with Tomasz, I suspect the queue flush > > operation (= VIRTIO_VIDEO_CMD_QUEUE_CLEAR) shouldn't work so, as it > > just forces pending QUEUE requests to be backed for video seek. > > So, a buffer can be readable from the device once it's queued until > > STREAM_DESTROY or RESOURCE_DESTROY is called. > > Speaking of v4l2, drivers usually get all buffers back on stop_streaming > (this > means seek(decoder), reset (encoder)). As per my understanding, this means > that the device should not read the buffers anymore after > stop_streaming(STREAMOFF) has been called. We can mention that after > VIRTIO_VIDEO_CMD_QUEUE_CLEAR no device access is allowed. > > So: > stop_streaming() = VIRTIO_VIDEO_CMD_QUEUE_CLEAR > REQBUFS(0) = RESOURCE_DESTROY Thank you for the explanation. It seems that the problem here is: "Should we _guarantee_ that the device won't read buffers after VIRTIO_VIDEO_CMD_QUEUE_CLEAR is called in the spec?" If we want to guarantee it and choose the way you suggested, the state machine and the table I posted were not enough to explain who can read/write which buffers. I suspect we would need to define states of streams like "on" and "off" in the spec. Then, VIRTIO_VIDEO_CMD_QUEUE_CLEAR will (1) force every pending command to being backed, and (2) change the stream state to "off". As a result, the table will become like this: state| Guest |Host --- created | Read| - queued| - | Read/Write dequeued (stream: on) | Read| Read dequeued (stream: off)| Read| - However, I doubt we really want to guarantee it going so far as introducing such more complexity. If we don't need to
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi,
On Thu, Jan 9, 2020 at 11:21 PM Tomasz Figa wrote:
>
> On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe
> wrote:
> >
> > Hi Gerd,
> >
> > On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann wrote:
> > >
> > > Hi,
> > >
> > > > > However that still doesn't let the driver know which buffers will be
> > > > > dequeued when. A simple example of this scenario is when the guest is
> > > > > done displaying a frame and requeues the buffer back to the decoder.
> > > > > Then the decoder will not choose it for decoding next frames into as
> > > > > long as the frame in that buffer is still used as a reference frame,
> > > > > even if one sends the drain request.
> > > > It might be that I'm getting your point wrong, but do you mean some
> > > > hardware
> > > > can mark a buffer as ready to be displayed yet still using the
> > > > underlying
> > > > memory to decode other frames?
> > >
> > > Yes, this is how I understand Tomasz Figa.
> > >
> > > > This means, if you occasionally/intentionally
> > > > write to the buffer you mess up the whole decoding pipeline.
> > >
> > > And to avoid this the buffer handling aspect must be clarified in the
> > > specification. Is the device allowed to continue using the buffer after
> > > finishing decoding and completing the queue request? If so, how do we
> > > hand over buffer ownership back to the driver so it can free the pages?
> > > drain request? How do we handle re-using buffers? Can the driver
> > > simply re-queue them and expect the device figures by itself whenever it
> > > can use the buffer or whenever it is still needed as reference frame?
> >
> > The device shouldn't be able to access buffers after it completes a
> > queue request.
> > The device can touch buffer contents from when a queue request is sent
> > until the device responds it.
> > In contrast, the driver must not modify buffer contents in that period.
> >
> > Regarding re-using, the driver can simply re-queue buffers returned by
> > the device. If the device needs a buffer as reference frame, it must
> > not return the buffer.
>
> I think that might not be what we expect. We want the decoder to
> return a decoded frame as soon as possible, but that decoded frame may
> be also needed for decoding next frames. In V4L2 stateful decoder, the
> API is defined that the client must not modify the decoded
> framebuffer, otherwise decoding next frames may not be correct.
Thanks Tomasz! I didn't know the V4L2's convention.
So, the host should be able to read a frame buffer after it is
returned by responding RESOURCE_QUEUE command.
> We may
> need something similar, with an explicit operation that makes the
> buffers not accessible to the host anymore. I think the queue flush
> operation could work as such.
After offline discussion with Tomasz, I suspect the queue flush
operation (= VIRTIO_VIDEO_CMD_QUEUE_CLEAR) shouldn't work so, as it
just forces pending QUEUE requests to be backed for video seek.
So, a buffer can be readable from the device once it's queued until
STREAM_DESTROY or RESOURCE_DESTROY is called.
In my understanding, the life cycle of video buffers is defined as
this state machine.
https://drive.google.com/file/d/1c6oY5S_9bhpJlrOt4UfoQex0CanpG-kZ/view?usp=sharing
```
# The definition of the state machine in DOT language
digraph G {
graph [ rankdir = LR, layout = dot ];
init [shape=point]
created [label="created", shape=circle]
dequeued [label="dequeued", shape=circle]
queued [label="queued", shape=circle]
init -> created [label="RESOURCE_CREATE"]
created -> queued [label="RESOURCE_QUEUE \n is sent"]
dequeued -> queued [label="RESOURCE_QUEUE \n is sent"]
queued -> dequeued [label="RESOURCE_QUEUE \n is returned"]
created -> init [label="DESTROY"]
dequeued -> init [label="DESTROY"]
}
```
In each state of this figure, the accessibility of each buffer should
be like the following:
# Input buffers
state | Guest|Host
---
created | Read/Write | -
queued | - | Read
dequeued | Read/Write | -
# Output buffers
state | Guest|Host
--
created | Read | -
queued | - | Read/Write
dequeued | Read | Read
Does it make sense?
If ok, I'll have this state machine and tables in the next version of
spec to describe device/driver requirements by adding a subsection
about buffer life cycle.
By the way, regarding destruction of buffers, I suspect it doesn't
make much sense to have RESOURCE_DESTROY command. Though it was
suggested as a per-buffer command, it doesn't match the existing V4L2
API, as REQBUFS(0) destroys all buffers at once. I guess per-buffer
destruction would require hardware or firmware supports.
Even if we want to destroy buffers at once, we can destroy the stream
and recreate one. So, I wonder if we can remove RESOURCE_DESTROY
command from the first version of spec at least.
What do you think?
Best regards,
Keiichi
>
> > I'll describe
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Keiichi,
thank you for the update.
On Dienstag, 14. Januar 2020 08:18:50 CET Keiichi Watanabe wrote:
> Hi,
>
> On Thu, Jan 9, 2020 at 11:21 PM Tomasz Figa wrote:
> > On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe
wrote:
> > > Hi Gerd,
> > >
> > > On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann wrote:
> > > > Hi,
> > > >
> > > > > > However that still doesn't let the driver know which buffers will
> > > > > > be
> > > > > > dequeued when. A simple example of this scenario is when the guest
> > > > > > is
> > > > > > done displaying a frame and requeues the buffer back to the
> > > > > > decoder.
> > > > > > Then the decoder will not choose it for decoding next frames into
> > > > > > as
> > > > > > long as the frame in that buffer is still used as a reference
> > > > > > frame,
> > > > > > even if one sends the drain request.
> > > > >
> > > > > It might be that I'm getting your point wrong, but do you mean some
> > > > > hardware can mark a buffer as ready to be displayed yet still using
> > > > > the underlying memory to decode other frames?
> > > >
> > > > Yes, this is how I understand Tomasz Figa.
> > > >
> > > > > This means, if you occasionally/intentionally
> > > > > write to the buffer you mess up the whole decoding pipeline.
> > > >
> > > > And to avoid this the buffer handling aspect must be clarified in the
> > > > specification. Is the device allowed to continue using the buffer
> > > > after
> > > > finishing decoding and completing the queue request? If so, how do we
> > > > hand over buffer ownership back to the driver so it can free the
> > > > pages?
> > > > drain request? How do we handle re-using buffers? Can the driver
> > > > simply re-queue them and expect the device figures by itself whenever
> > > > it
> > > > can use the buffer or whenever it is still needed as reference frame?
> > >
> > > The device shouldn't be able to access buffers after it completes a
> > > queue request.
> > > The device can touch buffer contents from when a queue request is sent
> > > until the device responds it.
> > > In contrast, the driver must not modify buffer contents in that period.
> > >
> > > Regarding re-using, the driver can simply re-queue buffers returned by
> > > the device. If the device needs a buffer as reference frame, it must
> > > not return the buffer.
> >
> > I think that might not be what we expect. We want the decoder to
> > return a decoded frame as soon as possible, but that decoded frame may
> > be also needed for decoding next frames. In V4L2 stateful decoder, the
> > API is defined that the client must not modify the decoded
> > framebuffer, otherwise decoding next frames may not be correct.
>
> Thanks Tomasz! I didn't know the V4L2's convention.
> So, the host should be able to read a frame buffer after it is
> returned by responding RESOURCE_QUEUE command.
>
> > We may
> > need something similar, with an explicit operation that makes the
> > buffers not accessible to the host anymore. I think the queue flush
> > operation could work as such.
>
> After offline discussion with Tomasz, I suspect the queue flush
> operation (= VIRTIO_VIDEO_CMD_QUEUE_CLEAR) shouldn't work so, as it
> just forces pending QUEUE requests to be backed for video seek.
> So, a buffer can be readable from the device once it's queued until
> STREAM_DESTROY or RESOURCE_DESTROY is called.
Speaking of v4l2, drivers usually get all buffers back on stop_streaming (this
means seek(decoder), reset (encoder)). As per my understanding, this means
that the device should not read the buffers anymore after
stop_streaming(STREAMOFF) has been called. We can mention that after
VIRTIO_VIDEO_CMD_QUEUE_CLEAR no device access is allowed.
So:
stop_streaming() = VIRTIO_VIDEO_CMD_QUEUE_CLEAR
REQBUFS(0) = RESOURCE_DESTROY
>
> In my understanding, the life cycle of video buffers is defined as
> this state machine.
> https://drive.google.com/file/d/1c6oY5S_9bhpJlrOt4UfoQex0CanpG-kZ/view?usp=s
> haring
>
> ```
> # The definition of the state machine in DOT language
> digraph G {
> graph [ rankdir = LR, layout = dot ];
>
> init [shape=point]
> created [label="created", shape=circle]
> dequeued [label="dequeued", shape=circle]
> queued [label="queued", shape=circle]
>
> init -> created [label="RESOURCE_CREATE"]
>
> created -> queued [label="RESOURCE_QUEUE \n is sent"]
> dequeued -> queued [label="RESOURCE_QUEUE \n is sent"]
> queued -> dequeued [label="RESOURCE_QUEUE \n is returned"]
>
> created -> init [label="DESTROY"]
> dequeued -> init [label="DESTROY"]
> }
> ```
>
> In each state of this figure, the accessibility of each buffer should
> be like the following:
>
> # Input buffers
> state | Guest|Host
> ---
> created | Read/Write | -
> queued | - | Read
> dequeued | Read/Write | -
>
> # Output buffers
> state | Guest|Host
> --
> created | Read | -
> queued
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Gerd, On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann wrote: > > Hi, > > > > However that still doesn't let the driver know which buffers will be > > > dequeued when. A simple example of this scenario is when the guest is > > > done displaying a frame and requeues the buffer back to the decoder. > > > Then the decoder will not choose it for decoding next frames into as > > > long as the frame in that buffer is still used as a reference frame, > > > even if one sends the drain request. > > It might be that I'm getting your point wrong, but do you mean some hardware > > can mark a buffer as ready to be displayed yet still using the underlying > > memory to decode other frames? > > Yes, this is how I understand Tomasz Figa. > > > This means, if you occasionally/intentionally > > write to the buffer you mess up the whole decoding pipeline. > > And to avoid this the buffer handling aspect must be clarified in the > specification. Is the device allowed to continue using the buffer after > finishing decoding and completing the queue request? If so, how do we > hand over buffer ownership back to the driver so it can free the pages? > drain request? How do we handle re-using buffers? Can the driver > simply re-queue them and expect the device figures by itself whenever it > can use the buffer or whenever it is still needed as reference frame? The device shouldn't be able to access buffers after it completes a queue request. The device can touch buffer contents from when a queue request is sent until the device responds it. In contrast, the driver must not modify buffer contents in that period. Regarding re-using, the driver can simply re-queue buffers returned by the device. If the device needs a buffer as reference frame, it must not return the buffer. I'll describe this rule in the next version of the patch. Best regards, Keiichi > > cheers, > Gerd > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > Regarding re-using, the driver can simply re-queue buffers returned by > the device. If the device needs a buffer as reference frame, it must > not return the buffer. Ok, that'll work. > I'll describe this rule in the next version of the patch. Good. You should also add a note about ordering. If the device returns the buffers as soon as they are no longer needed for decoding they might be completed out-of-order, specifically B-Frames might complete before the reference I/P frame. Is the device allowed to do that or should it complete the buffers in playback order? cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe wrote: > > Hi Gerd, > > On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann wrote: > > > > Hi, > > > > > > However that still doesn't let the driver know which buffers will be > > > > dequeued when. A simple example of this scenario is when the guest is > > > > done displaying a frame and requeues the buffer back to the decoder. > > > > Then the decoder will not choose it for decoding next frames into as > > > > long as the frame in that buffer is still used as a reference frame, > > > > even if one sends the drain request. > > > It might be that I'm getting your point wrong, but do you mean some > > > hardware > > > can mark a buffer as ready to be displayed yet still using the underlying > > > memory to decode other frames? > > > > Yes, this is how I understand Tomasz Figa. > > > > > This means, if you occasionally/intentionally > > > write to the buffer you mess up the whole decoding pipeline. > > > > And to avoid this the buffer handling aspect must be clarified in the > > specification. Is the device allowed to continue using the buffer after > > finishing decoding and completing the queue request? If so, how do we > > hand over buffer ownership back to the driver so it can free the pages? > > drain request? How do we handle re-using buffers? Can the driver > > simply re-queue them and expect the device figures by itself whenever it > > can use the buffer or whenever it is still needed as reference frame? > > The device shouldn't be able to access buffers after it completes a > queue request. > The device can touch buffer contents from when a queue request is sent > until the device responds it. > In contrast, the driver must not modify buffer contents in that period. > > Regarding re-using, the driver can simply re-queue buffers returned by > the device. If the device needs a buffer as reference frame, it must > not return the buffer. I think that might not be what we expect. We want the decoder to return a decoded frame as soon as possible, but that decoded frame may be also needed for decoding next frames. In V4L2 stateful decoder, the API is defined that the client must not modify the decoded framebuffer, otherwise decoding next frames may not be correct. We may need something similar, with an explicit operation that makes the buffers not accessible to the host anymore. I think the queue flush operation could work as such. > I'll describe this rule in the next version of the patch. > > Best regards, > Keiichi > > > > > cheers, > > Gerd > > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Wed, Jan 8, 2020 at 10:11 PM Dmitry Sepp wrote:
>
> Hi Tomasz, Keiichi,
>
> On Mittwoch, 8. Januar 2020 13:46:25 CET Tomasz Figa wrote:
> > On Wed, Jan 8, 2020 at 9:15 PM Keiichi Watanabe
> wrote:
> > > Hi Dmitry,
> > >
> > > On Wed, Jan 8, 2020 at 7:00 PM Dmitry Sepp
> wrote:
> > > > Hi Keiichi,
> > > >
> > > > On Mittwoch, 8. Januar 2020 07:59:22 CET Keiichi Watanabe wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Wed, Jan 8, 2020 at 1:50 AM Dmitry Sepp
> > > > >
> > > >
> > > > wrote:
> > > > > > Hi Keiichi,
> > > > > >
> > > > > > thanks for the updates, please see my comments below.
> > > > > >
> > > > > > On Dienstag, 7. Januar 2020 14:24:31 CET Keiichi Watanabe wrote:
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On Mon, Jan 6, 2020 at 11:59 PM Dmitry Sepp
> > > > > > >
> > > > > >
> > > > > > wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > a couple of new comments:
> > > > > > > >
> > > > > > > > On Mittwoch, 18. Dezember 2019 14:02:14 CET Keiichi Watanabe
> wrote:
> > > > > > > > > From: Dmitry Sepp
> > > > > > > > >
> > > > > > > > > The virtio video encoder device and decoder device provide
> > > > > > > > > functionalities
> > > > > > > > > to encode and decode video stream respectively.
> > > > > > > > > Though video encoder and decoder are provided as different
> > > > > > > > > devices,
> > > > > > > > > they
> > > > > > > > > use a same protocol.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Dmitry Sepp
> > > > > > > > > Signed-off-by: Keiichi Watanabe
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > content.tex | 1 +
> > > > > > > > > virtio-video.tex | 579
> > > > > > > > > +++
> > > > > > > > > 2 files changed, 580 insertions(+)
> > > > > > > > > create mode 100644 virtio-video.tex
> > > > > > > > >
> > > > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > > > index 556b373..9e56839 100644
> > > > > > > > > --- a/content.tex
> > > > > > > > > +++ b/content.tex
> > > > > > > > > @@ -5743,6 +5743,7 @@ \subsubsection{Legacy Interface: Framing
> > > > > > > > > Requirements}\label{sec:Device \input{virtio-vsock.tex}
> > > > > > > > >
> > > > > > > > > \input{virtio-fs.tex}
> > > > > > > > > \input{virtio-rpmb.tex}
> > > > > > > > >
> > > > > > > > > +\input{virtio-video.tex}
> > > > > > > > >
> > > > > > > > > \chapter{Reserved Feature Bits}\label{sec:Reserved Feature
> > > > > > > > > Bits}
> > > > > > > > >
> > > > > > > > > diff --git a/virtio-video.tex b/virtio-video.tex
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000..30e728d
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/virtio-video.tex
> > > > > > > > > @@ -0,0 +1,579 @@
> > > > > > > > > +\section{Video Device}\label{sec:Device Types / Video Device}
> > > > > > > > > +
> > > > > > > > > +The virtio video encoder device and decoder device are
> > > > > > > > > virtual
> > > > > > > > > devices
> > > > > > > > > that +supports encoding and decoding respectively. Though the
> > > > > > > > > encoder
> > > > > > > > > and the decoder +are different devices, they use the same
> > > > > > > > > protocol.
> > > > > > > > > +
> > > > > > > > > +\subsection{Device ID}\label{sec:Device Types / Video Device
> > > > > > > > > /
> > > > > > > > > Device
> > > > > > > > > ID}
> > > > > > > > > +
> > > > > > > > > +\begin{description}
> > > > > > > > > +\item[30] encoder device
> > > > > > > > > +\item[31] decoder device
> > > > > > > > > +\end{description}
> > > > > > > > > +
> > > > > > > > > +\subsection{Virtqueues}\label{sec:Device Types / Video Device
> > > > > > > > > /
> > > > > > > > > Virtqueues} +
> > > > > > > > > +\begin{description}
> > > > > > > > > +\item[0] controlq - queue for sending control commands.
> > > > > > > > > +\item[1] eventq - queue for sending events happened in the
> > > > > > > > > device.
> > > > > > > > > +\end{description}
> > > > > > > > > +
> > > > > > > > > +\subsection{Feature bits}\label{sec:Device Types / Video
> > > > > > > > > Device /
> > > > > > > > > Feature
> > > > > > > > > bits} +
> > > > > > > > > +\begin{description}
> > > > > > > > > +\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages
> > > > > > > > > can be
> > > > > > > > > used
> > > > > > > > > for
> > > > > > > > > video + buffers.
> > > > > > > > > +\end{description}
> > > > > > > > > +
> > > > > > > > > +\devicenormative{\subsubsection}{Feature bits}{Device Types /
> > > > > > > > > Video
> > > > > > > > > Device
> > > > > > > > > / Feature bits} +
> > > > > > > > > +The device MUST offer at least one of feature bits.
> > > > > > > > > +
> > > > > > > > > +\subsection{Device configuration layout}\label{sec:Device
> > > > > > > > > Types /
> > > > > > > > > Video
> > > > > > > > > Device / Device configuration layout} +
> > > > > > > > > +Video device configuration uses the following layout
> > > > > > > > > structure:
> > > > > > > > > +
> > > > > > > > > +\begin{lstlisting}
> > >
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Gerd, Thank you so much for the review. I'm sorry for not replying earlier. On Thu, Dec 19, 2019 at 10:02 PM Gerd Hoffmann wrote: > > Hi, > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > the response for a queue request? When it finished decoding (i.e. frame > > > is ready for playback), or when it doesn't need the buffer any more for > > > decoding (i.e. buffer can be re-queued or pages can be released)? The answer is "when it doesn't need the buffer any more for decoding". The device can access buffer contents from when a queue request is sent until the device responds it. So, the device must not responds a queue request before finishing all process that requires the buffer content. Actually, the first one "When it finished decoding (i.e. frame is ready for playback)" doesn't make much sense, as it's not necessary to have a one-to-one correspondence between an input bitstream buffer and a decoded frame. It's okay to decode one input buffer contains bitstream data for two frames. Also, a user can pass bitstream for one frame as two input buffers. I'll document it in the spec. Best regards, Keiichi > > In my eyes the both statements mean almost the same and both are valid. > > Well, no. When the device decoded a P-Frame it can notify the device, > saying "here is your decoded frame". But the device might still need > the buffer with the decoded frame to properly decode the following B/I > Frames which reference the P-Frame. > > cheers, > Gerd > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > We also see the need to add a max_streams value to this structure so as to > > explicitly provide a limit on the number of streams the guest can create. > > What would be the advantage over just trying to create one and > failing? The maximum number would be only meaningful for the special > case when the streams are always only created by one user space > process. Otherwise, if several processes do that, they are not aware > of each other and the number could be higher than they can actually > create, because other processes could have some streams created > already. Also the number of streams might not be fixed but depend on stream parameters, i.e. hardware can decode one hd or two sd streams ... cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Mon, Dec 30, 2019 at 9:16 PM Dmitry Sepp wrote:
>
>
> On Donnerstag, 19. Dezember 2019 14:28:23 CET Dmitry Sepp wrote:
> > Hi Keiichi,
> >
> > Thank you for the update. Please see some comments below.
> >
> > Also, we need to bring the virtio_video_control back as it is in fact used
> > by the driver to enumerate supported encoder controls. But yes, it still
> > needs to be documemnted, it's true.
> >
> > On Mittwoch, 18. Dezember 2019 14:02:14 CET Keiichi Watanabe wrote:
> > > From: Dmitry Sepp
> > >
> > > The virtio video encoder device and decoder device provide functionalities
> > > to encode and decode video stream respectively.
> > > Though video encoder and decoder are provided as different devices, they
> > > use a same protocol.
> > >
> > > Signed-off-by: Dmitry Sepp
> > > Signed-off-by: Keiichi Watanabe
> > > ---
> > >
> > > content.tex | 1 +
> > > virtio-video.tex | 579 +++
> > > 2 files changed, 580 insertions(+)
> > > create mode 100644 virtio-video.tex
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 556b373..9e56839 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -5743,6 +5743,7 @@ \subsubsection{Legacy Interface: Framing
> > > Requirements}\label{sec:Device \input{virtio-vsock.tex}
> > >
> > > \input{virtio-fs.tex}
> > > \input{virtio-rpmb.tex}
> > >
> > > +\input{virtio-video.tex}
> > >
> > > \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >
> > > diff --git a/virtio-video.tex b/virtio-video.tex
> > > new file mode 100644
> > > index 000..30e728d
> > > --- /dev/null
> > > +++ b/virtio-video.tex
> > > @@ -0,0 +1,579 @@
> > > +\section{Video Device}\label{sec:Device Types / Video Device}
> > > +
> > > +The virtio video encoder device and decoder device are virtual devices
> > > that +supports encoding and decoding respectively. Though the encoder and
> > > the decoder +are different devices, they use the same protocol.
> > > +
> > > +\subsection{Device ID}\label{sec:Device Types / Video Device / Device ID}
> > > +
> > > +\begin{description}
> > > +\item[30] encoder device
> > > +\item[31] decoder device
> > > +\end{description}
> > > +
> > > +\subsection{Virtqueues}\label{sec:Device Types / Video Device /
> > > Virtqueues} +
> > > +\begin{description}
> > > +\item[0] controlq - queue for sending control commands.
> > > +\item[1] eventq - queue for sending events happened in the device.
> > > +\end{description}
> > > +
> > > +\subsection{Feature bits}\label{sec:Device Types / Video Device / Feature
> > > bits} +
> > > +\begin{description}
> > > +\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages can be used
> > > for
> > > video + buffers.
> > > +\end{description}
> > > +
> > > +\devicenormative{\subsubsection}{Feature bits}{Device Types / Video
> > > Device
> > > / Feature bits} +
> > > +The device MUST offer at least one of feature bits.
> > > +
> > > +\subsection{Device configuration layout}\label{sec:Device Types / Video
> > > Device / Device configuration layout} +
> > > +Video device configuration uses the following layout structure:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_video_config {
> > > +le32 max_cap_len;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +\item[\field{max_cap_len}] defines the maximum length of a descriptor
> > > + required to call VIRTIO_VIDEO_GET_CAPABILITY in bytes. The device
> > > + MUST set this value.
> > > +\end{description}
> > > +
> > > +\subsection{Device Initialization}\label{sec:Device Types / Video Device
> > > /
> > > Device Initialization} +
> > > +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> > > Video Device / Device Initialization} +
> > > +The driver SHOULD query device capability by using the
> > > +VIRTIO_VIDEO_T_GET_CAPABILITY and use that information for the initial
> > > +setup.
> > > +
> > > +\subsection{Device Operation}\label{sec:Device Types / Video Device /
> > > Device Operation} +
> > > +The driver allocates input and output buffers and queues the buffers
> > > +to the device. The device performs operations on the buffers according
> > > +to the function in question.
> > > +
> > > +\subsubsection{Device Operation: Create stream}
> > > +
> > > +To process buffers, the device needs to associate them with a certain
> > > +video stream (essentially, a context). Streams are created by
> > > +VIRTIO_VIDEO_T_STREAM_CREATE with a default set of parameters
> > > +determined by the device.
> > > +
> > > +\subsubsection{Device Operation: Create buffers}
> > > +
> > > +Buffers are used to store the actual data as well as the relevant
> > > +metadata. Scatter lists are supported, so the buffer doesn't need to
> > > +be contiguous in guest physical memory.
> > > +
> > > +\begin{itemize*}
> > > +\item Use VIRTIO_VIDEO_T_RESOURCE_CREATE to create a virtio video
> > > + resource that is backed by a buffer allocated from the driver's
> > > + mem
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, On Montag, 6. Januar 2020 09:33:35 CET Gerd Hoffmann wrote: > Hi, > > > > We also see the need to add a max_streams value to this structure so as > > > to > > > explicitly provide a limit on the number of streams the guest can > > > create. > > > > What would be the advantage over just trying to create one and > > failing? The maximum number would be only meaningful for the special > > case when the streams are always only created by one user space > > process. Otherwise, if several processes do that, they are not aware > > of each other and the number could be higher than they can actually > > create, because other processes could have some streams created > > already. > > Also the number of streams might not be fixed but depend on stream > parameters, i.e. hardware can decode one hd or two sd streams ... > Ok, you are right. We'd better return an error from the device side. Regards, Dmitry. > cheers, > Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Donnerstag, 19. Dezember 2019 14:28:23 CET Dmitry Sepp wrote:
> Hi Keiichi,
>
> Thank you for the update. Please see some comments below.
>
> Also, we need to bring the virtio_video_control back as it is in fact used
> by the driver to enumerate supported encoder controls. But yes, it still
> needs to be documemnted, it's true.
>
> On Mittwoch, 18. Dezember 2019 14:02:14 CET Keiichi Watanabe wrote:
> > From: Dmitry Sepp
> >
> > The virtio video encoder device and decoder device provide functionalities
> > to encode and decode video stream respectively.
> > Though video encoder and decoder are provided as different devices, they
> > use a same protocol.
> >
> > Signed-off-by: Dmitry Sepp
> > Signed-off-by: Keiichi Watanabe
> > ---
> >
> > content.tex | 1 +
> > virtio-video.tex | 579 +++
> > 2 files changed, 580 insertions(+)
> > create mode 100644 virtio-video.tex
> >
> > diff --git a/content.tex b/content.tex
> > index 556b373..9e56839 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5743,6 +5743,7 @@ \subsubsection{Legacy Interface: Framing
> > Requirements}\label{sec:Device \input{virtio-vsock.tex}
> >
> > \input{virtio-fs.tex}
> > \input{virtio-rpmb.tex}
> >
> > +\input{virtio-video.tex}
> >
> > \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >
> > diff --git a/virtio-video.tex b/virtio-video.tex
> > new file mode 100644
> > index 000..30e728d
> > --- /dev/null
> > +++ b/virtio-video.tex
> > @@ -0,0 +1,579 @@
> > +\section{Video Device}\label{sec:Device Types / Video Device}
> > +
> > +The virtio video encoder device and decoder device are virtual devices
> > that +supports encoding and decoding respectively. Though the encoder and
> > the decoder +are different devices, they use the same protocol.
> > +
> > +\subsection{Device ID}\label{sec:Device Types / Video Device / Device ID}
> > +
> > +\begin{description}
> > +\item[30] encoder device
> > +\item[31] decoder device
> > +\end{description}
> > +
> > +\subsection{Virtqueues}\label{sec:Device Types / Video Device /
> > Virtqueues} +
> > +\begin{description}
> > +\item[0] controlq - queue for sending control commands.
> > +\item[1] eventq - queue for sending events happened in the device.
> > +\end{description}
> > +
> > +\subsection{Feature bits}\label{sec:Device Types / Video Device / Feature
> > bits} +
> > +\begin{description}
> > +\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages can be used
> > for
> > video + buffers.
> > +\end{description}
> > +
> > +\devicenormative{\subsubsection}{Feature bits}{Device Types / Video
> > Device
> > / Feature bits} +
> > +The device MUST offer at least one of feature bits.
> > +
> > +\subsection{Device configuration layout}\label{sec:Device Types / Video
> > Device / Device configuration layout} +
> > +Video device configuration uses the following layout structure:
> > +
> > +\begin{lstlisting}
> > +struct virtio_video_config {
> > +le32 max_cap_len;
> > +};
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > +\item[\field{max_cap_len}] defines the maximum length of a descriptor
> > + required to call VIRTIO_VIDEO_GET_CAPABILITY in bytes. The device
> > + MUST set this value.
> > +\end{description}
> > +
> > +\subsection{Device Initialization}\label{sec:Device Types / Video Device
> > /
> > Device Initialization} +
> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> > Video Device / Device Initialization} +
> > +The driver SHOULD query device capability by using the
> > +VIRTIO_VIDEO_T_GET_CAPABILITY and use that information for the initial
> > +setup.
> > +
> > +\subsection{Device Operation}\label{sec:Device Types / Video Device /
> > Device Operation} +
> > +The driver allocates input and output buffers and queues the buffers
> > +to the device. The device performs operations on the buffers according
> > +to the function in question.
> > +
> > +\subsubsection{Device Operation: Create stream}
> > +
> > +To process buffers, the device needs to associate them with a certain
> > +video stream (essentially, a context). Streams are created by
> > +VIRTIO_VIDEO_T_STREAM_CREATE with a default set of parameters
> > +determined by the device.
> > +
> > +\subsubsection{Device Operation: Create buffers}
> > +
> > +Buffers are used to store the actual data as well as the relevant
> > +metadata. Scatter lists are supported, so the buffer doesn't need to
> > +be contiguous in guest physical memory.
> > +
> > +\begin{itemize*}
> > +\item Use VIRTIO_VIDEO_T_RESOURCE_CREATE to create a virtio video
> > + resource that is backed by a buffer allocated from the driver's
> > + memory.
> > +\item Use VIRTIO_VIDEO_T_RESOURCE_DESTROY to destroy a resource that
> > + is no longer needed.
> > +\end{itemize*}
> > +
> > +\subsubsection{Device Operation: Stream parameter control}
> > +
> > +\begin{itemize*}
> > +\item Use VIRTIO_VIDEO_T_GET_PARAMS to get the current stream param
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > However that still doesn't let the driver know which buffers will be > > dequeued when. A simple example of this scenario is when the guest is > > done displaying a frame and requeues the buffer back to the decoder. > > Then the decoder will not choose it for decoding next frames into as > > long as the frame in that buffer is still used as a reference frame, > > even if one sends the drain request. > It might be that I'm getting your point wrong, but do you mean some hardware > can mark a buffer as ready to be displayed yet still using the underlying > memory to decode other frames? Yes, this is how I understand Tomasz Figa. > This means, if you occasionally/intentionally > write to the buffer you mess up the whole decoding pipeline. And to avoid this the buffer handling aspect must be clarified in the specification. Is the device allowed to continue using the buffer after finishing decoding and completing the queue request? If so, how do we hand over buffer ownership back to the driver so it can free the pages? drain request? How do we handle re-using buffers? Can the driver simply re-queue them and expect the device figures by itself whenever it can use the buffer or whenever it is still needed as reference frame? cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Thu, Dec 19, 2019 at 7:55 PM Dmitry Sepp wrote: > > Hi Tomasz, > > On Donnerstag, 19. Dezember 2019 10:59:02 CET Tomasz Figa wrote: > > On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp > wrote: > > > Hi, > > > > > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann > wrote: > > > > > > Hi, > > > > > > > > > > > > > +The device MUST mark the last buffer with the > > > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > > > +sequence. > > > > > > > > > > > > No, that would build a race condition into the protocol. The device > > > > > > could complete the last buffer after the driver has sent the drain > > > > > > command but before the device saw it. So the flag would not be > > > > > > reliable. > > > > > > > > > > > > I also can't see why the flag is needed in the first place. The > > > > > > driver > > > > > > should know which buffers are queued still and be able to figure > > > > > > whenever the drain is complete or not without depending on that > > > > > > flag. > > > > > > So I'd suggest to simply drop it. > > > > > > > > > > Unfortunately video decoders are not that simple. There are always > > > > > going to be some buffers on the decoder side used as reference frames. > > > > > Only the decoder knows when to release them, as it continues decoding > > > > > the stream. > > > > > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > > the response for a queue request? When it finished decoding (i.e. frame > > > > is ready for playback), or when it doesn't need the buffer any more for > > > > decoding (i.e. buffer can be re-queued or pages can be released)? > > > > > > In my eyes the both statements mean almost the same and both are valid. I > > > think whatever underlying libraries are used for decoding on the device > > > side, they simply won't return us the buffer as long as the HW device > > > needs them to continue its normal operation. So your first sentence > > > applies to output buffers, the second - to input buffers. > > > > > > My understanding is as follows: we send the response for a queue request > > > as > > > soon as the HW device on the host side passes the buffer ownership back to > > > the client (like when VIDIOC_DQBUF has returned a buffer). > > > > That's how it's defined in V4L2 and what makes the most sense from the > > video decoding point of view, as one wants to display frames as soon > > as they are available. > > > > However that still doesn't let the driver know which buffers will be > > dequeued when. A simple example of this scenario is when the guest is > > done displaying a frame and requeues the buffer back to the decoder. > > Then the decoder will not choose it for decoding next frames into as > > long as the frame in that buffer is still used as a reference frame, > > even if one sends the drain request. > It might be that I'm getting your point wrong, but do you mean some hardware > can mark a buffer as ready to be displayed yet still using the underlying > memory to decode other frames? This means, if you occasionally/intentionally > write to the buffer you mess up the whole decoding pipeline. That would be > strange at least... That's correct. It depends on the hardware, but in principle we don't want to copy the frames decoded to temporary buffers for using them as reference frames, as that would waste bandwidth and increase latency. The contract between the kernel and the application is that it must not write to the frame buffers if it wants to get correct decoding results. But after all, I don't see a reason why the application would write to those buffers. Best regards, Tomasz ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, > > Not clearly defined in the spec: When is the decoder supposed to send > > the response for a queue request? When it finished decoding (i.e. frame > > is ready for playback), or when it doesn't need the buffer any more for > > decoding (i.e. buffer can be re-queued or pages can be released)? > In my eyes the both statements mean almost the same and both are valid. Well, no. When the device decoded a P-Frame it can notify the device, saying "here is your decoded frame". But the device might still need the buffer with the decoded frame to properly decode the following B/I Frames which reference the P-Frame. cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp wrote: > > Hi, > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > > Hi, > > > > > > > > > +The device MUST mark the last buffer with the > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > +sequence. > > > > > > > > No, that would build a race condition into the protocol. The device > > > > could complete the last buffer after the driver has sent the drain > > > > command but before the device saw it. So the flag would not be > > > > reliable. > > > > > > > > I also can't see why the flag is needed in the first place. The driver > > > > should know which buffers are queued still and be able to figure > > > > whenever the drain is complete or not without depending on that flag. > > > > So I'd suggest to simply drop it. > > > > > > Unfortunately video decoders are not that simple. There are always > > > going to be some buffers on the decoder side used as reference frames. > > > Only the decoder knows when to release them, as it continues decoding > > > the stream. > > > > Not clearly defined in the spec: When is the decoder supposed to send > > the response for a queue request? When it finished decoding (i.e. frame > > is ready for playback), or when it doesn't need the buffer any more for > > decoding (i.e. buffer can be re-queued or pages can be released)? > In my eyes the both statements mean almost the same and both are valid. I > think whatever underlying libraries are used for decoding on the device side, > they simply won't return us the buffer as long as the HW device needs them to > continue its normal operation. So your first sentence applies to output > buffers, > the second - to input buffers. > > My understanding is as follows: we send the response for a queue request as > soon as the HW device on the host side passes the buffer ownership back to the > client (like when VIDIOC_DQBUF has returned a buffer). That's how it's defined in V4L2 and what makes the most sense from the video decoding point of view, as one wants to display frames as soon as they are available. However that still doesn't let the driver know which buffers will be dequeued when. A simple example of this scenario is when the guest is done displaying a frame and requeues the buffer back to the decoder. Then the decoder will not choose it for decoding next frames into as long as the frame in that buffer is still used as a reference frame, even if one sends the drain request. > > Thanks for reviewing! > > Regards, > Dmitry. > > > > > > How we defined this in the V4L2 stateful decoder interface is that if > > > the decoder happened to return the last framebuffer before the drain > > > request arrived, it would return one more, empty. > > > > Ok. That is not clear from the spec. I've read the drain request as as > > "please stop decoding and complete all queue requests which are in the > > virtqueue, even when you didn't process them yet". In which case > > completing the last pending queue request would clearly indicate the > > draining request is complete. Also completing the drain request should > > only happen when the operation is finished. > > > > I think the various states a buffer can have and how queuing & deciding > > & draining changes these states should be clarified in the > > specification. > > > > cheers, > > Gerd > > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > > Hi, > > > > > +The device MUST mark the last buffer with the > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > +sequence. > > > > No, that would build a race condition into the protocol. The device > > could complete the last buffer after the driver has sent the drain > > command but before the device saw it. So the flag would not be > > reliable. > > > > I also can't see why the flag is needed in the first place. The driver > > should know which buffers are queued still and be able to figure > > whenever the drain is complete or not without depending on that flag. > > So I'd suggest to simply drop it. > > Unfortunately video decoders are not that simple. There are always > going to be some buffers on the decoder side used as reference frames. > Only the decoder knows when to release them, as it continues decoding > the stream. Not clearly defined in the spec: When is the decoder supposed to send the response for a queue request? When it finished decoding (i.e. frame is ready for playback), or when it doesn't need the buffer any more for decoding (i.e. buffer can be re-queued or pages can be released)? > How we defined this in the V4L2 stateful decoder interface is that if > the decoder happened to return the last framebuffer before the drain > request arrived, it would return one more, empty. Ok. That is not clear from the spec. I've read the drain request as as "please stop decoding and complete all queue requests which are in the virtqueue, even when you didn't process them yet". In which case completing the last pending queue request would clearly indicate the draining request is complete. Also completing the drain request should only happen when the operation is finished. I think the various states a buffer can have and how queuing & deciding & draining changes these states should be clarified in the specification. cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi Tomasz, On Donnerstag, 19. Dezember 2019 10:59:02 CET Tomasz Figa wrote: > On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp wrote: > > Hi, > > > > On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > > > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > > > Hi, > > > > > > > > > > > +The device MUST mark the last buffer with the > > > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > > > +sequence. > > > > > > > > > > No, that would build a race condition into the protocol. The device > > > > > could complete the last buffer after the driver has sent the drain > > > > > command but before the device saw it. So the flag would not be > > > > > reliable. > > > > > > > > > > I also can't see why the flag is needed in the first place. The > > > > > driver > > > > > should know which buffers are queued still and be able to figure > > > > > whenever the drain is complete or not without depending on that > > > > > flag. > > > > > So I'd suggest to simply drop it. > > > > > > > > Unfortunately video decoders are not that simple. There are always > > > > going to be some buffers on the decoder side used as reference frames. > > > > Only the decoder knows when to release them, as it continues decoding > > > > the stream. > > > > > > Not clearly defined in the spec: When is the decoder supposed to send > > > the response for a queue request? When it finished decoding (i.e. frame > > > is ready for playback), or when it doesn't need the buffer any more for > > > decoding (i.e. buffer can be re-queued or pages can be released)? > > > > In my eyes the both statements mean almost the same and both are valid. I > > think whatever underlying libraries are used for decoding on the device > > side, they simply won't return us the buffer as long as the HW device > > needs them to continue its normal operation. So your first sentence > > applies to output buffers, the second - to input buffers. > > > > My understanding is as follows: we send the response for a queue request > > as > > soon as the HW device on the host side passes the buffer ownership back to > > the client (like when VIDIOC_DQBUF has returned a buffer). > > That's how it's defined in V4L2 and what makes the most sense from the > video decoding point of view, as one wants to display frames as soon > as they are available. > > However that still doesn't let the driver know which buffers will be > dequeued when. A simple example of this scenario is when the guest is > done displaying a frame and requeues the buffer back to the decoder. > Then the decoder will not choose it for decoding next frames into as > long as the frame in that buffer is still used as a reference frame, > even if one sends the drain request. It might be that I'm getting your point wrong, but do you mean some hardware can mark a buffer as ready to be displayed yet still using the underlying memory to decode other frames? This means, if you occasionally/intentionally write to the buffer you mess up the whole decoding pipeline. That would be strange at least... Regds, Dmitry. > > > Thanks for reviewing! > > > > Regards, > > Dmitry. > > > > > > How we defined this in the V4L2 stateful decoder interface is that if > > > > the decoder happened to return the last framebuffer before the drain > > > > request arrived, it would return one more, empty. > > > > > > Ok. That is not clear from the spec. I've read the drain request as as > > > "please stop decoding and complete all queue requests which are in the > > > virtqueue, even when you didn't process them yet". In which case > > > completing the last pending queue request would clearly indicate the > > > draining request is complete. Also completing the drain request should > > > only happen when the operation is finished. > > > > > > I think the various states a buffer can have and how queuing & deciding > > > & draining changes these states should be clarified in the > > > specification. > > > > > > cheers, > > > > > > Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Hi, On Donnerstag, 19. Dezember 2019 08:46:39 CET Gerd Hoffmann wrote: > On Wed, Dec 18, 2019 at 11:08:37PM +0900, Tomasz Figa wrote: > > On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > > Hi, > > > > > > > +The device MUST mark the last buffer with the > > > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > > > +sequence. > > > > > > No, that would build a race condition into the protocol. The device > > > could complete the last buffer after the driver has sent the drain > > > command but before the device saw it. So the flag would not be > > > reliable. > > > > > > I also can't see why the flag is needed in the first place. The driver > > > should know which buffers are queued still and be able to figure > > > whenever the drain is complete or not without depending on that flag. > > > So I'd suggest to simply drop it. > > > > Unfortunately video decoders are not that simple. There are always > > going to be some buffers on the decoder side used as reference frames. > > Only the decoder knows when to release them, as it continues decoding > > the stream. > > Not clearly defined in the spec: When is the decoder supposed to send > the response for a queue request? When it finished decoding (i.e. frame > is ready for playback), or when it doesn't need the buffer any more for > decoding (i.e. buffer can be re-queued or pages can be released)? In my eyes the both statements mean almost the same and both are valid. I think whatever underlying libraries are used for decoding on the device side, they simply won't return us the buffer as long as the HW device needs them to continue its normal operation. So your first sentence applies to output buffers, the second - to input buffers. My understanding is as follows: we send the response for a queue request as soon as the HW device on the host side passes the buffer ownership back to the client (like when VIDIOC_DQBUF has returned a buffer). Thanks for reviewing! Regards, Dmitry. > > > How we defined this in the V4L2 stateful decoder interface is that if > > the decoder happened to return the last framebuffer before the drain > > request arrived, it would return one more, empty. > > Ok. That is not clear from the spec. I've read the drain request as as > "please stop decoding and complete all queue requests which are in the > virtqueue, even when you didn't process them yet". In which case > completing the last pending queue request would clearly indicate the > draining request is complete. Also completing the drain request should > only happen when the operation is finished. > > I think the various states a buffer can have and how queuing & deciding > & draining changes these states should be clarified in the > specification. > > cheers, > Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
On Wed, Dec 18, 2019 at 10:40 PM Gerd Hoffmann wrote: > > Hi, > > > +The device MUST mark the last buffer with the > > +VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain > > +sequence. > > No, that would build a race condition into the protocol. The device > could complete the last buffer after the driver has sent the drain > command but before the device saw it. So the flag would not be > reliable. > > I also can't see why the flag is needed in the first place. The driver > should know which buffers are queued still and be able to figure > whenever the drain is complete or not without depending on that flag. > So I'd suggest to simply drop it. Unfortunately video decoders are not that simple. There are always going to be some buffers on the decoder side used as reference frames. Only the decoder knows when to release them, as it continues decoding the stream. Moreover, with certain features of certain codecs (e.g. framebuffer reordering in H.264), there could be decoded frames that need to be held by decoder, because later bitstream may contain earlier frames. How we defined this in the V4L2 stateful decoder interface is that if the decoder happened to return the last framebuffer before the drain request arrived, it would return one more, empty. From our experience that was the easiest thing to deal with from both the driver and the application, so I believe the same should apply to the host device implementation and the guest driver. > > That is the only issue I've spotted in the protocol on a first quick > look. There are a few places where the spec text could be improved. > I'll try to set aside some time to go over this in detail, but I can't > promise I'll find the time to do that before xmas and new year. Thanks a lot for review. Best regards, Tomasz ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
