Introduce VASurfaceAttribDRMFormatModifiers

[emersion]

This allows users to specify a list of modifiers when allocating a
new surface. The driver will pick a modifier from the list.

Example usage:

// List of modifiers suitable for the buffer usage, coming from e.g. EGL or KMS
uint64_t modifiers[2] = {
    I915_FORMAT_MOD_X_TILED,
    I915_FORMAT_MOD_Y_TILED,
};
VADRMFormatModifierList modifier_list = {
    .num_modifiers = 2,
    .modifiers = modifiers,
};

VASurfaceAttrib attribs[2] = {0};
attribs[0].type = VASurfaceAttribPixelFormat;
attribs[0].value.type = VAGenericValueTypeInteger;
attribs[0].value.value.i = VA_FOURCC_NV12;
attribs[1].type = VASurfaceAttribDRMFormatModifiers;
attribs[1].value.type = VAGenericValueTypePointer;
attribs[1].value.value.p = &modifier_list;

VASurfaceID surface;
vaCreateSurfaces(display, VA_RT_FORMAT_YUV420, width, height, &surface, 1, attribs, 2);

Closes: #502

[fhvwy]

What is the intent of the array of these attributes? (Alternatively: what is the use-case of allocating a set of surfaces with different format modifiers per-surface?)

[emersion]

I've just matched what VASurfaceAttribMemoryType does here.

[HeJunyan]

So we can get the modifier lists based on the configID, just like the VASurfaceAttribMemoryType?
And we can get all supported modifier belonging to this config, right?
For encoder/decoder, it's OK, but for vpp, how can we know the input/output side?
For example, some of them are just supported in input side while some of them just
for output side.

[emersion]

So we can get the modifier lists based on the configID, just like the VASurfaceAttribMemoryType?
And we can get all supported modifier belonging to this config, right?

This is not implemented in this pull request. This pull request is just about selecting modifiers when allocating a new surface, for now.

For encoder/decoder, it's OK, but for vpp, how can we know the input/output side?
For example, some of them are just supported in input side while some of them just
for output side.

Hmm. How does this work for VASurfaceAttribPixelFormat? Are we lucky and the formats supported for input are exactly the same as the formats supported for output?

[HeJunyan]

So far as I know, the modifier is decisive by the usage of surface. For example,
when we create a surface for encoder, using the hint of VA_SURFACE_ATTRIB_USAGE_HINT_ENCODER,
the modifier should be a Y_TILE modifier, we can not change it to a linear one. Can we really
select the modifier with the usage hint?

[emersion]

For AMD hardware, we can. See the original issue: #502.

When decoding NV12 buffers, AMD hardware can select between LINEAR and tiled. For some use-cases (rotated direct scanout) tiled buffers are required.

[XinfengZhang]

it should be capability report , for example, Intel VDEnc support both tile and linear surface, it could report 2 modifiers for encode config, Decode just support tile surface, it could just report 1 modifiers.
so, I have similar question with @fhvwy , who will allocate the list, application or driver? if it is application , application will set num_modifiers as allocation size, driver should set it to the list size the config support

[fhvwy]

Perhaps this should also be readable, returning the list of supported modifiers to vaQuerySurfaceAttributes()?

(Unclear how the memory works in that case, maybe it needs to be a pointer to something allocated inside the driver.)

[emersion]

Yeah, the memory ownership issue is the reason why I haven't added this as a readable attrib. Since the supported modifiers depend on the VAConfig, the driver can't have a single global modifier list.

Maybe a better path forward would be to return one attrib per supported modifier, but then we can't re-use the same attrib.

[XinfengZhang]

I suppose it should be readable
it is a list for query , but vaCreateSurfaces call, the list should just has 1 element. you could not create one surface with two+ modifiers.
another one is, we already could use https://github.com/intel/libva/blob/master/va/va_drmcommon.h#L137 for external surface, so need a comment to describe it not used for PRIME2

[HeJunyan]

