Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 25 commits into from
Closed

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

wants to merge 25 commits into from

Conversation

@fhvwy
Copy link
Contributor

@fhvwy fhvwy commented Jul 9, 2017

This is an attempt to fix #10 and related problems around inability to describe surfaces in a way usable with external APIs. Since it isn't fixable with current interfaces, we need to add something new to be able to do it and libva2 seems like a good opportunity for that. Therefore, this proposes a much more flexible interface for import/export of DRM objects.

This is still looking for general comments, so I haven't actually implemented any of this yet. I would intend to make something in Mesa and mpv to test it on AMD at least.

The descriptor is constructed with the intent that after export one can import directly to EGL using https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import.txt and https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt.

Wrt the complexity of the descriptor, we would like to be able to handle at least these three cases for NV12-like surfaces:

  • One object, two layers (DRM_FORMAT_R8, DRM_FORMAT_RG88) each containing of one plane .
  • Two objects, each a single layer (DRM_FORMAT_R8, DRM_FORMAT_RG88) with one plane.
  • One object, one layer of DRM_FORMAT_NV12, containing two planes.

I define new a new pair of Acquire/Release functions to support this - the current VABufferInfo structure is insufficient to support the necessary information, so new functions seemed sensible. They also act directly on surfaces rather than buffers of derived images - I believe this is the only case which anyone would ever use, though please do say if there is some other use-case which I'm not aware of.

Locking is left unclear here. I think it would be cleaner if the export functions did not do any synchronisation, and instead request that the user calls vaSyncSurface() as necessary for their use-case (with behaviour undefined if they act on an object with another API during a render operation). That would support mapping a single time and then synchronising as necessary without the need to keep creating and destroying objects, but there might be some other reason I'm not aware of why this causes problems.

(To compare, other cases of sharing between video and processing APIs tend to have separate mapping and synchronisation - see e.g. https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/cl_khr_dx9_media_sharing.html or https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/cl_khr_d3d11_sharing.html.)

Some other random questions:

  • Would there be any value in adding a read/write distinction to import or export?
  • Is any more detail needed for Mesa - for example, to support separated fields?
  • How many planes do we need, at most? (YUVA or RGBA planar formats need four.)
  • Do we want to support any other memory types with this new API? (For example, something similar could work for flink handles rather than PRIME fds if that has any value. No idea about others - Android?)

All thoughts welcome. I'm not attached to any particular detail here - feel free to suggest changing everything :)

@xhaihao
Copy link
Contributor

xhaihao commented Jul 10, 2017

@fhvwy @scott-d-phillips has some commits for dma buffer modifier too
d-scott-phillips/intel-vaapi-driver@5aebbb3
d-scott-phillips@52e006f#diff-ce40de7930eb5bb0c9f5801e5101ce29, could you have a look?

@xhaihao
Copy link
Contributor

xhaihao commented Jul 10, 2017

Locking is left unclear here. I think it would be cleaner if the export functions did not do any synchronisation, and instead request that the user calls vaSyncSurface() as necessary for their use-case (with behaviour undefined if they act on an object with another API during a render operation). That would support mapping a single time and then synchronising as necessary without the need to keep creating and destroying objects, but there might be some other reason I'm not aware of why this causes problems.

Agree with you that the export functions shouldn't do synchronisation

@xhaihao
Copy link
Contributor

xhaihao commented Jul 10, 2017

This is still looking for general comments, so I haven't actually implemented any of this yet. I would intend to make something in Mesa and mpv to test it on AMD at least.

AFAIK, Scott did some testing in gstreamer, please refer to https://bugzilla.gnome.org/show_bug.cgi?id=779146

@fhvwy
Copy link
Contributor Author

fhvwy commented Jul 10, 2017

