[Bug 779146] New: dmabuf: be able to negotiate tiled surfaces

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Bug ID: 779146
Summary: dmabuf: be able to negotiate tiled surfaces
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: enhancement
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@intel.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

The memory format natively supported by intel-vaapi-driver is Y-tiled NV12 for
YUV 420 surfaces. Currently the gstreamer-vaapi dmabuf allocator only allocates
non-tiled surfaces which the intel-vaapi-driver needs to then tile before
encoding/untile after decoding.

Having some way of negotiating support for tiling would save this unnecessary
tiling or untiling step. I would appreciate input on the best way to implement
this. Should it be a new video format, like NV12_128Y32? A video format is a
little funny to me because this is for dmabuf negotiation only and the tiled
plane will never be seen in virtual memory. Should there be some gem-specific
dmabuf allocation query parameters?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ndufresne.ca

--- Comment #1 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
If the dmabuf are not mapable to CPU, I don't think it's worth adding a new
format. When I added NV12_64Z32, the dmabufs were all mappable (CMA). Exposing
the format was a great way to help with testing, as I could then use
videoconvert and videotestsrc to produce data that I could just copy into those
DMA buffers. On Exynos 4412, this format came back in many IP blocks, so it was
very useful to test each blocks seperatly.

So question is, is this format mapable ? Would having videotestsrc or
videoconvert support for this format useful ?

I was thinking that we should expose something like:

video/x-raw-opaque(memory:DMABuf),format=XYZ

In some cases.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #2 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #1)

Post by "GStreamer" (GNOME Bugzilla)
If the dmabuf are not mapable to CPU, I don't think it's worth adding a new
format. When I added NV12_64Z32, the dmabufs were all mappable (CMA).
Exposing the format was a great way to help with testing, as I could then
use videoconvert and videotestsrc to produce data that I could just copy
into those DMA buffers. On Exynos 4412, this format came back in many IP
blocks, so it was very useful to test each blocks seperatly.
So question is, is this format mapable ? Would having videotestsrc or
videoconvert support for this format useful ?

The format is mappable, but the memory controller makes the memory look linear
in virtual memory when you do the VA-API map call. To actually view the memory
as tiled you would need to do some side-run around vaapi's map api.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #3 from Julien Isorce <***@gmail.com> ---
Hi Scott, do you mean that here
https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/gstvaapipluginbase.c#n512
you would like to remove the LINEAR flag in some cases ? Like the idea in line
552 (note that it is not really implemented yet).
Except that in your case it will be even more tricky, i.e. you would like to be
tiled even when not using the same devices for import / export ? (ex: v4l2src !
vaapienc ?)

The general idea line 552 is to say that we want linear when using same devices
for import / export or export / import. And tiled if same devices. We could
change "same devices" to "compatible devices". And we could also do that with
list/map of vendor ids / device ids. Do you think that would work ? And/or do
you have another idea how to detect that ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #4 from Scott D Phillips <***@intel.com> ---
(In reply to Julien Isorce from comment #3)

Post by "GStreamer" (GNOME Bugzilla)
Hi Scott, do you mean that here
https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/
gstvaapipluginbase.c#n512 you would like to remove the LINEAR flag in some
cases ?

Yes, exactly that.

Post by "GStreamer" (GNOME Bugzilla)
Like the idea in line 552 (note that it is not really implemented
yet).
Except that in your case it will be even more tricky, i.e. you would like to
v4l2src ! vaapienc ?)

Right, the exact case I have in mind is (icamerasrc ! vaapih264enc)
https://github.com/01org/icamerasrc . The icamerasrc uses a separate image
processing unit that is capable of supporting the same Y-tiled format. Right
now that source element has tiling hardcoded off, but ideally it would enable
tiling when sharing dmabuf memory with vaapih264enc and maybe even mesa(?).

Post by "GStreamer" (GNOME Bugzilla)
The general idea line 552 is to say that we want linear when using same
devices for import / export or export / import. And tiled if same devices.
We could change "same devices" to "compatible devices". And we could also do
that with list/map of vendor ids / device ids. Do you think that would work
? And/or do you have another idea how to detect that ?

Right, I'm not really sure what the best way to do this is. I was thinking
something along the lines of custom flags in an allocation query but I don't
know what the best way would be end-to-end.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #5 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
OK, so you basically want to negotiate a second format, which is the hardware
format. I suppose this format can be described as a DRM 64bit format (with
modifiers). If you want to use the allocation query, you'll probably need to
set the supported backend formats, when known (linear is assumed if unknown).
This way, the information can be read by exporters downstream. Then downstream
will intersect with it's on set of backend formats (this will mostly be 1
format), and return back the information.

This way, regardless who's the exporter, both side can make the right decision.
This is all doable with allocation_meta(). Which is just a GType + a structure.
We could stage this work in plugin-bad allocator library to start with, and
when happy, we could eventually move that to the -base allocator library. What
we need there is to register the GType and add documentation about the
GstStructure layout. Ideally, the formats, which will by definition not be
GstVideoFormat, shall have a unique definition. To recap, let's run through two
use cases:

v4l2src ! vaapienc

v4l2src will run an allocation query, the GsDmabufBackedFormatMeta will be
missing, because V4L2 does not expose this information. VAAPI will receive the
empty allocation query in propose_allocation(), and decide to go linear.

icamerasrc ! vaapienc

icamerasrc will set GsDmabufBackedFormatMeta,foramt=INTEL_Y_TILED,LIENEAR.
VAAPI will read find this meta, as it supports this tiling, it will leave the
meta as-is, and return the filled allocation query. At this point, both
elements knows they can operate with tiles.

randomcamsrc ! vaapienc

randomcamsrc will set GsDmabufBackedFormatMeta,foramt=REANDOM_FORMAT,LINEAR.
VAAPI will read the meta, and fine that the intersection is LINEAR. VAAPI will
update the allocation query, and return.

randomcamsrc2 ! vaapienc

randomcamsrc2 will set GsDmabufBackedFormatMeta,RANDOM_FORMAT,RANDOM_FORMAT_2.
VAAPI won't recognize any format, and will remove all of them. Both side will
know that it's not worth bothering doing DMABuf.