Is this used for list all supported modifiers of some config? For VPP, we really think the query result
of VASurfaceAttribPixelFormat is both supported for input and output. Also for the modifier, if input and
output can support the same types of modifiers, we can still do the same way. But if not, we may need to
differentiate which modifiers are for input and which are for output.

[emersion]

I see.

No, this is not used to list all supported modifiers of a given config. It's only used with vaCreateSurfaces.

[HeJunyan]

So, I think the ideal usage of this modifier should be:

1. query the supported modifiers list based on the configID. For example, an encoder
   configID on AMD platform, the query may contain linear and Y_TILE, but on Intel
   platform it may just contain Y_TILE.
2. Then, we select one supported modifier using this attribute to create surface.

For VPP, maybe the things are a little complicated, if input and output modifiers are
different.

[emersion]

I don't think the libva client should be responsible for picking the modifier.

There may be multiple modifiers suitable for a given usage. For instance, both Y_TILED and X_TILED may be supported by KMS. However the client can't guess what modifier is "the best one".

The VA-API driver is a better place to decide which exact modifier to pick. The VA-API driver knows that Y_TILED is better than X_TILED.

That's what the GBM API does too: gbm_bo_create_with_modifiers takes a list of modifiers, and the driver picks the best one from the list. See https://gitlab.freedesktop.org/mesa/mesa/-/blob/f532202f2d55b9ac475b7e3f8c96a4dd23489299/src/gbm/main/gbm.h#L275.

[HeJunyan]

A real problem we need to face is the GPU memory/surface sharing between modules.
The VA output surface may be used as mesa's image/pbo by egl's import. Also, the 3D
render's result can be the input of the VA encoder. For example, we know the
output of the VPP will be imported by the EGL as a texture for 3D rendering, we
should use the X_TILE for this surface(Assuming the 3D driver only support X_TILE).
So I think, a better way may be: Query the supported modifier list first, clip it
with the real usage need, and provide as many choice as we can to let the VA driver
choose the best one.

[emersion]

This sounds like a good plan to me.

However you may notice that the libva query is not strictly necessary. The libva client can just pass the list of modifiers suitable for the usage (e.g. the list of modifiers supported by EGL or KMS). The libva driver can internally intersect the user list with its own list of supported modifiers. That's why I left the query part out.

I'm still willing to add a query API nonetheless if you want it, but it's still unclear to me how to address the memory ownership issue. The driver needs to return something more complex than just a single integer to the user. VAGenericValue can hold a pointer, but who's responsible for allocating/free'ing the memory?

[HeJunyan]

As a gstreamer developer, the query is useful for negotiation. For example, if the
query result of supported modifiers of VPP output is LINEAR and Y_TILE, but the
3D module only support X_TILE input, we can quit in very early stage and report
the error reason more precise. This may also apply to other media frameworks such
as FFMpeg, etc.

About the "memory ownership issue", I am not the VA expert:)

@XinfengZhang

[emersion]

Right. By "not strictly necessary", I mean that it would be nice to have but would still work without it. You'd just get an error from vaCreateSurfaces if VPP output doesn't support EGL modifiers.

Thanks for the feedback!

[emersion]

Ping @XinfengZhang

[emersion]

Thinking about this a little bit more, I think it would be better to have a separate endpoint for querying the list of modifiers. This would allow the driver to return a different list depending on the usage/memtype/etc. Something like:

VAStatus vaQuerySurfaceDRMFormatModifiers(
    VADisplay dpy, /* in */
    VAConfigID config, /* in */
    const VASurfaceAttrib *attrib_list, /* in */
    unsigned int num_attribs, /* in */
    uint64_t *modifiers, /* out */
    unsigned int *num_modifiers /* in/out */
);

[HeJunyan]

Besides the modifier itself, what are the other "attributes" we can or need to get by this API?

[emersion]

Just the list of modifiers compatible with the config and attribs.

For reference, EGL has something similar with eglQueryDmaBufModifiersEXT.

