Add a more flexible interface for import/export of DRM objects

[fhvwy]

Updated #85 (can't change source branch in github PRs).

The two component parts (add function, add DRM PRIME support) are now separate patches, otherwise this is the same as the previous version.

[fhvwy]

@xhaihao The version from @scott-d-phillips instructed some of the changes from the initial RFC version of this, see discussion in #85. The current version here should support those scanout use-cases, though if would be helpful if someone directly involved in that could check through to make sure they are happy with it.

[fhvwy]

mpv tree using this for playback: https://github.com/fhvwy/mpv/tree/vaapi-drm.

Mesa patches implementing this for use with AMD: https://lists.freedesktop.org/archives/mesa-dev/2017-October/171246.html.

[chivakker]

This approach looks good to me. Also it would be good to settle before libva-2.0 official release. This may trigger a pre3 release with validation using the regular tools (including libyami) and using mpv to validate this particular behavior. Just please don't merge without proper and complete testing. Also a unittest could make sense.

[fhvwy]

@chivakker Do you have any thoughts for how a unit test could work? It is difficult to test this change in isolation, because you need something else (EGL, OpenCL, etc.) to import the DRM objects to to see the result.

I have been using it for playback with mpv on both Intel and AMD/Mesa for the last month and haven't had any issues with the API as it is now. (Mesa itself has gone through significant development during that time, so the driver underneath it has changed.) I have also been using it a bit in ffmpeg with OpenCL (Beignet), though not as much.

libyami shouldn't be affected at all by the addition. As far as I know there is currently no reason for it to use this interface - it is only targetting the Intel driver, and the existing export API is sufficient there for cases not involving modifiers (i.e. not KMS/scanout use).

[chivakker]

@fhvwy I am talking about API unittest that should do as it says. Setting DRM_PRIME2 flag exporting/importing the buffer. IMO it could be useful in case it gets broken in the future. I agree full functionality can only be achieved with mpv. I am currently trying to compile mpv to see it working.

[xhaihao]

@sreerenjb who is working on https://bugzilla.gnome.org/show_bug.cgi?id=779146 now? I'd like to make sure gst-vaapi works with the this interface before merging it to master.

[sreerenjb]

@xhaihao AFAIK, No one working on this at the moment but we will. I am afraid we can't do it immediately if 2.0 release window is too close.

I will look into this, but not immediate because of other works.