This might be over-simplistic, and extremely Intel specific, but could serve as
a base. It could also be GstDmabufConstraintMeta. And formats could just be one
of the parameter (e.g. allocation type like CMA, SG/page-aligned, vmalloc,
memory alignement, etc). We are a bit ahead of what the kernel currently expose
to usespace though.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #6 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(We should put extra care, so gst_structure_intersect() can be used, otherwise
it's just a pain, just thinking about the rest of the alocation query items)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igalia.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #7 from Julien Isorce <***@gmail.com> ---
I think that Nicolas's suggestion is a very good idea. Let's try to apply it on
the following use cases:

vaapidec ! glimagesink doing HW decoding on integrated intel gpu and GL
rendering on the integrated gpu too.

vaapidec will set GsDmabufBackedFormatMeta,format=INTEL_Y_TILED,LINEAR. GstGL
will read the meta, and fine that the intersection is INTEL_Y_TILED. GstGL will
update the allocation query, and return. So that vaapidec will clear the linear
flag for the vaapi driver which will work in tiled mode.

vaapidec ! glimagesink doing HW decoding on integrated intel gpu and GL
rendering on discrete AMD or NVIDIA gpu.

vaapidec will set GsDmabufBackedFormatMeta,format=INTEL_Y_TILED,LINEAR. GstGL
will read the meta, and fine that the intersection is LINEAR. GstGL will update
the allocation query, and return. So that vaapidec will set the linear flag for
the vaapi driver.

vaapidec ! glimagesink doing HW decoding on discrete NVIDIA/nouveau gpu and
GL rendering on the integrated intel gpu.

vaapidec will set GsDmabufBackedFormatMeta,format=NVIDIA_Y_TILED,LINEAR. GstGL
will read the meta, and fine that the intersection is LINEAR. GstGL will update
the allocation query, and return. So that vaapidec will set the linear flag for
the vaapi driver.

And so on. But then I think the format modifiers should also appear in -base::
GstDmaBufAllocator. As there is already gst_dmabuf_memory_get_fd, we could add
gst_dmabuf_memory_get_modifier. So that then in GstGL we could pass the
modifiers LO/HI to eglCreateImage in gst_egl_image_from_dmabuf. Also see
https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt
.

Extract from drmfourcc.h:

/* Vendor Ids: */
#define DRM_FORMAT_MOD_NONE 0
#define DRM_FORMAT_MOD_VENDOR_INTEL 0x01
#define DRM_FORMAT_MOD_VENDOR_AMD 0x02
#define DRM_FORMAT_MOD_VENDOR_NV 0x03
#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04
#define DRM_FORMAT_MOD_VENDOR_QCOM 0x05
/* add more to the end as needed */

