[Mesa-dev] [PATCH] drm: add FOURCC formats for compute dma_buf interop.

[Gwenole Beauchesne]

Hi,

2014-03-15 12:28 GMT+01:00 Daniel Vetter <daniel at ffwll.ch>:
> On Sat, Mar 15, 2014 at 05:41:05AM +0100, Gwenole Beauchesne wrote:
>> Hi,
>>
>> 2014-03-14 22:52 GMT+01:00 Daniel Vetter <daniel at ffwll.ch>:
>> > On Fri, Mar 14, 2014 at 06:59:21PM +0100, Gwenole Beauchesne wrote:
>> >> This is a follow-up to:
>> >> http://lists.freedesktop.org/archives/mesa-dev/2014-March/055742.html
>> >>
>> >> Add formats meant to convey a "compute" dimension when a DRM fourcc
>> >> format is needed for dma_buf interop (EGL, OpenCL).
>> >>
>> >> Intended FOURCC format: ('T',<num_elements>,<type>,<size_element>).
>> >> - <num_elements>: number of elements in the tuple. Range: [0..3].
>> >> - <type>: type of each element. Values: {'_','I','U','F'},
>> >>   respectively for normalized to [0..1] range, signed integers,
>> >>   unsigned integers, floating-point.
>> >> - <size_element>: size of the element. Values: {1, 2, 4, 8}.
>> >>
>> >> All entities are represented in native-endian byte-order in memory.
>> >> For example, 'T2F4' format would represent the (float, float) tuple
>> >> where elements are stored in little-endian byte-order on x86.
>> >>
>> >> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
>> >
>> > So this fourcc stuff started out as pixel formats for the ADDFB2 ioctl,
>> > i.e. stuff that can be displayed. Hence my first question: How can we
>> > display these bastards here?
>>
>> It's not meant to be displayed. The idea was maybe we could detect the
>> fourcc (e.g. (fourcc & 0xff) == 'T') and reject the buffer accordingly
>> when submitted to one of the display ioctl?
>
> Well we already have explicit lists for all that, so no need to add a pile
> of formats just to be able to reject them ;-)
>
>> > So given all this: Why do we need this in the kernel's header? It sounds
>> > like purely a userspace business with no need at all the enshrine this
>> > into kernel ABI headers. Note that e.g. the mesa import/export interface
>> > have their own fourcc #defines for exactly this reason.
>>
>> I could have stuffed everything in gbm.h for instance, or another new
>> header, but the EXT_dma_buf_import extension actually mentions
>> drm_fourcc.h. So, that's why I finally moved the definitions to there.
>> :)
>
> That's a bit unfortunate ... But the spec also clearly states "as used by
> the drm_mode_fb_cmd2 ioctl". And imo we can't make a case that this is
> true.
>
>> What would be the better place? Can we make the userspace libdrm
>> version of the file live totally unsynchronized from the kernel
>> headers then?
>
> I think the right approach would be to ref the specification and also
> allow other fourcc codes for non-display related buffer layaouts, maybe in
> a different namespace. This entire fourcc stuff was only done since it was
> a somewhat ill-defined "standard" for all kinds of YUV buffers. We've
> already had to massivel pimp it to make it work properly with RGB.
>
> Pimping this even further to support all kinds of random compute/gl
> buffers sounds ill-advised. I think we need to steal a new namespace from
> some existing standard for all this, maybe ogl or ocl has something? Ofc
> that means creating a new dma_buf_import atttribute for eglCreateImageKHR
> which could be used instead of DRM_FOURCC_EXT.

Thinking further about it, the patch is totally useless. It should be
enough to amend the EXT_image_dma_buf_import spec to allow for using
the GL format/type enums instead... Now, what are the chances to have
the required changes to appear in a new revision of the spec? :)

I'd suggest the following formalism:
- If EGL_LINUX_DRM_FOURCC_EXT attribute is 0 (zero), then a GL
format/type pair needs to be supplied to the <attrib_list>
- Accepted as an attribute to <attrib_list> would be EGL_GL_FORMAT_EXT
/ EGL_GL_TYPE_EXT, where EGL_GL_FORMAT_EXT attribute value would be
GL_{RED,RG,RGB,RGBA} as usual, and EGL_GL_TYPE_EXT value would be
GL_{{UNSIGNED_,}{BYTE,SHORT,INT},{HALF_,}FLOAT}.

Alternative would be to provide a unique "internal format", but this
would complicate implementations, for sanity checks (too large switch
cases).

WDYT?