[v2,0/1] VirtIO video device specification

Message ID	20191218130214.170703-1-keiichiw@chromium.org (mailing list archive)
Headers	show Return-Path: <SRS0=kwvz=2I=vger.kernel.org=linux-media-owner@kernel.org> From: Keiichi Watanabe <keiichiw@chromium.org> To: virtio-dev@lists.oasis-open.org Cc: linux-media@vger.kernel.org, acourbot@chromium.org, alexlau@chromium.org, daniel@ffwll.ch, dgreid@chromium.org, dmitry.sepp@opensynergy.com, egranata@google.com, fziglio@redhat.com, hverkuil@xs4all.nl, keiichiw@chromium.org, kraxel@redhat.com, marcheu@chromium.org, posciak@chromium.org, spice-devel@lists.freedesktop.org, stevensd@chromium.org, tfiga@chromium.org, uril@redhat.com Subject: [PATCH v2 0/1] VirtIO video device specification Date: Wed, 18 Dec 2019 22:02:13 +0900 Message-Id: <20191218130214.170703-1-keiichiw@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk
Series	VirtIO video device specification \| expand [v2,0/1] VirtIO video device specification [v2,1/1] virtio-video: Add virtio video device specification

Keiichi Watanabe Dec. 18, 2019, 1:02 p.m. UTC

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/1eT5fEckBoor2iHZR4f4GLxYzFMVapOFx?usp=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 capabilities.
* Added a way to represent supported combinations of formats.
- A field "mask" in virtio_video_format_desc plays this role.
* Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play any meaningful roles.
* Removed VIRTIO_VIDEO_T_STREAM_{ATTACH, DETACH}_BACKING and merged them into RESOURCE_{CREATE, DESTROY}.
* Added a way to notify/specify resource creation method.
- Added a feature flag.
- Defined enum virtio_video_mem_type.
- Added new fields in video_stream_create.
* Modified fields in virtio_video_params.
- Added crop information.
* Removed enum virtio_video_channel_type because we can get this information by image format.
* Renamed virtio_video_pin to virtio_video_buf_type.
- It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
* Added an error event.
* Reordered some subsections.
* Changed styles to make it consistent with other devices.

Dmitry Sepp (1):
virtio-video: Add virtio video device specification

content.tex | 1 +
virtio-video.tex | 579 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 580 insertions(+)
create mode 100644 virtio-video.tex

--
2.24.1.735.g03f4e72817-goog

Dmitry Sepp Dec. 20, 2019, 3:58 p.m. UTC | #1

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/1eT5fEckBoor2iHZR4f4GLxYzFMVapOFx?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 capabilities.
> * Added a way to represent supported combinations of formats.
>   - A field "mask" in virtio_video_format_desc plays this role.
> * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play any
> meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH, DETACH}_BACKING
> and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> notify/specify resource creation method.
>   - Added a feature flag.
>   - Defined enum virtio_video_mem_type.
>   - Added new fields in video_stream_create.
> * Modified fields in virtio_video_params.
>   - Added crop information.
> * Removed enum virtio_video_channel_type because we can get this information
> by image format. 
Could you please explain this? How do you get the information?

Suppose you have some piece of HW on the host side that wants I420 as one 
contig buffer w/ some offsets. But on the driver side, say, gralloc gives you 
three separate buffers, one per channel. How do we pass those to the device 
then?

Best regards,
Dmitry.

> * Renamed virtio_video_pin to virtio_video_buf_type.
>   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> * Added an error event.
> * Reordered some subsections.
> * Changed styles to make it consistent with other devices.
> 
> Dmitry Sepp (1):
>   virtio-video: Add virtio video device specification
> 
>  content.tex      |   1 +
>  virtio-video.tex | 579 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 580 insertions(+)
>  create mode 100644 virtio-video.tex
> 
> --
> 2.24.1.735.g03f4e72817-goog

Keiichi Watanabe Dec. 21, 2019, 4:36 a.m. UTC | #2

Hi Dmitry,

On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
<dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYzFMVapOFx?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 capabilities.
> > * Added a way to represent supported combinations of formats.
> >   - A field "mask" in virtio_video_format_desc plays this role.
> > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play any
> > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH, DETACH}_BACKING
> > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> > notify/specify resource creation method.
> >   - Added a feature flag.
> >   - Defined enum virtio_video_mem_type.
> >   - Added new fields in video_stream_create.
> > * Modified fields in virtio_video_params.
> >   - Added crop information.
> > * Removed enum virtio_video_channel_type because we can get this information
> > by image format.
> Could you please explain this? How do you get the information?

It means that if image formats are well-defined, channel information
(e.g. the order of channels) is uniquely determined.

>
> Suppose you have some piece of HW on the host side that wants I420 as one
> contig buffer w/ some offsets. But on the driver side, say, gralloc gives you
> three separate buffers, one per channel. How do we pass those to the device
> then?

You're talking about CrOS use case where buffers are allocated by
virtio-gpu, right?
In this case, virtio-gpu allocates one contiguous host-side buffer and
the client regards a pair of (buffer FD, offset) as one channel.
And, we can register this pair to the device when the buffer is imported.
In the virtio-vdec spec draft, this pair corresponds to struct
virtio_vdec_plane in struct virtio_vdec_plane.

So, I suppose we will need similar structs when we add a control to
import buffers. However, I don't think it's necessary when guest pages
are used.

Best regards,
Keiichi


>
> Best regards,
> Dmitry.
>
> > * Renamed virtio_video_pin to virtio_video_buf_type.
> >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > * Added an error event.
> > * Reordered some subsections.
> > * Changed styles to make it consistent with other devices.
> >
> > Dmitry Sepp (1):
> >   virtio-video: Add virtio video device specification
> >
> >  content.tex      |   1 +
> >  virtio-video.tex | 579 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 580 insertions(+)
> >  create mode 100644 virtio-video.tex
> >
> > --
> > 2.24.1.735.g03f4e72817-goog
>
>

Tomasz Figa Dec. 21, 2019, 6:18 a.m. UTC | #3

On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe <keiichiw@chromium.org> wrote:
>
> Hi Dmitry,
>
> On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYzFMVapOFx?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 capabilities.
> > > * Added a way to represent supported combinations of formats.
> > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play any
> > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH, DETACH}_BACKING
> > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> > > notify/specify resource creation method.
> > >   - Added a feature flag.
> > >   - Defined enum virtio_video_mem_type.
> > >   - Added new fields in video_stream_create.
> > > * Modified fields in virtio_video_params.
> > >   - Added crop information.
> > > * Removed enum virtio_video_channel_type because we can get this information
> > > by image format.
> > Could you please explain this? How do you get the information?
>
> It means that if image formats are well-defined, channel information
> (e.g. the order of channels) is uniquely determined.
>
> >
> > Suppose you have some piece of HW on the host side that wants I420 as one
> > contig buffer w/ some offsets. But on the driver side, say, gralloc gives you
> > three separate buffers, one per channel. How do we pass those to the device
> > then?
>
> You're talking about CrOS use case where buffers are allocated by
> virtio-gpu, right?
> In this case, virtio-gpu allocates one contiguous host-side buffer and
> the client regards a pair of (buffer FD, offset) as one channel.
> And, we can register this pair to the device when the buffer is imported.
> In the virtio-vdec spec draft, this pair corresponds to struct
> virtio_vdec_plane in struct virtio_vdec_plane.
>
> So, I suppose we will need similar structs when we add a control to
> import buffers. However, I don't think it's necessary when guest pages
> are used.

I think we need some way for the guest to know whether it can allocate
the planes in separate buffers, even when guest pages are used. This
would be equivalent to V4L2 M and non-M formats, but mixing this into
FourCC in V4L2 is an acknowledged mistake, so we should add a query or
something.

For future V4L2 development we came up with the idea of a format flag
which could mean that the hardware allows putting planes in separate
buffers. We could have a similar per-format flag in the capabilities,
as we already have a list of all the supported formats there.

Best regards,
Tomasz

>
> Best regards,
> Keiichi
>
>
> >
> > Best regards,
> > Dmitry.
> >
> > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > * Added an error event.
> > > * Reordered some subsections.
> > > * Changed styles to make it consistent with other devices.
> > >
> > > Dmitry Sepp (1):
> > >   virtio-video: Add virtio video device specification
> > >
> > >  content.tex      |   1 +
> > >  virtio-video.tex | 579 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 580 insertions(+)
> > >  create mode 100644 virtio-video.tex
> > >
> > > --
> > > 2.24.1.735.g03f4e72817-goog
> >
> >

Tomasz Figa Dec. 21, 2019, 6:19 a.m. UTC | #4

On Sat, Dec 21, 2019 at 3:18 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe <keiichiw@chromium.org> wrote:
> >
> > Hi Dmitry,
> >
> > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYzFMVapOFx?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 capabilities.
> > > > * Added a way to represent supported combinations of formats.
> > > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play any
> > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH, DETACH}_BACKING
> > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> > > > notify/specify resource creation method.
> > > >   - Added a feature flag.
> > > >   - Defined enum virtio_video_mem_type.
> > > >   - Added new fields in video_stream_create.
> > > > * Modified fields in virtio_video_params.
> > > >   - Added crop information.
> > > > * Removed enum virtio_video_channel_type because we can get this information
> > > > by image format.
> > > Could you please explain this? How do you get the information?
> >
> > It means that if image formats are well-defined, channel information
> > (e.g. the order of channels) is uniquely determined.
> >
> > >
> > > Suppose you have some piece of HW on the host side that wants I420 as one
> > > contig buffer w/ some offsets. But on the driver side, say, gralloc gives you
> > > three separate buffers, one per channel. How do we pass those to the device
> > > then?
> >
> > You're talking about CrOS use case where buffers are allocated by
> > virtio-gpu, right?
> > In this case, virtio-gpu allocates one contiguous host-side buffer and
> > the client regards a pair of (buffer FD, offset) as one channel.
> > And, we can register this pair to the device when the buffer is imported.
> > In the virtio-vdec spec draft, this pair corresponds to struct
> > virtio_vdec_plane in struct virtio_vdec_plane.
> >
> > So, I suppose we will need similar structs when we add a control to
> > import buffers. However, I don't think it's necessary when guest pages
> > are used.
>
> I think we need some way for the guest to know whether it can allocate
> the planes in separate buffers, even when guest pages are used. This
> would be equivalent to V4L2 M and non-M formats, but mixing this into
> FourCC in V4L2 is an acknowledged mistake, so we should add a query or
> something.
>
> For future V4L2 development we came up with the idea of a format flag
> which could mean that the hardware allows putting planes in separate
> buffers. We could have a similar per-format flag in the capabilities,
> as we already have a list of all the supported formats there.

Sorry, forgot to paste the link from future V4L2 work notes from this year ELCE:
https://www.spinics.net/lists/linux-media/msg159789.html

>
> Best regards,
> Tomasz
>
> >
> > Best regards,
> > Keiichi
> >
> >
> > >
> > > Best regards,
> > > Dmitry.
> > >
> > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > * Added an error event.
> > > > * Reordered some subsections.
> > > > * Changed styles to make it consistent with other devices.
> > > >
> > > > Dmitry Sepp (1):
> > > >   virtio-video: Add virtio video device specification
> > > >
> > > >  content.tex      |   1 +
> > > >  virtio-video.tex | 579 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 580 insertions(+)
> > > >  create mode 100644 virtio-video.tex
> > > >
> > > > --
> > > > 2.24.1.735.g03f4e72817-goog
> > >
> > >

Dmitry Sepp Jan. 3, 2020, 1:05 p.m. UTC | #5

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 <tfiga@chromium.org> wrote:
> > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe <keiichiw@chromium.org> 
wrote:
> > > Hi Dmitry,
> > > 
> > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > 
> > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYzFMVa
> > > > > 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
> > > > > capabilities.
> > > > > * Added a way to represent supported combinations of formats.
> > > > > 
> > > > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > > > 
> > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play
> > > > > any
> > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > DETACH}_BACKING
> > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> > > > > notify/specify resource creation method.
> > > > > 
> > > > >   - Added a feature flag.
> > > > >   - Defined enum virtio_video_mem_type.
> > > > >   - Added new fields in video_stream_create.
> > > > > 
> > > > > * Modified fields in virtio_video_params.
> > > > > 
> > > > >   - Added crop information.
> > > > > 
> > > > > * Removed enum virtio_video_channel_type because we can get this
> > > > > information by image format.
> > > > 
> > > > Could you please explain this? How do you get the information?
> > > 
> > > It means that if image formats are well-defined, channel information
> > > (e.g. the order of channels) is uniquely determined.
> > > 
> > > > Suppose you have some piece of HW on the host side that wants I420 as
> > > > one
> > > > contig buffer w/ some offsets. But on the driver side, say, gralloc
> > > > gives you three separate buffers, one per channel. How do we pass
> > > > those to the device then?
> > > 
> > > You're talking about CrOS use case where buffers are allocated by
> > > virtio-gpu, right?
> > > In this case, virtio-gpu allocates one contiguous host-side buffer and
> > > the client regards a pair of (buffer FD, offset) as one channel.
> > > And, we can register this pair to the device when the buffer is
> > > imported.
> > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > 
> > > So, I suppose we will need similar structs when we add a control to
> > > import buffers. However, I don't think it's necessary when guest pages
> > > are used.
> > 
> > I think we need some way for the guest to know whether it can allocate
> > the planes in separate buffers, even when guest pages are used. This
> > would be equivalent to V4L2 M and non-M formats, but mixing this into
> > FourCC in V4L2 is an acknowledged mistake, so we should add a query or
> > something.
> > 

