Re: [PATCH 0/1] [RFC] drm/fourcc: Add new unsigned R16_UINT/RG1616_UINT formats

[Jason Ekstrand]

On Fri, 2022-07-15 at 11:20 +0100, Dennis Tsiang wrote:
> On 30/06/2022 08:47, Pekka Paalanen wrote:
> > On Wed, 29 Jun 2022 14:53:49 +0000
> > Simon Ser <contact@xxxxxxxxxxx> wrote:
> > 
> > > On Wednesday, June 29th, 2022 at 16:46, Dennis Tsiang
> > > <dennis.tsiang@xxxxxxx> wrote:
> > > 
> > > > Thanks for your comments. This is not intended to be used for
> > > > KMS, where
> > > > indeed there would be no difference. This proposal is for other
> > > > Graphics
> > > > APIs such as Vulkan, which requires the application to be
> > > > explicit
> > > > upfront about how they will interpret the data, whether that be
> > > > UNORM,
> > > > UINT .etc. We want to be able to import dma_bufs which create a
> > > > VkImage
> > > > with a "_UINT" VkFormat. However there is currently no explicit
> > > > mapping
> > > > between the DRM fourccs + modifiers combos to "_UINT"
> > > > VkFormats. One
> > > > solution is to encode that into the fourccs, which is what this
> > > > RFC is
> > > > proposing.
> > > 
> > > As a general comment, I don't think it's reasonable to encode all
> > > of the
> > > VkFormat information inside DRM FourCC. For instance, VkFormat
> > > has SRGB/UNORM
> > > variants which describe whether pixel values are electrical or
> > > optical
> > > (IOW, EOTF-encoded or not). Moreover, other APIs may encode
> > > different
> > > information in their format enums.
> > 
> > Yeah, do not add any of that information to the DRM pixel format
> > codes.
> > 
> > There is *so much* other stuff you also need to define than what's
> > already mentioned, and which bits you need for the API at hand
> > depends
> > totally on the API at hand. After the API has defined some parts of
> > the
> > metadata, the API user has to take care of the remaining parts of
> > the
> > metadata in other ways, like dynamic range or color space.
> > 
> > Besides, when you deal with dmabuf, you already need to pass a lot
> > of
> > metadata explicitly, like the pixel format, width, height, stride,
> > modifier, etc. so it's better to add more of those (like we will be
> > doing in Wayland, and not specific to dmabuf even) than to try make
> > pixel formats a huge mess through combinatorial explosion and
> > sometimes
> > partial and sometimes conflicting image metadata.
> > 
> > You might be able to get a glimpse of what all metadata there could
> > be
> > by reading
> > https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/pixels_color.md
> > .
> > 
> > Compare Vulkan formats to e.g.
> > https://docs.microsoft.com/en-us/windows/win32/api/dxgicommon/ne-dxgicommon-dxgi_color_space_type
> > and you'll see that while DXGI color space enumeration is mostly
> > about
> > other stuff, it also has overlap with Vulkan formats I think, at
> > least
> > the SRGB vs. not part.
> > 
> > Btw. practically all buffers you see used, especially if they are 8
> > bpc, they are almost guaranteed to be "SRGB" non-linearly encoded,
> > but
> > do you ever see that fact being explicitly communicated?
> > 
> > Then there is the question that if you have an SRGB-encoded buffer,
> > do
> > you want to read out SRGB-encoded or linear values? That depends on
> > what you are doing with the buffer, so if you always mapped dmabuf
> > to
> > Vulkan SRGB formats (or always to non-SRGB formats), then you need
> > some
> > other way in Vulkan for the app to say whether to sample encoded or
> > linear (electrical or optical) values. And whether texture
> > filtering is
> > done in encoded or linear space, because that makes a difference
> > too.
> > 
> > IOW, there are cases where the format mapping depends on the user
> > of the
> > buffer and not only on the contents of the buffer.
> > 
> > Therefore you simply cannot create a static mapping table between
> > two
> > format definition systems when the two systems are fundamentally
> > different, like Vulkan and DRM fourcc.
> > 
> > 
> > Thanks,
> > pq
> 
> Thanks all for your comments. We'll look into an alternative approach
> to
> achieve what we need.

I mostly agree with Pekka here.  The fourcc formats as they currently
are defined only specify a bit pattern and channel order, not an
interpretation.  Vulkan formats, on the other hand, have everything you
need in order to know how to convert float vec4s to/from that format in
a shader.  That's not the same as knowing what the data represents
(colorspace, wite values, etc.) but it's a lot more than fourcc.

That said, the Vulkan APIs for querying modifier support will give you
much more fine-grained information about exactly the Vulkan formats you
request.  So if you ask for modifier support for VK_FORMAT_R16G16_UINT,
that's what you'll get.  I *think* it should be fine to use
VK_FORMAT_R16G16_UINT with DRM_FOURCC_GR1616.  Of course, the API on
the other side will also need a more precise format specifier than
fourcc if it's to know the difference between R16G16_UINT and
R16G16_UNORM.

--Jason