[HeJunyan]

OK, so the attributes are input. Sounds good:)

[emersion]

Yeah, added some comments to make this clearer.

Alternatively, we could just take the usage flags as input instead of the attributes, but this may not be enough information for all drivers.

[XinfengZhang]

I suppose , it is better to be a capability which could be gotten by vaQuerySurfaceAttributes and be set with vaCreateSurfaces. we also should describe the default behavior without this new definitions

[XinfengZhang]

I suppose it should be readable
it is a list for query , but vaCreateSurfaces call, the list should just has 1 element. you could not create one surface with two+ modifiers.
another one is, we already could use https://github.com/intel/libva/blob/master/va/va_drmcommon.h#L137 for external surface, so need a comment to describe it not used for PRIME2

[XinfengZhang]

it should be capability report , for example, Intel VDEnc support both tile and linear surface, it could report 2 modifiers for encode config, Decode just support tile surface, it could just report 1 modifiers.
so, I have similar question with @fhvwy , who will allocate the list, application or driver? if it is application , application will set num_modifiers as allocation size, driver should set it to the list size the config support

[emersion]

Thanks for the feedback!

I suppose , it is better to be a capability which could be gotten by vaQuerySurfaceAttributes and be set with vaCreateSurfaces. we also should describe the default behavior without this new definitions

Ah, so drivers could advertise support for the new VASurfaceAttribDRMFormatModifiers attrib? That does sound like a good idea. Do you want to introduce a new attrib with a bitfield of capabilities, or just a simple attrib that if present indicates support, or do you have something else in mind?

I suppose it should be readable

We've discussed this above. I don't think it's a good idea to make it readable, because the supported modifiers depend on many things, including VASurfaceAttribUsageHint. So I've made a different proposal to add a new entrypoint to query supported DRM modifiers. For now, this pull request doesn't allow applications to retrieve the list of supported modifiers, but is still useful to make VA-API allocate a buffer suitable for KMS or OpenGL.

who will allocate the list, application or driver? if it is application , application will set num_modifiers as allocation size, driver should set it to the list size the config support

That's another issue with making the attribute readable.

I don't think we can make the application allocate the list, because vaQuerySurfaceAttributes overwrites everything in the attrib_list argument. Thus there is no way for the application to pass an already-allocated chunk of memory to vaQuerySurfaceAttributes.

I don't think it's reasonable to make the driver allocate the list, because it's not clear when the memory can be free'd. The driver can't really keep a static list of supported modifiers, because the list changes depending on the VAConfigID and usage.

Am I missing something here?

it is a list for query , but vaCreateSurfaces call, the list should just has 1 element. you could not create one surface with two+ modifiers.

I'd really prefer to let the application provide a list of modifiers to the driver. The driver can then pick the best one from the list. The application can't figure out what the best modifier is, because this is vendor-specific.

This is already what other allocation APIs are doing, including Vulkan and GBM. See for instance gbm_bo_create: it takes a list of modifiers and lets the driver select the best one.

another one is, we already could use https://github.com/intel/libva/blob/master/va/va_drmcommon.h#L137 for external surface, so need a comment to describe it not used for PRIME2

That's a good point. Added a sentence to this effect:

This can only be used when allocating a new buffer, it's invalid to use this attribute when importing an existing buffer.

[XinfengZhang]

Thanks for the feedback!

I suppose , it is better to be a capability which could be gotten by vaQuerySurfaceAttributes and be set with vaCreateSurfaces. we also should describe the default behavior without this new definitions

Ah, so drivers could advertise support for the new VASurfaceAttribDRMFormatModifiers attrib? That does sound like a good idea. Do you want to introduce a new attrib with a bitfield of capabilities, or just a simple attrib that if present indicates support, or do you have something else in mind?

VASurfaceAttrib is already defined value type, it could be a pointer, so, the list of Modifers(same as kernel driver definition) is ok to me.

I suppose it should be readable

