I uploaded some patches for this issue to my branch.
https://github.com/zzoon/gst-plugins-bad/commits/dmabuf

Probably we can discuss on it if you don't mind.

Created attachment 370037 [details] [review]
msdk: specify the way to find a proper cached response by  request

The current way to find proper response by just comparing request's
value is wrong.  We need to compare the size of a frame and the 
number of suggested frames.

Refer to the sample in https://github.com/Intel-Media-SDK/samples.

Created attachment 370038 [details] [review]
msdk: generalize the parameter of msdk video memory  functions

There needs to be generalized for the parameter from
GstVideoMsdkVideoMemory to GstMemory.

Created attachment 370039 [details] [review]
msdk: allocator: get dmabuf handle when allocation if  required

Call vaAcquireBufferHandle to get dmabuf fd and size.

Note that vaExportSurfaceHandle is released in new version of VA-API,
which is not available in msdk currently.

Created attachment 370040 [details] [review]
msdk: adds new function to get dmabuf information from  surface.

Created attachment 370041 [details] [review]
msdk: dmabuf support

This patch includes:
1\ Implements MsdkDmaBufAllocator and allocation of msdk dmabuf memroy.
2\ Each msdk dmabuf memory include its own msdk surface kept by GQuark.
3\ Adds new option GST_BUFFER_POOL_OPTION_MSDK_USE_DMABUF

Created attachment 370042 [details] [review]
msdkdec: use dmabuf if possible

MSDK decoders starts using dmabuf only if downstream supports DMABuf capsfeature.

Review of attachment 370039 [details] [review]:

::: sys/msdk/gstmsdkallocator_libva.c
@@ +121,3 @@
+        va_status =
+            vaAcquireBufferHandle (gst_msdk_context_get_handle (context),
+            msdk_mids[i].image.buf, &msdk_mids[i].info);

When this vaAcquireBufferHandle() is released with vaReleaseBufferHandle()? Otherwise a memory/fd leak will show up.

Created attachment 370134 [details] [review]
msdk: allocator: get dmabuf handle when allocation if  required

(In reply to Víctor Manuel Jáquez Leal from comment #8)
> Review of attachment 370039 [details] [review] [review]:
> 
> ::: sys/msdk/gstmsdkallocator_libva.c
> @@ +121,3 @@
> +        va_status =
> +            vaAcquireBufferHandle (gst_msdk_context_get_handle (context),
> +            msdk_mids[i].image.buf, &msdk_mids[i].info);
> 
> When this vaAcquireBufferHandle() is released with vaReleaseBufferHandle()?
> Otherwise a memory/fd leak will show up.

Thanks. I forgot it :(

This one adding only "dmabuf-export" right? Please change the bug report title then.
We need the "dmabuf-import"(in msdk plugin) for v4l2 use case, better file a separate bug for that.

Review of attachment 370042 [details] [review]:

::: sys/msdk/gstmsdkdec.c
@@ +423,3 @@
+
+  if (gst_msdk_find_preferred_caps_feature (thiz,
+          GST_CAPS_FEATURE_MEMORY_DMABUF))

This routine is not for finding preferred capsfeatures, rather it just checking whether downstream has support for DMABufCapsFeatures.

It could be useful in future for checking other capsfeatures too but for now, it is confusing. Could you please change the function name?

Review of attachment 370134 [details] [review]:

I think you need to make sure that the gst_msdk_frame_lock() never tries to map(vaDeriveImage) if dmabuf is in use since the alloc() already derived the image for AcquireBufferHandle operation.
As long as we negotiate the "DMABuf" capsfeatures, gstreamer is not supposed to map, but in case if msdk tries to do the same it will cause issues.

(In reply to sreerenj from comment #11)
> This one adding only "dmabuf-export" right? Please change the bug report
> title then.
> We need the "dmabuf-import"(in msdk plugin) for v4l2 use case, better file a
> separate bug for that.

I'm not sure how to implement dmabuf-import in msdk elements.
Because it should create a VA surface when it gets dmabuf handle from upstream but, by design, allocation should be done at initialization time in msdk.

Any idea to handle this?

I think we should achieve the dmabuf-import in v4l2 case fist. As you know, there've been discuss in bug #792034.

By the way, I don't have permission to edit this bug :)

(In reply to sreerenj from comment #13)
> Review of attachment 370134 [details] [review] [review]:
> 
> I think you need to make sure that the gst_msdk_frame_lock() never tries to
> map(vaDeriveImage) if dmabuf is in use since the alloc() already derived the
> image for AcquireBufferHandle operation.
> As long as we negotiate the "DMABuf" capsfeatures, gstreamer is not supposed
> to map, but in case if msdk tries to do the same it will cause issues.

Good point. Agreed.

Review of attachment 370041 [details] [review]:

Q1)There is code to duplicate the fd, but I haven't seen a place which closing it. Am I missing something here?


Q2)Another thing I have noticed is the confusion in preallocation of buffers:
When the GsttBufferpool preallocate the minimum number of buffers (when gstbufferpool activate the pool) there are few things happening:
The basecalss allocate the buffer (default_start() implementation) and push back to the pool by invoking the release_buffer(). But we have our own release_buffer implementation which
will invoke gst_msdk_context_put_surface_available(). As a result, during preallocation we use the same mfxFrameSurface1 in each buffer creation.
It can work with out any issue, but I want to make sure that it is the expected behavior.

::: sys/msdk/gstmsdkbufferpool.c
@@ +310,3 @@
+      gst_buffer_unset_flags (buf, GST_BUFFER_FLAG_TAG_MEMORY);
+    }
+    gst_msdk_get_dmabuf_info_from_surface (surface, &fd, NULL);

I'm totally confused here!
It seems gst_dmabuf_memory_get_fd (gst_buffer_peek_memory (buf, 0)) != fd) will be always "TRUE".