Yes, this is what I mean. In fact, we already do face the situation when the 
device side is not happy with the sgt and wants contig. I think we'll add a 
module parameter for now.

Regards,
Dmitry.

> > For future V4L2 development we came up with the idea of a format flag
> > which could mean that the hardware allows putting planes in separate
> > buffers. We could have a similar per-format flag in the capabilities,
> > as we already have a list of all the supported formats there.
> 
> Sorry, forgot to paste the link from future V4L2 work notes from this year
> ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> 
> > Best regards,
> > Tomasz
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > Best regards,
> > > > Dmitry.
> > > > 
> > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > 
> > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > 
> > > > > * Added an error event.
> > > > > * Reordered some subsections.
> > > > > * Changed styles to make it consistent with other devices.
> > > > > 
> > > > > Dmitry Sepp (1):
> > > > >   virtio-video: Add virtio video device specification
> > > > >  
> > > > >  content.tex      |   1 +
> > > > >  virtio-video.tex | 579
> > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 580 insertions(+)
> > > > >  create mode 100644 virtio-video.tex
> > > > > 
> > > > > --
> > > > > 2.24.1.735.g03f4e72817-goog

Keiichi Watanabe Jan. 6, 2020, 10:30 a.m. UTC | #6

Hi Dmitry, Tomasz,


On Fri, Jan 3, 2020 at 10:05 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 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 <tfiga@chromium.org> wrote:
> > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe <keiichiw@chromium.org>
> wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > >
> > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYzFMVa
> > > > > > 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
> > > > > > capabilities.
> > > > > > * Added a way to represent supported combinations of formats.
> > > > > >
> > > > > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > > > >
> > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't play
> > > > > > any
> > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > DETACH}_BACKING
> > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way to
> > > > > > notify/specify resource creation method.
> > > > > >
> > > > > >   - Added a feature flag.
> > > > > >   - Defined enum virtio_video_mem_type.
> > > > > >   - Added new fields in video_stream_create.
> > > > > >
> > > > > > * Modified fields in virtio_video_params.
> > > > > >
> > > > > >   - Added crop information.
> > > > > >
> > > > > > * Removed enum virtio_video_channel_type because we can get this
> > > > > > information by image format.
> > > > >
> > > > > Could you please explain this? How do you get the information?
> > > >
> > > > It means that if image formats are well-defined, channel information
> > > > (e.g. the order of channels) is uniquely determined.
> > > >
> > > > > Suppose you have some piece of HW on the host side that wants I420 as
> > > > > one
> > > > > contig buffer w/ some offsets. But on the driver side, say, gralloc
> > > > > gives you three separate buffers, one per channel. How do we pass
> > > > > those to the device then?
> > > >
> > > > You're talking about CrOS use case where buffers are allocated by
> > > > virtio-gpu, right?
> > > > In this case, virtio-gpu allocates one contiguous host-side buffer and
> > > > the client regards a pair of (buffer FD, offset) as one channel.
> > > > And, we can register this pair to the device when the buffer is
> > > > imported.
> > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > >
> > > > So, I suppose we will need similar structs when we add a control to
> > > > import buffers. However, I don't think it's necessary when guest pages
> > > > are used.
> > >
> > > I think we need some way for the guest to know whether it can allocate
> > > the planes in separate buffers, even when guest pages are used. This
> > > would be equivalent to V4L2 M and non-M formats, but mixing this into
> > > FourCC in V4L2 is an acknowledged mistake, so we should add a query or
> > > something.
> > >
>
> Yes, this is what I mean. In fact, we already do face the situation when the
> device side is not happy with the sgt and wants contig. I think we'll add a
> module parameter for now.

Okay. So, I suppose we'll be able to update structs:
* Add a flag in virtio_video_format_desc that indicates whether planes
can be in separate buffers, and
* Add a flag in virtio_video_format_desc that indicates that the
device requires contiguous buffers for this format.

Does it make sense?

Best regards,
Keiichi

>
> Regards,
> Dmitry.
>
> > > For future V4L2 development we came up with the idea of a format flag
> > > which could mean that the hardware allows putting planes in separate
> > > buffers. We could have a similar per-format flag in the capabilities,
> > > as we already have a list of all the supported formats there.
> >
> > Sorry, forgot to paste the link from future V4L2 work notes from this year
> > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> >
> > > Best regards,
> > > Tomasz
> > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > > Best regards,
> > > > > Dmitry.
> > > > >
> > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > >
> > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > >
> > > > > > * Added an error event.
> > > > > > * Reordered some subsections.
> > > > > > * Changed styles to make it consistent with other devices.
> > > > > >
> > > > > > Dmitry Sepp (1):
> > > > > >   virtio-video: Add virtio video device specification
> > > > > >
> > > > > >  content.tex      |   1 +
> > > > > >  virtio-video.tex | 579
> > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 580 insertions(+)
> > > > > >  create mode 100644 virtio-video.tex
> > > > > >
> > > > > > --
> > > > > > 2.24.1.735.g03f4e72817-goog
>
>

Dmitry Sepp Jan. 6, 2020, 11:28 a.m. UTC | #7

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 <dmitry.sepp@opensynergy.com> 
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 <tfiga@chromium.org> wrote:
> > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > <keiichiw@chromium.org>
> > 
> > wrote:
> > > > > Hi Dmitry,
> > > > > 
> > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > 
> > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYz
> > > > > > > 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
> > > > > > > capabilities.
> > > > > > > * Added a way to represent supported combinations of formats.
> > > > > > > 
> > > > > > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > > > > > 
> > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't
> > > > > > > play
> > > > > > > any
> > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > DETACH}_BACKING
> > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way
> > > > > > > to
> > > > > > > notify/specify resource creation method.
> > > > > > > 
> > > > > > >   - Added a feature flag.
> > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > >   - Added new fields in video_stream_create.
> > > > > > > 
> > > > > > > * Modified fields in virtio_video_params.
> > > > > > > 
> > > > > > >   - Added crop information.
> > > > > > > 
> > > > > > > * Removed enum virtio_video_channel_type because we can get this
> > > > > > > information by image format.
> > > > > > 
> > > > > > Could you please explain this? How do you get the information?
> > > > > 
> > > > > It means that if image formats are well-defined, channel information
> > > > > (e.g. the order of channels) is uniquely determined.
> > > > > 
> > > > > > Suppose you have some piece of HW on the host side that wants I420
> > > > > > as
> > > > > > one
> > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > gralloc
> > > > > > gives you three separate buffers, one per channel. How do we pass
> > > > > > those to the device then?
> > > > > 
> > > > > You're talking about CrOS use case where buffers are allocated by
> > > > > virtio-gpu, right?
> > > > > In this case, virtio-gpu allocates one contiguous host-side buffer
> > > > > and
> > > > > the client regards a pair of (buffer FD, offset) as one channel.
> > > > > And, we can register this pair to the device when the buffer is
> > > > > imported.
> > > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > 
> > > > > So, I suppose we will need similar structs when we add a control to
> > > > > import buffers. However, I don't think it's necessary when guest
> > > > > pages
> > > > > are used.
> > > > 
> > > > I think we need some way for the guest to know whether it can allocate
> > > > the planes in separate buffers, even when guest pages are used. This
> > > > would be equivalent to V4L2 M and non-M formats, but mixing this into
> > > > FourCC in V4L2 is an acknowledged mistake, so we should add a query or
> > > > something.
> > 
> > Yes, this is what I mean. In fact, we already do face the situation when
> > the device side is not happy with the sgt and wants contig. I think we'll
> > add a module parameter for now.
> 
> Okay. So, I suppose we'll be able to update structs:
> * Add a flag in virtio_video_format_desc that indicates whether planes
> can be in separate buffers, and
> * Add a flag in virtio_video_format_desc that indicates that the
> device requires contiguous buffers for this format.
> 
> Does it make sense?
> 
Sorry, I don't understand the difference between the two above: isn't the first 
case is just when the flag is not set?

Regards,
Dmitry.

> Best regards,
> Keiichi
> 
> > Regards,
> > Dmitry.
> > 
> > > > For future V4L2 development we came up with the idea of a format flag
> > > > which could mean that the hardware allows putting planes in separate
> > > > buffers. We could have a similar per-format flag in the capabilities,
> > > > as we already have a list of all the supported formats there.
> > > 
> > > Sorry, forgot to paste the link from future V4L2 work notes from this
> > > year
> > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > 
> > > > Best regards,
> > > > Tomasz
> > > > 
> > > > > Best regards,
> > > > > Keiichi
> > > > > 
> > > > > > Best regards,
> > > > > > Dmitry.
> > > > > > 
> > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > 
> > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > > > 
> > > > > > > * Added an error event.
> > > > > > > * Reordered some subsections.
> > > > > > > * Changed styles to make it consistent with other devices.
> > > > > > > 
> > > > > > > Dmitry Sepp (1):
> > > > > > >   virtio-video: Add virtio video device specification
> > > > > > >  
> > > > > > >  content.tex      |   1 +
> > > > > > >  virtio-video.tex | 579
> > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 580 insertions(+)
> > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > 
> > > > > > > --
> > > > > > > 2.24.1.735.g03f4e72817-goog

Keiichi Watanabe Jan. 7, 2020, 10:25 a.m. UTC | #8

Hi Dmitry,

On Mon, Jan 6, 2020 at 8:28 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 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 <dmitry.sepp@opensynergy.com>
> 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 <tfiga@chromium.org> wrote:
> > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > <keiichiw@chromium.org>
> > >
> > > wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > >
> > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4GLxYz
> > > > > > > > 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
> > > > > > > > capabilities.
> > > > > > > > * Added a way to represent supported combinations of formats.
> > > > > > > >
> > > > > > > >   - A field "mask" in virtio_video_format_desc plays this role.
> > > > > > > >
> > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they don't
> > > > > > > > play
> > > > > > > > any
> > > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > DETACH}_BACKING
> > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a way
> > > > > > > > to
> > > > > > > > notify/specify resource creation method.
> > > > > > > >
> > > > > > > >   - Added a feature flag.
> > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > >
> > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > >
> > > > > > > >   - Added crop information.
> > > > > > > >
> > > > > > > > * Removed enum virtio_video_channel_type because we can get this
> > > > > > > > information by image format.
> > > > > > >
> > > > > > > Could you please explain this? How do you get the information?
> > > > > >
> > > > > > It means that if image formats are well-defined, channel information
> > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > >
> > > > > > > Suppose you have some piece of HW on the host side that wants I420
> > > > > > > as
> > > > > > > one
> > > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > > gralloc
> > > > > > > gives you three separate buffers, one per channel. How do we pass
> > > > > > > those to the device then?
> > > > > >
> > > > > > You're talking about CrOS use case where buffers are allocated by
> > > > > > virtio-gpu, right?
> > > > > > In this case, virtio-gpu allocates one contiguous host-side buffer
> > > > > > and
> > > > > > the client regards a pair of (buffer FD, offset) as one channel.
> > > > > > And, we can register this pair to the device when the buffer is
> > > > > > imported.
> > > > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > >
> > > > > > So, I suppose we will need similar structs when we add a control to
> > > > > > import buffers. However, I don't think it's necessary when guest
> > > > > > pages
> > > > > > are used.
> > > > >
> > > > > I think we need some way for the guest to know whether it can allocate
> > > > > the planes in separate buffers, even when guest pages are used. This
> > > > > would be equivalent to V4L2 M and non-M formats, but mixing this into
> > > > > FourCC in V4L2 is an acknowledged mistake, so we should add a query or
> > > > > something.
> > >
> > > Yes, this is what I mean. In fact, we already do face the situation when
> > > the device side is not happy with the sgt and wants contig. I think we'll
> > > add a module parameter for now.
> >
> > Okay. So, I suppose we'll be able to update structs:
> > * Add a flag in virtio_video_format_desc that indicates whether planes
> > can be in separate buffers, and
> > * Add a flag in virtio_video_format_desc that indicates that the
> > device requires contiguous buffers for this format.
> >
> > Does it make sense?
> >
> Sorry, I don't understand the difference between the two above: isn't the first
> case is just when the flag is not set?

Ah, I was confused and wrote something strange. Yeah,  these two are the same.
Sorry for that.

So, the suggestion is to add a field "planes_layout" in
virtio_video_format_desc, which is one of the following enums:

enum virtio_video_planes_layout {
    VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement */
    VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
};

If we have a better idea or naming, please let me know.

Best regards,
Keiichi

>
> Regards,
> Dmitry.
>
> > Best regards,
> > Keiichi
> >
> > > Regards,
> > > Dmitry.
> > >
> > > > > For future V4L2 development we came up with the idea of a format flag
> > > > > which could mean that the hardware allows putting planes in separate
> > > > > buffers. We could have a similar per-format flag in the capabilities,
> > > > > as we already have a list of all the supported formats there.
> > > >
> > > > Sorry, forgot to paste the link from future V4L2 work notes from this
> > > > year
> > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > > > Best regards,
> > > > > > Keiichi
> > > > > >
> > > > > > > Best regards,
> > > > > > > Dmitry.
> > > > > > >
> > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > >
> > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > > > >
> > > > > > > > * Added an error event.
> > > > > > > > * Reordered some subsections.
> > > > > > > > * Changed styles to make it consistent with other devices.
> > > > > > > >
> > > > > > > > Dmitry Sepp (1):
> > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > >
> > > > > > > >  content.tex      |   1 +
> > > > > > > >  virtio-video.tex | 579
> > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.24.1.735.g03f4e72817-goog
>
>