We've discussed this above. I don't think it's a good idea to make it readable, because the supported modifiers depend on many things, including VASurfaceAttribUsageHint. So I've made a different proposal to add a new entrypoint to query supported DRM modifiers. For now, this pull request doesn't allow applications to retrieve the list of supported modifiers, but is still useful to make VA-API allocate a buffer suitable for KMS or OpenGL.

I think , vaConfig should cover all of the dependencies, import other variant lead to redundancy. flexibility always conflict with easy use. about the UsageHint, decode/encode already been covered by vaConfig, the only impact should be VPP_READ and VPP_WRITE, the input of VPP may support different modifiers. but the story is same as VASurfaceAttribPixelFormat. the support format list of input and output maybe is different, but we assume the input and output should be same. if we consider the usagehint, it means usagehint will be a input conditions for the query function. it is not current design, maybe we could push it to 3.x ...
another one to resolve the problem is to add the configure attributes for VPP, then you call vaCreateConfig with different attributes, you will get different configs, different configures will get different query result when call vaQuerySurfaceAttributes

who will allocate the list, application or driver? if it is application , application will set num_modifiers as allocation size, driver should set it to the list size the config support

That's another issue with making the attribute readable.

I don't think we can make the application allocate the list, because vaQuerySurfaceAttributes overwrites everything in the attrib_list argument. Thus there is no way for the application to pass an already-allocated chunk of memory to vaQuerySurfaceAttributes.

I don't think it's reasonable to make the driver allocate the list, because it's not clear when the memory can be free'd. The driver can't really keep a static list of supported modifiers, because the list changes depending on the VAConfigID and usage.

Am I missing something here?

refer the vaQuerySurfaceAttributes comments https://github.com/intel/libva/blob/master/va/va.h#L1683. the story is same. we should let user to allocate the list, then driver to fill them.

it is a list for query , but vaCreateSurfaces call, the list should just has 1 element. you could not create one surface with two+ modifiers.

I'd really prefer to let the application provide a list of modifiers to the driver. The driver can then pick the best one from the list. The application can't figure out what the best modifier is, because this is vendor-specific.

from this perspective, you should not send the parameter with modifiers, the behavior is same as before, driver select the best.
why still provide a list? it looks strange. it means driver behavior will be complex, it has to have different combination and different prefer modifier with different combination.

This is already what other allocation APIs are doing, including Vulkan and GBM. See for instance gbm_bo_create: it takes a list of modifiers and lets the driver select the best one.

another one is, we already could use https://github.com/intel/libva/blob/master/va/va_drmcommon.h#L137 for external surface, so need a comment to describe it not used for PRIME2

That's a good point. Added a sentence to this effect:

This can only be used when allocating a new buffer, it's invalid to use this attribute when importing an existing buffer.

[emersion]

VASurfaceAttrib is already defined value type, it could be a pointer, so, the list of Modifers(same as kernel driver definition) is ok to me.

Sorry, I don't understand what you're suggesting here...

I think , vaConfig should cover all of the dependencies

Hmm, ok, then @HeJunyan's concerns are not addressed… It's true that VASurfaceAttribPixelFormat has the same problem though. I suppose it's something that should be fixed separately.

refer the vaQuerySurfaceAttributes comments https://github.com/intel/libva/blob/master/va/va.h#L1683. the story is same. we should let user to allocate the list, then driver to fill them.

It's not the same, because the attrib_list parameter in vaQuerySurfaceAttributes is an out parameter. The driver will not read anything from this list, it will only write its attributes. attrib_list is just malloc'ed by the application, it has undefined contents.

See for instance the va_impl_query_surface_attributes fallback in libva. A similar logic is in the Mesa implementation: https://gitlab.freedesktop.org/mesa/mesa/-/blob/6298347ec71e7e03c02bf262f37d176dbcc2b987/src/gallium/frontends/va/surface.c#L554

So there's no way for applications to pass an already-allocated list of modifiers in the attrib_list parameter.

