Skip to content
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

Introduce VASurfaceAttribDRMFormatModifiers #505

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Introduce VASurfaceAttribDRMFormatModifiers #505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions va/va.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1581,6 +1581,13 @@ typedef enum {
/** \brief Surface usage hint, gives the driver a hint of intended usage
* to optimize allocation (e.g. tiling) (int, read/write). */
VASurfaceAttribUsageHint,
/** \brief List of possible DRM format modifiers (pointer, write).
*
* The value must be a pointer to a VADRMFormatModifierList. This can only
* be used when allocating a new buffer, it's invalid to use this attribute
* when importing an existing buffer.
*/
VASurfaceAttribDRMFormatModifiers,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/** \brief Number of surface attributes. */
VASurfaceAttribCount
} VASurfaceAttribType;
Expand Down
17 changes: 17 additions & 0 deletions va/va_drmcommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,5 +154,22 @@ typedef struct _VADRMPRIMESurfaceDescriptor {
} layers[4];
} VADRMPRIMESurfaceDescriptor;

/**
* \brief List of DRM format modifiers.
*
* To allocate surfaces with one of the modifiers specified in the array, call
* vaCreateSurfaces() with the VASurfaceAttribDRMFormatModifiers attribute set
* to point to an array of num_surfaces instances of this structure. The driver
* will select the optimal modifier in the list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just matched what VASurfaceAttribMemoryType does here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* DRM format modifiers are defined in drm_fourcc.h in the Linux kernel.
*/
typedef struct _VADRMFormatModifierList {
/** Number of modifiers. */
uint32_t num_modifiers;
/** Array of modifiers. */
uint64_t *modifiers;
emersion marked this conversation as resolved.
Show resolved Hide resolved
} VADRMFormatModifierList;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@emersion emersion Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@HeJunyan HeJunyan Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


#endif /* VA_DRM_COMMON_H */