Dmitry Sepp Jan. 9, 2020, 2:56 p.m. UTC | #9

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 <dmitry.sepp@opensynergy.com> 
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
> > > <dmitry.sepp@opensynergy.com>
> > 
> > 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 <tfiga@chromium.org> 
wrote:
> > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > <keiichiw@chromium.org>
> > > > 
> > > > wrote:
> > > > > > > Hi Dmitry,
> > > > > > > 
> > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > 
> > > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f4G
> > > > > > > > > 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
> > > > > > > > > capabilities.
> > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > formats.
> > > > > > > > > 
> > > > > > > > >   - A field "mask" in virtio_video_format_desc plays this
> > > > > > > > >   role.
> > > > > > > > > 
> > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they
> > > > > > > > > don't
> > > > > > > > > play
> > > > > > > > > any
> > > > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > DETACH}_BACKING
> > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a
> > > > > > > > > way
> > > > > > > > > to
> > > > > > > > > notify/specify resource creation method.
> > > > > > > > > 
> > > > > > > > >   - Added a feature flag.
> > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > 
> > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > 
> > > > > > > > >   - Added crop information.
> > > > > > > > > 
> > > > > > > > > * Removed enum virtio_video_channel_type because we can get
> > > > > > > > > this
> > > > > > > > > information by image format.
> > > > > > > > 
> > > > > > > > Could you please explain this? How do you get the information?
> > > > > > > 
> > > > > > > It means that if image formats are well-defined, channel
> > > > > > > information
> > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > 
> > > > > > > > Suppose you have some piece of HW on the host side that wants
> > > > > > > > I420
> > > > > > > > as
> > > > > > > > one
> > > > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > > > gralloc
> > > > > > > > gives you three separate buffers, one per channel. How do we
> > > > > > > > pass
> > > > > > > > those to the device then?
> > > > > > > 
> > > > > > > You're talking about CrOS use case where buffers are allocated
> > > > > > > by
> > > > > > > virtio-gpu, right?
> > > > > > > In this case, virtio-gpu allocates one contiguous host-side
> > > > > > > buffer
> > > > > > > and
> > > > > > > the client regards a pair of (buffer FD, offset) as one channel.
> > > > > > > And, we can register this pair to the device when the buffer is
> > > > > > > imported.
> > > > > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > 
> > > > > > > So, I suppose we will need similar structs when we add a control
> > > > > > > to
> > > > > > > import buffers. However, I don't think it's necessary when guest
> > > > > > > pages
> > > > > > > are used.
> > > > > > 
> > > > > > I think we need some way for the guest to know whether it can
> > > > > > allocate
> > > > > > the planes in separate buffers, even when guest pages are used.
> > > > > > This
> > > > > > would be equivalent to V4L2 M and non-M formats, but mixing this
> > > > > > into
> > > > > > FourCC in V4L2 is an acknowledged mistake, so we should add a
> > > > > > query or
> > > > > > something.
> > > > 
> > > > Yes, this is what I mean. In fact, we already do face the situation
> > > > when
> > > > the device side is not happy with the sgt and wants contig. I think
> > > > we'll
> > > > add a module parameter for now.
> > > 
> > > Okay. So, I suppose we'll be able to update structs:
> > > * Add a flag in virtio_video_format_desc that indicates whether planes
> > > can be in separate buffers, and
> > > * Add a flag in virtio_video_format_desc that indicates that the
> > > device requires contiguous buffers for this format.
> > > 
> > > Does it make sense?
> > 
> > Sorry, I don't understand the difference between the two above: isn't the
> > first case is just when the flag is not set?
> 
> Ah, I was confused and wrote something strange. Yeah,  these two are the
> same. Sorry for that.
> 
> So, the suggestion is to add a field "planes_layout" in
> virtio_video_format_desc, which is one of the following enums:
> 
> enum virtio_video_planes_layout {
>     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement */
>     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> };
> 
> If we have a better idea or naming, please let me know.
> 

The naming looks good for me, I might only change to CONTIG as we have UNSPEC.

Best regards,
Dmitry.

> Best regards,
> Keiichi
> 
> > Regards,
> > Dmitry.
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > Regards,
> > > > Dmitry.
> > > > 
> > > > > > For future V4L2 development we came up with the idea of a format
> > > > > > flag
> > > > > > which could mean that the hardware allows putting planes in
> > > > > > separate
> > > > > > buffers. We could have a similar per-format flag in the
> > > > > > capabilities,
> > > > > > as we already have a list of all the supported formats there.
> > > > > 
> > > > > Sorry, forgot to paste the link from future V4L2 work notes from
> > > > > this
> > > > > year
> > > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > 
> > > > > > Best regards,
> > > > > > Tomasz
> > > > > > 
> > > > > > > Best regards,
> > > > > > > Keiichi
> > > > > > > 
> > > > > > > > Best regards,
> > > > > > > > Dmitry.
> > > > > > > > 
> > > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > > > 
> > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > > > > > 
> > > > > > > > > * Added an error event.
> > > > > > > > > * Reordered some subsections.
> > > > > > > > > * Changed styles to make it consistent with other devices.
> > > > > > > > > 
> > > > > > > > > Dmitry Sepp (1):
> > > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > > >  
> > > > > > > > >  content.tex      |   1 +
> > > > > > > > >  virtio-video.tex | 579
> > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > 
> > > > > > > > > --
> > > > > > > > > 2.24.1.735.g03f4e72817-goog

Dmitry Sepp Jan. 10, 2020, 10:16 a.m. UTC | #10

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 <dmitry.sepp@opensynergy.com>
> 
> 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
> > > > <dmitry.sepp@opensynergy.com>
> > > 
> > > 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 <tfiga@chromium.org>
> 
> wrote:
> > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > <keiichiw@chromium.org>
> > > > > 
> > > > > wrote:
> > > > > > > > Hi Dmitry,
> > > > > > > > 
> > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > 
> > > > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f
> > > > > > > > > > 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
> > > > > > > > > > capabilities.
> > > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > > formats.
> > > > > > > > > > 
> > > > > > > > > >   - A field "mask" in virtio_video_format_desc plays this
> > > > > > > > > >   role.
> > > > > > > > > > 
> > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they
> > > > > > > > > > don't
> > > > > > > > > > play
> > > > > > > > > > any
> > > > > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a
> > > > > > > > > > way
> > > > > > > > > > to
> > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > 
> > > > > > > > > >   - Added a feature flag.
> > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > 
> > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > 
> > > > > > > > > >   - Added crop information.
> > > > > > > > > > 
> > > > > > > > > > * Removed enum virtio_video_channel_type because we can
> > > > > > > > > > get
> > > > > > > > > > this
> > > > > > > > > > information by image format.
> > > > > > > > > 
> > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > information?
> > > > > > > > 
> > > > > > > > It means that if image formats are well-defined, channel
> > > > > > > > information
> > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > 
> > > > > > > > > Suppose you have some piece of HW on the host side that
> > > > > > > > > wants
> > > > > > > > > I420
> > > > > > > > > as
> > > > > > > > > one
> > > > > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > > > > gralloc
> > > > > > > > > gives you three separate buffers, one per channel. How do we
> > > > > > > > > pass
> > > > > > > > > those to the device then?
> > > > > > > > 
> > > > > > > > You're talking about CrOS use case where buffers are allocated
> > > > > > > > by
> > > > > > > > virtio-gpu, right?
> > > > > > > > In this case, virtio-gpu allocates one contiguous host-side
> > > > > > > > buffer
> > > > > > > > and
> > > > > > > > the client regards a pair of (buffer FD, offset) as one
> > > > > > > > channel.
> > > > > > > > And, we can register this pair to the device when the buffer
> > > > > > > > is
> > > > > > > > imported.
> > > > > > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > 
> > > > > > > > So, I suppose we will need similar structs when we add a
> > > > > > > > control
> > > > > > > > to
> > > > > > > > import buffers. However, I don't think it's necessary when
> > > > > > > > guest
> > > > > > > > pages
> > > > > > > > are used.
> > > > > > > 
> > > > > > > I think we need some way for the guest to know whether it can
> > > > > > > allocate
> > > > > > > the planes in separate buffers, even when guest pages are used.
> > > > > > > This
> > > > > > > would be equivalent to V4L2 M and non-M formats, but mixing this
> > > > > > > into
> > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should add a
> > > > > > > query or
> > > > > > > something.
> > > > > 
> > > > > Yes, this is what I mean. In fact, we already do face the situation
> > > > > when
> > > > > the device side is not happy with the sgt and wants contig. I think
> > > > > we'll
> > > > > add a module parameter for now.
> > > > 
> > > > Okay. So, I suppose we'll be able to update structs:
> > > > * Add a flag in virtio_video_format_desc that indicates whether planes
> > > > can be in separate buffers, and
> > > > * Add a flag in virtio_video_format_desc that indicates that the
> > > > device requires contiguous buffers for this format.
> > > > 
> > > > Does it make sense?
> > > 
> > > Sorry, I don't understand the difference between the two above: isn't
> > > the
> > > first case is just when the flag is not set?
> > 
> > Ah, I was confused and wrote something strange. Yeah,  these two are the
> > same. Sorry for that.
> > 
> > So, the suggestion is to add a field "planes_layout" in
> > virtio_video_format_desc, which is one of the following enums:
> > 
> > enum virtio_video_planes_layout {
> > 
> >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement */
> >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > 
> > };
> > 
> > If we have a better idea or naming, please let me know.
> 
> The naming looks good for me, I might only change to CONTIG as we have
> UNSPEC.
> 

So here we are talking about plane layout in memory, am I correct? But I think 
we also need a way to communicate memory requirements of the device: the 
device might require CMA buffers or it can be ok with SG lists. What about 
adding something like this to virtio_video_format_desc:

enum virtio_video_mem_layout {
	VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,

	VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
	VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
};

struct virtio_video_format_desc {
    __le64 mask;
    __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
    __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types */
    __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
    ...
};

Best regards,
Dmitry.

> Best regards,
> Dmitry.
> 
> > Best regards,
> > Keiichi
> > 
> > > Regards,
> > > Dmitry.
> > > 
> > > > Best regards,
> > > > Keiichi
> > > > 
> > > > > Regards,
> > > > > Dmitry.
> > > > > 
> > > > > > > For future V4L2 development we came up with the idea of a format
> > > > > > > flag
> > > > > > > which could mean that the hardware allows putting planes in
> > > > > > > separate
> > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > capabilities,
> > > > > > > as we already have a list of all the supported formats there.
> > > > > > 
> > > > > > Sorry, forgot to paste the link from future V4L2 work notes from
> > > > > > this
> > > > > > year
> > > > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > 
> > > > > > > Best regards,
> > > > > > > Tomasz
> > > > > > > 
> > > > > > > > Best regards,
> > > > > > > > Keiichi
> > > > > > > > 
> > > > > > > > > Best regards,
> > > > > > > > > Dmitry.
> > > > > > > > > 
> > > > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > > > > 
> > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > > > > > > 
> > > > > > > > > > * Added an error event.
> > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > * Changed styles to make it consistent with other devices.
> > > > > > > > > > 
> > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > > > >  
> > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > 
> > > > > > > > > > --
> > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Keiichi Watanabe Jan. 10, 2020, 1:53 p.m. UTC | #11

Hi,