from this perspective, you should not send the parameter with modifiers, the behavior is same as before, driver select the best.
why still provide a list? it looks strange. it means driver behavior will be complex, it has to have different combination and different prefer modifier with different combination.

Let's take a step back. vaCreateSurfaces can be used for two different things: buffer allocation and buffer import. We need to add proper modifier negotiation in both cases.

This pull request only adds modifier negotiation for buffer allocation. As described in the original issue (#502), here's a scenario where this is required: I want to decode a video with VA-API and display it with KMS.

• VA-API supports the modifiers A, B and C
• KMS only supports modifiers B and C

When creating the surface, I need to tell VA-API to not use modifier A, because KMS doesn't support it. However I don't know whether B is better (more optimized) than C.

This pull request adds a way for applications to tell VA-API to use either modifier B or C. Does this make sense to you?

---

Another scenario where we need to add modifier negotiation is necessary is buffer import. Let's say I render a 3D scene with OpenGL, I want to export a DMA-BUF from OpenGL and import it into VA-API for video encoding.

• VA-API supports modifiers A, B and C
• OpenGL supports modifiers A and D

I need to make sure I don't allocate an OpenGL buffer with modifier D, or else I won't be able to import the buffer in VA-API.

One way to fix this is to let applications query the list of modifiers supported by VA-API, as you suggest. This is not done in this pull request, because I'm only interested in fixing the buffer allocation case described above.

[emersion]

Ping @XinfengZhang

[XinfengZhang]

sorry for late response.

Let's take a step back. vaCreateSurfaces can be used for two different things: buffer allocation and buffer import. We need to add proper modifier negotiation in both cases.

This pull request only adds modifier negotiation for buffer allocation. As described in the original issue (#502), here's a scenario where this is required: I want to decode a video with VA-API and display it with KMS.

• VA-API supports the modifiers A, B and C
• KMS only supports modifiers B and C

When creating the surface, I need to tell VA-API to not use modifier A, because KMS doesn't support it. However I don't know whether B is better (more optimized) than C.

This pull request adds a way for applications to tell VA-API to use either modifier B or C. Does this make sense to you?

I still think, B or C should be chosen by application, not by driver. application should just choose one , not a list. which one is better between B or C is is another problem, it should be fixed by the query function by the order or some flag.

Another scenario where we need to add modifier negotiation is necessary is buffer import. Let's say I render a 3D scene with OpenGL, I want to export a DMA-BUF from OpenGL and import it into VA-API for video encoding.

• VA-API supports modifiers A, B and C
• OpenGL supports modifiers A and D

I need to make sure I don't allocate an OpenGL buffer with modifier D, or else I won't be able to import the buffer in VA-API.

One way to fix this is to let applications query the list of modifiers supported by VA-API, as you suggest. This is not done in this pull request, because I'm only interested in fixing the buffer allocation case described above.

I think we should consider both two directions, one for query, another for set. there are an assumption for this PR, application already know something about the modifier support list.

yes, you are correct. refer the code you shared , the pre-allocated list is not ok. because the pointer will be a potential issue. even it is not break current definition, but maybe cause the regressions. so the definition of https://github.com/intel/libva/blob/master/va/va.h#L1535 is not very friendly for query function
seems query function just support "integer"
could we add a new element in the union, such as int64_t dw_value; it will break the compatibility of 32 bit , but will be ok for 64 bit. the union size will be kept un-changed for 32bit, because there are already a pointer.

[XinfengZhang]

continue, and if you set B, C. after vaCreateSurfaces, you still dont know which take effect until vaExportSurfaceHandle :)

[emersion]

I still think, B or C should be chosen by application, not by driver. application should just choose one , not a list. which one is better between B or C is is another problem, it should be fixed by the query function by the order or some flag.

What's the motivation for this?

That'd be inconsistent with all of the other format modifier APIs (GBM, Vulkan). The application is never in a good position to decide.

continue, and if you set B, C. after vaCreateSurfaces, you still dont know which take effect until vaExportSurfaceHandle :)