BTW, If this is about new API (assuming no change in VABufferInfo structure and VASurfaceAttribExternalBuffers, sorry haven't gone through all of your patches/commits yet)then why can't we postpone to 2.1?

Note: BTW we have many customers who use gstreamer-vaapi over dmabuf (export and import + v4l2src, export+GL) in their products, so we should be careful before integrating these. So first we need integration to libva, intel-vaapi-driver and gstreamer-vaapi internal branches (not in upstream branch) and QA Validation with differnt camerasrc (icamerasrc, v4l2src ) and gl renderer elements(glimagesink).

[fhvwy]

The pressing use from my point of view is in Mesa on AMD, which currently is unable to export surfaces at all (see #10). It was suggested in the response to the original PR in July that it could also be used for the scanout cases on Intel, and I redefined it in such a way that that use-case is also possible. I don't think there would be any issue if Intel did not implement this interface at all in the first release, if that's a problem for you from a QA perspective (we can continue to use the existing vaDeriveImage() + vaAcquireBufferHandle() sequence there).

[fhvwy]

Mesa support added in https://cgit.freedesktop.org/mesa/mesa/commit/?id=c4ed39f85b1ebd062eaa51880fcc79cfbcb4e5c3. mpv support is pending at mpv-player/mpv#4964 (with other libva2 parts), and will continue to work as it does now when vaExportSurfaceHandle() is not available.

Please merge this for the Mesa support in 2.0. If Intel driver support is too late for 2.0, we can postpone it to 2.1. (No other Intel projects are affected, because the existing vaDeriveImage() + vaAcquireBufferHandle() interface continues to work in the Intel driver.)

[xhaihao]

Is the the support for scanout verified by Mesa+MPV too? If not, maybe we will have to refine the API again if scanout doesn't work as expected.

[fhvwy]

AMD does not currently support direct scanout of NV12/P010 surfaces.

If you are particularly concerned about this, I could remove the separate-layers/composite-object flags, since Mesa only supports the separate-layers case - see https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/va/surface.c#n924. (That would then require re-adding the flags later for Intel scanout support, and updating Mesa to reject the unsupported case.)

[fhvwy]

Thinking a bit further about that, adding the flags later wouldn't sensibly work in any backward-compatible manner without defining something further (e.g. some sort of surface-layout-flags mask for which invalid values could be rejected now). Given that there isn't any reasonable third choice beyond composite-object and separate-layers, either keeping the flags as-is or collapsing them into a single bit (zero for the default choice of separate-layers) is the more compatible path.

[fhvwy]

I've verified that this is sufficient for NV12 scanout on Intel.

I used:

• Skylake GT3e (6260U).
• Linux 4.13.5.
• NV12 scanout patch, revision 5 from https://patchwork.freedesktop.org/series/25377/.
• git ffmpeg (77f7d710).
• libva as this PR (51a277ef).
• i965 as corresponding driver PR (0cec7606).
• Incredibly hacky test program http://ixia.jkqxz.net/~mrt/kms_va_nv12.c.

This decodes and plays back an input stream directly to scanout with no postprocessing. A lot of things are hard-coded (CRTC and connector IDs, notably), so it will need a bit of modification to work on another machine.

[fhvwy]

@xhaihao The verified cases are now:

• Exporting separate layers to use with EGL import for rendering in mpv - working on both Intel and AMD / Mesa.
• Exporting composed layers to use with KMS for scanout - working on Intel in test above.

The fourth case, exporting composed layers on AMD, is not possible because the surfaces there are not single objects.

Is anything else needed here?

[sreerenjb]

Overall the patch LGTM. Ideally, I would like to see a driver implementation(and some test application code too) for Import too because that is what going to bring changes in existing APIs(vaCreateSurface2)

[fhvwy]

@sreerenjb I've removed mention of import to eliminate that concern (no code change, just comments). It can be re-added later.

[xhaihao]

Could you add some reserved bytes at the end of VADRMPRIMESurfaceDescriptor in case we will extend the feature in future? And please use portable data type in the structure.

[fhvwy]

@xhaihao Updated, thanks.

[sreerenjb]

Honestly, I don't think it is a good idea to push a big feature(new API) like this at the moment since we already in the "feature freeze" state. How about pushing these for 2.1 release? We have other pending APIs too :)

[fhvwy]

@sreerenjb It fixes a long-standing bug for AMD/Mesa, and has been fully verified there (it is merged to git Mesa and mpv already). From my point of view it is not needed for Intel to implement it in this version (the old interface is sufficient on Intel for the existing use-cases), but it should still be merged for AMD/Mesa.

[sreerenjb]

@fhvwy I 100% agreed that it is a needed feature, we need it in intel driver and gstreamer too, to avoid some hacks (in order to negotiate the TILING capability ).
But I still feel that it is better to wait for the next release so that we can implement/test the end to end solution for both Import and Export.
Let's get the suggestion from other developers @xhaihao @lizhong1008 @QuPengfei @chivakker Anyone else?

[sreerenjb]

BTW, I think the mesa developers accidentally merged the patch thinking that this new API is something already available in libva official repository :)

[xhaihao]

@fhvwy I just realized I forgot to send out my comments about merging the two PRs into libva 2.0 release on https://github.com/01org/libva/issues/72. First I totally agree the two PRs are useful for vaapi user, but before your comments, we have built the pre packages for testing and it is code-frozen. We won't accept the new code for libva 2.0 except it is critical bug fix. I apologize for any inconvenience caused by this decision. note this decision doesn't block merging your PRs into master branch.

[fhvwy]

@xhaihao Well, thank you for telling me now. Though it would have been very helpful to know that earlier before the patches were marked as approved, so that they would not have been merged to other projects assuming it would be present in VAAPI 1.0.0.

Since I now need to update the target versions on those (https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/va/context.c#n92, etc.), what version should they be changed to?

[ghost]

That seems disappointing.

[sreerenjb]

@fhvwy you should have mentioned to mesa developers that "this new API is not yet landed in libva master".
Ideally, Mesa should revert the patch until we officially support the new API in libva.

[ghost]

FFmpeg and mpv would have to revert things too.

Can't this just be included?

What's the hurry anyway?

[xhaihao]

Since I now need to update the target versions on those (https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/va/context.c#n92, etc.), what version should they be changed to?

I created https://github.com/01org/libva/milestone/1 (libva 2.1 release)

[yol]

@fhvwy I have a semi-related question for clarification: If VA_EXPORT_SURFACE_SEPARATE_LAYERS is used, is it still possible that one layer has multiple planes? If so, under which conditions/for what practical use case?

[fhvwy]

@pkerling With VA_EXPORT_SURFACE_SEPARATE_LAYERS set, layers should all have exactly one plane. The use for this form is for import to other GPU processing APIs (OpenGL, OpenCL, Vulkan, etc.), which (barring some vendor extensions) always want planes separately.

For the second part of your question, there are definitely cases where mixed layers can appear, though I don't have any real use-case so I haven't included them. For example, consider how YUV 4:2:0 + alpha should work on hardware using NV12 for YUV 4:2:0:

1. As a single object, three planes Y + UV + A.
2. As two objects, one with two planes Y + UV (i.e. NV12) and a separate alpha plane.
3. As three objects, Y, UV and A.

(1) will be wanted for KMS overlay cases, and should be returned when COMPOSED_LAYERS is set.
(2) is the natural form for codecs - in H.264 terms, the primary coded picture as YUV 4:2:0 and an indepedent auxiliary picture for the alpha plane. This may have use inside a driver, but I don't see a use-case for exporting it - if someone does have one, we could add another flag (SEPARATE_ALPHA, say?) which would return it.
(3) will be wanted for import to other APIs, and should be returned for SEPARATE_LAYERS.

On the other hand, it's sensible for all three of these forms to work for import if the driver can support them.

[yol]

@fhvwy Thanks for the quick reply! I think it would be nice to document these facts (e.g. that SEPARATE_LAYERS will never return more than one plane per layer) with the flags so programs can make reasonable assumptions about the layers/planes they will receive.

We are looking to implement this in Kodi. As far as we are concerned we currently would not have any sensible use case for (2) either.

[xhaihao]

@fhvwy could you help to document your explanation (https://github.com/01org/libva/pull/125#issuecomment-340274373) in va.h?

[xhaihao]

I'm sorry some pushed commits caused conflicts, Could you rebase this PR again?

[fhvwy]

Rebased again.