Re: [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: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
Hi, > > Well, no. Tomasz Figa had splitted the devices into three groups: > > > > (1) requires single buffer. > > (2) allows any layout (including the one (1) devices want). > > (3) requires per-plane buffers. > > > > Category (3) devices are apparently rare and old. Both category (1)+(2) > > devices can handle single buffers just fine. So maybe support only > > that? > > From the guest implementation point of view, Linux V4L2 currently > supports 2 cases, if used in allocation-mode (i.e. the buffers are > allocated locally by V4L2): > > i) single buffer with plane offsets predetermined by the format, (can > be handled by devices from category 1) and 2)) > ii) per-plane buffers with planes at the beginning of their own > buffers. (can be handled by devices from category 2) and 3)) > > Support for ii) is required if one wants to be able to import buffers > with arbitrary plane offsets, so I'd consider it unavoidable. If you have (1) hardware you simply can't import buffers with arbitrary plane offsets, so I'd expect software would prefer the single buffer layout (i) over (ii), even when using another driver + dmabuf export/import, to be able to support as much hardware as possible. So (ii) might end up being unused in practice. But maybe not, was just an idea, feel free to scratch it. cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
On Mon, Jan 13, 2020 at 8:05 PM Gerd Hoffmann wrote: > > On Mon, Jan 13, 2020 at 11:41:45AM +0100, Dmitry Sepp wrote: > > Hi Gerd, > > > > Thanks for reviewing! > > > > On Montag, 13. Januar 2020 10:56:36 CET Gerd Hoffmann wrote: > > > Hi, > > > > > > > This also means that we cannot have unspec for planes layout. Device > > > > either > > > > expects planes in separate buffers or in one buffer with some offsets, > > > > there cannot be mixed cases. > > > > > > Hmm. Is it useful to support both? Or maybe support the "one buffer + > > > offsets" case only? Splitting one buffer into multiple smaller ones > > > internally if needed shouldn't be a problem, and it would simplify the > > > interface a bit ... > > > > > > > Ok, let's consider the following case: > > - the device expects planes in separate buffers. > > - say, Android on the guest side also imports planes in separate buffers > > into the driver. > > - but he driver only supports one buffer + offsets. > > > > Do you mean the driver then can join the underlying page lists and set > > offsets then? Yes, > > this would probably make sense. > > Well, no. Tomasz Figa had splitted the devices into three groups: > > (1) requires single buffer. > (2) allows any layout (including the one (1) devices want). > (3) requires per-plane buffers. > > Category (3) devices are apparently rare and old. Both category (1)+(2) > devices can handle single buffers just fine. So maybe support only > that? >From the guest implementation point of view, Linux V4L2 currently supports 2 cases, if used in allocation-mode (i.e. the buffers are allocated locally by V4L2): i) single buffer with plane offsets predetermined by the format, (can be handled by devices from category 1) and 2)) ii) per-plane buffers with planes at the beginning of their own buffers. (can be handled by devices from category 2) and 3)) Support for ii) is required if one wants to be able to import buffers with arbitrary plane offsets, so I'd consider it unavoidable. Given that, I'd suggest going with per-plane buffer specifiers. I feel like it doesn't have much cost associated, but gives the most flexibility. Best regards, Tomasz - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support
@@ -1597,9 +1597,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio
Transport Options / Vi
}
\hline
\mmioreg{Version}{Device version number}{0x004}{R}{%
-0x2.
+0x3.
\begin{note}
- Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO
/ Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO /
Legacy interface}) used 0x1.
+ Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO
/ Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO /
Legacy interface}) used 0x1 or 0x2.
"Legacy devices" refers to pre-VIRTIO 1.0 devices. 0x2 is VIRTIO 1.0
and therefore not "Legacy". I suggest the following wording:
Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over
MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over
MMIO / Legacy interface}) used 0x1.
+ VIRTIO 1.0 and 1.1 used 0x2.
Thanks for the guide.
Did you consider using a transport feature bit instead of changing the
device version number? Feature bits allow more graceful compatibility:
old drivers will continue to work with new devices and new drivers will
continue to work with old devices.
Yes, we considered using a feature bit from 24~37 or above, while a concern
is that,
the device which uses such bit only represents the behavior of mmio
transport layer but not common behavior
of virtio device. Or am I missing some "transport" feature bit range?
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-416
Feature bit 39 is reserved and can be used.
This is similar to when the SR_IOV (37) feature bit was added for
virtio-pci.
That's great. Let's try to use bit 39 for MMIO MSI and bit 40 for MMIO
notifications capabilities.
\end{note}
}
\hline
@@ -1671,25 +1671,23 @@ \subsection{MMIO Device Register
Layout}\label{sec:Virtio Transport Options / Vi
accesses apply to the queue selected by writing to \field{QueueSel}.
}
\hline
- \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
-Writing a value to this register notifies the device that
-there are new buffers to process in a queue.
+ \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
+Reading from the register returns the virtqueue notification configuration.
-When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-the value written is the queue index.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific
Initialization And Device Operation / Notification Address}
+for the configuration format.
-When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
-the \field{Notification data} value has the following format:
+Writing when the notification address calculated by the notification
configuration
+is just located at this register.
I don't understand this sentence. What happens when the driver writes
to this register?
We're trying to define the notification mechanism that, driver MUST read
0x50 to get the notification configuration
and calculate the notify address. The writing case here is that, the notify
address is just located here e.g. notify_base=0x50, notify_mul=0.
I still don't understand what this means. It's just an English issue
and it will become clear if you can rephrase what you're saying.
Sure, let me try to explain it. :)
The different notification locations are calculated via the structure
returned by reading this register.
le32 {
notify_base : 16;
notify_multiplier : 16;
};
location=notify_base + queue_index * notify_multiplier
The location might be the same when mul=0, and furthermore, it might be
equal to 0x50 (notify_base=0x50, notify_mul=0) so we make this register
W too.
So we said, the register is RW and W is only for such scenario.
Feel free to tell me if it's still confusing.
-\lstinputlisting{notifications-le.c}
-
-See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues /
Driver notifications}
-for the definition of the components.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific
Initialization And Device Operation / Available Buffer Notifications}
+to see the notification data format.
}
\hline
\mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
Reading from this register returns a bit mask of events that
-caused the device interrupt to be asserted.
+caused the device interrupt to be asserted. This is only used
+when MSI is not enabled.
The following events are possible:
\begin{description}
\item[Used Buffer Notification] - bit 0 - the interrupt was asserted
@@ -1703,7 +1701,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio
Transport Options / Vi
\mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
Writing a value with bits set as defined in \field{InterruptStatus}
to this register notif
Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
On Mon, Jan 13, 2020 at 11:41:45AM +0100, Dmitry Sepp wrote: > Hi Gerd, > > Thanks for reviewing! > > On Montag, 13. Januar 2020 10:56:36 CET Gerd Hoffmann wrote: > > Hi, > > > > > This also means that we cannot have unspec for planes layout. Device > > > either > > > expects planes in separate buffers or in one buffer with some offsets, > > > there cannot be mixed cases. > > > > Hmm. Is it useful to support both? Or maybe support the "one buffer + > > offsets" case only? Splitting one buffer into multiple smaller ones > > internally if needed shouldn't be a problem, and it would simplify the > > interface a bit ... > > > > Ok, let's consider the following case: > - the device expects planes in separate buffers. > - say, Android on the guest side also imports planes in separate buffers > into the driver. > - but he driver only supports one buffer + offsets. > > Do you mean the driver then can join the underlying page lists and set > offsets then? Yes, > this would probably make sense. Well, no. Tomasz Figa had splitted the devices into three groups: (1) requires single buffer. (2) allows any layout (including the one (1) devices want). (3) requires per-plane buffers. Category (3) devices are apparently rare and old. Both category (1)+(2) devices can handle single buffers just fine. So maybe support only that? Hope it's more clear this time, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
Hi Gerd, Thanks for reviewing! On Montag, 13. Januar 2020 10:56:36 CET Gerd Hoffmann wrote: > Hi, > > > This also means that we cannot have unspec for planes layout. Device > > either > > expects planes in separate buffers or in one buffer with some offsets, > > there cannot be mixed cases. > > Hmm. Is it useful to support both? Or maybe support the "one buffer + > offsets" case only? Splitting one buffer into multiple smaller ones > internally if needed shouldn't be a problem, and it would simplify the > interface a bit ... > Ok, let's consider the following case: - the device expects planes in separate buffers. - say, Android on the guest side also imports planes in separate buffers into the driver. - but he driver only supports one buffer + offsets. Do you mean the driver then can join the underlying page lists and set offsets then? Yes, this would probably make sense. Regards, Dmitry. > cheers, > Gerd
Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
Hi, > This also means that we cannot have unspec for planes layout. Device either > expects planes in separate buffers or in one buffer with some offsets, there > cannot be mixed cases. Hmm. Is it useful to support both? Or maybe support the "one buffer + offsets" case only? Splitting one buffer into multiple smaller ones internally if needed shouldn't be a problem, and it would simplify the interface a bit ... cheers, Gerd - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification
Hi Tomasz, On Samstag, 11. Januar 2020 17:06:46 CET Tomasz Figa wrote: > On Sat, Jan 11, 2020 at 12:12 AM Dmitry Sepp > > wrote: > > Hi Keiichi, > > > > On Freitag, 10. Januar 2020 14:53:01 CET Keiichi Watanabe wrote: > > > Hi, > > > > > > On Fri, Jan 10, 2020 at 7:16 PM Dmitry Sepp > > > > > > > wrote: > > > > Hi, > > > > > > > > On Donnerstag, 9. Januar 2020 15:56:08 CET Dmitry Sepp wrote: > > > > > Hi, > > > > > > > > > > On Dienstag, 7. Januar 2020 11:25:56 CET Keiichi Watanabe wrote: > > > > > > Hi Dmitry, > > > > > > > > > > > > On Mon, Jan 6, 2020 at 8:28 PM Dmitry Sepp > > > > > > > > > > > > > > > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Montag, 6. Januar 2020 11:30:22 CET Keiichi Watanabe wrote: > > > > > > > > Hi Dmitry, Tomasz, > > > > > > > > > > > > > > > > On Fri, Jan 3, 2020 at 10:05 PM Dmitry Sepp > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > Hi Tomasz, Keiichi, > > > > > > > > > > > > > > > > > > On Samstag, 21. Dezember 2019 07:19:23 CET Tomasz Figa wrote: > > > > > > > > > > On Sat, Dec 21, 2019 at 3:18 PM Tomasz Figa > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hi Keiichi, > > > > > > > > > > > > > > > > > > > > > > > > > > On Mittwoch, 18. Dezember 2019 14:02:13 CET Keiichi > > > > > > > > > > > > > Watanabe > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > This is the 2nd version of virtio-video patch. The > > > > > > > > > > > > > > PDF > > > > > > > > > > > > > > is > > > > > > > > > > > > > > available > > > > > > > > > > > > > > in [1]. > > > > > > > > > > > > > > The first version was sent at [2]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Any feedback would be appreciated. Thank you. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > > > > Keiichi > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1]: > > > > > > > > > > > > > > https://drive.google.com/drive/folders/1eT5fEckBoo > > > > > > > > > > > > > > r2iH > > > > > > > > > > > > > > ZR4f > > > > > > > > > > > > > > 4G > > > > > > > > > > > > > > LxYz > > > > > > > > > > > > > > FMVa > > > > > > > > > > > > > > pOFx?us > > > > > > > > > > > > > > p=sharing [2]: > > > > > > > > > > > > > > https://markmail.org/message/gc6h25acct22niut > > > > > > > > > > > > > > > > > > > > > > > > > > > > Change log: > > > > > > > > > > > > > > > > > > > > > > > > > > > > v2: > > > > > > > > > > > > > > * Removed functionalities except encoding and > > > > > > > > > > > > > > decoding. > > > > > > > > > > > > > > * Splited encoder and decoder into different > > > > > > > > > > > > > > devices > > > > > > > > > > > > > > that > > > > > > > > > > > > > > use > > > > > > > > > > > > > > the > > > > > > > > > > > > > > same > > > > > > > > > > > > > > protocol. * Replaced GET_FUNCS with > > > > > > > > > > > > > > GET_CAPABILITY. > > > > > > > > > > > > > > * Updated structs for capabilities. > > > > > > > > > > > > > > > > > > > > > > > > > > > > - Defined new structs and enums such as image > > > > > > > > > > > > > > formats, > > > > > > > > > > > > > > profiles, > > > > > > > > > > > > > > range > > > > > > > > > > > > > > > > > > > > > > > > > > > > (min, max, step), etc > > > > > > > > > > > > > > > > > > > > > > > > > > > > * For virtio_video_pixel_format, chose a > > > > > > > > > > > > > > naming > > > > > > > > > > > > > > convention > > > > > > > > > > > > > > that > > > > > > > > > > > > > > is used > > > > > > > > > > > > > > > > > > > > > > > > > > > > in DRM. We removed XBGR, NV21 and I422, as > > > > > > > > > > > > > > they > > > > > > > > > > > > > > are > > > > > > > > > > > > > > not > > > > > > > > > > > > > > used > > > > > > > > > > > > > > in the > > > > > > > > > > > > > > current draft implementation. > > > > > > > > > > > > > > https://lwn.net/Articles/806416/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > - Removed virtio_video_control, whose usage was > > > > > > > > > > > > > > not > > > > > > > > > > > > > > documented > > > > > > > > > > > > > > yet > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > > > > > > > > > which is not necessary for the simplest decoding > > > > > > > > > > > > > > scenario. > > > > > > > > > > > > > > > > > > > > > > > > > > > > - Removed virtio_video_desc, as it is no longer > > > > > > > > > > > > > > needed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > * Updated struct virtio_video_config for changes > > > > > > > > > > > > > > around
