All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Dmitry Sepp <dmitry.sepp@opensynergy.com>
Cc: "Gerd Hoffmann" <kraxel@redhat.com>,
	"Keiichi Watanabe" <keiichiw@chromium.org>,
	virtio-dev@lists.oasis-open.org,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Alexandre Courbot" <acourbot@chromium.org>,
	"Alex Lau" <alexlau@chromium.org>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Dylan Reid" <dgreid@chromium.org>,
	"Enrico Granata" <egranata@google.com>,
	fziglio@redhat.com, "Hans Verkuil" <hverkuil@xs4all.nl>,
	"Stéphane Marchesin" <marcheu@chromium.org>,
	"Pawel Osciak" <posciak@chromium.org>,
	spice-devel@lists.freedesktop.org,
	"David Stevens" <stevensd@chromium.org>,
	uril@redhat.com
Subject: Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Date: Thu, 19 Dec 2019 18:59:02 +0900	[thread overview]
Message-ID: <CAAFQd5BgkEUwBFWdv2ZH98egjm=u0dBRgtexqkzjES+J1SEmag@mail.gmail.com> (raw)
In-Reply-To: <3550989.gzE5nMqd4t@os-lin-dmo>

On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 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 <kraxel@redhat.com> 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
>
>

WARNING: multiple messages have this Message-ID (diff)
From: Tomasz Figa <tfiga@chromium.org>
To: Dmitry Sepp <dmitry.sepp@opensynergy.com>
Cc: "Gerd Hoffmann" <kraxel@redhat.com>,
	"Keiichi Watanabe" <keiichiw@chromium.org>,
	virtio-dev@lists.oasis-open.org,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Alexandre Courbot" <acourbot@chromium.org>,
	"Alex Lau" <alexlau@chromium.org>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Dylan Reid" <dgreid@chromium.org>,
	"Enrico Granata" <egranata@google.com>,
	fziglio@redhat.com, "Hans Verkuil" <hverkuil@xs4all.nl>,
	"Stéphane Marchesin" <marcheu@chromium.org>,
	"Pawel Osciak" <posciak@chromium.org>,
	spice-devel@lists.freedesktop.org,
	"David Stevens" <stevensd@chromium.org>,
	uril@redhat.com
Subject: Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification
Date: Thu, 19 Dec 2019 18:59:02 +0900	[thread overview]
Message-ID: <CAAFQd5BgkEUwBFWdv2ZH98egjm=u0dBRgtexqkzjES+J1SEmag@mail.gmail.com> (raw)
In-Reply-To: <3550989.gzE5nMqd4t@os-lin-dmo>

On Thu, Dec 19, 2019 at 6:48 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 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 <kraxel@redhat.com> 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
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


  reply	other threads:[~2019-12-19  9:59 UTC|newest]