On Fri, Jan 10, 2020 at 7:16 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 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 <dmitry.sepp@opensynergy.com>
> >
> > 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
> > > > > <dmitry.sepp@opensynergy.com>
> > > >
> > > > 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 <tfiga@chromium.org>
> >
> > wrote:
> > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > > <keiichiw@chromium.org>
> > > > > >
> > > > > > wrote:
> > > > > > > > > Hi Dmitry,
> > > > > > > > >
> > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > >
> > > > > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iHZR4f
> > > > > > > > > > > 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
> > > > > > > > > > > capabilities.
> > > > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > > > formats.
> > > > > > > > > > >
> > > > > > > > > > >   - A field "mask" in virtio_video_format_desc plays this
> > > > > > > > > > >   role.
> > > > > > > > > > >
> > > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they
> > > > > > > > > > > don't
> > > > > > > > > > > play
> > > > > > > > > > > any
> > > > > > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a
> > > > > > > > > > > way
> > > > > > > > > > > to
> > > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > >
> > > > > > > > > > >   - Added a feature flag.
> > > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > >
> > > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > >
> > > > > > > > > > >   - Added crop information.
> > > > > > > > > > >
> > > > > > > > > > > * Removed enum virtio_video_channel_type because we can
> > > > > > > > > > > get
> > > > > > > > > > > this
> > > > > > > > > > > information by image format.
> > > > > > > > > >
> > > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > > information?
> > > > > > > > >
> > > > > > > > > It means that if image formats are well-defined, channel
> > > > > > > > > information
> > > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > >
> > > > > > > > > > Suppose you have some piece of HW on the host side that
> > > > > > > > > > wants
> > > > > > > > > > I420
> > > > > > > > > > as
> > > > > > > > > > one
> > > > > > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > > > > > gralloc
> > > > > > > > > > gives you three separate buffers, one per channel. How do we
> > > > > > > > > > pass
> > > > > > > > > > those to the device then?
> > > > > > > > >
> > > > > > > > > You're talking about CrOS use case where buffers are allocated
> > > > > > > > > by
> > > > > > > > > virtio-gpu, right?
> > > > > > > > > In this case, virtio-gpu allocates one contiguous host-side
> > > > > > > > > buffer
> > > > > > > > > and
> > > > > > > > > the client regards a pair of (buffer FD, offset) as one
> > > > > > > > > channel.
> > > > > > > > > And, we can register this pair to the device when the buffer
> > > > > > > > > is
> > > > > > > > > imported.
> > > > > > > > > In the virtio-vdec spec draft, this pair corresponds to struct
> > > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > >
> > > > > > > > > So, I suppose we will need similar structs when we add a
> > > > > > > > > control
> > > > > > > > > to
> > > > > > > > > import buffers. However, I don't think it's necessary when
> > > > > > > > > guest
> > > > > > > > > pages
> > > > > > > > > are used.
> > > > > > > >
> > > > > > > > I think we need some way for the guest to know whether it can
> > > > > > > > allocate
> > > > > > > > the planes in separate buffers, even when guest pages are used.
> > > > > > > > This
> > > > > > > > would be equivalent to V4L2 M and non-M formats, but mixing this
> > > > > > > > into
> > > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should add a
> > > > > > > > query or
> > > > > > > > something.
> > > > > >
> > > > > > Yes, this is what I mean. In fact, we already do face the situation
> > > > > > when
> > > > > > the device side is not happy with the sgt and wants contig. I think
> > > > > > we'll
> > > > > > add a module parameter for now.
> > > > >
> > > > > Okay. So, I suppose we'll be able to update structs:
> > > > > * Add a flag in virtio_video_format_desc that indicates whether planes
> > > > > can be in separate buffers, and
> > > > > * Add a flag in virtio_video_format_desc that indicates that the
> > > > > device requires contiguous buffers for this format.
> > > > >
> > > > > Does it make sense?
> > > >
> > > > Sorry, I don't understand the difference between the two above: isn't
> > > > the
> > > > first case is just when the flag is not set?
> > >
> > > Ah, I was confused and wrote something strange. Yeah,  these two are the
> > > same. Sorry for that.
> > >
> > > So, the suggestion is to add a field "planes_layout" in
> > > virtio_video_format_desc, which is one of the following enums:
> > >
> > > enum virtio_video_planes_layout {
> > >
> > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement */
> > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > >
> > > };
> > >
> > > If we have a better idea or naming, please let me know.
> >
> > The naming looks good for me, I might only change to CONTIG as we have
> > UNSPEC.
> >
>
> So here we are talking about plane layout in memory, am I correct? But I think
> we also need a way to communicate memory requirements of the device: the
> device might require CMA buffers or it can be ok with SG lists. What about
> adding something like this to virtio_video_format_desc:
>
> enum virtio_video_mem_layout {
>         VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,
>
>         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
>         VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
> };
>
> struct virtio_video_format_desc {
>     __le64 mask;
>     __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
>     __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types */
>     __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
>     ...
> };

Good.
I'd not like to call it NON_CONTIG, as it sounds like CMA buffers
aren't allowed.
Instead, how about this definition?

enum virtio_video_mem_layout {
        VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC = 0,  /* no special requirement */
        VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 1,
};

With this enum,
* the device can simply ignore this field if it doesn't have any
requirement and the struct is zero-initialized, and
* if we need to add other types of memory layout requirements, we can
add them as 2, 4, 8, etc to represent combinations of requirements.

Just to confirm, are the following combination of planes_layout and
mem_layout valid?
(1) (planes_layout, mem_layout) = (contig, not specified)
(2) (planes_layout, mem_layout) = (not specified, contig)

In my understanding, (1) means that each plane must be a contiguous
buffer while different planes don't have to be in a contig memory, but
(2) is invalid.
Is it correct?

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
> > Best regards,
> > Dmitry.
> >
> > > Best regards,
> > > Keiichi
> > >
> > > > Regards,
> > > > Dmitry.
> > > >
> > > > > Best regards,
> > > > > Keiichi
> > > > >
> > > > > > Regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > > > For future V4L2 development we came up with the idea of a format
> > > > > > > > flag
> > > > > > > > which could mean that the hardware allows putting planes in
> > > > > > > > separate
> > > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > > capabilities,
> > > > > > > > as we already have a list of all the supported formats there.
> > > > > > >
> > > > > > > Sorry, forgot to paste the link from future V4L2 work notes from
> > > > > > > this
> > > > > > > year
> > > > > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Tomasz
> > > > > > > >
> > > > > > > > > Best regards,
> > > > > > > > > Keiichi
> > > > > > > > >
> > > > > > > > > > Best regards,
> > > > > > > > > > Dmitry.
> > > > > > > > > >
> > > > > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > > > > >
> > > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT, CAPTURE}.
> > > > > > > > > > >
> > > > > > > > > > > * Added an error event.
> > > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > > * Changed styles to make it consistent with other devices.
> > > > > > > > > > >
> > > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > > > > >
> > > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

Dmitry Sepp Jan. 10, 2020, 3:11 p.m. UTC | #12

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 <dmitry.sepp@opensynergy.com> 
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
> > > > <dmitry.sepp@opensynergy.com>
> > > 
> > > 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
> > > > > > <dmitry.sepp@opensynergy.com>
> > > > > 
> > > > > 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
> > > > > > > > <tfiga@chromium.org>
> > > 
> > > wrote:
> > > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > > > <keiichiw@chromium.org>
> > > > > > > 
> > > > > > > wrote:
> > > > > > > > > > Hi Dmitry,
> > > > > > > > > > 
> > > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > > > 
> > > > > > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iH
> > > > > > > > > > > > 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
> > > > > > > > > > > > capabilities.
> > > > > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > > > > formats.
> > > > > > > > > > > > 
> > > > > > > > > > > >   - A field "mask" in virtio_video_format_desc plays
> > > > > > > > > > > >   this
> > > > > > > > > > > >   role.
> > > > > > > > > > > > 
> > > > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because
> > > > > > > > > > > > they
> > > > > > > > > > > > don't
> > > > > > > > > > > > play
> > > > > > > > > > > > any
> > > > > > > > > > > > meaningful roles. * Removed
> > > > > > > > > > > > VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. *
> > > > > > > > > > > > Added a
> > > > > > > > > > > > way
> > > > > > > > > > > > to
> > > > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > > > 
> > > > > > > > > > > >   - Added a feature flag.
> > > > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > > > 
> > > > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > > > 
> > > > > > > > > > > >   - Added crop information.
> > > > > > > > > > > > 
> > > > > > > > > > > > * Removed enum virtio_video_channel_type because we
> > > > > > > > > > > > can
> > > > > > > > > > > > get
> > > > > > > > > > > > this
> > > > > > > > > > > > information by image format.
> > > > > > > > > > > 
> > > > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > > > information?
> > > > > > > > > > 
> > > > > > > > > > It means that if image formats are well-defined, channel
> > > > > > > > > > information
> > > > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > > > 
> > > > > > > > > > > Suppose you have some piece of HW on the host side that
> > > > > > > > > > > wants
> > > > > > > > > > > I420
> > > > > > > > > > > as
> > > > > > > > > > > one
> > > > > > > > > > > contig buffer w/ some offsets. But on the driver side,
> > > > > > > > > > > say,
> > > > > > > > > > > gralloc
> > > > > > > > > > > gives you three separate buffers, one per channel. How
> > > > > > > > > > > do we
> > > > > > > > > > > pass
> > > > > > > > > > > those to the device then?
> > > > > > > > > > 
> > > > > > > > > > You're talking about CrOS use case where buffers are
> > > > > > > > > > allocated
> > > > > > > > > > by
> > > > > > > > > > virtio-gpu, right?
> > > > > > > > > > In this case, virtio-gpu allocates one contiguous
> > > > > > > > > > host-side
> > > > > > > > > > buffer
> > > > > > > > > > and
> > > > > > > > > > the client regards a pair of (buffer FD, offset) as one
> > > > > > > > > > channel.
> > > > > > > > > > And, we can register this pair to the device when the
> > > > > > > > > > buffer
> > > > > > > > > > is
> > > > > > > > > > imported.
> > > > > > > > > > In the virtio-vdec spec draft, this pair corresponds to
> > > > > > > > > > struct
> > > > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > > > 
> > > > > > > > > > So, I suppose we will need similar structs when we add a
> > > > > > > > > > control
> > > > > > > > > > to
> > > > > > > > > > import buffers. However, I don't think it's necessary when
> > > > > > > > > > guest
> > > > > > > > > > pages
> > > > > > > > > > are used.
> > > > > > > > > 
> > > > > > > > > I think we need some way for the guest to know whether it
> > > > > > > > > can
> > > > > > > > > allocate
> > > > > > > > > the planes in separate buffers, even when guest pages are
> > > > > > > > > used.
> > > > > > > > > This
> > > > > > > > > would be equivalent to V4L2 M and non-M formats, but mixing
> > > > > > > > > this
> > > > > > > > > into
> > > > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should add
> > > > > > > > > a
> > > > > > > > > query or
> > > > > > > > > something.
> > > > > > > 
> > > > > > > Yes, this is what I mean. In fact, we already do face the
> > > > > > > situation
> > > > > > > when
> > > > > > > the device side is not happy with the sgt and wants contig. I
> > > > > > > think
> > > > > > > we'll
> > > > > > > add a module parameter for now.
> > > > > > 
> > > > > > Okay. So, I suppose we'll be able to update structs:
> > > > > > * Add a flag in virtio_video_format_desc that indicates whether
> > > > > > planes
> > > > > > can be in separate buffers, and
> > > > > > * Add a flag in virtio_video_format_desc that indicates that the
> > > > > > device requires contiguous buffers for this format.
> > > > > > 
> > > > > > Does it make sense?
> > > > > 
> > > > > Sorry, I don't understand the difference between the two above:
> > > > > isn't
> > > > > the
> > > > > first case is just when the flag is not set?
> > > > 
> > > > Ah, I was confused and wrote something strange. Yeah,  these two are
> > > > the
> > > > same. Sorry for that.
> > > > 
> > > > So, the suggestion is to add a field "planes_layout" in
> > > > virtio_video_format_desc, which is one of the following enums:
> > > > 
> > > > enum virtio_video_planes_layout {
> > > > 
> > > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement
> > > >     */
> > > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > > > 
> > > > };
> > > > 
> > > > If we have a better idea or naming, please let me know.
> > > 
> > > The naming looks good for me, I might only change to CONTIG as we have
> > > UNSPEC.
> > 
> > So here we are talking about plane layout in memory, am I correct? But I
> > think we also need a way to communicate memory requirements of the
> > device: the device might require CMA buffers or it can be ok with SG
> > lists. What about adding something like this to virtio_video_format_desc:
> > 
> > enum virtio_video_mem_layout {
> > 
> >         VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,
> >         
> >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
> >         VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
> > 
> > };
> > 
> > struct virtio_video_format_desc {
> > 
> >     __le64 mask;
> >     __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> >     __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types */
> >     __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
> >     ...
> > 
> > };
> 
> Good.
> I'd not like to call it NON_CONTIG, as it sounds like CMA buffers
> aren't allowed.
> Instead, how about this definition?
> 
> enum virtio_video_mem_layout {
>         VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC = 0,  /* no special requirement */
>         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 1,
> };

Yes, I agree, that would be more correct. We just need an empty line to be 
aligned with other enums.

> 
> With this enum,
> * the device can simply ignore this field if it doesn't have any
> requirement and the struct is zero-initialized, and
> * if we need to add other types of memory layout requirements, we can
> add them as 2, 4, 8, etc to represent combinations of requirements.
> 
> Just to confirm, are the following combination of planes_layout and
> mem_layout valid?
> (1) (planes_layout, mem_layout) = (contig, not specified)
> (2) (planes_layout, mem_layout) = (not specified, contig)
> 
> In my understanding, (1) means that each plane must be a contiguous
> buffer while different planes don't have to be in a contig memory, but
> (2) is invalid.
> Is it correct?

Let me tell a bit more about my vision:

mem_layout: device can handle SG lists (e.g. using iommu) or it cannot and 
needs CMA. If it can handle SG, CMA is also ok. So it is ether 'I don't care' 
or 'give me CMA'.

planes_layout: some devices might want to see multiplanar data in one buffer. 
So if we allocate two sets of mem entries (one per each plane) and send them 
to the host, the device will fail to handle those.

So from my perspective you example can be interpreted as follows:
(1) means all your planes are hosted by one buffer instance (we can see it as 
one fd for all planes plus per-plane offsets inside the buffer), the buffer 
itself consists of arbitrary pages (or can consist, as it can of course also 
be CMA, because of UNSPEC).
(2) valid, means each plane has its own buffer, each buffer is contiguous in 
memory (must be allocated from CMA), but planes are not necessarily adjacent 
to each other in memory.

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. So it should look like this:

enum virtio_video_planes_layout {
    VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* default, invalid */