What I can see is that the gst_msdk_dmabuf_memory_new_with_surface() always duplicate the fd, but once you do "new_fd = dup(old_fd) ", new_fd and fd will have different values.
Which means, the fd attaching to the memory will never gonna match with surface->fd (in any iteration).

(In reply to Hyunjun Ko from comment #14)
> (In reply to sreerenj from comment #11)
> > This one adding only "dmabuf-export" right? Please change the bug report
> > title then.
> > We need the "dmabuf-import"(in msdk plugin) for v4l2 use case, better file a
> > separate bug for that.
> 
> I'm not sure how to implement dmabuf-import in msdk elements.
> Because it should create a VA surface when it gets dmabuf handle from
> upstream but, by design, allocation should be done at initialization time in
> msdk.
> 
> Any idea to handle this?

Currently, the decoder is doing in Init() in decide_allocation(), we should do the same in encoder. Delay the Initialization until propose_allocation() so that you will get a handle of v4l2src proposed pool. 
Then we may be able to do pre-allocation of buffers within msdk element, grab the dmabuf ids from the memory and use it for mfx/vaSurface allocation.

But I'm not sure whether v2l4src has other constraints on buffer allocation like it "has to be done at a specific point after downstream pool negotiation"

> 
> I think we should achieve the dmabuf-import in v4l2 case fist. As you know,
> there've been discuss in bug #792034.

V4l2 always prefer "dmabuf-export" (Nicolas well explained the reasons in the other bug reports) .
BTW, v4l2 "dmabuf-import" has been set to userptr unless I am mistaken. 
Which means MSDK-"dmabuf-export" is a mandatory feature for zero copy camera encode.

> 
> By the way, I don't have permission to edit this bug :)

Question, if upstream was to "commit" it's decided allocation, would that help ? In recent research, we found that delaying allocation to when first buffer comes increases startup latency (specially with CMA), so it's something we are considering, optional of course, for backward compatibility. To help understand, this would mimic the caps query followed by caps event model.

Review of attachment 370040 [details] [review]:

Do we need the new header? 
Is the gst_msdk_get_dmabuf_info_from_surface () not a candidate for msdk.c/msdk.h ?

Review of attachment 370038 [details] [review]:

lgtm

Review of attachment 370037 [details] [review]:

I still have concerns over this when thinking about the complex transcode pipeline (with multipe decoders).
But since this is what the msdk samples are doing, let's get this in.

(In reply to sreerenj from comment #16)
> Review of attachment 370041 [details] [review] [review]:
> 
> Q1)There is code to duplicate the fd, but I haven't seen a place which
> closing it. Am I missing something here?

In gstfdmemory, it's closed when releasing the memory.

> 
> 
> Q2)Another thing I have noticed is the confusion in preallocation of buffers:
> When the GsttBufferpool preallocate the minimum number of buffers (when
> gstbufferpool activate the pool) there are few things happening:
> The basecalss allocate the buffer (default_start() implementation) and push
> back to the pool by invoking the release_buffer(). But we have our own
> release_buffer implementation which
> will invoke gst_msdk_context_put_surface_available(). As a result, during
> preallocation we use the same mfxFrameSurface1 in each buffer creation.
> It can work with out any issue, but I want to make sure that it is the
> expected behavior.

Yeah, I knew this. 
I did decide to let it be since it doesn't cause any problem.

> 
> ::: sys/msdk/gstmsdkbufferpool.c
> @@ +310,3 @@
> +      gst_buffer_unset_flags (buf, GST_BUFFER_FLAG_TAG_MEMORY);
> +    }
> +    gst_msdk_get_dmabuf_info_from_surface (surface, &fd, NULL);
> 
> I'm totally confused here!
> It seems gst_dmabuf_memory_get_fd (gst_buffer_peek_memory (buf, 0)) != fd)
> will be always "TRUE".
> 
> What I can see is that the gst_msdk_dmabuf_memory_new_with_surface() always
> duplicate the fd, but once you do "new_fd = dup(old_fd) ", new_fd and fd
> will have different values.
> Which means, the fd attaching to the memory will never gonna match with
> surface->fd (in any iteration).

Correct. This is a bit related to the Qeustion 1.
gst_dmabuf_allocator_alloc calls just gst_fd_allocator_alloc with GST_FD_MEMORY_FLAG_NONE flag. But we need to set GST_FD_MEMORY_FLAG_DONT_CLOSE flag but we can't with gstdmabuf.h

