[FFmpeg-devel] [PATCH v2 1/2] lavu: Add DRM hwcontext

[Mark Thompson]

On 06/07/17 09:00, wm4 wrote:
> On Thu, 6 Jul 2017 00:02:08 +0100
> Mark Thompson <sw at jkqxz.net> wrote:
> 
>> ---
>> Updated following discussion:
>> * Back to using nested arrays.
>> * Documentation improved.
>> * Configure option now called libdrm.
>> * Other minor fixups.
>>
>>
>>  configure                      |   3 +
>>  libavutil/Makefile             |   2 +
>>  libavutil/hwcontext.c          |   4 +
>>  libavutil/hwcontext.h          |   1 +
>>  libavutil/hwcontext_drm.c      | 294 +++++++++++++++++++++++++++++++++++++++++
>>  libavutil/hwcontext_drm.h      | 166 +++++++++++++++++++++++
>>  libavutil/hwcontext_internal.h |   1 +
>>  libavutil/pixdesc.c            |   4 +
>>  libavutil/pixfmt.h             |   4 +
>>  9 files changed, 479 insertions(+)
>>  create mode 100644 libavutil/hwcontext_drm.c
>>  create mode 100644 libavutil/hwcontext_drm.h
> 
> Generally LGTM, though I don't know when LongChair can try it.

I won't apply it without that implementation showing it being used.

>> +#include <fcntl.h>
>> +#include <sys/mman.h>
>> +#if HAVE_UNISTD_H
>> +#   include <unistd.h>
>> +#endif
> 
> Is there really going to be a system that has fcntl.h and sys/mman.h,
> but not unistd.h? This code is going to work on POSIX only anyway,
> which requires this header.

This was just copied from other uses.  Agree; removed.

>> +typedef struct AVDRMObjectDescriptor {
>> +    /**
>> +     * DRM PRIME fd for the object.
>> +     */
>> +    int fd;
>> +    /**
>> +     * Total size of the object.
>> +     *
>> +     * (This includes any parts not which do not contain image data.)
>> +     */
>> +    size_t size;
>> +    /**
>> +     * Format modifier applied to the object (DRM_FORMAT_MOD_*).
>> +     */
>> +    uint64_t format_modifier;
>> +} AVDRMObjectDescriptor;
> 
> So I guess this thing really is worth the trouble, but I still don't
> entirely understand it. Why is the size needed, and what do the
> format_modifiers do?

Size is needed for some import APIs:

<https://cgit.freedesktop.org/beignet/tree/include/CL/cl_intel.h#n141>
<https://github.com/01org/libva/blob/master/va/va.h#L920>

Having it also makes the generic mapping sensible - without it, we would need to map each plane individually, which would require knowledge of what all of the formats mean to know how big they are.


Format modifiers are a relatively new mechanism to communicate tiling modes and other related metadata in a way common to all drivers (with current single-driver setups this is already done in some cases by magic internal properties).

EGL import supports them:

<https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt>

(Ignore the fact that modifiers there are per-plane - the extension was codified before the decision that mixing modifiers within the same object was madness and that feature completely removed:  <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bae781b259269590109e8a4a8227331362b88212>.)