@scott-d-phillips Some thoughts on that branch:

  • Do we really want DRM-specific API calls here? vaCreateSurfaces() can already hold arbitrary attributes, and an extended export function could easily support more than one type rather than being hardcoded to this one structure.
  • For multiple plane formats, I don't think we should be using DRM_FORMAT_NV12 and other composed layouts - they seem to me to be intended for inflexible embedded cases with implicit conversion. We should use R8/GR88 instead of NV12 (and R16/GR32 instead of P010, etc.) so that they interoperate directly with APIs like EGL and OpenCL. Not sure on formats like YUYV which interleave both ways - it might be easier to avoid those in the first version (you generally can't represent them in the graphics APIs without hackery or nonstandard extensions anyway).
  • As a consequence of that, I think the descriptor structure there is not sufficiently general: format needs to be per-part-of-object (called 'layer' in my version, better name welcome). Similarly, the format modifier needs to be per-object rather than per-surface. (This wants to be possible on import, even if the export format is fixed by the driver.)
  • Object size is required by some import APIs.
  • Freeing an object needs to be clarified - the current implementation seems to require the user to deduplicate the file descriptors before closing them. (This motivated the separate objects array in what I wrote.)
  • dma-buf is a kernel-internal name. Calling them DRM objects (here referred to via PRIME fds and manipulated with libdrm) feels clearer to me, and fits with the existing naming in libva.

@d-scott-phillips
Copy link

For multiple plane formats, I don't think we should be using DRM_FORMAT_NV12 and other composed layouts - they seem to me to be intended for inflexible embedded cases with implicit conversion. We should use R8/GR88 instead of NV12 (and R16/GR32 instead of P010, etc.) so that they interoperate directly with APIs like EGL and OpenCL. Not sure on formats like YUYV which interleave both ways - it might be easier to avoid those in the first version (you generally can't represent them in the graphics APIs without hackery or nonstandard extensions anyway).

On the other hand, if you want to pass the surface to KMS for display, or to some other process for libva encoding then you do want NV12, P010, etc IIUC. Maybe this preference for the form of multi-planar export can be expressed with the API?

Otherwise agree with your points @fhvwy

@fhvwy
Copy link
Contributor Author

fhvwy commented Jul 10, 2017

Can Y-tiled NV12 or P010 be used for scanout on Intel? I didn't think they could. In any case, if you want correct colours (with arbitrary colour spaces and HDR and whatever other features people dream up to mess with output) then you need to do a proper rendering to RGB for display, so I'm not sure how useful it could be (people typically use EGL+OpenGL for this, which is happy to render to KMS buffers).

For encoding, there isn't actually any hardware restriction on the plane locations on Intel is there? The current driver does restrict it, but only because it has to be in a single object - the offsets within the object can be arbitrary, I think. So, that should be fixable without too much difficulty.

I guess making it configurable would be fine with me, as long as the single-plane formats are accessible for the use-cases I care about.

@d-scott-phillips
Copy link

Can Y-tiled NV12 or P010 be used for scanout on Intel?

Yes, it can. Patches to add that support in the kernel are just now hitting the intel-gfx list.

For encoding, there isn't actually any hardware restriction on the plane locations on Intel is there?

The hardware requires the address of the UV plane be greater than the address of the Y plane, so not just any two R8 and GR88 planes can be used as NV12.

@xhaihao xhaihao force-pushed the v2.0-branch branch 2 times, most recently from da7c3dc to c73cef6 Compare August 16, 2017 08:14
xhaihao and others added 21 commits August 31, 2017 13:53
@xhaihao
libva 2.0.0.pre1 for development
e8187ef 
ABI was broken in the previous versions, so we will bump the VA API version
to 1.0.0 and library version to 2.0.0 for next release. Some data
structures will be changed or removed in the next commits

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@yakuizhao @xhaihao
Refine the data type of slice_data_flag in VASliceParameterBufferHEVC…
5fb0b9a 
… to be consistent with other codecs

Currently the data type of slice_data_flag in VASliceParameterBufferHEVC is
uint16_t while it is uint32_t for other codecs. It is inconsistent.
As the memory is allocated as aligned, it still allocates four bytes for it.
So it will be better to use the consistent data type.