    VIRTIO_VIDEO_PLANES_LAYOUT_CONTIG = 0x100,
    VIRTIO_VIDEO_PLANES_LAYOUT_NON_CONTIG,
};

Best regards,
Dmitry

> 
> Best regards,
> Keiichi
> 
> > Best regards,
> > Dmitry.
> > 
> > > Best regards,
> > > Dmitry.
> > > 
> > > > Best regards,
> > > > Keiichi
> > > > 
> > > > > Regards,
> > > > > Dmitry.
> > > > > 
> > > > > > Best regards,
> > > > > > Keiichi
> > > > > > 
> > > > > > > Regards,
> > > > > > > Dmitry.
> > > > > > > 
> > > > > > > > > For future V4L2 development we came up with the idea of a
> > > > > > > > > format
> > > > > > > > > flag
> > > > > > > > > which could mean that the hardware allows putting planes in
> > > > > > > > > separate
> > > > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > > > capabilities,
> > > > > > > > > as we already have a list of all the supported formats
> > > > > > > > > there.
> > > > > > > > 
> > > > > > > > Sorry, forgot to paste the link from future V4L2 work notes
> > > > > > > > from
> > > > > > > > this
> > > > > > > > year
> > > > > > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > > > 
> > > > > > > > > Best regards,
> > > > > > > > > Tomasz
> > > > > > > > > 
> > > > > > > > > > Best regards,
> > > > > > > > > > Keiichi
> > > > > > > > > > 
> > > > > > > > > > > Best regards,
> > > > > > > > > > > Dmitry.
> > > > > > > > > > > 
> > > > > > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > > > > > > 
> > > > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT,
> > > > > > > > > > > >   CAPTURE}.
> > > > > > > > > > > > 
> > > > > > > > > > > > * Added an error event.
> > > > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > > > * Changed styles to make it consistent with other
> > > > > > > > > > > > devices.
> > > > > > > > > > > > 
> > > > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > > > > > >  
> > > > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > > > 
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> > > 
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Tomasz Figa Jan. 11, 2020, 4:06 p.m. UTC | #13

On Sat, Jan 11, 2020 at 12:12 AM Dmitry Sepp
<dmitry.sepp@opensynergy.com> 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 <dmitry.sepp@opensynergy.com>
> 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
> > > > > <dmitry.sepp@opensynergy.com>
> > > >
> > > > 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
> > > > > > > <dmitry.sepp@opensynergy.com>
> > > > > >
> > > > > > 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
> > > > > > > > > <tfiga@chromium.org>
> > > >
> > > > wrote:
> > > > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > > > > <keiichiw@chromium.org>
> > > > > > > >
> > > > > > > > wrote:
> > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > >
> > > > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > > > >
> > > > > > > > > > > <dmitry.sepp@opensynergy.com> 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/1eT5fEckBoor2iH
> > > > > > > > > > > > > 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
> > > > > > > > > > > > > capabilities.
> > > > > > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > > > > > formats.
> > > > > > > > > > > > >
> > > > > > > > > > > > >   - A field "mask" in virtio_video_format_desc plays
> > > > > > > > > > > > >   this
> > > > > > > > > > > > >   role.
> > > > > > > > > > > > >
> > > > > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because
> > > > > > > > > > > > > they
> > > > > > > > > > > > > don't
> > > > > > > > > > > > > play
> > > > > > > > > > > > > any
> > > > > > > > > > > > > meaningful roles. * Removed
> > > > > > > > > > > > > VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. *
> > > > > > > > > > > > > Added a
> > > > > > > > > > > > > way
> > > > > > > > > > > > > to
> > > > > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > > > >
> > > > > > > > > > > > >   - Added a feature flag.
> > > > > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > > > >
> > > > > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > > > >
> > > > > > > > > > > > >   - Added crop information.
> > > > > > > > > > > > >
> > > > > > > > > > > > > * Removed enum virtio_video_channel_type because we
> > > > > > > > > > > > > can
> > > > > > > > > > > > > get
> > > > > > > > > > > > > this
> > > > > > > > > > > > > information by image format.
> > > > > > > > > > > >
> > > > > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > > > > information?
> > > > > > > > > > >
> > > > > > > > > > > It means that if image formats are well-defined, channel
> > > > > > > > > > > information
> > > > > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > > > >
> > > > > > > > > > > > Suppose you have some piece of HW on the host side that
> > > > > > > > > > > > wants
> > > > > > > > > > > > I420
> > > > > > > > > > > > as
> > > > > > > > > > > > one
> > > > > > > > > > > > contig buffer w/ some offsets. But on the driver side,
> > > > > > > > > > > > say,
> > > > > > > > > > > > gralloc
> > > > > > > > > > > > gives you three separate buffers, one per channel. How
> > > > > > > > > > > > do we
> > > > > > > > > > > > pass
> > > > > > > > > > > > those to the device then?
> > > > > > > > > > >
> > > > > > > > > > > You're talking about CrOS use case where buffers are
> > > > > > > > > > > allocated
> > > > > > > > > > > by
> > > > > > > > > > > virtio-gpu, right?
> > > > > > > > > > > In this case, virtio-gpu allocates one contiguous
> > > > > > > > > > > host-side
> > > > > > > > > > > buffer
> > > > > > > > > > > and
> > > > > > > > > > > the client regards a pair of (buffer FD, offset) as one
> > > > > > > > > > > channel.
> > > > > > > > > > > And, we can register this pair to the device when the
> > > > > > > > > > > buffer
> > > > > > > > > > > is
> > > > > > > > > > > imported.
> > > > > > > > > > > In the virtio-vdec spec draft, this pair corresponds to
> > > > > > > > > > > struct
> > > > > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > > > >
> > > > > > > > > > > So, I suppose we will need similar structs when we add a
> > > > > > > > > > > control
> > > > > > > > > > > to
> > > > > > > > > > > import buffers. However, I don't think it's necessary when
> > > > > > > > > > > guest
> > > > > > > > > > > pages
> > > > > > > > > > > are used.
> > > > > > > > > >
> > > > > > > > > > I think we need some way for the guest to know whether it
> > > > > > > > > > can
> > > > > > > > > > allocate
> > > > > > > > > > the planes in separate buffers, even when guest pages are
> > > > > > > > > > used.
> > > > > > > > > > This
> > > > > > > > > > would be equivalent to V4L2 M and non-M formats, but mixing
> > > > > > > > > > this
> > > > > > > > > > into
> > > > > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should add
> > > > > > > > > > a
> > > > > > > > > > query or
> > > > > > > > > > something.
> > > > > > > >
> > > > > > > > Yes, this is what I mean. In fact, we already do face the
> > > > > > > > situation
> > > > > > > > when
> > > > > > > > the device side is not happy with the sgt and wants contig. I
> > > > > > > > think
> > > > > > > > we'll
> > > > > > > > add a module parameter for now.
> > > > > > >
> > > > > > > Okay. So, I suppose we'll be able to update structs:
> > > > > > > * Add a flag in virtio_video_format_desc that indicates whether
> > > > > > > planes
> > > > > > > can be in separate buffers, and
> > > > > > > * Add a flag in virtio_video_format_desc that indicates that the
> > > > > > > device requires contiguous buffers for this format.
> > > > > > >
> > > > > > > Does it make sense?
> > > > > >
> > > > > > Sorry, I don't understand the difference between the two above:
> > > > > > isn't
> > > > > > the
> > > > > > first case is just when the flag is not set?
> > > > >
> > > > > Ah, I was confused and wrote something strange. Yeah,  these two are
> > > > > the
> > > > > same. Sorry for that.
> > > > >
> > > > > So, the suggestion is to add a field "planes_layout" in
> > > > > virtio_video_format_desc, which is one of the following enums:
> > > > >
> > > > > enum virtio_video_planes_layout {
> > > > >
> > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special requirement
> > > > >     */
> > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > > > >
> > > > > };
> > > > >
> > > > > If we have a better idea or naming, please let me know.
> > > >
> > > > The naming looks good for me, I might only change to CONTIG as we have
> > > > UNSPEC.
> > >
> > > So here we are talking about plane layout in memory, am I correct? But I
> > > think we also need a way to communicate memory requirements of the
> > > device: the device might require CMA buffers or it can be ok with SG
> > > lists. What about adding something like this to virtio_video_format_desc:
> > >
> > > enum virtio_video_mem_layout {
> > >
> > >         VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,
> > >
> > >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
> > >         VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
> > >
> > > };
> > >
> > > struct virtio_video_format_desc {
> > >
> > >     __le64 mask;
> > >     __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > >     __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types */
> > >     __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
> > >     ...
> > >
> > > };
> >
> > Good.
> > I'd not like to call it NON_CONTIG, as it sounds like CMA buffers
> > aren't allowed.
> > Instead, how about this definition?
> >
> > enum virtio_video_mem_layout {
> >         VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC = 0,  /* no special requirement */
> >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 1,
> > };
>
> Yes, I agree, that would be more correct. We just need an empty line to be
> aligned with other enums.
>
> >
> > With this enum,
> > * the device can simply ignore this field if it doesn't have any
> > requirement and the struct is zero-initialized, and
> > * if we need to add other types of memory layout requirements, we can
> > add them as 2, 4, 8, etc to represent combinations of requirements.
> >
> > Just to confirm, are the following combination of planes_layout and
> > mem_layout valid?
> > (1) (planes_layout, mem_layout) = (contig, not specified)
> > (2) (planes_layout, mem_layout) = (not specified, contig)
> >
> > In my understanding, (1) means that each plane must be a contiguous
> > buffer while different planes don't have to be in a contig memory, but
> > (2) is invalid.
> > Is it correct?
>
> Let me tell a bit more about my vision:
>
> mem_layout: device can handle SG lists (e.g. using iommu) or it cannot and
> needs CMA. If it can handle SG, CMA is also ok. So it is ether 'I don't care'
> or 'give me CMA'.
>
> planes_layout: some devices might want to see multiplanar data in one buffer.
> So if we allocate two sets of mem entries (one per each plane) and send them
> to the host, the device will fail to handle those.
>
> So from my perspective you example can be interpreted as follows:
> (1) means all your planes are hosted by one buffer instance (we can see it as
> one fd for all planes plus per-plane offsets inside the buffer), the buffer
> itself consists of arbitrary pages (or can consist, as it can of course also
> be CMA, because of UNSPEC).
> (2) valid, means each plane has its own buffer, each buffer is contiguous in
> memory (must be allocated from CMA), but planes are not necessarily adjacent
> to each other in memory.
>
> 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.

I might be misunderstanding the above, but just to make sure we're on
the same page, here are the cases that we found to exist in practice
with V4L2 stateful decoders:

1) device expects planes in one buffer laid out one after another
without any padding; the device accepts only 1 pointer and no offsets
- this corresponds to V4L2 non-M formats, such as NV12, which exactly
specify the location of planes in the buffer,

2) device accepts separate pointers to all planes - the planes can be
located anywhere in memory, which could be separate buffers, 1 buffer
with planes laid out at arbitrary offsets or even exactly the same
layout as required for 1).

3) device _requires_ planes to be located in separate buffers
allocated from designated areas in memory, e.g. different physical
memory banks, for performance purposes. This is actually a very rare
case and observed only on old generations of Samsung Exynos SoCs.

Please let me know if that matches your expectation,

Best regards,
Tomasz

> So it should look like this:
>
> enum virtio_video_planes_layout {
>     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* default, invalid */
>
>     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIG = 0x100,
>     VIRTIO_VIDEO_PLANES_LAYOUT_NON_CONTIG,
> };
>
> Best regards,
> Dmitry
>
> >
> > Best regards,
> > Keiichi
> >
> > > Best regards,
> > > Dmitry.
> > >
> > > > Best regards,
> > > > Dmitry.
> > > >
> > > > > Best regards,
> > > > > Keiichi
> > > > >
> > > > > > Regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > > Best regards,
> > > > > > > Keiichi
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Dmitry.
> > > > > > > >
> > > > > > > > > > For future V4L2 development we came up with the idea of a
> > > > > > > > > > format
> > > > > > > > > > flag
> > > > > > > > > > which could mean that the hardware allows putting planes in
> > > > > > > > > > separate
> > > > > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > > > > capabilities,
> > > > > > > > > > as we already have a list of all the supported formats
> > > > > > > > > > there.
> > > > > > > > >
> > > > > > > > > Sorry, forgot to paste the link from future V4L2 work notes
> > > > > > > > > from
> > > > > > > > > this
> > > > > > > > > year
> > > > > > > > > ELCE: https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > > > >
> > > > > > > > > > Best regards,
> > > > > > > > > > Tomasz
> > > > > > > > > >
> > > > > > > > > > > Best regards,
> > > > > > > > > > > Keiichi
> > > > > > > > > > >
> > > > > > > > > > > > Best regards,
> > > > > > > > > > > > Dmitry.
> > > > > > > > > > > >
> > > > > > > > > > > > > * Renamed virtio_video_pin to virtio_video_buf_type.
> > > > > > > > > > > > >
> > > > > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT,
> > > > > > > > > > > > >   CAPTURE}.
> > > > > > > > > > > > >
> > > > > > > > > > > > > * Added an error event.
> > > > > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > > > > * Changed styles to make it consistent with other
> > > > > > > > > > > > > devices.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > > > > >   virtio-video: Add virtio video device specification
> > > > > > > > > > > > >
> > > > > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