#define fourcc_mod_code(vendor, val) \
((((__u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val &
0x00ffffffffffffffULL))

#define I915_FORMAT_MOD_X_TILED fourcc_mod_code(INTEL, 1)
#define I915_FORMAT_MOD_Y_TILED fourcc_mod_code(INTEL, 2)
#define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
#define DRM_FORMAT_MOD_SAMSUNG_64_32_TILE fourcc_mod_code(SAMSUNG, 1)

I can see there are 3 different way to do tiling for intel: X, Y, Yf. But the
vaapi spec (va.h from libva) only defines one flag:
VA_SURFACE_EXTBUF_DESC_ENABLE_TILING. But I am guessing this is not really a
problem because at importation time it will internally call
"libdrm::dri_bo_get_tiling" to retrieve exact internal tiling and swizzle.

Also maybe the name should be GsDmabufModifierMeta instead of
GsDmabufBackedFormatMeta ?
Or could we just add a new field "guint64 tiling/format_modifier" to the
existing -base::GstVideoMeta ? + eventually a new GstVideoFrameFlags
GST_VIDEO_FRAME_FLAG_LINEAR which would indicates that when mapping the dmabuf
it will be accessible as linear even if the modifier indicates tiled.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #8 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I think you have narrowed something here. Basically, there is DRM modifiers
that may not be reflected by GstVideoFormat (because there is memory hardware
to hide it, or simply that the GstVideoFormat does not matter, the memory isn't
mappable). I like GsDmabufModifierMeta, but I wonder if we shouldn't call those
GsDmabufDrmModifierMeta for extra clarity. I would not merge this into
GstVideo, because it's really an extension, specific to DMABuf and DRM BOs
handles. Obviously, the DMABuf handles are the one to be shared between
components.

Small step back, I'd probably define an allocation meta as GstDmabufMeta, and
make the drm-modifiers one of the field. This way we can extend later, e.g.
with the kernel-allocator, or other things like that (if that ever get exposed
obviously).

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Daniel Stone <***@fooishbar.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@fooishbar.org

--- Comment #9 from Daniel Stone <***@fooishbar.org> ---
(In reply to Julien Isorce from comment #7)

Post by "GStreamer" (GNOME Bugzilla)
I can see there are 3 different way to do tiling for intel: X, Y, Yf. But
VA_SURFACE_EXTBUF_DESC_ENABLE_TILING. But I am guessing this is not really a
problem because at importation time it will internally call
"libdrm::dri_bo_get_tiling" to retrieve exact internal tiling and swizzle.

The get_tiling() call is very much unsupported, and doesn't even work for some
of the newer tiling formats. The replacement is to pass modifiers all along the
way - which I suppose would make the libva API insufficient.

Post by "GStreamer" (GNOME Bugzilla)
Also maybe the name should be GsDmabufModifierMeta instead of
GsDmabufBackedFormatMeta ?
Or could we just add a new field "guint64 tiling/format_modifier" to the
existing -base::GstVideoMeta ? + eventually a new GstVideoFrameFlags
GST_VIDEO_FRAME_FLAG_LINEAR which would indicates that when mapping the
dmabuf it will be accessible as linear even if the modifier indicates tiled.

Please just format_modifier, and it's not just for tiling. Support is coming up
for render-compression modes from various hardware vendors, which is either a
single buffer in a proprietary compression format, or a normal flat buffer with
external compression metadata (thus occupying more space in memory, but needing
to read less of it) in another plane.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #10 from Scott D Phillips <***@intel.com> ---
Created attachment 347168
--> https://bugzilla.gnome.org/attachment.cgi?id=347168&action=edit
[PATCH gst-plugins-bad 1/2] allocators: Add GstDmabufDrmModifierMeta

This meta carries the drm modifier value for the plane contained
in attached dmabuf memory. The modifier expresses platform
specific swizzling/tiling/compression/etc and should only be
attached to buffer if the src and sink have both indicated support
for it during the allocation query.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #11 from Scott D Phillips <***@intel.com> ---
Created attachment 347169
--> https://bugzilla.gnome.org/attachment.cgi?id=347169&action=edit
[PATCH gst-plugins-bad 2/2] allocators: add meson.build support

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #12 from Scott D Phillips <***@intel.com> ---
Here is a patch to add GstDmabufDrmModifierMeta as discussed. I'll work on
adding usage of it into vaapi. Is there a more formal way to document the
structure in the allocation query or should there be an intersect function or
something so that the structure representation is internal?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #13 from Julien Isorce <***@gmail.com> ---
Review of attachment 347168:
--> (https://bugzilla.gnome.org/review?bug=779146&attachment=347168)

Overall looks good thx! Just a few minor remarks. Also the name should be
reviewed by Nicolas too since I am confused with comment #8, see my question in
the review.

::: gst-libs/gst/allocators/gstdmabufdrmmodifiermeta.c
@@ +88,3 @@
+
+static void
+gst_dmabuf_drm_modifier_meta_free (GstMeta * sync_meta, GstBuffer * buffer)

What is the "sync_" ?

@@ +89,3 @@
+static void
+gst_dmabuf_drm_modifier_meta_free (GstMeta * sync_meta, GstBuffer * buffer)
+{

Should it set the modifier to 0 /* DRM_FORMAT_MOD_NONE */ for cosmetic reasons
?

@@ +147,3 @@
+ g_strdup_printf
+ ("GstDmabufDrmModifierMeta, dmabuf.drm_modifier=(guint64){ %" PRIu64
+ " };", modifier);

G_GUINT64_FORMAT instead of PRIu64 ?

::: gst-libs/gst/allocators/gstdmabufdrmmodifiermeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 modifier;

I think comment #8 says GstDmabufMeta with field drm_modifier but I may have
missed something.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #14 from Scott D Phillips <***@intel.com> ---
Ah ok, I think I misunderstood. So this new meta will be GstDmabufMeta and will
be used to negotiate the use of dmabuf at all. Also, as a feature on the side,
will be the drm modifier list which may be present and may be used to negotiate
something other than DRM_FORMAT_MOD_NONE. Does that sound right?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #15 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
More of less, GstDmabufMeta, will first only implement the modifiers. Modifiers
are still negotiated through the allocation query, and then set in this meta.
Not all modifiers will go through this though, e.g. the Samsung Tile won't, as
it already has GST_VIDEO_FORMAT_NV12_64Z32 that matches, and the memory control
won't "fix it" if you map it. So the format from userspace point of view is the
same as the hardware one.

Why using a generic meta name, well, there is other type of information that
would later be attached. One potential subject being discussed, is the Y-FLIP
flags (specific to some DMABuf create from GL using specific stack, not sure
anything else then Renesas behaves this way ...)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #16 from Daniel Stone <***@fooishbar.org> ---
(In reply to Nicolas Dufresne (stormer) from comment #15)

Post by "GStreamer" (GNOME Bugzilla)
More of less, GstDmabufMeta, will first only implement the modifiers.
Modifiers are still negotiated through the allocation query, and then set in
this meta. Not all modifiers will go through this though, e.g. the Samsung
Tile won't, as it already has GST_VIDEO_FORMAT_NV12_64Z32 that matches, and
the memory control won't "fix it" if you map it. So the format from
userspace point of view is the same as the hardware one.

Again, this isn't actually true.

For Intel, X-tiled and Y-tiled surfaces _can_ be linearly mapped through the
GTT, but do not have to be; there is a resource/performance tradeoff to doing
so. From my understanding, Yf- and Ys-tiled _cannot_ be linearly mapped. CCS
(render compression) definitely cannot be linearly mapped, at least for write
access. AFBC (ARM framebuffer compression) cannot be mapped to userspace in a
linear uncompressed view either, at least on hardware I know of. And that's
just scraping the surface of available modifiers.

The very specific case of Intel's current VA-API driver with two particular
tiling formats does expose a linear view, but this is not and will not be true
of even the majority of cases, let alone all.

If you depend on the hardware exposing a view which matches only the format
(rather than format+modifier), this API is broken and needs to be fixed before
it can be used for anything real.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #17 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Not sure why you state the Exynos case for stating that this isn't true. The
integration choices remains the same, regardless of what your hardware is.

I'm giving the option to:
1. Add a new GST format, like NV12_64Z32
2. Expose the format seen on map() and add the modifiers needed for the rest
3. Disable map() using Dmabuf caps feature, and expose the format + modifiers

Case 1 will enable full GStreamer support, you'll be able to software color
convert, possibly even use overlays and stuff. With case 2, you also get full
support, with some extra cost when doing map(), and you need to ensure
downstream can support that modifier, as the caps won't cover it.

The case 3, well, it's a bit unfortunate, all you'll be able to do is pass it
up to another driver. But that driver could be GL with the new extension, so it
remains quite flexible.

So what matters here, if you think this is broken design, is to describe what
isn't covered. It does not matter to me if the implementation in VA is wrong or
not, we'll catch this at review or testing. This of course is just the
theoretical basis, a lot of this is currently impossible / not-exposed by the
kernel interface (e.g. DRM). In VAAPI, there is hardware specific user-space
driver were you can hardcode this knowledge.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #18 from Scott D Phillips <***@intel.com> ---
(In reply to Daniel Stone from comment #16)

Post by "GStreamer" (GNOME Bugzilla)
If you depend on the hardware exposing a view which matches only the format
(rather than format+modifier), this API is broken and needs to be fixed
before it can be used for anything real.

The fact that a surface can be mapped linear isn't really important here, and
the proposed GstDmabufMeta isn't depending on that. It is just a mechanism to
negotiate the mutual support of some vendor-specific modified format between
elements.

(In reply to Nicolas Dufresne (stormer) from comment #17)

Post by "GStreamer" (GNOME Bugzilla)
1. Add a new GST format, like NV12_64Z32

In the case of NV12 + I915_TILING_Y, I don't think adding it as a
GstVideoFormat would be practical (ignoring the fact that it can be mapped
linearly). The specifics of that tiling format vary by processor generation and
even hardware configuration (some address bits get swizzled when multiple
memory channels are present to balance accesses).

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #19 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(In reply to Scott D Phillips from comment #18)

Post by "GStreamer" (GNOME Bugzilla)
The fact that a surface can be mapped linear isn't really important here,
and the proposed GstDmabufMeta isn't depending on that. It is just a
mechanism to negotiate the mutual support of some vendor-specific modified
format between elements.

Well, you need to negotiate the caps before the allocation (hence the
modifiers). If you don't have DMABuf caps feature in your caps, you cannot do
3. Any software in-place filters will prevent doing DMABuf caps feature here.
Now, if producing linear dmabuf cost the same as using a special map, there is
really no point looking at that, we don't need two equivalent fallback here.

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Nicolas Dufresne (stormer) from comment #17)

Post by "GStreamer" (GNOME Bugzilla)
1. Add a new GST format, like NV12_64Z32

In the case of NV12 + I915_TILING_Y, I don't think adding it as a
GstVideoFormat would be practical (ignoring the fact that it can be mapped
linearly). The specifics of that tiling format vary by processor generation
and even hardware configuration (some address bits get swizzled when
multiple memory channels are present to balance accesses).

Good to know, though the options are not mutually exclusive. I picked 1. for
the Exynos, but it does not apply to Intel titling. The current approach used
in VAAPI is 1, but with existing color formats (linear formats).

I think the simplest for now it to ignore 2. and implement 3. So basically you
keep it linear like it is now by default, and try to negotiate the tiling if
downstream has the DMABuf caps feature in it's caps. The negotiation of the
modifiers remains unaffected, but you need to negotiate the caps first to get
there anyway.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #20 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
In real life, a pipeline like:

... ! vaapidecode ! textoverlay ! vaapiencode ! ..

Will use linear memory backend. So still not copying, but will pay the
overhead. While if you do:

... ! vaapidecode ! vaapiencode ! ...

You'll negotiate DMABuf caps feature + the modifiers supported. Modififers
negotiation could eventually be added to glupload/download, waylandsink and
kmssink, but that seems far in the future though.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #21 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #20)

Post by "GStreamer" (GNOME Bugzilla)
... ! vaapidecode ! textoverlay ! vaapiencode ! ..
Will use linear memory backend. So still not copying, but will pay the

Well, with gstreamer-vaapi + intel-vaapi-driver as it exists today, the only
situation where linear surfaces are actually created is with dmabuf. In all the
non-dmabuf cases gstreamer-vaapi doesn't specify tiled or linear in allocations
and the intel-vaapi-driver chooses tiled. The reason is that the intel encode
and decode hardware units are able to operate on tiled surfaces only. When a
linear surface is given somehow then it will either by tiled by the cpu
(vaDeriveImage + memcpy) or the gpu (vaPutImage), but someone needs to pay the
tiling cost before encode can happen.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Scott D Phillips <***@intel.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347168|0 |1
is obsolete| |

--- Comment #22 from Scott D Phillips <***@intel.com> ---
Created attachment 347945
--> https://bugzilla.gnome.org/attachment.cgi?id=347945&action=edit
[PATCH gst-plugins-bad v2 1/2] allocators: Add GstDmabufMeta

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Scott D Phillips <***@intel.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347169|0 |1
is obsolete| |

--- Comment #23 from Scott D Phillips <***@intel.com> ---
Created attachment 347946
--> https://bugzilla.gnome.org/attachment.cgi?id=347946&action=edit
[PATCH gst-plugins-bad v2 2/2] allocators: add meson.build support

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347945|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347946|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #24 from Julien Isorce <***@gmail.com> ---
Thx everyone's inputs and thx Scott for the patches and its update. I think it
is safe to push it since it is quite generic and will go in -bad for now.
If it is more convenient maybe keep this same bug for the gstreamer-vaapi usage
of this meta.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347945|accepted-commit_now |reviewed
status| |

--- Comment #25 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Comment on attachment 347945
--> https://bugzilla.gnome.org/attachment.cgi?id=347945
[PATCH gst-plugins-bad v2 1/2] allocators: Add GstDmabufMeta

Not yet, we need something using it first. Never commit to a new API without
something using it first please.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347946|accepted-commit_now |reviewed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347945|reviewed |needs-work
status| |

--- Comment #26 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 347945:
--> (https://bugzilla.gnome.org/review?bug=779146&attachment=347945)

::: gst-libs/gst/allocators/gstdmabufmeta.c
@@ +25,3 @@
+ *
+ * #GstDmabufMeta carries metadata that goes along with
+ * dmabuf memory in the buffer, like drm modifier.

Document the negotiation process.

@@ +133,3 @@
+
+void
+gst_query_add_allocation_dmabuf_meta (GstQuery * query, guint64 drm_modifier)

This is confusing name, does it add the allocation meta, or it add a modifier.
To be honnest, I have never see a meta working like this. Why not simply pass
an array of guint64 here ? Also, undocumented.

::: gst-libs/gst/allocators/gstdmabufmeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 drm_modifier;

Modifiers are applied per plane in other API. This must be incorrect.

@@ +44,3 @@
+};
+
+GST_EXPORT

We don't currently use these, please remove until further notice.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #27 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
For the reference on how modifiers actually works, looks at
/usr/include/drm/drm_mode.h struct drm_mode_fb_cmd2.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #28 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #26)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.c
@@ +25,3 @@
+ *
+ * #GstDmabufMeta carries metadata that goes along with
+ * dmabuf memory in the buffer, like drm modifier.
Document the negotiation process.
@@ +133,3 @@
+
+void
+gst_query_add_allocation_dmabuf_meta (GstQuery * query, guint64
drm_modifier)
This is confusing name, does it add the allocation meta, or it add a
modifier. To be honnest, I have never see a meta working like this. Why not
simply pass an array of guint64 here ? Also, undocumented.

Sure, I can change it to an array. I suppose it will be a length and then an
array pointer, because there is no possible sentinel value for the end of an
array of modifiers (as zero is valid and not necessarily the last choice in
everyone's preference list). But yes, I can see that being more ergonomic than
what is there currently.

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 drm_modifier;
Modifiers are applied per plane in other API. This must be incorrect.
Note that even though it looks like we have a modifier
per-plane, we in fact do not. The modifier for each plane must
be identical. Thus all combinations of different data layouts
for multi plane formats must be enumerated as separate
modifiers.
@@ +44,3 @@
+};
+
+GST_EXPORT
We don't currently use these, please remove until further notice.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #29 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---

(In reply to Scott D Phillips from comment #28)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Nicolas Dufresne (stormer) from comment #26)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.c
@@ +25,3 @@
+ *
+ * #GstDmabufMeta carries metadata that goes along with
+ * dmabuf memory in the buffer, like drm modifier.
Document the negotiation process.
@@ +133,3 @@
+
+void
+gst_query_add_allocation_dmabuf_meta (GstQuery * query, guint64
drm_modifier)
This is confusing name, does it add the allocation meta, or it add a
modifier. To be honnest, I have never see a meta working like this. Why not
simply pass an array of guint64 here ? Also, undocumented.

Sure, I can change it to an array. I suppose it will be a length and then an
array pointer, because there is no possible sentinel value for the end of an
array of modifiers (as zero is valid and not necessarily the last choice in
everyone's preference list). But yes, I can see that being more ergonomic
than what is there currently.

As I already mention, use GstStructure types, so you can then
gst_structure_intersect(). We have everything needed.

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 drm_modifier;
Modifiers are applied per plane in other API. This must be incorrect.

Modifiers are per-plane, as per the Kernel API. This also need to be reflected
in the negotiations stuff (you'll notice the value 4, use GST_VIDEO_MAX_PLANES
in gst though).

http://lxr.free-electrons.com/source/include/uapi/drm/drm_mode.h#L426

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #30 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #29)

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 drm_modifier;
Modifiers are applied per plane in other API. This must be incorrect.

Modifiers are per-plane, as per the Kernel API. This also need to be
reflected in the negotiations stuff (you'll notice the value 4, use
GST_VIDEO_MAX_PLANES in gst though).
http://lxr.free-electrons.com/source/include/uapi/drm/drm_mode.h#L426

There is a single modifier for the ADDFB2 command, even though there are 4
modifier fields:

http://lxr.free-electrons.com/source/include/uapi/drm/drm_mode.h#L416

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #31 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---

(In reply to Scott D Phillips from comment #28)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Nicolas Dufresne (stormer) from comment #26)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.c
@@ +25,3 @@
+ *
+ * #GstDmabufMeta carries metadata that goes along with
+ * dmabuf memory in the buffer, like drm modifier.
Document the negotiation process.
@@ +133,3 @@
+
+void
+gst_query_add_allocation_dmabuf_meta (GstQuery * query, guint64
drm_modifier)
This is confusing name, does it add the allocation meta, or it add a
modifier. To be honnest, I have never see a meta working like this. Why not
simply pass an array of guint64 here ? Also, undocumented.

Sure, I can change it to an array. I suppose it will be a length and then an
array pointer, because there is no possible sentinel value for the end of an
array of modifiers (as zero is valid and not necessarily the last choice in
everyone's preference list). But yes, I can see that being more ergonomic
than what is there currently.

As I already mention, use GstStructure types, so you can then
gst_structure_intersect().

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/allocators/gstdmabufmeta.h
@@ +41,3 @@
+ GstMeta parent;
+
+ guint64 drm_modifier;
Modifiers are applied per plane in other API. This must be incorrect.

Modifiers are per-plane, as per the Kernel API. This also need to be reflected
in the negotiations stuff (you'll notice the value 4, but in gst use
GST_VIDEO_MAX_PLANES).

http://lxr.free-electrons.com/source/include/uapi/drm/drm_mode.h#L426

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #32 from Daniel Stone <***@fooishbar.org> ---
Check the bottom of the comment linked there: though there are per-plane
modifier fields, the kernel _requires_ them to be identical for all planes (cf.
the addfb2 ioctl handler).

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #33 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Ok, mid-air collision. I just saw the note, ok then, single modifier. I'll let
you fix the rest. You should add a unit test to verify intersection works, we
don't don't need to manually intersect the results.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #34 from Julien Isorce <***@gmail.com> ---
Hi Scott, any update on this ? Also do you have a patch to actually use this
meta in gstreamer-vaapi ? and GstGL ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #35 from Scott D Phillips <***@intel.com> ---
(In reply to Julien Isorce from comment #34)

Post by "GStreamer" (GNOME Bugzilla)
Hi Scott, any update on this ? Also do you have a patch to actually use this
meta in gstreamer-vaapi ? and GstGL ?

Per daniels's comment about libva as-is being insufficient, I've made some
patches to libva and intel-vaapi-driver:

https://github.com/scott-d-phillips/libva/commit/52e006fe0126d8535d88d045872307d8e332386f#diff-ce40de7930eb5bb0c9f5801e5101ce29

https://github.com/scott-d-phillips/intel-vaapi-driver/commit/5aebbb3043cda3626f62698f03d112a987eccc64

I haven't yet proposed these on the libva mailing list, I'm waiting for the
patches which add support for the modifiers to land in i915 and mesa. My plan
is to pick this back up when those patches are merged so we can have an
end-to-end working usecase and we know the modifier negotiation is good.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #36 from Daniel Stone <***@fooishbar.org> ---
Wow cool, thanks for doing that! The only comment I'd have is that it'd be nice
to have a creation API similar to gbm_surface_create_with_modifiers() or
VkExportImageDmaBufInfoMESAX (passed at creation time, rather than afterwards).
Both of those specify a list of acceptable modifiers the implementation can
choose from, which is nice for interop. But aside from that, looks great!

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #37 from Scott D Phillips <***@intel.com> ---
Thanks for taking a look Daniel,

(In reply to Daniel Stone from comment #36)

Post by "GStreamer" (GNOME Bugzilla)
Wow cool, thanks for doing that! The only comment I'd have is that it'd be
nice to have a creation API similar to gbm_surface_create_with_modifiers()
or VkExportImageDmaBufInfoMESAX (passed at creation time, rather than
afterwards). Both of those specify a list of acceptable modifiers the
implementation can choose from, which is nice for interop. But aside from
that, looks great!

that sounds good, I'll add a way to do that. And I guess libva will also need
something akin to eglQueryDmaBufModifiersEXT so we can do the negotiation flow
Nicolas suggests.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #38 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
(In reply to Scott D Phillips from comment #35)

Post by "GStreamer" (GNOME Bugzilla)
I haven't yet proposed these on the libva mailing list, I'm waiting for the
patches which add support for the modifiers to land in i915 and mesa. My
plan is to pick this back up when those patches are merged so we can have an
end-to-end working usecase and we know the modifier negotiation is good.

Cool! I'm waiting for them!! :)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

kevin <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #39 from kevin <***@gmail.com> ---
Do you have any update? We also need this for i.MX8 tiled and compressed video
frame buffer negotiation.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

***@intel.com changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@intel.com

--- Comment #40 from ***@intel.com ---
Mark provides a new interface in libva to offer surface metadata e.g. tiling to
the 3rd party library, could you have a look?

https://github.com/01org/libva/pull/85

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ocrete.ca

--- Comment #41 from Olivier Crête <***@ocrete.ca> ---
For all formats that are not linear and not the Samsung 64Z32 one, content of
the memory is pretty much opaque. And using capsfeatures & a custom meta makes
negotiation debugging a pain. I think it would be easier to just add a new caps
like "video/x-drm, format=(string)fourcc-from-drm_fourcc.h, modifier=(uint64),
width=, height=, framerate=".. Using video/x-raw means that we would want to
implement support for them in GstVideoInfo, but that's pretty much pointless if
the content is otherwise opaque.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #42 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
That's also an option, but it's also very under specified. One need to cary the
plane layout information, this is still a bit fuzzy, since it many cases the
driver speaks in strides and offsets, but strides is under specified for tiled
or compressed formats. This information is needed in order to get two drivers
to actually agree on what the buffer contains.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #44 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Would be nice to point to documentation that explains what the strides means,
the state of stride is terrible right now in DRM, here's what we implement in
Gst:

- If dumb buffer, strides means bit-per-pixel * width / 8
- Otherwise it's the stride of the first plane
- If it's tiled, it will follow alignment and rules, and represent the stride
if the the image was made linear

I don't want to carry this horror in Gst. I'm sure random driver create random
rules they prefer to stick with in a creative way.

The problem is that it's not clear that for N plane we have N strides, the
content of the stride is not our problem of course, if drivers don't agree (and
that will happen all the time of course) it's driver bug, as long as we can
pass this information, but for this we need to store it some not so nasty
structure.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #43 from Daniel Stone <***@fooishbar.org> ---
Stride isn't underspecified, it's an integral part of the format modifier
definition. You can have tiled surfaces with wider allocations than the display
width would suggest, just as with linear.

It's true that the meaning of stride is a lot more complex than linear, and
this often causes discussion whilst the modifier token is being defined. But
once it's locked down, it's very well defined, and essential.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #45 from Daniel Stone <***@fooishbar.org> ---
What about 'the state of stride' is terrible? For linear buffers, stride is the
number of bytes between (x,y) and (x,y+1).

As devices often have hard requirements on stride alignment, when you use an
allocator you often do not get to specify stride yourself. For dumb buffers,
there is a field in the ioctl which is filled out (along with the buffer
handle) with the stride value you must use for a given width.

As you've noted, this applies separately to each plane of a multi-plane image.
Stride being a buffer-storage property, this makes sense. Consider the
definition of the I915_FORMAT_MOD_Y_CCS modifier, where one plane is 32bpp with
supertiling, meaning you need quite a bit of padding. The other plane
represents 128 pixels horizontally in a _single byte_. You could in theory
probably have a single stride value for the overall combined image, but this
would require overallocation of the auxiliary plane by a factor of 512.

Random drivers don't create random rules, they don't disagree on what stride
means, and they're not desperate to create some random avant-garde
interpretation of it. They _do_ have their own hard requirements on how stride
must be aligned, which comes very directly from their hardware. If you ever do
find inconsistencies between drivers or random values, or anything else which
makes you think GPU drivers treat stride as a PRNG, then please let me know.

Tiling _is_ more difficult to think about, especially supertiling. But the
definition above holds; it might be easier to invert what you said about
converting to linear, and instead just imagine linear as a 1x1 tiling format.
In that case, the rule becomes 'from (x,y) to (x,y+TH), where TH is the
greatest/outer tile height in the format'.

This isn't inconsistent or even an issue that should bother GSt. If you know
the tiling format, then you know how to lay the data out including stride (as
with NV12MT). If you don't, then you don't ever see the data, and stride is
another piece of data to transfer without needing to inspect. If you have a
pipeline involving conversions from one layout to another (e.g. linear for
software processing, converted to tiled on GPU upload), then you will have
differing strides at different points in your pipeline, which is fine: the
buffers you're addressing have fundamentally different properties, and the two
strides are no more comparable than the buffers themselves.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #46 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
The mains problem remains the stride and other memory related. The thing that
comes out is that it's given back at allocation time. With this proposal, there
is no possible to fallback because GST does not have the knowledge. So
everything has to be in the caps (stride, size, alignment, number of memory
segment, etc) if you want any guarantee that the buffer exchange to work.
Allocating for every format to get the info make no sense and asking for API
will just never happen. So how do we make it work, not just sometimes and by
chance ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #47 from Daniel Stone <***@fooishbar.org> ---
(In reply to Nicolas Dufresne (stormer) from comment #46)

Post by "GStreamer" (GNOME Bugzilla)
The mains problem remains the stride and other memory related. The thing
that comes out is that it's given back at allocation time. With this
proposal, there is no possible to fallback because GST does not have the
knowledge. So everything has to be in the caps (stride, size, alignment,
number of memory segment, etc) if you want any guarantee that the buffer
exchange to work. Allocating for every format to get the info make no sense
and asking for API will just never happen. So how do we make it work, not
just sometimes and by chance ?

Sorry, I don't really understand the question: what is not working today, that
you would like to 'make work'?

If you're looking for a generic formula which will let you calculate the stride
for some given configuration in a totally generic way, no, we just don't have
that. (Though the work on the 'allocator' project is intended to provide this
to client applications, provided you can reason about producer/consumer devices
when allocating.)

Display _hardware_ (again, this is not an arbitrary rule invented for fun in
drivers because hate our users - this is literally what the hardware demands)
requires varying stride alignments, depending on all of format, usage, vendor,
generation, etc. Often 32px is fine if it's linear. But sometimes it's 512px,
or 512 bytes. If you want accelerated rotation on OMAP, it's a fixed 4096
bytes. But try not to be too generous with over-allocation, because everyone
has stride _limits_, which can be surprisingly low.

Obviously these requirements and restrictions change with the modifier, as you
discovered with NV12MT where you need to factor tile size into account as well
as alignment requirements.

The model that KMS, Wayland, EGL, Vulkan, X11 DRI3, etc are built around, is
that stride is determined (for each individual plane) in a device-specific way
by whoever performs the allocation, attached to the plane, and passed to any
consumers of the plane. Unfortunately GStreamer seems to have created stride
exactly backwards - specific stride demanded by consumer which allocator must
respect; attached to the logical image/framebuffer rather than each individual
plane memory allocation - and that is unfortunate, but we can't change our
model because it just won't work on hardware if we do.

I'm sorry I don't have better news for you, but hardware does not allow us to
tell you up front what stride will be, and we do not (yet) have a library with
the algorithms for every known piece of hardware which you can interrogate.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #48 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(In reply to Daniel Stone from comment #47)

Post by "GStreamer" (GNOME Bugzilla)
The model that KMS, Wayland, EGL, Vulkan, X11 DRI3, etc are built around, is
that stride is determined (for each individual plane) in a device-specific
way by whoever performs the allocation, attached to the plane, and passed to
any consumers of the plane.

In practice, during kmssink dev we found that most display driver does not
gives this liberty to the allocating party, resulting in hard failure (or
miss-rendering) if the expected stride and alignment is not met (also
type/location of physical memory, but that is just invisible to userpace, so I
keep it quite and hope that we have sys/iommus).

Post by "GStreamer" (GNOME Bugzilla)
Unfortunately GStreamer seems to have created
stride exactly backwards - specific stride demanded by consumer which
allocator must respect; attached to the logical image/framebuffer rather
than each individual plane memory allocation - and that is unfortunate, but
we can't change our model because it just won't work on hardware if we do.

This is a miss-interpretation of what GStreamer do with strides, and a
miss-understanding of the problems we are facing in user space today. The
problem with stride passing is that some (call them legacy) driver interface
provides 1 stride, regardless of the number of planes, and other passes (or
receive) multiple strides. This duality affects both DRM and V4L2 interface,
and probably other driver interface I'm not familiar with. So far, we handle
this in userpace because we know the format well enough that we can extrapolate
the other strides values, and that makes things works a lot more. GStreamer is
also stride/offset/alignment aware for a reason, it implements a large set of
software filters. These software filters are not always too slow, an example,
STM used the software renderers for sub-titles instead of dedicated plane. That
saved previous planes that could then be used by their compositor for other
tasks. But for that part, Olivier's protocol make sense, as these format are
not well known, they should not be considered raw formats in the GStreamer
sense.

When this interpolation is not possible or uncertain (requires too much
guessing), we end up with a support that can fail or miss-render on one system
and works on another. This is just a sample of the possible issues we keep
facing. If we just enable all this, it will fail in application it didn't fail
before.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #49 from Daniel Stone <***@fooishbar.org> ---
(In reply to Nicolas Dufresne (stormer) from comment #48)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Daniel Stone from comment #47)

Post by "GStreamer" (GNOME Bugzilla)
The model that KMS, Wayland, EGL, Vulkan, X11 DRI3, etc are built around, is
that stride is determined (for each individual plane) in a device-specific
way by whoever performs the allocation, attached to the plane, and passed to
any consumers of the plane.

In practice, during kmssink dev we found that most display driver does not
gives this liberty to the allocating party, resulting in hard failure (or
miss-rendering) if the expected stride and alignment is not met (also
type/location of physical memory, but that is just invisible to userpace, so
I keep it quite and hope that we have sys/iommus).

If you're using dumb buffers, then you do not get to even try to specify the
stride: stride is an out-value only, along with the handle. For other buffers,
the drivers I've worked with fail when required stride alignment is not met. If
you find drivers which accept buffers with particular stride values but fail to
display them correctly, please do file bugs.

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
Unfortunately GStreamer seems to have created
stride exactly backwards - specific stride demanded by consumer which
allocator must respect; attached to the logical image/framebuffer rather
than each individual plane memory allocation - and that is unfortunate, but
we can't change our model because it just won't work on hardware if we do.

This is a miss-interpretation of what GStreamer do with strides, and a
miss-understanding of the problems we are facing in user space today. The
problem with stride passing is that some (call them legacy) driver interface
provides 1 stride, regardless of the number of planes, and other passes (or
receive) multiple strides. This duality affects both DRM and V4L2 interface,
and probably other driver interface I'm not familiar with.

Where does it affect it with DRM? When creating dumb buffers in DRM, there is a
single stride and a single handle: dumb buffers only support single-planar
formats, and only linear (no modifiers). This is by design of dumb buffers.
drmModeAddFB takes a single stride token, but again, it only supports
single-planar formats. drmModeAddFB2 and drmModeAddFB2WithModifiers take
per-plane stride/offset/handle.

Wayland's dmabuf interface has per-plane stride/offset/fd, as do the EGL and
Vulkan dmabuf import interfaces. So I don't know of anywhere in the graphics
world where stride is magically inferred from one plane to the other, except
for X11 Xv.

Post by "GStreamer" (GNOME Bugzilla)
So far, we handle
this in userpace because we know the format well enough that we can
extrapolate the other strides values, and that makes things works a lot
more. GStreamer is also stride/offset/alignment aware for a reason, it
implements a large set of software filters. These software filters are not
always too slow, an example, STM used the software renderers for sub-titles
instead of dedicated plane. That saved previous planes that could then be
used by their compositor for other tasks. But for that part, Olivier's
protocol make sense, as these format are not well known, they should not be
considered raw formats in the GStreamer sense.

That makes sense, again as long as you a) ensure the buffers are linear, or b)
implement detiling/decompression for whatever modifiers you receive.

Post by "GStreamer" (GNOME Bugzilla)
When this interpolation is not possible or uncertain (requires too much
guessing), we end up with a support that can fail or miss-render on one
system and works on another. This is just a sample of the possible issues we
keep facing. If we just enable all this, it will fail in application it
didn't fail before.

I still don't really understand the problem. Is it just that some unnamed KMS
drivers display the wrong content when stride alignment does not meet their
expectations?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

This is just a sample of the possible issues we keep facing.

Also, I would really like to know, quite specifically, what these issues are.
I've tried to guess a few times at what you mean (e.g. by 'the state of
stride') and am still clueless.

If you let me know then either we can fix them, or fix the usage where the
latter is incorrect - for instance, where you say 'If dumb buffer, strides
means bit-per-pixel * width / 8', then as above, this is incorrect because the
driver _returns_ an aligned stride value from the ioctl, along with the handle.
This is somewhat documented in the ioctl:
https://cgit.freedesktop.org/drm/drm-tip/tree/include/uapi/drm/drm_mode.h?h=drm-tip#n667

Can you please enumerate somewhere (it doesn't have to be here), a list of the
problems you see, rather than just saying that the entire graphics stack is
completely and hopelessly broken, as if it were a known issue? :\

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #51 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
The single stride is a recurring issue with V4L2, but also UVC and other
standard to which the planar formats were added after the fact. One thing to
note, GStreamer, outside vaapi, is rarely involved in DRM to DRM memory
sharing. Instead we deal with cross kernel domain sharing, and the is little
talk among devs there. Hopefully padovan nomination will help long term.
Another issue is the cache coherency, e.g. shall we use the dma sync API, when,
why. Which case is driver bug ? I'm thinking about the visual artifact when you
render UVC or vivid buffer to DRM display. I've reported this a long time ago,
and I still have no clue who's faut, what need fixing.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #52 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(In reply to Daniel Stone from comment #50)

Post by "GStreamer" (GNOME Bugzilla)
Can you please enumerate somewhere (it doesn't have to be here), a list of
the problems you see, rather than just saying that the entire graphics stack
is completely and hopelessly broken, as if it were a known issue? :\

I would like to remind that this is not about the graphic stack. Again, DRM to
DRM is a tiny little aspect here. This bug is about a proposal on how to
exchange unknown pixel formats across drivers, what information need to be
carried and were (caps, meta, etc). Its also about how to avoid the traps that
will make the pipeline fail in an unrecoverable way, specially in cases it
worked before using a slower path.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #53 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I've just re-read that thread, again, and am still not convinced witch way will
do.

The meta way, which will likely end up as GstDrmMeta at this pace, has a lot of
drawback. It inherit the GstVideoInfo/Format and GstVideoMeta for cases where
the format won't be specified inside GStreamer. Of course some of the
compatible fieds could be reused, like strides, but a lot is not. Also, the
formats will be negotiated based on the assumption driver always support a DRM
fourcc with and without the modifiers. Strides are also not negotiated in a
context where this cannot fallback to video_frame_copy() slow path.

On the other side, the video/x-opaque-drm caps negotiation would better
negotiate the format, but exposing the stride to further guaranty that
negotiation is a success does not seem something available API wise. In that
scenario though, GstDrmMeta becomes a clear subset of information passed
blindly to driver (which is what we want) that is sufficient for the exchange.

In the end, I think the modifiers need to be in the caps, which give a plus to
Olivier's proposal. But we also need to signal the usual stuff like
colorimetry, chroma-siting, 3d layout, framerate, par, etc, which need to be
parsed from the caps, for which reusing the VideoInfo code would be nice. So we
need an in-between. But I'm quite clear now that the modifiers must go in the
caps.

Then for fallback, In think the answer is renegotiation. By dropping the
non-working buffers, and redoing the nego without the format/modifiers that
just failed. This is a lot of non-trivial work though, specially for decoders.
But for vaapi, it's the vpp that would handle that, the decoders can stay
inflexible.Thanks for taking the time to report this.
However, you are using a version that is too old and not supported anymore by
GNOME developers. GNOME developers are no longer working on that version, so
unfortunately there will not be any bug fixes by GNOME developers for the
version that you use.

By upgrading to a newer version of GNOME you could receive bug fixes and new
functionality. You may need to upgrade your Linux distribution to obtain a
newer version of GNOME.

Please feel free to reopen this bug report if the problem still occurs with a
recent version of GNOME, or feel free to report this bug in the bug tracking
system of your Linux distribution if your distribution still supports the
version that you are using.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|OBSOLETE |---

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #54 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Sorry, I have just hit the obsolete quick link by accident.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

Nicolas Dufresne (ndufresne) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks| |797071

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #55 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Let me update the current status on this matter, as far as I know:

After Scott's efforts, Mark jumped in and finally merged in libva a new API for
exporting DRM objects: vaExportSurfaceHandle()
https://github.com/intel/libva/pull/125

Here's a simple example of using that API:
http://ixia.jkqxz.net/~mrt/kms_va_nv12.c

There's an implementation for the intel-vaapi-driver:
https://github.com/intel/intel-vaapi-driver/pull/278

for AMD Mesa's state tracker
https://lists.freedesktop.org/archives/mesa-dev/2017-September/169953.html

and for media-driver (MSDK)
https://github.com/intel/media-driver/pull/177

We have to keep in mind that not all the drivers might implement it
(libva-v4l2-request? for example)

When I was trying to understand the issue, my first thought was to write a
vaapidownload element which would transform the memory:VASurface buffers into
dmabuf (and later perhaps others), but we would face the problem of
auto-connecting it, except to adding it into vaapodecodebin.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #56 from Nicolas Dufresne (ndufresne) <***@ndufresne.ca> ---
(In reply to Víctor Manuel Jáquez Leal from comment #55)

Post by "GStreamer" (GNOME Bugzilla)
We have to keep in mind that not all the drivers might implement it
(libva-v4l2-request? for example)

Well, this one is brand new and does not work for anything else then Kodi at
the moment, so I would not worry too much. Note that implementing PRIME_2 for
V4L2 driver is a bit weird.

The other aspect is that anything else then vaExportSurfaceHandle() is just
unsafe to export, because crucial information is missing. So long term, we
should not support anything else in regard to DMABuf exportation.

The main issue is that we don't get to chose the titling and the compression,
we just read it back after allocating a surface. With the caps feature, we
would need to allocate at caps negotiation, rather then allocation query.

We also implement a dmabuf map check, thinking that a dmabuf may or may not be
mappable. I'm still to find a non-mappable dmabuf in any upstream driver.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

We also implement a dmabuf map check, thinking that a dmabuf may or may not be mappable. I'm still to find a non-mappable dmabuf in any upstream driver.

dmabufs you come across in the wild will generally be mappable. There are
explicit ioctls to manage CPU <-> device cache coherency:
https://cgit.freedesktop.org/drm/drm-tip/tree/include/uapi/linux/dma-buf.h#n25

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

--- Comment #58 from Nicolas Dufresne (ndufresne) <***@ndufresne.ca> ---
DMABuf syncrhonization has beed added already, see:

https://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=8ee306eb3fb69c34c579874f6aedcecb6d94a855

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779146

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #59 from GStreamer system administrator <***@gstreamer.net> ---
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been
closed from further activity.

You can subscribe and participate further through the new bug through this link
to our GitLab instance:
https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/issues/49.