Yes, that's fine. That's how all other APIs work as well.

I think we should consider both two directions, one for query, another for set. there are an assumption for this PR, application already know something about the modifier support list.

Yes, but the two can be done separately. I only need one direction, I don't need the other.

could we add a new element in the union, such as int64_t dw_value

What would this be used for? This can't be used for storing a modifier, because a modifier isn't enough: we need to expose (format, modifier) pairs. Not all formats are compatible with all modifiers.

[emersion]

Would advice from other graphics people help convince you that this is the right thing to do? I can start a thread on the dri-devel mailing list to ask them what they think.

[XinfengZhang]

Yes, but the two can be done separately. I only need one direction, I don't need the other
yes, maybe the PR is enough for your usage. the assumption is you already know all about the story, you know the support list. but most real usage case should query firstly. then set.

I just wonder whether we could re-use your PR for both query and set.

till now, it looks difficult.

could we add a new element in the union, such as int64_t dw_value

What would this be used for? This can't be used for storing a modifier, because a modifier isn't enough: we need to expose (format, modifier) pairs. Not all formats are compatible with all modifiers.

yes, different format maybe support different modifiers, modifiers tell the layout /compress information basing on the format. . but I suppose , in practice , all format should support same modifiers. ---- maybe its incorrect.

[fooishbar]

@XinfengZhang Hi! I'm one of the people who worked on the EGL extension and Wayland protocol specifications, the implementations of those, and also the original KMS implementation. To be honest, I have to agree with @emersion here.

yes, different format maybe support different modifiers, modifiers tell the layout /compress information basing on the format. . but I suppose , in practice , all format should support same modifiers. ---- maybe its incorrect.

It would be really nice if this was true! Unfortunately it's not correct. For instance, Intel's own Gen12 compression only supports a very limited number of formats - it does not support 10bpc or 16bpp RGB, for example.

One of the principles of modifiers is that modifiers have no meaning by themselves. Modifiers are only meaningful when combined together with a format. For this reason, every other API groups format and modifier together.

I still think, B or C should be chosen by application, not by driver. application should just choose one , not a list. which one is better between B or C is is another problem, it should be fixed by the query function by the order or some flag.

The application cannot determine which is 'better'. On Intel, the answer is quite easy: CCS is 'better' than Y-tiling, Y-tiling is better than X-tiling for rendering (but a lot worse for display!), and both are better than linear. For AMD and NVIDIA in particular, the choice of the best modifier depends heavily on not just the format but also things like the surface dimensions. In order for VA-API to inform the client which choice is best, it would have to expose a no-op/dry-run surface-creation entrypoint with all of the parameters required to create a surface, but only returning a modifier choice instead of a surface. This seems like unnecessary duplication.

Above all though, I think it is most important for VA-API to be consistent with the rest of the ecosystem. As can be seen in this thread, there is a lot of confusion already about modifiers and how they should work. To reduce this confusion, every other API (EGL, Vulkan, Wayland, X11, KMS, GStreamer, etc) follows a standard set of principles:

• the user is able to query a list of (format,modifier) combinations to see which combinations could possibly work when allocating: this is not a guarantee that they will work in every case, just an offer that they might work (for instance, NVIDIA hardware has restrictions on which tile sizes can work with which surface dimensions) if you try them
• the user can intersect these sets between multiple APIs: for instance, if a user wishes to use EGL/OpenGL to render a buffer, display that buffer to KMS, and also provide that surface to VA-API for encoding, it would obtain the list of supported modifiers for its chosen format from all three APIs and intersect those three lists to find the common set of acceptable modifiers
• when the user requests a buffer be allocated (e.g. creating a VASurface, EGLSurface, or gbm_surface), it provides a list of modifiers to the allocating API, and the API is responsible for choosing which modifier it thinks is 'best' for the use
• after the buffer has been allocated, it has exactly one modifier which may be queried and remains constant for the lifetime of the buffer