Dmitry Sepp Jan. 13, 2020, 9:50 a.m. UTC | #14

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
> 
> <dmitry.sepp@opensynergy.com> 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
> > > <dmitry.sepp@opensynergy.com>
> > 
> > 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
> > > > > > <dmitry.sepp@opensynergy.com>
> > > > > 
> > > > > 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
> > > > > > > > <dmitry.sepp@opensynergy.com>
> > > > > > > 
> > > > > > > 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
> > > > > > > > > > <tfiga@chromium.org>
> > > > > 
> > > > > wrote:
> > > > > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > > > > > <keiichiw@chromium.org>
> > > > > > > > > 
> > > > > > > > > wrote:
> > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > 
> > > > > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > > > > > 
> > > > > > > > > > > > <dmitry.sepp@opensynergy.com> 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
> > > > > > > > > > > > > > capabilities.
> > > > > > > > > > > > > > * Added a way to represent supported combinations
> > > > > > > > > > > > > > of
> > > > > > > > > > > > > > formats.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >   - A field "mask" in virtio_video_format_desc
> > > > > > > > > > > > > >   plays
> > > > > > > > > > > > > >   this
> > > > > > > > > > > > > >   role.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP}
> > > > > > > > > > > > > > because
> > > > > > > > > > > > > > they
> > > > > > > > > > > > > > don't
> > > > > > > > > > > > > > play
> > > > > > > > > > > > > > any
> > > > > > > > > > > > > > meaningful roles. * Removed
> > > > > > > > > > > > > > VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. *
> > > > > > > > > > > > > > Added a
> > > > > > > > > > > > > > way
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >   - Added a feature flag.
> > > > > > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >   - Added crop information.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > * Removed enum virtio_video_channel_type because
> > > > > > > > > > > > > > we
> > > > > > > > > > > > > > can
> > > > > > > > > > > > > > get
> > > > > > > > > > > > > > this
> > > > > > > > > > > > > > information by image format.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > > > > > information?
> > > > > > > > > > > > 
> > > > > > > > > > > > It means that if image formats are well-defined,
> > > > > > > > > > > > channel
> > > > > > > > > > > > information
> > > > > > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > > > > > 
> > > > > > > > > > > > > Suppose you have some piece of HW on the host side
> > > > > > > > > > > > > that
> > > > > > > > > > > > > wants
> > > > > > > > > > > > > I420
> > > > > > > > > > > > > as
> > > > > > > > > > > > > one
> > > > > > > > > > > > > contig buffer w/ some offsets. But on the driver
> > > > > > > > > > > > > side,
> > > > > > > > > > > > > say,
> > > > > > > > > > > > > gralloc
> > > > > > > > > > > > > gives you three separate buffers, one per channel.
> > > > > > > > > > > > > How
> > > > > > > > > > > > > do we
> > > > > > > > > > > > > pass
> > > > > > > > > > > > > those to the device then?
> > > > > > > > > > > > 
> > > > > > > > > > > > You're talking about CrOS use case where buffers are
> > > > > > > > > > > > allocated
> > > > > > > > > > > > by
> > > > > > > > > > > > virtio-gpu, right?
> > > > > > > > > > > > In this case, virtio-gpu allocates one contiguous
> > > > > > > > > > > > host-side
> > > > > > > > > > > > buffer
> > > > > > > > > > > > and
> > > > > > > > > > > > the client regards a pair of (buffer FD, offset) as
> > > > > > > > > > > > one
> > > > > > > > > > > > channel.
> > > > > > > > > > > > And, we can register this pair to the device when the
> > > > > > > > > > > > buffer
> > > > > > > > > > > > is
> > > > > > > > > > > > imported.
> > > > > > > > > > > > In the virtio-vdec spec draft, this pair corresponds
> > > > > > > > > > > > to
> > > > > > > > > > > > struct
> > > > > > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > > > > > 
> > > > > > > > > > > > So, I suppose we will need similar structs when we add
> > > > > > > > > > > > a
> > > > > > > > > > > > control
> > > > > > > > > > > > to
> > > > > > > > > > > > import buffers. However, I don't think it's necessary
> > > > > > > > > > > > when
> > > > > > > > > > > > guest
> > > > > > > > > > > > pages
> > > > > > > > > > > > are used.
> > > > > > > > > > > 
> > > > > > > > > > > I think we need some way for the guest to know whether
> > > > > > > > > > > it
> > > > > > > > > > > can
> > > > > > > > > > > allocate
> > > > > > > > > > > the planes in separate buffers, even when guest pages
> > > > > > > > > > > are
> > > > > > > > > > > used.
> > > > > > > > > > > This
> > > > > > > > > > > would be equivalent to V4L2 M and non-M formats, but
> > > > > > > > > > > mixing
> > > > > > > > > > > this
> > > > > > > > > > > into
> > > > > > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should
> > > > > > > > > > > add
> > > > > > > > > > > a
> > > > > > > > > > > query or
> > > > > > > > > > > something.
> > > > > > > > > 
> > > > > > > > > Yes, this is what I mean. In fact, we already do face the
> > > > > > > > > situation
> > > > > > > > > when
> > > > > > > > > the device side is not happy with the sgt and wants contig.
> > > > > > > > > I
> > > > > > > > > think
> > > > > > > > > we'll
> > > > > > > > > add a module parameter for now.
> > > > > > > > 
> > > > > > > > Okay. So, I suppose we'll be able to update structs:
> > > > > > > > * Add a flag in virtio_video_format_desc that indicates
> > > > > > > > whether
> > > > > > > > planes
> > > > > > > > can be in separate buffers, and
> > > > > > > > * Add a flag in virtio_video_format_desc that indicates that
> > > > > > > > the
> > > > > > > > device requires contiguous buffers for this format.
> > > > > > > > 
> > > > > > > > Does it make sense?
> > > > > > > 
> > > > > > > Sorry, I don't understand the difference between the two above:
> > > > > > > isn't
> > > > > > > the
> > > > > > > first case is just when the flag is not set?
> > > > > > 
> > > > > > Ah, I was confused and wrote something strange. Yeah,  these two
> > > > > > are
> > > > > > the
> > > > > > same. Sorry for that.
> > > > > > 
> > > > > > So, the suggestion is to add a field "planes_layout" in
> > > > > > virtio_video_format_desc, which is one of the following enums:
> > > > > > 
> > > > > > enum virtio_video_planes_layout {
> > > > > > 
> > > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special
> > > > > >     requirement
> > > > > >     */
> > > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > > > > > 
> > > > > > };
> > > > > > 
> > > > > > If we have a better idea or naming, please let me know.
> > > > > 
> > > > > The naming looks good for me, I might only change to CONTIG as we
> > > > > have
> > > > > UNSPEC.
> > > > 
> > > > So here we are talking about plane layout in memory, am I correct? But
> > > > I
> > > > think we also need a way to communicate memory requirements of the
> > > > device: the device might require CMA buffers or it can be ok with SG
> > > > lists. What about adding something like this to
> > > > virtio_video_format_desc:
> > > > 
> > > > enum virtio_video_mem_layout {
> > > > 
> > > >         VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,
> > > >         
> > > >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
> > > >         VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
> > > > 
> > > > };
> > > > 
> > > > struct virtio_video_format_desc {
> > > > 
> > > >     __le64 mask;
> > > >     __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > >     __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types
> > > >     */
> > > >     __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
> > > >     ...
> > > > 
> > > > };
> > > 
> > > Good.
> > > I'd not like to call it NON_CONTIG, as it sounds like CMA buffers
> > > aren't allowed.
> > > Instead, how about this definition?
> > > 
> > > enum virtio_video_mem_layout {
> > > 
> > >         VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC = 0,  /* no special requirement
> > >         */
> > >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 1,
> > > 
> > > };
> > 
> > Yes, I agree, that would be more correct. We just need an empty line to be
> > aligned with other enums.
> > 
> > > With this enum,
> > > * the device can simply ignore this field if it doesn't have any
> > > requirement and the struct is zero-initialized, and
> > > * if we need to add other types of memory layout requirements, we can
> > > add them as 2, 4, 8, etc to represent combinations of requirements.
> > > 
> > > Just to confirm, are the following combination of planes_layout and
> > > mem_layout valid?
> > > (1) (planes_layout, mem_layout) = (contig, not specified)
> > > (2) (planes_layout, mem_layout) = (not specified, contig)
> > > 
> > > In my understanding, (1) means that each plane must be a contiguous
> > > buffer while different planes don't have to be in a contig memory, but
> > > (2) is invalid.
> > > Is it correct?
> > 
> > Let me tell a bit more about my vision:
> > 
> > mem_layout: device can handle SG lists (e.g. using iommu) or it cannot and
> > needs CMA. If it can handle SG, CMA is also ok. So it is ether 'I don't
> > care' or 'give me CMA'.
> > 
> > planes_layout: some devices might want to see multiplanar data in one
> > buffer. So if we allocate two sets of mem entries (one per each plane)
> > and send them to the host, the device will fail to handle those.
> > 
> > So from my perspective you example can be interpreted as follows:
> > (1) means all your planes are hosted by one buffer instance (we can see it
> > as one fd for all planes plus per-plane offsets inside the buffer), the
> > buffer itself consists of arbitrary pages (or can consist, as it can of
> > course also be CMA, because of UNSPEC).
> > (2) valid, means each plane has its own buffer, each buffer is contiguous
> > in memory (must be allocated from CMA), but planes are not necessarily
> > adjacent to each other in memory.
> > 
> > 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.
> 
> I might be misunderstanding the above, but just to make sure we're on
> the same page, here are the cases that we found to exist in practice
> with V4L2 stateful decoders:
> 
> 1) device expects planes in one buffer laid out one after another
> without any padding; the device accepts only 1 pointer and no offsets
> - this corresponds to V4L2 non-M formats, such as NV12, which exactly
> specify the location of planes in the buffer,
> 
> 2) device accepts separate pointers to all planes - the planes can be
> located anywhere in memory, which could be separate buffers, 1 buffer
> with planes laid out at arbitrary offsets or even exactly the same
> layout as required for 1).
> 
> 3) device _requires_ planes to be located in separate buffers
> allocated from designated areas in memory, e.g. different physical
> memory banks, for performance purposes. This is actually a very rare
> case and observed only on old generations of Samsung Exynos SoCs.
> 
> Please let me know if that matches your expectation,
> 

Thank you for the update!

Yes, I think this matches our expectations, and this also does not contradict 
the enums I provided in the previous email.

Btw, you haven't mentioned anything regarding mem layout. Do you agree with 
what was proposed?

Best regards,
Dmitry

> Best regards,
> Tomasz
> 
> > So it should look like this:
> > 
> > enum virtio_video_planes_layout {
> > 
> >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* default, invalid */
> >     
> >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIG = 0x100,
> >     VIRTIO_VIDEO_PLANES_LAYOUT_NON_CONTIG,
> > 
> > };
> > 
> > Best regards,
> > Dmitry
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > Best regards,
> > > > Dmitry.
> > > > 
> > > > > Best regards,
> > > > > Dmitry.
> > > > > 
> > > > > > Best regards,
> > > > > > Keiichi
> > > > > > 
> > > > > > > Regards,
> > > > > > > Dmitry.
> > > > > > > 
> > > > > > > > Best regards,
> > > > > > > > Keiichi
> > > > > > > > 
> > > > > > > > > Regards,
> > > > > > > > > Dmitry.
> > > > > > > > > 
> > > > > > > > > > > For future V4L2 development we came up with the idea of
> > > > > > > > > > > a
> > > > > > > > > > > format
> > > > > > > > > > > flag
> > > > > > > > > > > which could mean that the hardware allows putting planes
> > > > > > > > > > > in
> > > > > > > > > > > separate
> > > > > > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > > > > > capabilities,
> > > > > > > > > > > as we already have a list of all the supported formats
> > > > > > > > > > > there.
> > > > > > > > > > 
> > > > > > > > > > Sorry, forgot to paste the link from future V4L2 work
> > > > > > > > > > notes
> > > > > > > > > > from
> > > > > > > > > > this
> > > > > > > > > > year
> > > > > > > > > > ELCE:
> > > > > > > > > > https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > > > > > 
> > > > > > > > > > > Best regards,
> > > > > > > > > > > Tomasz
> > > > > > > > > > > 
> > > > > > > > > > > > Best regards,
> > > > > > > > > > > > Keiichi
> > > > > > > > > > > > 
> > > > > > > > > > > > > Best regards,
> > > > > > > > > > > > > Dmitry.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > * Renamed virtio_video_pin to
> > > > > > > > > > > > > > virtio_video_buf_type.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT,
> > > > > > > > > > > > > >   CAPTURE}.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > * Added an error event.
> > > > > > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > > > > > * Changed styles to make it consistent with other
> > > > > > > > > > > > > > devices.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > > > > > >   virtio-video: Add virtio video device
> > > > > > > > > > > > > >   specification
> > > > > > > > > > > > > >  
> > > > > > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> > > > > 
> > > > > --------------------------------------------------------------------
> > > > > -
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail:
> > > > > virtio-dev-help@lists.oasis-open.org

Gerd Hoffmann Jan. 13, 2020, 9:56 a.m. UTC | #15

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

Gerd Hoffmann Jan. 13, 2020, 11:05 a.m. UTC | #16

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