This fixes https://github.com/01org/libva/issues/58

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
@fhvwy @xhaihao
Move declarations for internal functions to new header
a114ae1 
The new header is internal-only and will not be installed.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Move some common macro definitions to the new internal header
a7b7cf7 
Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Move VADisplay allocation to common code
2e627a8 
This is slightly cleaner, and will be required to set common
options on a newly-created display.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Make logging callbacks library-safe
ca69769 
Move the logging callbacks into the display context, rather than
having them as global state.  Add user-context parameter as well so
that users can distinguish between callbacks in different instances.

The default behaviour does not change, and LIBVA_MESSAGING_LEVEL
continues to be respected in that case.

Since we're breaking API here, also rename vaMessageCallback to
VAMessageCallback to be consistent with all other types.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Add a macro to indicate deprecation
f194d2f 
This will generate a warning on gcc and compatible (clang, icc),
and do nothing with other compilers.  There is a separate macro
for enum variables, because gcc did not support deprecating them
until version 6.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Deprecate H.264 baseline profile and FMO support
9cbd453 
This is not now and never will be supported by any hardware, nor is
it supported by any current software.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Add namespace prefix to globals
6b870cf 
Add "va_" prefix to trace_flag, fool_codec and fool_postp to avoid
polluting the global namespace.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Add namespace prefix to global DRI functions in libva-x11
a5032a7 
Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Detect short reads when filling fool buffers
f5d40df 
Mainly to fix the warning (glibc marks read(2) with warn_unused_result).
The function continues anyway when this happens (matching current
behaviour when the file can't be opened at all).

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@fhvwy @xhaihao
Note flags which have been renamed in the H.264 standard
dbcac8d 
residual_colour_transform_flag and pic_order_present_flag have
different names in newer versions of the H.264 standard, so add a
comment with the new name to reduce confusion.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@chivakker @xhaihao
va_egl.h: add license to file
5e810dd 
license missing on header file.  Taken from c file

Signed-off-by: Daniel Charles <daniel.charles@intel.com>
@xhaihao
Fix a typo in VAConfigAttribValEncROI
b9ae34c 
The current delat is typo. This fix breaks API, fortunately
roi_rc_qp_delat_support isn't used widely

This fixes https://github.com/01org/libva/issues/32

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
update VAEncMacroblockParameterBufferH264
34d1e5c 
The type of bits should be a struct type, otherwise the flags in bits
will be impacted each other.

In addtion this adds a reserved byte to bits struct in inter_fields,
this should fix https://github.com/01org/libva/issues/35

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
Update the data structure for HEVC decoding
243787e 
We have defined a codec-independent slice parameter buffer (VASliceParameterBufferBase)
for base mode slice decoding for all codecs, so VASliceParameterBufferBaseHEVC is
unnecessary. Fortunately VASliceParameterBufferBaseHEVC isn't used publicly, hence
we remove VASliceParameterBufferBaseHEVC from VA-API 1.0.0

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
dec/VC1: Update the comment for b_picture_fraction
86f3332 
b_picture_fraction is used as an index in practice, so I updated the
comments to reduce the confusion to user

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
Delete libva-tpi and libva-egl backends
4086c98 
The APIs/data structures defined in va_tpi.h and va_egl.h are used
to implement buffer sharing between libva and the 3rd party libraries.
We have an official buffer sharing machnism in VA core, so libva-tpi and
libva-egl are unnecessary any more.

However va_egl.h or va_tpi.h is included uselessly in a few places,
e.g. https://github.com/01org/libyami/blob/apache/vaapi/vaapidisplay.h#L22
So va_egl.h and va_tpi.h are kept with a deprecation warning message.

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
Deprecate packed misc packed header
d608aa3 
All applications can use VAEncPackedHeaderRawData to insert a packed
header on demand

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
Use portable types for VA structures
51776ca 
We have already defined VA structures using portable types in some
header files. Use portable types in the remaining header files to
keep the style consistency,

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@xhaihao
Remove the comment about vaGetSurfaceAttributes()
fe7c7bd 
vaGetSurfaceAttributes() was removed years ago, so remove the
corresponding comments

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
mypopydev and others added 2 commits August 31, 2017 13:53
@mypopydev @xhaihao
va: Add the fourcc of I420 format
b287d72 
i965 will return the I420 in vaQuerySurfaceAttributes, but va.h didn't
add the pre-define fourcc, and will deprecate IYUV because it same as
I420 but most user perfer I420.

Signed-off-by: Jun Zhao <jun.zhao@intel.com>
@xhaihao
Fix deprecated warning
845157a 
enums marked as deprecated should not be used any more.

Signed-off-by: Xiang, Haihao <haihao.xiang@intel.com>
@XinfengZhang @xhaihao
optimize values for VAConfigAttribEncSliceStructure
f6c5c9a 
redefine VA_ENC_SLICE_STRUCTURE_ARBITRARY_ROW to non-zero value
all slice structure attributes value can OR'd together.

Signed-off-by: xfengcarl <carl.zhang@intel.com>
@fhvwy fhvwy force-pushed the v2.0-branch branch 2 times, most recently from eea0e4f to 2815347 Compare September 17, 2017 17:19
@fhvwy fhvwy changed the title RFC: Add a more flexible interface for import/export of DRM objects Add a more flexible interface for import/export of DRM objects Sep 17, 2017
@fhvwy
Copy link
Contributor Author

fhvwy commented Sep 17, 2017

Ok, here is a new version, now fully working for export.

Driver support for Intel:
fhvwy/intel-vaapi-driver@78a824c
Driver support for AMD (Mesa):
https://lists.freedesktop.org/archives/mesa-dev/2017-September/169953.html
Use in mpv for playback:
fhvwy/mpv@0a505f9

@fhvwy
Copy link
Contributor Author

fhvwy commented Sep 17, 2017

  • The read/write mode is included - Mesa has this so it seemed sensible to use it. Intel will currently always return read/write objects.
  • I've added a way to indicate that either separate planes (for further rendering) or composite objects (for scanout or similar immediate use) are desirable. The Intel driver supports both modes (with some restrictions - there is no DRM format for P010 so it isn't usable as composite, while YUYV isn't representable as separate planes), while the Mesa driver supports separate planes only.
  • Only export is implemented in either driver. Import is specified to work via vaCreateSurfaces(), but not yet done.
  • Additional memory types are still open. I only have interest in DRM currently, but it should be usable for anything else.
  • I haven't specified anything around making new objects with particular properties (this was suggested in the gstreamer discussion). I think this use case is better served by making them with gbm or similar and then importing, rather than cluttering libva with a lot more options to control this? Thoughts welcome on that.

@xhaihao
Copy link
Contributor

xhaihao commented Sep 19, 2017

How about to add width and height for the exporting surface in VADRMPRIMESurfaceDescriptor? The 3rd party library might need these metadata.

@fhvwy
Copy link
Contributor Author

fhvwy commented Sep 19, 2017

I was thinking they are already known because they are passed to vaCreateSurfaces() (for export by the creator, and you have to know them directly as an importer). But yeah, that makes sense, I'll add them.

@fhvwy
Add a new interface for exporting surfaces
b8bb722 
Initial definition supports exporting as DRM objects only.

Fixes intel#10.

Signed-off-by: Mark Thompson <sw@jkqxz.net>
@xhaihao
Copy link
Contributor

xhaihao commented Sep 28, 2017

Could you please change your PR to 'merge 1 commits to 01org:master'? All patches on v2.0-branch have been merged into master.

@xhaihao
Copy link
Contributor

xhaihao commented Sep 28, 2017

https://bugzilla.gnome.org/show_bug.cgi?id=779146 is using @scott-d-phillips's definition which is a little different from yours, could you have a look?

@fhvwy fhvwy closed this Sep 28, 2017
@fhvwy fhvwy mentioned this pull request Sep 28, 2017
Merged
@fhvwy
Copy link
Contributor Author

fhvwy commented Sep 28, 2017

(Moved to #125.)

@fhvwy fhvwy mentioned this pull request Oct 3, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fhvwy @xhaihao @d-scott-phillips @yakuizhao @chivakker @mypopydev @XinfengZhang