Thread overview: 147+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-18 13:02 [PATCH v2 0/1] VirtIO video device specification Keiichi Watanabe
2019-12-18 13:02 ` [virtio-dev] " Keiichi Watanabe
2019-12-18 13:02 ` [PATCH v2 1/1] virtio-video: Add virtio " Keiichi Watanabe
2019-12-18 13:02   ` [virtio-dev] " Keiichi Watanabe
2019-12-18 13:40   ` Gerd Hoffmann
2019-12-18 13:40     ` [virtio-dev] " Gerd Hoffmann
2019-12-18 14:08     ` Tomasz Figa
2019-12-18 14:08       ` Tomasz Figa
2019-12-19  7:46       ` Gerd Hoffmann
2019-12-19  7:46         ` Gerd Hoffmann
2019-12-19  9:48         ` Dmitry Sepp
2019-12-19  9:48           ` Dmitry Sepp
2019-12-19  9:59           ` Tomasz Figa [this message]
2019-12-19  9:59             ` Tomasz Figa
2019-12-19 10:54             ` Dmitry Sepp
2019-12-19 10:54               ` Dmitry Sepp
2019-12-19 12:05               ` Tomasz Figa
2019-12-19 12:05                 ` Tomasz Figa
2019-12-19 13:12               ` Gerd Hoffmann
2019-12-19 13:12                 ` Gerd Hoffmann
2020-01-08 13:52                 ` Keiichi Watanabe
2020-01-08 13:52                   ` Keiichi Watanabe
2020-01-09 13:40                   ` Gerd Hoffmann
2020-01-09 13:40                     ` Gerd Hoffmann
2020-01-09 14:20                   ` Tomasz Figa
2020-01-09 14:20                     ` Tomasz Figa
2020-01-14  7:18                     ` Keiichi Watanabe
2020-01-14  7:18                       ` Keiichi Watanabe
2020-01-14 10:35                       ` Dmitry Sepp
2020-01-14 10:35                         ` Dmitry Sepp
2020-01-15  7:49                         ` Keiichi Watanabe
2020-01-15  7:49                           ` Keiichi Watanabe
2020-01-15 11:12                         ` Tomasz Figa
2020-01-15 11:12                           ` Tomasz Figa
2019-12-19 13:01           ` Gerd Hoffmann
2019-12-19 13:01             ` Gerd Hoffmann
2020-01-08 13:50             ` Keiichi Watanabe
2020-01-08 13:50               ` Keiichi Watanabe
2019-12-19  9:26     ` Dmitry Sepp
2019-12-19  9:26       ` [virtio-dev] " Dmitry Sepp
2019-12-19  9:59       ` Tomasz Figa
2019-12-19  9:59         ` [virtio-dev] " Tomasz Figa
2019-12-19 12:54       ` Gerd Hoffmann
2019-12-19 12:54         ` [virtio-dev] " Gerd Hoffmann
2019-12-18 17:29   ` Frediano Ziglio
2019-12-20 14:05     ` Keiichi Watanabe
2019-12-20 14:05       ` [virtio-dev] " Keiichi Watanabe
2019-12-20 15:33       ` Dmitry Sepp
2019-12-20 15:33         ` [virtio-dev] " Dmitry Sepp
2019-12-19 13:28   ` Dmitry Sepp
2019-12-19 13:28     ` [virtio-dev] " Dmitry Sepp
2019-12-20 15:26     ` Keiichi Watanabe
2019-12-20 15:26       ` [virtio-dev] " Keiichi Watanabe
2019-12-20 15:46       ` Dmitry Sepp
2019-12-21  6:46         ` Tomasz Figa
2019-12-21  6:46           ` [virtio-dev] " Tomasz Figa
2019-12-30 12:16     ` Dmitry Sepp
2019-12-30 12:16       ` Dmitry Sepp
2020-01-06  6:31       ` Tomasz Figa
2020-01-06  6:31         ` Tomasz Figa
2020-01-06  8:33         ` Gerd Hoffmann
2020-01-06  8:33           ` Gerd Hoffmann
2020-01-06  9:29           ` Dmitry Sepp
2020-01-06  9:29             ` Dmitry Sepp
2020-01-03 15:47   ` Dmitry Sepp
2020-01-03 15:47     ` [virtio-dev] " Dmitry Sepp
2020-01-06  8:47     ` Gerd Hoffmann
2020-01-06  8:47       ` [virtio-dev] " Gerd Hoffmann
2020-01-06 10:21     ` Keiichi Watanabe
2020-01-06 10:21       ` [virtio-dev] " Keiichi Watanabe
2020-01-06 14:59   ` Dmitry Sepp
2020-01-06 14:59     ` [virtio-dev] " Dmitry Sepp
2020-01-07 13:24     ` Keiichi Watanabe
2020-01-07 13:24       ` [virtio-dev] " Keiichi Watanabe
2020-01-07 16:50       ` Dmitry Sepp
2020-01-07 16:50         ` [virtio-dev] " Dmitry Sepp
2020-01-08  6:59         ` Keiichi Watanabe
2020-01-08  6:59           ` [virtio-dev] " Keiichi Watanabe
2020-01-08 10:00           ` Dmitry Sepp
2020-01-08 10:00             ` [virtio-dev] " Dmitry Sepp
2020-01-08 12:14             ` Keiichi Watanabe
2020-01-08 12:14               ` [virtio-dev] " Keiichi Watanabe
2020-01-08 12:46               ` Tomasz Figa
2020-01-08 12:46                 ` [virtio-dev] " Tomasz Figa
2020-01-08 13:05                 ` Keiichi Watanabe
2020-01-08 13:05                   ` [virtio-dev] " Keiichi Watanabe
2020-01-08 13:11                 ` Dmitry Sepp
2020-01-08 13:11                   ` [virtio-dev] " Dmitry Sepp
2020-01-08 13:23                   ` Keiichi Watanabe
2020-01-08 13:23                     ` Keiichi Watanabe
2020-01-08 12:23   ` Keiichi Watanabe
2020-01-08 12:23     ` [virtio-dev] " Keiichi Watanabe
2019-12-20 15:58 ` [PATCH v2 0/1] VirtIO " Dmitry Sepp
2019-12-20 15:58   ` [virtio-dev] " Dmitry Sepp
2019-12-21  4:36   ` Keiichi Watanabe
2019-12-21  4:36     ` [virtio-dev] " Keiichi Watanabe
2019-12-21  6:18     ` Tomasz Figa
2019-12-21  6:18       ` [virtio-dev] " Tomasz Figa
2019-12-21  6:19       ` Tomasz Figa
2019-12-21  6:19         ` [virtio-dev] " Tomasz Figa
2020-01-03 13:05         ` Dmitry Sepp
2020-01-03 13:05           ` [virtio-dev] " Dmitry Sepp
2020-01-06 10:30           ` Keiichi Watanabe
2020-01-06 10:30             ` [virtio-dev] " Keiichi Watanabe
2020-01-06 11:28             ` Dmitry Sepp
2020-01-06 11:28               ` [virtio-dev] " Dmitry Sepp
2020-01-07 10:25               ` Keiichi Watanabe
2020-01-07 10:25                 ` [virtio-dev] " Keiichi Watanabe
2020-01-09 14:56                 ` Dmitry Sepp
2020-01-09 14:56                   ` [virtio-dev] " Dmitry Sepp
2020-01-10 10:16                   ` Dmitry Sepp
2020-01-10 10:16                     ` Dmitry Sepp
2020-01-10 13:53                     ` Keiichi Watanabe
2020-01-10 13:53                       ` Keiichi Watanabe
2020-01-10 15:11                       ` Dmitry Sepp
2020-01-10 15:11                         ` Dmitry Sepp
2020-01-11 16:06                         ` Tomasz Figa
2020-01-11 16:06                           ` Tomasz Figa
2020-01-13  9:50                           ` Dmitry Sepp
2020-01-13  9:50                             ` Dmitry Sepp
2020-01-15 11:23                             ` Keiichi Watanabe
2020-01-15 11:23                               ` Keiichi Watanabe
2020-01-13  9:56                         ` Gerd Hoffmann
2020-01-13  9:56                           ` Gerd Hoffmann
2020-01-13 10:41                           ` Dmitry Sepp
2020-01-13 11:05                             ` Gerd Hoffmann
2020-01-13 11:05                               ` Gerd Hoffmann
2020-01-13 11:59                               ` Tomasz Figa
2020-01-13 11:59                                 ` Tomasz Figa
2020-01-13 13:26                                 ` Gerd Hoffmann
2020-01-13 13:26                                   ` Gerd Hoffmann
2020-01-15 11:00                                   ` Tomasz Figa
2020-01-15 11:00                                     ` Tomasz Figa
2020-01-15 11:23                                     ` Keiichi Watanabe
2020-01-15 11:23                                       ` Keiichi Watanabe
2020-01-15 11:26                                     ` Gerd Hoffmann
2020-01-15 11:26                                       ` Gerd Hoffmann
2020-01-20  7:20                                       ` Keiichi Watanabe
2020-01-20  7:20                                         ` Keiichi Watanabe
2020-01-20 10:47                                         ` Gerd Hoffmann
2020-01-20 10:47                                           ` Gerd Hoffmann
2020-01-21  2:47                                           ` Keiichi Watanabe
2020-01-21  2:47                                             ` Keiichi Watanabe
2020-01-21  6:44                                             ` Gerd Hoffmann
2020-01-21  6:44                                               ` Gerd Hoffmann
2020-01-21  8:56                                               ` Keiichi Watanabe
2020-01-21  8:56                                                 ` Keiichi Watanabe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAFQd5BgkEUwBFWdv2ZH98egjm=u0dBRgtexqkzjES+J1SEmag@mail.gmail.com' \
    --to=tfiga@chromium.org \
    --cc=acourbot@chromium.org \
    --cc=alexlau@chromium.org \
    --cc=daniel@ffwll.ch \
    --cc=dgreid@chromium.org \
    --cc=dmitry.sepp@opensynergy.com \
    --cc=egranata@google.com \
    --cc=fziglio@redhat.com \
    --cc=hverkuil@xs4all.nl \
    --cc=keiichiw@chromium.org \
    --cc=kraxel@redhat.com \
    --cc=linux-media@vger.kernel.org \
    --cc=marcheu@chromium.org \
    --cc=posciak@chromium.org \
    --cc=spice-devel@lists.freedesktop.org \
    --cc=stevensd@chromium.org \
    --cc=uril@redhat.com \
    --cc=virtio-dev@lists.oasis-open.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.