Tomasz Figa Jan. 13, 2020, 11:59 a.m. UTC | #17

On Mon, Jan 13, 2020 at 8:05 PM Gerd Hoffmann <kraxel@redhat.com> 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

Gerd Hoffmann Jan. 13, 2020, 1:26 p.m. UTC | #18

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

Tomasz Figa Jan. 15, 2020, 11 a.m. UTC | #19

On Mon, Jan 13, 2020 at 10:27 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   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.

That's true, simple user space would often do that. However, if more
devices are in the game, often some extra alignment or padding between
planes is needed and that is not allowed by (1), even though all the
planes are in the same buffer.

My suggestion, based on the latest V4L2 discussion on unifying the
UAPI of i) and ii), is that we may want to instead always specify
buffers on a per-plane basis. Any additional requirements would be
then validated by the host, which could check if the planes end up in
the same buffer (or different buffers for (3)) and/or at the right
offsets.

WDYT?

Best regards,
Tomasz

Keiichi Watanabe Jan. 15, 2020, 11:23 a.m. UTC | #20

Hi,

On Wed, Jan 15, 2020 at 8:00 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Mon, Jan 13, 2020 at 10:27 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   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.
>
> That's true, simple user space would often do that. However, if more
> devices are in the game, often some extra alignment or padding between
> planes is needed and that is not allowed by (1), even though all the
> planes are in the same buffer.
>
> My suggestion, based on the latest V4L2 discussion on unifying the
> UAPI of i) and ii), is that we may want to instead always specify
> buffers on a per-plane basis. Any additional requirements would be
> then validated by the host, which could check if the planes end up in
> the same buffer (or different buffers for (3)) and/or at the right
> offsets.
>

It sounds reasonable. Even in the protocol design we discussed so far,
the driver sends an array of struct virtio_video_mem_entry in the
RESOURE_CREATE command:
struct virtio_video_mem_entry {
  le64 addr;
  le32 length;
  u8 padding[4];
};

struct virtio_video_resource_create {
  struct virtio_video_cmd_hdr hdr;
  le32 queue_type;
  le32 resource_id;
  le32 num_planes;
  le32 num_entries[VIRTIO_VIDEO_MAX_PLANES];
  u8 padding[4];
  /* Followed by struct virtio_video_mem_entry entries[] */
};

Does it match with your idea? We may need an |offset| in virtio_video_mem_entry?

Also, we should redesign "enum virtio_video_planes_layout" that we
originally discussed.
How about changing it like this?

enum virtio_video_planes_layout {
        VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER = 1 << 0,
        VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE = 1 << 1,
};

So, the device can combine flags to tell which (1), (2) or (3) it is.
Then, the device check if an incoming RESOURE_CREATE request violates
the requirement.
Does it make sense?

Best regards,
Keiichi

> WDYT?
>
> Best regards,
> Tomasz

Keiichi Watanabe Jan. 15, 2020, 11:23 a.m. UTC | #21

Hi Dmitry,
Thank you for the detailed explanation about your ideas on
planes_layout and mem_layout.

On Mon, Jan 13, 2020 at 6:50 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> 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
> >
> > <dmitry.sepp@opensynergy.com> 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
> > > > <dmitry.sepp@opensynergy.com>
> > >
> > > 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
> > > > > > > <dmitry.sepp@opensynergy.com>
> > > > > >
> > > > > > 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
> > > > > > > > > <dmitry.sepp@opensynergy.com>
> > > > > > > >
> > > > > > > > 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
> > > > > > > > > > > <tfiga@chromium.org>
> > > > > >
> > > > > > wrote:
> > > > > > > > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > > > > > > > <keiichiw@chromium.org>
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > > > > > Hi Dmitry,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > > > > > > >
> > > > > > > > > > > > > <dmitry.sepp@opensynergy.com> 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
> > > > > > > > > > > > > > > capabilities.
> > > > > > > > > > > > > > > * Added a way to represent supported combinations
> > > > > > > > > > > > > > > of
> > > > > > > > > > > > > > > formats.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >   - A field "mask" in virtio_video_format_desc
> > > > > > > > > > > > > > >   plays
> > > > > > > > > > > > > > >   this
> > > > > > > > > > > > > > >   role.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP}
> > > > > > > > > > > > > > > because
> > > > > > > > > > > > > > > they
> > > > > > > > > > > > > > > don't
> > > > > > > > > > > > > > > play
> > > > > > > > > > > > > > > any
> > > > > > > > > > > > > > > meaningful roles. * Removed
> > > > > > > > > > > > > > > VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > > > > > > > DETACH}_BACKING
> > > > > > > > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. *
> > > > > > > > > > > > > > > Added a
> > > > > > > > > > > > > > > way
> > > > > > > > > > > > > > > to
> > > > > > > > > > > > > > > notify/specify resource creation method.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >   - Added a feature flag.
> > > > > > > > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >   - Added crop information.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * Removed enum virtio_video_channel_type because
> > > > > > > > > > > > > > > we
> > > > > > > > > > > > > > > can
> > > > > > > > > > > > > > > get
> > > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > information by image format.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Could you please explain this? How do you get the
> > > > > > > > > > > > > > information?
> > > > > > > > > > > > >
> > > > > > > > > > > > > It means that if image formats are well-defined,
> > > > > > > > > > > > > channel
> > > > > > > > > > > > > information
> > > > > > > > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Suppose you have some piece of HW on the host side
> > > > > > > > > > > > > > that
> > > > > > > > > > > > > > wants
> > > > > > > > > > > > > > I420
> > > > > > > > > > > > > > as
> > > > > > > > > > > > > > one
> > > > > > > > > > > > > > contig buffer w/ some offsets. But on the driver
> > > > > > > > > > > > > > side,
> > > > > > > > > > > > > > say,
> > > > > > > > > > > > > > gralloc
> > > > > > > > > > > > > > gives you three separate buffers, one per channel.
> > > > > > > > > > > > > > How
> > > > > > > > > > > > > > do we
> > > > > > > > > > > > > > pass
> > > > > > > > > > > > > > those to the device then?
> > > > > > > > > > > > >
> > > > > > > > > > > > > You're talking about CrOS use case where buffers are
> > > > > > > > > > > > > allocated
> > > > > > > > > > > > > by
> > > > > > > > > > > > > virtio-gpu, right?
> > > > > > > > > > > > > In this case, virtio-gpu allocates one contiguous
> > > > > > > > > > > > > host-side
> > > > > > > > > > > > > buffer
> > > > > > > > > > > > > and
> > > > > > > > > > > > > the client regards a pair of (buffer FD, offset) as
> > > > > > > > > > > > > one
> > > > > > > > > > > > > channel.
> > > > > > > > > > > > > And, we can register this pair to the device when the
> > > > > > > > > > > > > buffer
> > > > > > > > > > > > > is
> > > > > > > > > > > > > imported.
> > > > > > > > > > > > > In the virtio-vdec spec draft, this pair corresponds
> > > > > > > > > > > > > to
> > > > > > > > > > > > > struct
> > > > > > > > > > > > > virtio_vdec_plane in struct virtio_vdec_plane.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So, I suppose we will need similar structs when we add
> > > > > > > > > > > > > a
> > > > > > > > > > > > > control
> > > > > > > > > > > > > to
> > > > > > > > > > > > > import buffers. However, I don't think it's necessary
> > > > > > > > > > > > > when
> > > > > > > > > > > > > guest
> > > > > > > > > > > > > pages
> > > > > > > > > > > > > are used.
> > > > > > > > > > > >
> > > > > > > > > > > > I think we need some way for the guest to know whether
> > > > > > > > > > > > it
> > > > > > > > > > > > can
> > > > > > > > > > > > allocate
> > > > > > > > > > > > the planes in separate buffers, even when guest pages
> > > > > > > > > > > > are
> > > > > > > > > > > > used.
> > > > > > > > > > > > This
> > > > > > > > > > > > would be equivalent to V4L2 M and non-M formats, but
> > > > > > > > > > > > mixing
> > > > > > > > > > > > this
> > > > > > > > > > > > into
> > > > > > > > > > > > FourCC in V4L2 is an acknowledged mistake, so we should
> > > > > > > > > > > > add
> > > > > > > > > > > > a
> > > > > > > > > > > > query or
> > > > > > > > > > > > something.
> > > > > > > > > >
> > > > > > > > > > Yes, this is what I mean. In fact, we already do face the
> > > > > > > > > > situation
> > > > > > > > > > when
> > > > > > > > > > the device side is not happy with the sgt and wants contig.
> > > > > > > > > > I
> > > > > > > > > > think
> > > > > > > > > > we'll
> > > > > > > > > > add a module parameter for now.
> > > > > > > > >
> > > > > > > > > Okay. So, I suppose we'll be able to update structs:
> > > > > > > > > * Add a flag in virtio_video_format_desc that indicates
> > > > > > > > > whether
> > > > > > > > > planes
> > > > > > > > > can be in separate buffers, and
> > > > > > > > > * Add a flag in virtio_video_format_desc that indicates that
> > > > > > > > > the
> > > > > > > > > device requires contiguous buffers for this format.
> > > > > > > > >
> > > > > > > > > Does it make sense?
> > > > > > > >
> > > > > > > > Sorry, I don't understand the difference between the two above:
> > > > > > > > isn't
> > > > > > > > the
> > > > > > > > first case is just when the flag is not set?
> > > > > > >
> > > > > > > Ah, I was confused and wrote something strange. Yeah,  these two
> > > > > > > are
> > > > > > > the
> > > > > > > same. Sorry for that.
> > > > > > >
> > > > > > > So, the suggestion is to add a field "planes_layout" in
> > > > > > > virtio_video_format_desc, which is one of the following enums:
> > > > > > >
> > > > > > > enum virtio_video_planes_layout {
> > > > > > >
> > > > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* no special
> > > > > > >     requirement
> > > > > > >     */
> > > > > > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIGUOUS,
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > If we have a better idea or naming, please let me know.
> > > > > >
> > > > > > The naming looks good for me, I might only change to CONTIG as we
> > > > > > have
> > > > > > UNSPEC.
> > > > >
> > > > > So here we are talking about plane layout in memory, am I correct? But
> > > > > I
> > > > > think we also need a way to communicate memory requirements of the
> > > > > device: the device might require CMA buffers or it can be ok with SG
> > > > > lists. What about adding something like this to
> > > > > virtio_video_format_desc:
> > > > >
> > > > > enum virtio_video_mem_layout {
> > > > >
> > > > >         VIRTIO_VIDEO_MEM_LAYOUT_UNDEFINED = 0,
> > > > >
> > > > >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 0x100,
> > > > >         VIRTIO_VIDEO_MEM_LAYOUT_NON_CONTIG,
> > > > >
> > > > > };
> > > > >
> > > > > struct virtio_video_format_desc {
> > > > >
> > > > >     __le64 mask;
> > > > >     __le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > > >     __le32 planes_layout; /* One of VIRTIO_VIDEO_PLANES_LAYOUT_* types
> > > > >     */
> > > > >     __le32 mem_layout; /* One of VIRTIO_VIDEO_MEM_LAYOUT_* types */
> > > > >     ...
> > > > >
> > > > > };
> > > >
> > > > Good.
> > > > I'd not like to call it NON_CONTIG, as it sounds like CMA buffers
> > > > aren't allowed.
> > > > Instead, how about this definition?
> > > >
> > > > enum virtio_video_mem_layout {
> > > >
> > > >         VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC = 0,  /* no special requirement
> > > >         */
> > > >         VIRTIO_VIDEO_MEM_LAYOUT_CONTIG = 1,
> > > >
> > > > };
> > >
> > > Yes, I agree, that would be more correct. We just need an empty line to be
> > > aligned with other enums.
> > >
> > > > With this enum,
> > > > * the device can simply ignore this field if it doesn't have any
> > > > requirement and the struct is zero-initialized, and
> > > > * if we need to add other types of memory layout requirements, we can
> > > > add them as 2, 4, 8, etc to represent combinations of requirements.
> > > >
> > > > Just to confirm, are the following combination of planes_layout and
> > > > mem_layout valid?
> > > > (1) (planes_layout, mem_layout) = (contig, not specified)
> > > > (2) (planes_layout, mem_layout) = (not specified, contig)
> > > >
> > > > In my understanding, (1) means that each plane must be a contiguous
> > > > buffer while different planes don't have to be in a contig memory, but
> > > > (2) is invalid.
> > > > Is it correct?
> > >
> > > Let me tell a bit more about my vision:
> > >
> > > mem_layout: device can handle SG lists (e.g. using iommu) or it cannot and
> > > needs CMA. If it can handle SG, CMA is also ok. So it is ether 'I don't
> > > care' or 'give me CMA'.
> > >
> > > planes_layout: some devices might want to see multiplanar data in one
> > > buffer. So if we allocate two sets of mem entries (one per each plane)
> > > and send them to the host, the device will fail to handle those.
> > >
> > > So from my perspective you example can be interpreted as follows:
> > > (1) means all your planes are hosted by one buffer instance (we can see it
> > > as one fd for all planes plus per-plane offsets inside the buffer), the
> > > buffer itself consists of arbitrary pages (or can consist, as it can of
> > > course also be CMA, because of UNSPEC).
> > > (2) valid, means each plane has its own buffer, each buffer is contiguous
> > > in memory (must be allocated from CMA), but planes are not necessarily
> > > adjacent to each other in memory.
> > >
> > > 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.
> >
> > I might be misunderstanding the above, but just to make sure we're on
> > the same page, here are the cases that we found to exist in practice
> > with V4L2 stateful decoders:
> >
> > 1) device expects planes in one buffer laid out one after another
> > without any padding; the device accepts only 1 pointer and no offsets
> > - this corresponds to V4L2 non-M formats, such as NV12, which exactly
> > specify the location of planes in the buffer,
> >
> > 2) device accepts separate pointers to all planes - the planes can be
> > located anywhere in memory, which could be separate buffers, 1 buffer
> > with planes laid out at arbitrary offsets or even exactly the same
> > layout as required for 1).
> >
> > 3) device _requires_ planes to be located in separate buffers
> > allocated from designated areas in memory, e.g. different physical
> > memory banks, for performance purposes. This is actually a very rare
> > case and observed only on old generations of Samsung Exynos SoCs.
> >
> > Please let me know if that matches your expectation,
> >
>
> Thank you for the update!
>
> Yes, I think this matches our expectations, and this also does not contradict
> the enums I provided in the previous email.
>
> Btw, you haven't mentioned anything regarding mem layout. Do you agree with
> what was proposed?