VA-API should follow these principles for consistency and correctness.

Thanks! Looking forward to having VA support modifiers.

[XinfengZhang]

thanks @fooishbar & @emersion , it is clear now. I am ok about this PR.

but I still have a question with principle 3. by the principle 1, seems we already could report a combination list. why we could not use the list order, or some flag to describe which one is "best". still need principle 3? seems a little redundant. of course, from driver implementation side. both should be ok.

another one question is : where to find these principles? seems we missed some information or decisions

[emersion]

Thanks for taking the time to read all of this and figuring out how modifiers are designed.

I just wonder whether we could re-use your PR for both query and set. till now, it looks difficult.

Yes. I'm still not sure how to design the part of the API that lets you query supported modifiers. I've posted a proposal earlier which adds a new separate function. That proposal is probably too complicated, it can probably be simplified a little by removing the attrib list.

why we could not use the list order, or some flag to describe which one is "best". still need principle 3? seems a little redundant.

I think @fooishbar explained the reason here: "the choice of the best modifier depends heavily on not just the format but also things like the surface dimensions". Maybe modifier A is better if the buffer is small but modifier B is better if the buffer is large. So there's no "best" modifier in general. Depending on the situation, one modifier may be the best one, but it's not the best one in all cases.

In other words, some drivers can't sort the list of modifiers by order of preference.

another one question is : where to find these principles? seems we missed some information or decisions

Yes, I agree we should document all of this. I'll discuss with @danvet to see what the best place for such docs would be. Maybe drm_fourcc.h in the kernel? Maybe a .rst file in the kernel docs?

The Vulkan extension already describes some of the principles (of course with Vulkan-specific bits): https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VK_EXT_image_drm_format_modifier.html#_usage_patterns

---

Moving forward: I'll start working on a Mesa implementation of this pull request. We still need to add a way for drivers to signal that they support VASurfaceAttribDRMFormatModifiers.

While I only need the API introduced in this PR, I could also help with an API to query supported (format, modifier) combinations for a given usage. Let me know if you're interested.

[fooishbar]

In other words, some drivers can't sort the list of modifiers by order of preference.

It's not just preference, sometimes it's a hard requirement. For instance, say your GPU can use either 16x16 or 32x32 tiling modes, and it prefers to use 16x16 because it's more optimal, but it also has a restriction that the surface cannot not exceed 64 tiles in width. So you do you put 16x16 at the top of the list because it's 'better' but doesn't work with width > 1024, or do you put 32x32 at the top of the list because it's slower but works in more cases?

(This is not a theoretical case ... obviously the numbers are simplified but this is a real limitation that exists for some vendors.)

[danvet]

Yes, I agree we should document all of this. I'll discuss with @danvet to see what the best place for such docs would be. Maybe drm_fourcc.h in the kernel? Maybe a .rst file in the kernel docs?

The Vulkan extension already describes some of the principles (of course with Vulkan-specific bits): https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VK_EXT_image_drm_format_modifier.html#_usage_patterns

If it's not too much text, adding another in-line DOC: section in drm_fourcc.h with these principles is probably best. You can use full rst formatting, including headings and everything (just need to be aware that the heading nesting is inpreted in the context of the .rst file that pulls in the DOC: section, there's no adjustment being done automatically, it's really just a fancy #include style statement)

Also would be really good to get that doc patch reviewed as widely as possible. @fooishbar can help with anything that's missing I'm sure :-)

[emersion]

I've opened #519 to allow applications to figure out whether a driver supports VASurfaceAttribDRMFormatModifiers (and other surface attributes). Let me know if you prefer a different solution.

[XinfengZhang]

@HeJunyan @dvrogozh if there are no other concern , please help +1, we will file another PR against https://github.com/intel/media-driver/ to implement it. and merge the PR

[emersion]

The Mesa implementation is available here: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/10134