add new interface vaMapBuffer2 for map operation optimization #377

XinfengZhang · 2020-02-24T08:30:43Z

add new vaMapBuffer2
backend driver need some usage hint to optimize the operation

Signed-off-by: Carl Zhang [email protected]

va/va.c

dvrogozh · 2020-02-24T15:35:53Z

va/va.h

+ */
+typedef enum
+{
+    VAMapBufferFlagDefault = 0,


I think we need to add a comment what default means (i.e. read|write). Also, I think we need to add a comment that these enum flags are being supported for bitwise operations.

please, resolve

I suggest this text:

/** \brief VAMapBufferFlagDefault is used when there are no flag specified * Same as VAMapBufferFlagRead | VAMapBufferFlagWrite. */

they are enum values , not macro, it is why I dont use VAMapBufferFlagRead | VAMapBufferFlagWrite, or next extended flag must set value to 0x4?

next extended flag must set value to 0x4?

Maybe we will just use #define VAMapBufferFlagDefault 0 and so on then?

va/va.c

dvrogozh · 2020-02-28T15:30:39Z

va/va.c

+  { 
+      va_status = ctx->vtable->vaMapBuffer( ctx, buf_id, pbuf );
+  }
+  VA_TRACE_ALL(va_TraceMapBuffer, dpy, buf_id, pbuf, 0);


VAMapBufferFlagDefault

dvrogozh · 2020-02-28T15:30:54Z

va/va.h

+ */
+typedef enum
+{
+    VAMapBufferFlagDefault = 0,


please, resolve

dmitryermilov · 2020-03-02T19:21:39Z

va/va.h

+{
+    /** \brief VAMapBufferFlagDefault is used when there are no flag specified*/
+    VAMapBufferFlagDefault = 0,
+    /** \brief application will read the surface after map */


Can the flags be used on OR ? It's not clear from text description.

+1. Not clear whether this VAMapBufferFlagRead | VAMapBufferFlagWrite can be done. For example:

/** \brief Bitwise flag: application will read the surface after map */

and same below for write:

/** \brief Bitwise flag: application will write the surface after map */

dvrogozh · 2020-03-02T19:32:52Z

va/va.h

+ */
+typedef enum
+{
+    VAMapBufferFlagDefault = 0,


I suggest this text:

/** \brief VAMapBufferFlagDefault is used when there are no flag specified * Same as VAMapBufferFlagRead | VAMapBufferFlagWrite. */

dvrogozh · 2020-03-02T19:34:45Z

va/va.h

+{
+    /** \brief VAMapBufferFlagDefault is used when there are no flag specified*/
+    VAMapBufferFlagDefault = 0,
+    /** \brief application will read the surface after map */


+1. Not clear whether this VAMapBufferFlagRead | VAMapBufferFlagWrite can be done. For example:

/** \brief Bitwise flag: application will read the surface after map */

and same below for write:

/** \brief Bitwise flag: application will write the surface after map */

IlyaSergeyev · 2020-07-30T14:46:00Z

@XinfengZhang What the current status? This change can significantly improve encoding performance.

dmitryermilov · 2020-11-09T21:14:10Z

va/va.h

+/** \brief VA_MAPBUFFER_FLAG_DEFAULT is used when there are no flag specified
+ * same as VA_MAPBUFFER_FLAG_READ | VA_MAPBUFFER_FLAG_WRITE.
+ */
+#define VA_MAPBUFFER_FLAG_DEFAULT 0


@dvrogozh , @XinfengZhang , can we do drop VA_MAPBUFFER_FLAG_DEFAULT because VA_MAPBUFFER_FLAG_DEFAULT in fact == VA_MAPBUFFER_FLAG_READ | VA_MAPBUFFER_FLAG_WRITE

I think that having explicitly said what's the behavior in case of naive default value (=0) is good. I would leave DEFAULT declared.

HeJunyan · 2021-04-15T06:10:33Z

va/va.c

  VA_FOOL_FUNC(va_FoolMapBuffer, dpy, buf_id, pbuf);
-
-  va_status = ctx->vtable->vaMapBuffer( ctx, buf_id, pbuf );
+  if (ctx->vtable->vaMapBuffer2)


It's really a good optimization because of the flags can save the un-needed copy.
The misc usage of vtable->vaMapBuffer and vtable->vaMapBuffer2 within vaMapBuffer vaMapBuffer2 APIs may be verbose. For vaMapBuffer2 API, if driver does not provide the virtual function point, we can return VA_STATUS_ERROR_UNIMPLEMENTED directly. The app user should consider the fallback cases.

in the future, I suppose, somedriver will just implement vaMapBuffer2, to be compatible with old app, need vaMapBuffer also be supported. this why I add similar fallback logic in vaMapBuffer and vaMapBuffer2

nowrep · 2023-10-07T18:09:33Z

What's the status of this? It would be great to have it merged.
Currently the issue in Mesa is that mapping all buffers read+write leads to bad performance on dGPUs.

dvrogozh · 2023-10-07T18:32:25Z

If I recall correctly, it was not merged because of lack of implementation on the (intel) driver side. However, considering that new driver appeared (vaon12) and obviously it has interest in this feature, I think we need to go forward with it. PR needs to be rebased and conflicts which appeared while it was hanging around resolved (@XinfengZhang : can you, please, do that? or I can pick it up). And PR needs re-review.

@nowrep : you are looking for this PR in regards of mesa's vaon12?

nowrep · 2023-10-07T18:37:18Z

That sounds good, thanks!

My interest for this PR is radeonsi but it will benefit all Mesa drivers, so that also includes vaon12.

dvrogozh · 2023-10-07T18:38:51Z

Is there any discussion on mesa side regarding this? Maybe some issue filed?

nowrep · 2023-10-07T18:54:57Z

This PR came up in discussion of this MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25597

Context: Mesa always mapped the buffer as write-only. Up until recently vaDeriveImage was not implemented for multi-planar formats on radeonsi, so applications would fallback to vaPut/GetImage. That's not the case anymore and it is now an issue when applications try to read from the mapped memory. The simple fix was to map the buffer as read+write, but it regressed performance when used for writing only so the change was reverted.

add new vaMapBuffer2 backend driver need some usage hint to optimize the operation Signed-off-by: Carl Zhang <[email protected]>

dvrogozh

still looks good

dvrogozh · 2023-10-09T14:22:22Z

va/va.h

+/** \brief VA_MAPBUFFER_FLAG_DEFAULT is used when there are no flag specified
+ * same as VA_MAPBUFFER_FLAG_READ | VA_MAPBUFFER_FLAG_WRITE.
+ */
+#define VA_MAPBUFFER_FLAG_DEFAULT 0


I think that having explicitly said what's the behavior in case of naive default value (=0) is good. I would leave DEFAULT declared.

dvrogozh · 2023-10-09T14:30:54Z

@nowrep, @sivileri : we are ready to merge in this libva change as soon as some driver(s) will raise a PR on their side to use this new VAAPI feature. Can you, please, help to prepare a change on mesa side and get it tried out?

nowrep · 2023-10-09T15:41:09Z

Mesa implementation: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25620

dvrogozh · 2023-10-10T20:56:31Z

Thank you @nowrep for quick follow up. Quoting perf example from https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25620#note_2119488:

"Example with patched ffmpeg to use vaMapBuffer2:

ffmpeg -vaapi_device /dev/dri/renderD128 -f lavfi -i testsrc=size=1280x720 -vf format=nv12,hwupload -c:v h264_vaapi -async_depth 1 -f null -

Before (read+write): 540fps
After  (write only): 620fps

It's around 10-15% faster depending on the resolution. At 2048x2048 it's 160fps before and 180fps after."

dvrogozh · 2023-10-10T20:57:01Z

I don't see a reason to hold it on any further. Merging in.

XinfengZhang requested review from dvrogozh, rojkov and onabiull February 24, 2020 08:30

dvrogozh requested changes Feb 24, 2020

View reviewed changes

XinfengZhang force-pushed the map2 branch from 353d451 to a4d28f4 Compare February 28, 2020 09:21

dvrogozh requested changes Feb 28, 2020

View reviewed changes

XinfengZhang force-pushed the map2 branch from a4d28f4 to 05bcec6 Compare March 2, 2020 13:55

dmitryermilov reviewed Mar 2, 2020

View reviewed changes

dvrogozh requested changes Mar 2, 2020

View reviewed changes

rojkov removed their request for review March 30, 2020 11:46

XinfengZhang force-pushed the map2 branch from 05bcec6 to a98d564 Compare April 12, 2020 14:38

dvrogozh approved these changes Apr 16, 2020

View reviewed changes

dvrogozh mentioned this pull request Nov 9, 2020

add HW assistant copy interface #243

Merged

dmitryermilov reviewed Nov 9, 2020

View reviewed changes

HeJunyan reviewed Apr 15, 2021

View reviewed changes

va:add new interface vaMapBuffer2 for map operation optimization

8f9ec28

add new vaMapBuffer2 backend driver need some usage hint to optimize the operation Signed-off-by: Carl Zhang <[email protected]>

XinfengZhang force-pushed the map2 branch 2 times, most recently from 15f6459 to 8f9ec28 Compare October 9, 2023 07:52

dvrogozh approved these changes Oct 9, 2023

View reviewed changes

dvrogozh merged commit 45afd79 into intel:master Oct 10, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new interface vaMapBuffer2 for map operation optimization #377

add new interface vaMapBuffer2 for map operation optimization #377

XinfengZhang commented Feb 24, 2020

dvrogozh Feb 24, 2020

dvrogozh Feb 28, 2020

dvrogozh Mar 2, 2020

XinfengZhang Mar 3, 2020

dvrogozh Mar 3, 2020

dvrogozh Feb 28, 2020

dvrogozh Feb 28, 2020

dmitryermilov Mar 2, 2020

dvrogozh Mar 2, 2020

dvrogozh Mar 2, 2020

dvrogozh Mar 2, 2020

IlyaSergeyev commented Jul 30, 2020

dmitryermilov Nov 9, 2020

dvrogozh Oct 9, 2023

HeJunyan Apr 15, 2021

XinfengZhang Apr 15, 2021

nowrep commented Oct 7, 2023

dvrogozh commented Oct 7, 2023

nowrep commented Oct 7, 2023

dvrogozh commented Oct 7, 2023

nowrep commented Oct 7, 2023

dvrogozh left a comment

dvrogozh Oct 9, 2023

dvrogozh commented Oct 9, 2023

nowrep commented Oct 9, 2023

dvrogozh commented Oct 10, 2023

dvrogozh commented Oct 10, 2023

add new interface vaMapBuffer2 for map operation optimization #377

add new interface vaMapBuffer2 for map operation optimization #377

Conversation

XinfengZhang commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaSergeyev commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nowrep commented Oct 7, 2023

dvrogozh commented Oct 7, 2023

nowrep commented Oct 7, 2023

dvrogozh commented Oct 7, 2023

nowrep commented Oct 7, 2023

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh commented Oct 9, 2023

nowrep commented Oct 9, 2023

dvrogozh commented Oct 10, 2023

dvrogozh commented Oct 10, 2023