If we calls gst_fd_allocator_alloc with GST_FD_MEMORY_FLAG_DONT_CLOSE(which is possible but not sure if it's proper), things got different.

In addition, I didn't find out any other solution other than replacing memory.

(In reply to sreerenj from comment #17)
> (In reply to Hyunjun Ko from comment #14)
> > (In reply to sreerenj from comment #11)
> > > This one adding only "dmabuf-export" right? Please change the bug report
> > > title then.
> > > We need the "dmabuf-import"(in msdk plugin) for v4l2 use case, better file a
> > > separate bug for that.
> > 
> > I'm not sure how to implement dmabuf-import in msdk elements.
> > Because it should create a VA surface when it gets dmabuf handle from
> > upstream but, by design, allocation should be done at initialization time in
> > msdk.
> > 
> > Any idea to handle this?
> 
> Currently, the decoder is doing in Init() in decide_allocation(), we should
> do the same in encoder. Delay the Initialization until propose_allocation()
> so that you will get a handle of v4l2src proposed pool. 

Just curious. Is it possible to get upstream's pool in propose_allocation?
If possible, yeah, we can implement like the way what you said.

(In reply to Nicolas Dufresne (stormer) from comment #18)
> Question, if upstream was to "commit" it's decided allocation, would that
> help ? In recent research, we found that delaying allocation to when first
> buffer comes increases startup latency (specially with CMA), so it's
> something we are considering, optional of course, for backward
> compatibility. To help understand, this would mimic the caps query followed
> by caps event model.

Hmm interesting.
Probably it would help our cases.

You can propose a new allocation function with flags if needed. That flag was added later, when FDMerory was introduced. As we are adding DMA sync to ensure memory coherency, allocating FDMemory instead will cause issues.

(In reply to sreerenj from comment #19)
> Review of attachment 370040 [details] [review] [review]:
> 
> Do we need the new header? 
> Is the gst_msdk_get_dmabuf_info_from_surface () not a candidate for
> msdk.c/msdk.h ?

Oh, I forgot answering this question.
This is actually what I thought for a while.

1. This is only available on Linux.
 -> Candidate was msdk_libva.h/c
2. This needs GstMsdkMemoryID.
 -> Needs to include gstmsdkallocator.h. But looks not good if msdk_libva includes this header since it gstmsdkallocator_libva.c includes msdk_libva.h

That's why I created new header but if you have better opinion I would appreciate it and modifiy it. :)

Created attachment 370324 [details] [review]
msdk: allocator: get dmabuf handle when allocation if  required

Call vaAcquireBufferHandle to get dmabuf fd and size.

Note that vaExportSurfaceHandle is released in new version of VA-API,
which is not available in msdk currently.

Created attachment 370325 [details] [review]
msdkdec: use dmabuf if possible

MSDK decoders starts using dmabuf only if downstream supports DMABuf capsfeature.

(In reply to Nicolas Dufresne (stormer) from comment #25)
> You can propose a new allocation function with flags if needed. That flag
> was added later, when FDMerory was introduced. As we are adding DMA sync to
> ensure memory coherency, allocating FDMemory instead will cause issues.

Thanks.
I open bug #794829. :)

Commet12 and 13 are still not updated in the patch, but let's get those as separate patches :)

Review of attachment 370037 [details] [review]:

Pushed as commit 762eb970921c2e9168bed81342b467243577d113

Review of attachment 370038 [details] [review]:

commit c3438b5a0fde3f57b7c0e59243f1cbb8df623c91

Review of attachment 370324 [details] [review]:

commit bd8ffcb29d036ce08b86106d434d41c474badc26

Review of attachment 370040 [details] [review]:

commit 5df06545efb2a900e350749cdce8e154e89c6111

Review of attachment 370040 [details] [review]:

commit 5df06545efb2a900e350749cdce8e154e89c6111

Review of attachment 370041 [details] [review]:

commit ea92da6e546068a8b7808f414257cc8594c7b4c6

Review of attachment 370325 [details] [review]:

commit cdc591dbc03806ca9772f03cdcab9a2e6d6755b6

(In reply to sreerenj from comment #30)
> Commet12 and 13 are still not updated in the patch, but let's get those as
> separate patches :)

Oops. My bad, Sree. Sorry about this.
This is like a waste of commits. I didn't want it. Sorry again.

Created attachment 370430 [details] [review]
msdk: allocator: libva: check if it's already using  dmabuf when mapping

As long as we negotiate the "DMABuf" capsfeatures for now, map can't be
working. So we need to confirm not to do it if using DMABuf memory.

Created attachment 370431 [details] [review]
msdk: dec: rename the function to what it means more  exactly.

Review of attachment 370430 [details] [review]:

pushed as commit 90491d889aae01a9028ed6898f88763993d06ad8

Review of attachment 370431 [details] [review]:

Pushed with slight changes: commit 35b6411d4db9e4f56a4b8af1f1f6fce040841cf8