Maybe a justification of what this caps feature will be used for ? In my recent implementation, it was never needed.

(In reply to Nicolas Dufresne (stormer) from comment #1)
> Maybe a justification of what this caps feature will be used for ? In my
> recent implementation, it was never needed.

No real reason, just to mimic what is done for caps features system memory. And actually I was starting and found that I cannot do exactly the same since there is no hook to gst_init. (_priv_gst_caps_features_initialize is called in gst_init).


So just the define would be enough for now right ? (+ some doc)

Created attachment 317223 [details] [review]
WIP allocators: define GST_CAPS_FEATURE_MEMORY_DMABUF

The doc does not appear in gst-plugins-base-libs/html/gst-plugins-base-libs-dmabuf.html, any idea ?

I believe defining it is just that. Now let's start a discussion about what use we will make of it.

So in addition to 

+ * It helps pipeline negotiation when the exporter is the upstream element from
+ * the importer point of view.

It is useful to force dmabuf usage in order to avoid any conversion between exporter and importer. The elements that cannot handle this caps feature will just put themself in passthrough mode (videobalance, videoscale, videoconvert ...)
Same idea as between omxvideodec and glimagesin when using memory:EGLImage.

For example if the exporter is gstvaapidecodebin and importer is glimagesink then the decoder may want to set GST_CAPS_FEATURE_MEMORY_DMABUF to ensure to terminate the pipeline if importation fails.

> It is useful to force dmabuf usage in order to avoid any conversion between
> exporter and importer. The elements that cannot handle this caps feature
> will just put themself in passthrough mode (videobalance, videoscale,
> videoconvert ...)
> Same idea as between omxvideodec and glimagesin when using memory:EGLImage.
> 
> For example if the exporter is gstvaapidecodebin and importer is glimagesink
> then the decoder may want to set GST_CAPS_FEATURE_MEMORY_DMABUF to ensure to
> terminate the pipeline if importation fails.

For EGLImage, where you can't map to CPU, that make sense, for for mappable dmabuf, I'm not so sure. The idea for EGLImage was that it would fail at negotiation because it would otherwise mail on memory map. For DMABuf, it won't fail in memory map.

So this feature is just to help developer to find missing color formats and/or features in their display sink element. To me, this is not a good reason, we could improve the CAT_PERFORMANCE instead.

(In reply to Julien Isorce from comment #5)
> So in addition to 
> 
> + * It helps pipeline negotiation when the exporter is the upstream element
> from
> + * the importer point of view.

I'm still unclear upon why upstream would propose memory pointers when dmabuf is available. My view might be limited to V4L2, but in V4L2, there is very low cost of always exporting. This is the solution I see. Both the downstream and upstream model can mimic the GL uploader, where you actually introspect the caps/buffers/memory choose the best method.

What I'd like to see, is proper instructions on how this meta shall be used, with understandable examples rather then supposition. The use case I have in mind, is the text overlays. Unless you have CompositionMeta overlay support in your sink, you can no longer render the overlay if the DMABuf meta is enabled. As of today's ARM CPUs, you may get more then acceptable performances. But then, if you add DMABuf meta support to the text overlay, you loose the ability to prevent any kind of SW rasterization to happen.

Here is the draft I am suggesting to describe how this caps feature should be used. That would be great if we can build from that. (even if we change all of it in the end). I.e., let try to refine the following logic:

1- General idea:
The exporter element either the sink (wldmabufsink) or decoder (gstvaapidecodebin / gstv4l2videodec), should try to map the first buffer. If it fails then it adds the caps feature, otherwise it does not add it.
That logic could be implemented in the src / decoder / transformer / sink bases classes or somewhere else if more appropriate.

2: Customizable
The idea is to assume the mmap is not expensive by default. Cases where it is known to be slow or known to fail to mmap, should be treated as a special case (platform dependent or so) in the derived gst element itself (example wldmabufsink ?).
Same note for the other way around, i.e. disable this first mmap if it is knows to succeeds or it is a requirement. This should treated as a special case.
We should have helpers to turn these ON/OFF easily.

3: the case where the user explicitly does:
... elem1 ! video/x-raw(memory:DMABuf) ! elem2 ...
should always be accepted between an exporter and an importer (or reverse case).
The pipeline should always try to run this way even if it fails to import or so.
(In other word exporter / importer should not reject the caps feature if someone provide it explicitly)

4: There is no negotiation of any allocator. By default an exporter will always try to push its buffers. In-place transformers element will succeeds according to that first "try map buffer". Then if importer fails the exporter should fall back to raw/software.
Again this default behavior should be customizable from the derived classes if needed.

Note:
At first glance the "confirmation" mmap might add an overhead during caps negotiation. But it should be compensated by the fact it will be already in the cache for processing by downstream elements. Also as said above it can be disabled if necessary, so it will only apply to the default behavior.

Do not hesitate to comment on any point, thx!

configure.ac already check if mmap function exist.
Maybe we can extend this check to test if mmap() fails rather then doing that at runtime ? You could also add an option to force enable/disable mmap to configuration time.

(In reply to Benjamin Gaignard from comment #8)
> configure.ac already check if mmap function exist.
> Maybe we can extend this check to test if mmap() fails rather then doing
> that at runtime ? You could also add an option to force enable/disable mmap
> to configuration time.

This check will not work in cross-compilation or when building generic packages for multiple boards.

(In reply to Benjamin Gaignard from comment #8)
> configure.ac already check if mmap function exist.
> Maybe we can extend this check to test if mmap() fails rather then doing
> that at runtime ? You could also add an option to force enable/disable mmap
> to configuration time.

Also one could make it explicitly not mappable at runtime with your "int smaf_set_secure(int fd, int secure); ", even if it was mappable in the first place or I missed something ? A question related to that: does it mean that during a laps of time the buffer is not secure ?

Even if a fd has been secure you can mmap it but the CPU will read only 0 and write requests will be ignore.

You are right, during the gap between allocation and secure calls the buffer isn't secure so it up to the secure world to set the correct access rights and clean up the buffer when smaf_set_secure() is called.

(In reply to Benjamin Gaignard from comment #11)
> Even if a fd has been secure you can mmap it but the CPU will read only 0
> and write requests will be ignore.

Hi Benjamin, could you precise "will read 0" ? So mmap will return NULL but not MAP_FAILED (i.e. (void *) -1 ) ? What does it mean that write requests will be ignore if the data is NULL in the first place ? Thx :)

mmap will return a valid pointer but when reading the data you will only see 0 and if you write into it the data won't change in memory.

(In reply to Benjamin Gaignard from comment #13)
> mmap will return a valid pointer but when reading the data you will only see
> 0 and if you write into it the data won't change in memory.

Thx for the clarification, out of curiosity, why not returning MAP_FAILED and setting EACCES "Permission denied" ? So one would have to call "smaf_get_secure" to know if this data does not mean anything ?

Is it possible for a kernel driver to call the kernel side implementation of "smaf_set_secure" when calling the user space side calls drmIoctl(fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args); ? My underlying question is whether it is "always" the responsibility for the user side to make it secure by calling "smaf_set_secure" manually, or can the user get directly a secured fd ?

It is possible to return an error code on mmap call it is up to the secure module to decide of it own policy: for example you can allow mmap for write but not for read.

static int smaf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
{
.... call to secure module with the direction (read and/or write) to request access for the cpu
ret = smaf_grant_access(handle, get_cpu_device(0), 0,
				handle->length, dir);

if (!ret)
     return -EINVAL;

return dma_buf_mmap(handle->db_alloc, vma, 0);
}

SMAF offer the same API in kernel then in userland so you can secure a buffer directly from the driver or test it secure status.

Thx for the details. It fits with our map trials.

I guess, if the secure thingy setup the memory to not fail on maps, and ignore read/write values, we should keep running as if it was correctly working memory. It's very wrong security policy imho, as it messes user experience, but it's not our problem ;-P

So for that we will add a call "smaf_secure_get" in gst_fd_mem_map before mmap. Or could be a call from gstreamer-vaapi for example.

I will update this patch next week, re-working the doc as well. It seems that its usage in waylandsink is ready if I am correct. Whereas work is still on going for gst-vaapi/gstgl (because we want to make the non-caps feature path to work before that)

Thanks, yes, it's mostly ready in waylandsink, I re-written some of the code in the dmabuf patch yesterday, but it's plan to be merged along with this at the opening of 1.11. We still need to figure-out the v4l2 side, which is slightly more work. Ideally, we should focus on v4l2src first.

Created attachment 336818 [details] [review]
allocators: define GST_CAPS_FEATURE_MEMORY_DMABUF

Re-work the doc.

Voilà, we can now start merging code using DMABuf caps feature, have fun !

Attachment 336818 [details] pushed as f5eb366 - allocators: define GST_CAPS_FEATURE_MEMORY_DMABUF