Tomasz, do you find any thought on mem layout Dmitry suggested?

I have one question about mem layout.
When the device reports VIRTIO_VIDEO_MEM_LAYOUT_UNSPEC, doesn't the
driver want to tell whether it uses contiguous memory or not?
In other words, I'm wondering if we should have a field "mem_layout"
in virtio_video_resource_create to store one of enum
virtio_video_mem_layout type.
If so,  we might want to add a new type like
VIRTIO_VIDEO_MEM_LAYOUT_SG_LIST to tell that contiguous memories
aren't used.

Best regards,
Keiichi


Best regards,
Keiichi

>
> Best regards,
> Dmitry
>
> > Best regards,
> > Tomasz
> >
> > > So it should look like this:
> > >
> > > enum virtio_video_planes_layout {
> > >
> > >     VIRTIO_VIDEO_PLANES_LAYOUT_UNSPEC = 0,  /* default, invalid */
> > >
> > >     VIRTIO_VIDEO_PLANES_LAYOUT_CONTIG = 0x100,
> > >     VIRTIO_VIDEO_PLANES_LAYOUT_NON_CONTIG,
> > >
> > > };
> > >
> > > Best regards,
> > > Dmitry
> > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > > Best regards,
> > > > > Dmitry.
> > > > >
> > > > > > Best regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > > Best regards,
> > > > > > > Keiichi
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Dmitry.
> > > > > > > >
> > > > > > > > > Best regards,
> > > > > > > > > Keiichi
> > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Dmitry.
> > > > > > > > > >
> > > > > > > > > > > > For future V4L2 development we came up with the idea of
> > > > > > > > > > > > a
> > > > > > > > > > > > format
> > > > > > > > > > > > flag
> > > > > > > > > > > > which could mean that the hardware allows putting planes
> > > > > > > > > > > > in
> > > > > > > > > > > > separate
> > > > > > > > > > > > buffers. We could have a similar per-format flag in the
> > > > > > > > > > > > capabilities,
> > > > > > > > > > > > as we already have a list of all the supported formats
> > > > > > > > > > > > there.
> > > > > > > > > > >
> > > > > > > > > > > Sorry, forgot to paste the link from future V4L2 work
> > > > > > > > > > > notes
> > > > > > > > > > > from
> > > > > > > > > > > this
> > > > > > > > > > > year
> > > > > > > > > > > ELCE:
> > > > > > > > > > > https://www.spinics.net/lists/linux-media/msg159789.html
> > > > > > > > > > >
> > > > > > > > > > > > Best regards,
> > > > > > > > > > > > Tomasz
> > > > > > > > > > > >
> > > > > > > > > > > > > Best regards,
> > > > > > > > > > > > > Keiichi
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Best regards,
> > > > > > > > > > > > > > Dmitry.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * Renamed virtio_video_pin to
> > > > > > > > > > > > > > > virtio_video_buf_type.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >   - It's similar to V4L2_BUF_TYPE_VIDEO_{OUTPUT,
> > > > > > > > > > > > > > >   CAPTURE}.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > * Added an error event.
> > > > > > > > > > > > > > > * Reordered some subsections.
> > > > > > > > > > > > > > > * Changed styles to make it consistent with other
> > > > > > > > > > > > > > > devices.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Dmitry Sepp (1):
> > > > > > > > > > > > > > >   virtio-video: Add virtio video device
> > > > > > > > > > > > > > >   specification
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >  content.tex      |   1 +
> > > > > > > > > > > > > > >  virtio-video.tex | 579
> > > > > > > > > > > > > > >  +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > > > > >  2 files changed, 580 insertions(+)
> > > > > > > > > > > > > > >  create mode 100644 virtio-video.tex
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > --
> > > > > > > > > > > > > > > 2.24.1.735.g03f4e72817-goog
> > > > > >
> > > > > > --------------------------------------------------------------------
> > > > > > -
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail:
> > > > > > virtio-dev-help@lists.oasis-open.org
>
>

Gerd Hoffmann Jan. 15, 2020, 11:26 a.m. UTC | #22

Hi,

> > 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.
> 
> That's true, simple user space would often do that. However, if more
> devices are in the game, often some extra alignment or padding between
> planes is needed and that is not allowed by (1), even though all the
> planes are in the same buffer.
> 
> My suggestion, based on the latest V4L2 discussion on unifying the
> UAPI of i) and ii), is that we may want to instead always specify
> buffers on a per-plane basis. Any additional requirements would be
> then validated by the host, which could check if the planes end up in
> the same buffer (or different buffers for (3)) and/or at the right
> offsets.

Hmm, using (ii) the API, then check whenever your three plane buffers
happen to have the correct layout for (1) hardware looks somewhat
backwards to me.

I'd suggest to use (i) API and allow the device specify alignment
requirements.  So (1) hardware would say "need_align=0", whereas (3)
hardware would probably say "need_align=PAGE_SIZE" so it can easily
split the single buffer into three per-plane buffers.

cheers,
  Gerd

Keiichi Watanabe Jan. 20, 2020, 7:20 a.m. UTC | #23

Hi,

On Wed, Jan 15, 2020 at 8:26 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > 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.
> >
> > That's true, simple user space would often do that. However, if more
> > devices are in the game, often some extra alignment or padding between
> > planes is needed and that is not allowed by (1), even though all the
> > planes are in the same buffer.
> >
> > My suggestion, based on the latest V4L2 discussion on unifying the
> > UAPI of i) and ii), is that we may want to instead always specify
> > buffers on a per-plane basis. Any additional requirements would be
> > then validated by the host, which could check if the planes end up in
> > the same buffer (or different buffers for (3)) and/or at the right
> > offsets.
>
> Hmm, using (ii) the API, then check whenever your three plane buffers
> happen to have the correct layout for (1) hardware looks somewhat
> backwards to me.

Can't this problem be solved by adding "offset" field in virtio_video_mem_entry?

struct virtio_video_mem_entry {
  le64 addr;
  le32 length;
  le32 offset;
  u8 padding[4];
};

Here, "addr" must be the same in every mem_entry for (1) hardware.

>
> I'd suggest to use (i) API and allow the device specify alignment
> requirements.  So (1) hardware would say "need_align=0", whereas (3)
> hardware would probably say "need_align=PAGE_SIZE" so it can easily
> split the single buffer into three per-plane buffers.

Just to confirm, is "need_align" a field added in virtio_video_format_desc?
It sounds workable, too.

Best regards,
Keiichi


>
> cheers,
>   Gerd
>

Gerd Hoffmann Jan. 20, 2020, 10:47 a.m. UTC | #24

Hi,

> > Hmm, using (ii) the API, then check whenever your three plane buffers
> > happen to have the correct layout for (1) hardware looks somewhat
> > backwards to me.
> 
> Can't this problem be solved by adding "offset" field in virtio_video_mem_entry?
> 
> struct virtio_video_mem_entry {
>   le64 addr;
>   le32 length;
>   le32 offset;
>   u8 padding[4];
> };
> 
> Here, "addr" must be the same in every mem_entry for (1) hardware.

No.  virtio_video_mem_entry is basically a scatter list entry, you use
an *array* of these entries to describe your buffer (unless you are
using CMA due to hardware requirements, in this special case you have
only one entry in your array).

> > I'd suggest to use (i) API and allow the device specify alignment
> > requirements.  So (1) hardware would say "need_align=0", whereas (3)
> > hardware would probably say "need_align=PAGE_SIZE" so it can easily
> > split the single buffer into three per-plane buffers.
> 
> Just to confirm, is "need_align" a field added in virtio_video_format_desc?

Given that different formats might have different alignment
requirements this looks like a good place to me.  Maybe rename to
plane_align to make clear what kind of alignment this is.

cheers,
  Gerd

Keiichi Watanabe Jan. 21, 2020, 2:47 a.m. UTC | #25

Hi,

On Mon, Jan 20, 2020 at 7:48 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Hmm, using (ii) the API, then check whenever your three plane buffers
> > > happen to have the correct layout for (1) hardware looks somewhat
> > > backwards to me.
> >
> > Can't this problem be solved by adding "offset" field in virtio_video_mem_entry?
> >
> > struct virtio_video_mem_entry {
> >   le64 addr;
> >   le32 length;
> >   le32 offset;
> >   u8 padding[4];
> > };
> >
> > Here, "addr" must be the same in every mem_entry for (1) hardware.
>
> No.  virtio_video_mem_entry is basically a scatter list entry, you use
> an *array* of these entries to describe your buffer (unless you are
> using CMA due to hardware requirements, in this special case you have
> only one entry in your array).

I see. I forgot about scatter list.
However, I'm still not sure about the usage for CMA.
if we're using CMA for a multiplanar format, how can the device know
where the second plane start from?
In my understanding, the number of entries in this case should be the
same with the number of planes and
"entries[0].addr + entries[0].length == entries[1].addr" should hold.

>
> > > I'd suggest to use (i) API and allow the device specify alignment
> > > requirements.  So (1) hardware would say "need_align=0", whereas (3)
> > > hardware would probably say "need_align=PAGE_SIZE" so it can easily
> > > split the single buffer into three per-plane buffers.
> >
> > Just to confirm, is "need_align" a field added in virtio_video_format_desc?
>
> Given that different formats might have different alignment
> requirements this looks like a good place to me.  Maybe rename to
> plane_align to make clear what kind of alignment this is.

"plane_align" sounds better. Thanks.

Best regards,
Keiichi

>
> cheers,
>   Gerd
>

Gerd Hoffmann Jan. 21, 2020, 6:44 a.m. UTC | #26

> > > Can't this problem be solved by adding "offset" field in virtio_video_mem_entry?
> > >
> > > struct virtio_video_mem_entry {
> > >   le64 addr;
> > >   le32 length;
> > >   le32 offset;
> > >   u8 padding[4];
> > > };
> > >
> > > Here, "addr" must be the same in every mem_entry for (1) hardware.
> >
> > No.  virtio_video_mem_entry is basically a scatter list entry, you use
> > an *array* of these entries to describe your buffer (unless you are
> > using CMA due to hardware requirements, in this special case you have
> > only one entry in your array).
> 
> I see. I forgot about scatter list.
> However, I'm still not sure about the usage for CMA.
> if we're using CMA for a multiplanar format, how can the device know
> where the second plane start from?
> In my understanding, the number of entries in this case should be the
> same with the number of planes and
> "entries[0].addr + entries[0].length == entries[1].addr" should hold.

With the one-buffer-per-frame model you could add a plane_offsets[4]
field to virtio_video_resource_create.

The virtio_video_mem_entry array describes the whole buffer for all
planes, the plane_offsets array says where the individual planes start
inside the buffer.

cheers,
  Gerd

Keiichi Watanabe Jan. 21, 2020, 8:56 a.m. UTC | #27

Hi,

On Tue, Jan 21, 2020 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > > Can't this problem be solved by adding "offset" field in virtio_video_mem_entry?
> > > >
> > > > struct virtio_video_mem_entry {
> > > >   le64 addr;
> > > >   le32 length;
> > > >   le32 offset;
> > > >   u8 padding[4];
> > > > };
> > > >
> > > > Here, "addr" must be the same in every mem_entry for (1) hardware.
> > >
> > > No.  virtio_video_mem_entry is basically a scatter list entry, you use
> > > an *array* of these entries to describe your buffer (unless you are
> > > using CMA due to hardware requirements, in this special case you have
> > > only one entry in your array).
> >
> > I see. I forgot about scatter list.
> > However, I'm still not sure about the usage for CMA.
> > if we're using CMA for a multiplanar format, how can the device know
> > where the second plane start from?
> > In my understanding, the number of entries in this case should be the
> > same with the number of planes and
> > "entries[0].addr + entries[0].length == entries[1].addr" should hold.
>
> With the one-buffer-per-frame model you could add a plane_offsets[4]
> field to virtio_video_resource_create.
>
> The virtio_video_mem_entry array describes the whole buffer for all
> planes, the plane_offsets array says where the individual planes start
> inside the buffer.

Got it. It makes sense to have plane_offsets[] separately.
Thanks!

Best regards,
Keiichi

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

[v2,0/1] VirtIO video device specification

Message

Comments