Hi Konrad,

On Fri, Dec 2, 2011 at 10:41 PM, Konrad Rzeszutek Wilk konrad.wilk@oracle.com wrote:

On Fri, Dec 02, 2011 at 02:27:31PM +0530, Sumit Semwal wrote:

...

This is the first step in defining a dma buffer sharing mechanism.

<snip>

...

Signed-off-by: Sumit Semwal sumit.semwal@linaro.org Signed-off-by: Sumit Semwal sumit.semwal@ti.com

You have a clone? You only need one SOB.

:) Thanks for your review - Well, not a clone, but I have two 'employers' :))

I have a rather weird reason for this - I am employed with Texas Instruments, but working with Linaro as well. And due to some 'non-technical' reasons, I need to send this work from @ti.com mail ID. At the same time, I would like to acknowledge that this work was done as part of the Linaro umbrella, so I put another SOB @linaro.org.

<snip>

...

• • Copyright(C) 2011 Linaro Limited. All rights reserved.
• • Author: Sumit Semwal sumit.semwal@ti.com

OK, so the SOB should be from @ti.com then.

...

<snip>

...

+static int dma_buf_mmap(struct file *file, struct vm_area_struct *vma) +{

• struct dma_buf *dmabuf;
• if (!is_dma_buf_file(file))
• return -EINVAL;
• dmabuf = file->private_data;

Should you check if dmabuf is NULL and or dmabuf->ops is NULL too?

Hm, you probably don't need to check for dmabuf, but from looking at  dma_buf_export one could pass  a NULL for the ops.

see next comment

...

• if (!dmabuf->ops->mmap)
• return -EINVAL;
• return dmabuf->ops->mmap(dmabuf, vma);

+}

+static int dma_buf_release(struct inode *inode, struct file *file) +{

• struct dma_buf *dmabuf;
• if (!is_dma_buf_file(file))
• return -EINVAL;
• dmabuf = file->private_data;

No checking here for ops or ops->release?

Hmmm.. you're right, of course. for this common check in mmap and release, I guess I'd add it to 'is_dma_buf_file()' helper [maybe call it is_valid_dma_buf_file() or something similar]

<snip>

...

+/**

I don't think the ** is anymore the current kernel doc format.

thanks for catching this :) - will correct.

...

• • dma_buf_export - Creates a new dma_buf, and associates an anon file
• • with this buffer,so it can be exported.

Put a space there.

ok

...

• • Also connect the allocator specific data and ops to the buffer.
• • @priv:    [in]    Attach private data of allocator to this buffer
• • @ops:     [in]    Attach allocator-defined dma buf ops to the new buffer.
• • @flags:   [in]    mode flags for the file.
• • Returns, on success, a newly created dma_buf object, which wraps the
• • supplied private data and operations for dma_buf_ops. On failure to
• • allocate the dma_buf object, it can return NULL.

"it can" I think the right word is "it will".

Right.

...

• */

+struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,

• int flags)

+{

• struct dma_buf *dmabuf;
• struct file *file;
• BUG_ON(!priv || !ops);

Whoa. Crash the whole kernel b/c of this? No no. You should use WARN_ON and just return NULL.

ok

...

• dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
• if (dmabuf == NULL)
• return dmabuf;

Hmm, why not return ERR_PTR(-ENOMEM); ?

ok

...

• dmabuf->priv = priv;
• dmabuf->ops = ops;
• file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
• dmabuf->file = file;
• mutex_init(&dmabuf->lock);
• INIT_LIST_HEAD(&dmabuf->attachments);
• return dmabuf;

+} +EXPORT_SYMBOL(dma_buf_export);

_GPL ?

sure; will change it.

...

+/**

• • dma_buf_fd - returns a file descriptor for the given dma_buf
• • @dmabuf:  [in]    pointer to dma_buf for which fd is required.
• • On success, returns an associated 'fd'. Else, returns error.
• */

+int dma_buf_fd(struct dma_buf *dmabuf) +{

• int error, fd;

Should you check if dmabuf is NULL first?

yes.

...

• if (!dmabuf->file)
• return -EINVAL;
• error = get_unused_fd_flags(0);
• if (error < 0)
• return error;
• fd = error;
• fd_install(fd, dmabuf->file);
• return fd;

+} +EXPORT_SYMBOL(dma_buf_fd);

GPL?

sure; will change it.

...

+/**

• • dma_buf_get - returns the dma_buf structure related to an fd
• • @fd:      [in]    fd associated with the dma_buf to be returned
• • On success, returns the dma_buf structure associated with an fd; uses
• • file's refcounting done by fget to increase refcount. returns ERR_PTR
• • otherwise.
• */

+struct dma_buf *dma_buf_get(int fd) +{

• struct file *file;
• file = fget(fd);
• if (!file)
• return ERR_PTR(-EBADF);
• if (!is_dma_buf_file(file)) {
• fput(file);
• return ERR_PTR(-EINVAL);
• }
• return file->private_data;

+} +EXPORT_SYMBOL(dma_buf_get);

GPL

sure; will change it.

...

+/**

• • dma_buf_put - decreases refcount of the buffer
• • @dmabuf:  [in]    buffer to reduce refcount of
• • Uses file's refcounting done implicitly by fput()
• */

+void dma_buf_put(struct dma_buf *dmabuf) +{

• BUG_ON(!dmabuf->file);

Yikes. BUG_ON? Can't you do WARN_ON and continue on without doing the refcounting?

ok

...

• fput(dmabuf->file);

+} +EXPORT_SYMBOL(dma_buf_put);

+/**

• • dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
• • calls attach() of dma_buf_ops to allow device-specific attach functionality
• • @dmabuf:  [in]    buffer to attach device to.
• • @dev:     [in]    device to be attached.
• • Returns struct dma_buf_attachment * for this attachment; may return NULL.
• */

+struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• struct device *dev)

'struct device' should be at the same column as 'struct dma_buf' ..

...

+{

• struct dma_buf_attachment *attach;
• int ret;
• BUG_ON(!dmabuf || !dev);

Again, BUG_ON...

will correct

...

• attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
• if (attach == NULL)
• goto err_alloc;
• mutex_lock(&dmabuf->lock);
• attach->dev = dev;
• attach->dmabuf = dmabuf;
• if (dmabuf->ops->attach) {

No checking first of dmabuf->ops?

Attach is told to be a mandatory operation for dmabuf exporter, but I understand your point - checking for it won't hurt.

...

• ret = dmabuf->ops->attach(dmabuf, dev, attach);
• if (!ret)
• goto err_attach;
• }
• list_add(&attach->node, &dmabuf->attachments);
• mutex_unlock(&dmabuf->lock);

+err_alloc:

• return attach;

+err_attach:

• kfree(attach);
• mutex_unlock(&dmabuf->lock);
• return ERR_PTR(ret);

+} +EXPORT_SYMBOL(dma_buf_attach);

GPL

sure; will change it. <snip>

Thanks and regards, ~Sumit.

On Mon, Dec 05, 2011 at 05:18:48PM +0000, Arnd Bergmann wrote:

On Friday 02 December 2011, Sumit Semwal wrote:

...

• /* allow allocator to take care of cache ops */
• void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
• void (*sync_sg_for_device)(struct dma_buf *, struct device *);

I don't see how this works with multiple consumers: For the streaming DMA mapping, there must be exactly one owner, either the device or the CPU. Obviously, this rule needs to be extended when you get to multiple devices and multiple device drivers, plus possibly user mappings. Simply assigning the buffer to "the device" from one driver does not block other drivers from touching the buffer, and assigning it to "the cpu" does not stop other hardware that the code calling sync_sg_for_cpu is not aware of.

The only way to solve this that I can think of right now is to mandate that the mappings are all coherent (i.e. noncachable on noncoherent architectures like ARM). If you do that, you no longer need the sync_sg_for_* calls.

Woops, totally missed the addition of these. Can somebody explain to used to rather coherent x86 what we need these for and the code-flow would look like in a typical example. I was kinda assuming that devices would bracket their use of a buffer with the attachment_map/unmap calls and any cache coherency magic that might be needed would be somewhat transparent to users of the interface?

The map call gets the dma_data_direction parameter, so it should be able to do the right thing. And because we keep the attachement around, any caching of mappings should be possible, too.

Yours, Daniel

PS: Slightly related, because it will make the coherency nightmare worse, afaict: Can we kill mmap support?

On Mon, Dec 05, 2011 at 08:29:49PM +0100, Arnd Bergmann wrote:

On Monday 05 December 2011 19:55:44 Daniel Vetter wrote:

...

...

The only way to solve this that I can think of right now is to mandate that the mappings are all coherent (i.e. noncachable on noncoherent architectures like ARM). If you do that, you no longer need the sync_sg_for_* calls.

Woops, totally missed the addition of these. Can somebody explain to used to rather coherent x86 what we need these for and the code-flow would look like in a typical example. I was kinda assuming that devices would bracket their use of a buffer with the attachment_map/unmap calls and any cache coherency magic that might be needed would be somewhat transparent to users of the interface?

I'll describe how the respective functions work in the streaming mapping API (dma_map_*): You start out with a buffer that is owned by the CPU, i.e. the kernel can access it freely. When you call dma_map_sg or similar, a noncoherent device reading the buffer requires the cache to be flushed in order to see the data that was written by the CPU into the cache.

After dma_map_sg, the device can perform both read and write accesses (depending on the flag to the map call), but the CPU is no longer allowed to read (which would allocate a cache line that may become invalid but remain marked as clean) or write (which would create a dirty cache line without writing it back) that buffer.

Once the device is done, the driver calls dma_unmap_* and the buffer is again owned by the CPU. The device can no longer access it (in fact the address may be no longer be backed if there is an iommu) and the CPU can again read and write the buffer. On ARMv6 and higher, possibly some other architectures, dma_unmap_* also needs to invalidate the cache for the buffer, because due to speculative prefetching, there may also be a new clean cache line with stale data from an earlier version of the buffer.

Since map/unmap is an expensive operation, the interface was extended to pass back the ownership to the CPU and back to the device while leaving the buffer mapped. dma_sync_sg_for_cpu invalidates the cache in the same way as dma_unmap_sg, so the CPU can access the buffer, and dma_sync_sg_for_device hands it back to the device by performing the same cache flush that dma_map_sg would do.

You could for example do this if you want video input with a cacheable buffer, or in an rdma scenario with a buffer accessed by a remote machine.

In case of software iommu (swiotlb, dmabounce), the map and sync functions don't do cache management but instead copy data between a buffer accessed by hardware and a different buffer accessed by the user.

Thanks a lot for this excellent overview. I think at least for the first version of dmabuf we should drop the sync_* interfaces and simply require users to bracket their usage of the buffer from the attached device by map/unmap. A dma_buf provider is always free to cache the mapping and simply call sync_sg_for of the streaming dma api.

If it later turns out that we want to be able to cache the sg list also on the use-site in the driver (e.g. map it into some hw sg list) we can always add that functionality later. I just fear that sync ops among N devices is a bit ill-defined and we already have a plethora of ill-defined issues at hand. Also the proposed api doesn't quite fit into what's already there, I think an s/device/dma_buf_attachment/ would be more consistent - otherwise the dmabuf provider might need to walk the list of attachements to get at the right one for the device.

...

The map call gets the dma_data_direction parameter, so it should be able to do the right thing. And because we keep the attachement around, any caching of mappings should be possible, too.

Yours, Daniel

PS: Slightly related, because it will make the coherency nightmare worse, afaict: Can we kill mmap support?

The mmap support is required (and only make sense) for consistent mappings, not for streaming mappings. The provider must ensure that if you map something uncacheable into the kernel in order to provide consistency, any mapping into user space must also be uncacheable. A driver that wants to have the buffer mapped to user space as many do should not need to know whether it is required to do cacheable or uncacheable mapping because of some other driver, and it should not need to worry about how to set up uncached mappings in user space.

Either I've always missed it or no one ever described it that consisely, but now I see the use-case for mmap: Simpler drivers (i.e. not gpus) might need to expose a userspace mapping and only the provider knows how to do that in a coherent fashion. I want this in the docs (if it's not there yet ...).

But even with that use-case in mind I still have some gripes with the current mmap interfaces as proposed: - This use-case explains why the dmabuf provider needs to expose an mmap function. It doesn't explain why we need to expose that to userspace (instead of faking whatever mmap the importing driver already exposes). Imo the userspace-facing part of dmabuf should be about buffer sharing and not about how to access that buffer, so I'm still missing the reason for that. - We need some way to tell the provider to sync all completed dma operations for userspace mmap access. sync_for_cpu would serve that use. But given that we strive for zero-copy I think the kernel shouldn't ever need to access the contents of a dmabuf and it would therefore make more sense to call it sync_for_mmap. - And the ugly one: Assuming you want this to be coherent for (almost) unchanged users of exisiting subsystem and want this to integrate seamlessly with gpu driver management, we also need a finish_mmap_access. We _can_ fake this by shooting down userspace ptes (if the provider knows about all of them, and it should thanks to the mmap interface) and we do that trick on i915 (for certain cases). But this is generally slow and painful and does not integrate well with other gpu memory management paradigms, where userspace is required to explicit bracket access.

Yours, Daniel

On Mon, Dec 05, 2011 at 11:04:09PM +0100, Arnd Bergmann wrote:

On Monday 05 December 2011 21:58:39 Daniel Vetter wrote:

...

On Mon, Dec 05, 2011 at 08:29:49PM +0100, Arnd Bergmann wrote:

...

...

Thanks a lot for this excellent overview. I think at least for the first version of dmabuf we should drop the sync_* interfaces and simply require users to bracket their usage of the buffer from the attached device by map/unmap. A dma_buf provider is always free to cache the mapping and simply call sync_sg_for of the streaming dma api.

I think we still have the same problem if we allow multiple drivers to access a noncoherent buffer using map/unmap:

driver A driver B

1. read/write

2. 				read/write
   

3. map()

4. 				read/write
   

5. dma

6. 				map()
   

7. dma

8. 				dma
   

9. unmap()

10. 				dma
    

11. read/write

12. 				unmap()						
    
    
    

In step 4, the buffer is owned by device A, but accessed by driver B, which is a bug. In step 11, the buffer is owned by device B but accessed by driver A, which is the same bug on the other side. In steps 7 and 8, the buffer is owned by both device A and B, which is currently undefined but would be ok if both devices are on the same coherency domain. Whether that point is meaningful depends on what the devices actually do. It would be ok if both are only reading, but not if they write into the same location concurrently.

As I mentioned originally, the problem could be completely avoided if we only allow consistent (e.g. uncached) mappings or buffers that are not mapped into the kernel virtual address space at all.

Alternatively, a clearer model would be to require each access to nonconsistent buffers to be exclusive: a map() operation would have to block until the current mapper (if any) has done an unmap(), and any access from the CPU would also have to call a dma_buf_ops pointer to serialize the CPU accesses with any device accesses. User mappings of the buffer can be easily blocked during a DMA access by unmapping the buffer from user space at map() time and blocking the vm_ops->fault() operation until the unmap().

See my other mail where I propose a more explicit coherency model, just a comment here: GPU drivers hate blocking interfaces. Loathe, actually. In general they're very happy to extend you any amount of rope if it can make userspace a few percent faster.

So I think the right answer here is: You've asked for trouble, you've got it. Also see the issue raised by Rob, at least for opengl (and also for other graphics interfaces) the kernel is not even aware of all outstanding rendering. So userspace needs to orchestrate access anyway if a gpu is involved.

Otherwise I agree with your points in this mail. -Daniel

On Mon, Dec 05, 2011 at 02:46:47PM -0600, Rob Clark wrote:

On Mon, Dec 5, 2011 at 11:18 AM, Arnd Bergmann arnd@arndb.de wrote:

...

In the patch 2, you have a section about migration that mentions that it is possible to export a buffer that can be migrated after it is already mapped into one user driver. How does that work when the physical addresses are mapped into a consumer device already?

I think you can do physical migration if you are attached, but probably not if you are mapped.

Yeah, that's very much how I see this, and also why map/unmap (at least for simple users like v4l) should only bracket actual usage. GPU memory managers need to be able to move around buffers while no one is using them.

[snip]

...

...

• /* allow allocator to take care of cache ops */
• void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
• void (*sync_sg_for_device)(struct dma_buf *, struct device *);

I don't see how this works with multiple consumers: For the streaming DMA mapping, there must be exactly one owner, either the device or the CPU. Obviously, this rule needs to be extended when you get to multiple devices and multiple device drivers, plus possibly user mappings. Simply assigning the buffer to "the device" from one driver does not block other drivers from touching the buffer, and assigning it to "the cpu" does not stop other hardware that the code calling sync_sg_for_cpu is not aware of.

The only way to solve this that I can think of right now is to mandate that the mappings are all coherent (i.e. noncachable on noncoherent architectures like ARM). If you do that, you no longer need the sync_sg_for_* calls.

My original thinking was that you either need DMABUF_CPU_{PREP,FINI} ioctls and corresponding dmabuf ops, which userspace is required to call before / after CPU access. Or just remove mmap() and do the mmap() via allocating device and use that device's equivalent DRM_XYZ_GEM_CPU_{PREP,FINI} or DRM_XYZ_GEM_SET_DOMAIN ioctls. That would give you a way to (a) synchronize with gpu/asynchronous pipeline, (b) synchronize w/ multiple hw devices vs cpu accessing buffer (ie. wait all devices have dma_buf_unmap_attachment'd). And that gives you a convenient place to do cache operations on noncoherent architecture.

I sort of preferred having the DMABUF shim because that lets you pass a buffer around userspace without the receiving code knowing about a device specific API. But the problem I eventually came around to: if your GL stack (or some other userspace component) is batching up commands before submission to kernel, the buffers you need to wait for completion might not even be submitted yet. So from kernel perspective they are "ready" for cpu access. Even though in fact they are not in a consistent state from rendering perspective. I don't really know a sane way to deal with that. Maybe the approach instead should be a userspace level API (in libkms/libdrm?) to provide abstraction for userspace access to buffers rather than dealing with this at the kernel level.

Well, there's a reason GL has an explicit flush and extensions for sync objects. It's to support such scenarios where the driver batches up gpu commands before actually submitting them. Also, recent gpus have all (or shortly will grow) multiple execution pipelines, so it's also important that you sync up with the right command stream. Syncing up with all of them is generally frowned upon for obvious reasons ;-)

So any userspace that interacts with an OpenGL driver needs to take care of this anyway. But I think for simpler stuff (v4l) kernel only coherency should work and userspace just needs to take care of gl interactions and call glflush and friends at the right points. I think we can flesh this out precisely when we spec the dmabuf EGL extension ... (or implement one of the preexisting ones already around).

On the topic of a coherency model for dmabuf, I think we need to look at dma_buf_attachment_map/unmap (and also the mmap variants cpu_start and cpu_finish or whatever they might get called) as barriers:

So after a dma_buf_map, all previsously completed dma operations (i.e. unmap already called) and any cpu writes (i.e. cpu_finish called) will be coherent. Similar rule holds for cpu access through the userspace mmap, only writes completed before the cpu_start will show up.

Similar, writes done by the device are only guaranteed to show up after the _unmap. Dito for cpu writes and cpu_finish.

In short we always need two function calls to denote the start/end of the "critical section".

Any concurrent operations are allowed to yield garbage, meaning any combination of the old or either of the newly written contents (i.e. non-overlapping writes might not actually all end up in the buffer, but instead some old contents). Maybe we even need to loosen that to the real "undefined behaviour", but atm I can't think of an example.

-Daniel

On Mon, Dec 05, 2011 at 04:11:46PM -0600, Rob Clark wrote:

On Mon, Dec 5, 2011 at 3:23 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Dec 05, 2011 at 02:46:47PM -0600, Rob Clark wrote:

...

I sort of preferred having the DMABUF shim because that lets you pass a buffer around userspace without the receiving code knowing about a device specific API.  But the problem I eventually came around to: if your GL stack (or some other userspace component) is batching up commands before submission to kernel, the buffers you need to wait for completion might not even be submitted yet.  So from kernel perspective they are "ready" for cpu access.  Even though in fact they are not in a consistent state from rendering perspective.  I don't really know a sane way to deal with that.  Maybe the approach instead should be a userspace level API (in libkms/libdrm?) to provide abstraction for userspace access to buffers rather than dealing with this at the kernel level.

Well, there's a reason GL has an explicit flush and extensions for sync objects. It's to support such scenarios where the driver batches up gpu commands before actually submitting them.

Hmm.. what about other non-GL APIs.. maybe vaapi/vdpau or similar? (Or something that I haven't thought of.)

They generally all have a concept of when they've actually commited the rendering to an X pixmap or egl image. Usually it's rather implicit, e.g. the driver will submit any outstanding batches before returning from any calls. -Daniel

On Tue, Dec 06, 2011 at 01:16:58PM +0000, Arnd Bergmann wrote:

On Monday 05 December 2011, Rob Clark wrote:

...

...

On the topic of a coherency model for dmabuf, I think we need to look at dma_buf_attachment_map/unmap (and also the mmap variants cpu_start and cpu_finish or whatever they might get called) as barriers:

So after a dma_buf_map, all previsously completed dma operations (i.e. unmap already called) and any cpu writes (i.e. cpu_finish called) will be coherent. Similar rule holds for cpu access through the userspace mmap, only writes completed before the cpu_start will show up.

Similar, writes done by the device are only guaranteed to show up after the _unmap. Dito for cpu writes and cpu_finish.

In short we always need two function calls to denote the start/end of the "critical section".

Yup, this was exactly my assumption. But I guess it is better to spell it out.

I still don't understand how this is going to help you if you let multiple drivers enter and leave the critical section without serializing against one another. That doesn't sound like what I know as critical section.

I already regret to having added that last "critical section" remark. Think barriers. It's just that you need a barrier in both directions that bracket the actual usage. In i915-land we call the first one generally invalidate (so that caches on the target domain don't contain stale data) and that second one flush (to get any data out of caches).

Given some reasonable constraints (all devices must be in the same coherency domain, for instance), you can probably define it in a way that you can have multiple devices mapping the same buffer at the same time, and when no device has mapped the buffer you can have as many concurrent kernel and user space accesses on the same buffer as you like. But you must still guarantee that no software touches a noncoherent buffer while it is mapped into any device and vice versa.

Why can't we just mandate that all mappings into the kernel must be coherent and that user space accesses must either be coherent as well or be done by user space that uses explicit serialization with all DMA accesses?

I agree with your points here, afaics the contentious issue is just whether dma_buf should _enforce_ this strict ordering. I'm leading towards a "no" for the following reasons:

- gpu people love nonblocking interfaces (and love to come up with abuses). In the generic case we'd need some more functions to properly flush everything while 2 devices access a buffer concurrently (which is imo a bit unrealistic). But e.g. 2 gpus rendering in SLI mode very much want to access the same buffer at the same time (and the kernel+userspace gpu driver already needs all the information about caches to make that happen, at least on x86).

- Buffer sharing alone has already some great potential for deadlock and lock recursion issues. Making dma_buf into something that very much acts like a new locking primitive itself (even exposed to userspace) will make this much worse. I've seen some of the kernel/userspace shared hwlock code of dri1 yonder, and it's horrible (and at least for the case of the dri1 hwlock, totally broken).

- All current subsystem already have the concept to pass the ownership of a buffer between the device and userspace (sometimes even more than just 2 domains, like in i915 ...). Userspace already needs to use this interface to get anything resembling correct data. I don't see any case where userspace can't enforce passing around buffer ownership if multiple devices are involved (we obviously need to clarify subsystem interfaces to make it clear when a buffer is in use and when another device taking part in the sharing could use it). So I don't see how the kernel enforcing strict access ordering helps implementing correct userspace.

- I don't see any security needs that would make it necessary for the kernel to enforce any consistency guarantees for concurrent access - we're only dealing with pixel data in all the currently discussed generic use-cases. So I think garbage as an end-result is acceptable if userspace does stupid things (or fails at trying to be clever).

Cheers, Daniel

On Wednesday 07 December 2011, Semwal, Sumit wrote:

Thanks for the excellent discussion - it indeed is very good learning for the relatively-inexperienced me :)

So, for the purpose of dma-buf framework, could I summarize the following and rework accordingly?:

1. remove mmap() dma_buf_op [and mmap fop], and introduce cpu_start(),

cpu_finish() ops to bracket cpu accesses to the buffer. Also add DMABUF_CPU_START / DMABUF_CPU_FINI IOCTLs?

I think we'd be better off for now without the extra ioctls and just document that a shared buffer must not be exported to user space using mmap at all, to avoid those problems. Serialization between GPU and CPU is on a higher level than the dma_buf framework IMHO.

2. remove sg_sync* ops for now (and we'll see if we need to add them

later if needed)

Just removing the sg_sync_* operations is not enough. We have to make the decision whether we want to allow a) only coherent mappings of the buffer into kernel memory (requiring an extension to the dma_map_ops on ARM to not flush caches at map/unmap time) b) not allowing any in-kernel mappings (same requirement on ARM, also limits the usefulness of the dma_buf if we cannot access it from the kernel or from user space) c) only allowing streaming mappings, even if those are non-coherent (requiring strict serialization between CPU (in-kernel) and dma users of the buffer)

This issue has to be solved or we get random data corruption.

Arnd

On Thursday 08 December 2011, Daniel Vetter wrote:

...

c) only allowing streaming mappings, even if those are non-coherent (requiring strict serialization between CPU (in-kernel) and dma users of the buffer)

I think only allowing streaming access makes the most sense:

• I don't see much (if any need) for the kernel to access a dma_buf -

in all current usecases it just contains pixel data and no hw-specific things (like sg tables, command buffers, ..). At most I see the need for the kernel to access the buffer for dma bounce buffers, but that is internal to the dma subsystem (and hence does not need to be exposed).

• Userspace can still access the contents through the exporting

subsystem (e.g. use some gem mmap support). For efficiency reason gpu drivers are already messing around with cache coherency in a platform specific way (and hence violated the dma api a bit), so we could stuff the mmap coherency in there, too. When we later on extend dma_buf support so that other drivers than the gpu can export dma_bufs, we can then extend the official dma api with already a few drivers with use-patterns around.

But I still think that the kernel must not be required to enforce correct access ordering for the reasons outlined in my other mail.

I still don't think that's possible. Please explain how you expect to change the semantics of the streaming mapping API to allow multiple mappers without having explicit serialization points that are visible to all users. For simplicity, let's assume a cache coherent system with bounce buffers where map() copies the buffer to a dma area and unmap() copies it back to regular kernel memory. How does a driver know if it can touch the buffer in memory or from DMA at any given point in time? Note that this problem is the same as the cache coherency problem but may be easier to grasp.

Arnd

On Fri, Dec 9, 2011 at 15:24, Alan Cox alan@lxorguk.ukuu.org.uk wrote:

...

I still don't think that's possible. Please explain how you expect to change the semantics of the streaming mapping API to allow multiple mappers without having explicit serialization points that are visible to all users. For simplicity, let's assume a cache coherent system

I think I understand the cache flushing issues on arm (we're doing a similar madness with manually flushing caches for i915 because the gpu isn't coherent with the cpu and other dma devices). And I also agree that you cannot make concurrent mappings work without adding something on top of the current streaming api/dma-buf proposal. I just think that it's not the kernel's business (well, at least not dma_buf's business) to enforce that. If userspace (through some driver calls) tries to do stupid things, it'll just get garbage. See Message-ID: CAKMK7uHeXYn-v_8cmpLNWsFY14KtmuRZy8YRKR5Xst2-2WdFSQ@mail.gmail.com for my reasons why it think this is the right way to go forward. So in essence I'm really interested in the reasons why you want the kernel to enforce this (or I'm completely missing what's the contentious issue here).

I would agree. It's not just about barriers but in many cases where you have multiple mappings by hardware devices the actual hardware interface is specific to the devices. Just take a look at the fencing in TTM and the graphics drivers.

Its not something the low level API can deal with, it requires high level knowledge.

Yes, we might want to add some form of in-kernel sync objects on top of dma_buf, but I'm not really convinced that this will buy us much above simply synchronizing access in userspace with the existing ipc tools. In my experience the expensive part of syncing two graphics engines with the cpu is that we wake up the cpu and prevent it from going into low-power states if we do this too often. Adding some more overhead by going through userspace doesn't really make it much worse. It's a completely different story if there's a hw facility to synchronize engines without the cpu's involvement (like there is on every multi-pipe gpu) and there sync objects make tons of sense. But I'm not aware of that existing on SoCs between different IP blocks.

Cheers, Daniel

Hi Arnd, Daniel,

On Mon, Dec 12, 2011 at 10:18 PM, Arnd Bergmann arnd@arndb.de wrote:

On Saturday 10 December 2011, Daniel Vetter wrote:

...

If userspace (through some driver calls) tries to do stupid things, it'll just get garbage. See Message-ID: CAKMK7uHeXYn-v_8cmpLNWsFY14KtmuRZy8YRKR5Xst2-2WdFSQ@mail.gmail.com for my reasons why it think this is the right way to go forward. So in essence I'm really interested in the reasons why you want the kernel to enforce this (or I'm completely missing what's the contentious issue here).

This has nothing to do with user space mappings. Whatever user space does, you get garbage if you don't invalidate cache lines that were introduced through speculative prefetching before you access cache lines that were DMA'd from a device.

I didn't see a consensus on whether dma_buf should enforce some form of serialization within the API - so atleast for v1 of dma-buf, I propose to 'not' impose a restriction, and we can tackle it (add new ops or enforce as design?) whenever we see the first need of it - will that be ok? [I am bending towards the thought that it is a problem to solve at a bigger platform than dma_buf.]

Arnd

Best regards, ~Sumit.

--

On Tue, Dec 20, 2011 at 9:41 AM, Arnd Bergmann arnd@arndb.de wrote:

On Monday 19 December 2011, Semwal, Sumit wrote:

...

I didn't see a consensus on whether dma_buf should enforce some form of serialization within the API - so atleast for v1 of dma-buf, I propose to 'not' impose a restriction, and we can tackle it (add new ops or enforce as design?) whenever we see the first need of it - will that be ok? [I am bending towards the thought that it is a problem to solve at a bigger platform than dma_buf.]

The problem is generally understood for streaming mappings with a single device using it: if you have a long-running mapping, you have to use dma_sync_*. This obviously falls apart if you have multiple devices and no serialization between the accesses.

If you don't want serialization, that implies that we cannot have use the  dma_sync_* API on the buffer, which in turn implies that we cannot have streaming mappings. I think that's ok, but then you have to bring back the mmap API on the buffer if you want to allow any driver to provide an mmap function for a shared buffer.

I'm thinking for a first version, we can get enough mileage out of it by saying: 1) only exporter can mmap to userspace 2) only importers that do not need CPU access to buffer..

This way we can get dmabuf into the kernel, maybe even for 3.3. I know there are a lot of interesting potential uses where this stripped down version is good enough. It probably isn't the final version, maybe more features are added over time to deal with importers that need CPU access to buffer, sync object, etc. But we have to start somewhere.

BR, -R

Arnd

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 20 December 2011, Daniel Vetter wrote:

...

I'm thinking for a first version, we can get enough mileage out of it by saying:

1. only exporter can mmap to userspace
2. only importers that do not need CPU access to buffer..

Ok, that sounds possible. The alternative to this would be:

1) The exporter has to use dma_alloc_coherent() or dma_alloc_writecombine() to allocate the buffer 2. Every user space mapping has to go through dma_mmap_coherent() or dma_mmap_writecombine()

We can extend the rules later to allow either one after we have gained some experience using it.

...

This way we can get dmabuf into the kernel, maybe even for 3.3. I know there are a lot of interesting potential uses where this stripped down version is good enough. It probably isn't the final version, maybe more features are added over time to deal with importers that need CPU access to buffer, sync object, etc. But we have to start somewhere.

I agree with Rob here - I think especially for the coherency discussion some actual users of dma_buf on a bunch of insane platforms (i915 qualifies here too, because we do some cacheline flushing behind everyones back) would massively help in clarifying things.

Yes, agreed.

It also sounds like that at least for proper userspace mmap support we'd need some dma api extensions on at least arm, and that might take a while ...

I think it's actually the opposite -- you'd need dma api extensions on everything else other than arm, which already has dma_mmap_coherent() and dma_mmap_writecombine() for this purpose.

Arnd

On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann arnd@arndb.de wrote:

On Tuesday 20 December 2011, Daniel Vetter wrote:

...

...

I'm thinking for a first version, we can get enough mileage out of it by saying:

1. only exporter can mmap to userspace
2. only importers that do not need CPU access to buffer..

Thanks Rob - and the exporter can do the mmap outside of dma-buf usage, right? I mean, we don't need to provide an mmap to dma_buf() and restrict it to exporter, when the exporter has more 'control' of the buffer anyways.

BR, ~Sumit.

Hi Arnd,

On Fri, Dec 09, 2011 at 02:13:03PM +0000, Arnd Bergmann wrote:

On Thursday 08 December 2011, Daniel Vetter wrote:

...

...

c) only allowing streaming mappings, even if those are non-coherent (requiring strict serialization between CPU (in-kernel) and dma users of the buffer)

I think only allowing streaming access makes the most sense:

• I don't see much (if any need) for the kernel to access a dma_buf -

in all current usecases it just contains pixel data and no hw-specific things (like sg tables, command buffers, ..). At most I see the need for the kernel to access the buffer for dma bounce buffers, but that is internal to the dma subsystem (and hence does not need to be exposed).

• Userspace can still access the contents through the exporting

subsystem (e.g. use some gem mmap support). For efficiency reason gpu drivers are already messing around with cache coherency in a platform specific way (and hence violated the dma api a bit), so we could stuff the mmap coherency in there, too. When we later on extend dma_buf support so that other drivers than the gpu can export dma_bufs, we can then extend the official dma api with already a few drivers with use-patterns around.

But I still think that the kernel must not be required to enforce correct access ordering for the reasons outlined in my other mail.

I still don't think that's possible. Please explain how you expect to change the semantics of the streaming mapping API to allow multiple mappers without having explicit serialization points that are visible to all users. For simplicity, let's assume a cache coherent system with bounce buffers where map() copies the buffer to a dma area and unmap() copies it back to regular kernel memory. How does a driver know if it can touch the buffer in memory or from DMA at any given point in time? Note that this problem is the same as the cache coherency problem but may be easier to grasp.

(I'm jumping into the discussion in the middle, and might miss something that has already been talked about. I still hope what I'm about to say is relevant. :-))

In subsystems such as V4L2 where drivers deal with such large buffers, the buffers stay mapped all the time. The user explicitly gives the control of the buffers to the driver and eventually gets them back. This is already part of those APIs, whether they're using dma_buf or not. The user could have, and often has, the same buffers mapped elsewhere.

When it comes to passing these buffers between different hardware devices, either V4L2 or not, the user might not want to perform extra cache flush when the buffer memory itself is not being touched by the CPU in the process at all. I'd consider it impossible for the driver to know how the user space intends to user the buffer.

Flushing the cache is quite expensive: typically it's the best to flush the whole data cache when one needs to flush buffers. The V4L2 DQBUF and QBUF IOCTLs already have flags to suggest special cache handling for buffers.

Kind regards,

Hi Arnd,

On Tue, Dec 20, 2011 at 03:36:49PM +0000, Arnd Bergmann wrote:

On Tuesday 20 December 2011, Sakari Ailus wrote:

...

(I'm jumping into the discussion in the middle, and might miss something that has already been talked about. I still hope what I'm about to say is relevant. :-))

It certainly is relevant.

...

In subsystems such as V4L2 where drivers deal with such large buffers, the buffers stay mapped all the time. The user explicitly gives the control of the buffers to the driver and eventually gets them back. This is already part of those APIs, whether they're using dma_buf or not. The user could have, and often has, the same buffers mapped elsewhere.

Do you normally use streaming (dma_{map,sync,unmap}_*) or consistent (dma_{alloc,free}_*) mappings for this then?

The OMAP 3 ISP driver I'm familiar with uses the OMAP 3 IOMMU / IOVMM API which is to be replaced by dmabuf. I'm trying to understand how the dma api / dma-buf should be used to achieve a superset of that functionality.

I think I'm interested in the DMA mapping API. I replied to Sumit's new patchset, you're cc'd.

...

When it comes to passing these buffers between different hardware devices, either V4L2 or not, the user might not want to perform extra cache flush when the buffer memory itself is not being touched by the CPU in the process at all. I'd consider it impossible for the driver to know how the user space intends to user the buffer.

The easiest solution to this problem would be to only allow consistent mappings to be shared using the dma_buf mechanism. That means we never have to flush.

Do you mean the memory would be non-cacheable? Accessing memory w/o caching is typically prohibitively expensive, so I don't think this could ever be the primary means to do the above.

In some cases non-cacheable can perform better, taking into account the time which is required for flusing the cache and the other consequences of that, but I still think it's more of an exception than a rule.

If you don't need the CPU to touch the buffer, that would not have any cost at all, we could even have no kernel mapping at all instead of an uncached mapping on ARM.

I think in general creating unused mappings should really be avoided. Creating them consumes time, effort at creation time and possibly also in cache related operations.

...

Flushing the cache is quite expensive: typically it's the best to flush the whole data cache when one needs to flush buffers. The V4L2 DQBUF and QBUF IOCTLs already have flags to suggest special cache handling for buffers.

[sidenote: whether it makes sense to flush individual cache lines or the entire cache is a decision best left to the architectures. On systems with larger caches than on ARM, e.g. 64MB instead of 512KB, you really want to keep the cache intact.]

That also depend on the buffer size and what the rest of the system is doing. I could imagine buffer size, system memory data rate, CPU frequency, cache line width and the properties of the cache all affect how fast both of the operations are.

It would probably be possible to perform a heuristic analysis on this at system startup similar to different software raid algorithm implementations ( e.g. to use MMX or SSE for sw raid).

Some additional complexity will arise from the fact that on some ARM machines one must know all the CPU MMU mappings pointing to a piece of physical memory to properly flush them, AFAIR. Naturally a good alternative on such system is to pperform full dcache flush / cleaning.

Also, cache handling only affects systems without coherent cache. ARM CPUs are finding their ways to servers as well, but I'd guess it'll still take a while before we have ARM CPUs with 64 MiB of cache..

Kind regards,

(I've been away for the past two weeks, so I'm only now catching up)

On Thursday 08 December 2011 22:44:08 Daniel Vetter wrote:

On Wed, Dec 7, 2011 at 14:40, Arnd Bergmann arnd@arndb.de wrote:

...

On Wednesday 07 December 2011, Semwal, Sumit wrote:

...

Thanks for the excellent discussion - it indeed is very good learning for the relatively-inexperienced me :)

So, for the purpose of dma-buf framework, could I summarize the following and rework accordingly?:

1. remove mmap() dma_buf_op [and mmap fop], and introduce cpu_start(),

cpu_finish() ops to bracket cpu accesses to the buffer. Also add DMABUF_CPU_START / DMABUF_CPU_FINI IOCTLs?

I think we'd be better off for now without the extra ioctls and just document that a shared buffer must not be exported to user space using mmap at all, to avoid those problems. Serialization between GPU and CPU is on a higher level than the dma_buf framework IMHO.

Agreed.

...

...

2. remove sg_sync* ops for now (and we'll see if we need to add them

later if needed)

Just removing the sg_sync_* operations is not enough. We have to make the decision whether we want to allow a) only coherent mappings of the buffer into kernel memory (requiring an extension to the dma_map_ops on ARM to not flush caches at map/unmap time) b) not allowing any in-kernel mappings (same requirement on ARM, also limits the usefulness of the dma_buf if we cannot access it from the kernel or from user space) c) only allowing streaming mappings, even if those are non-coherent (requiring strict serialization between CPU (in-kernel) and dma users of the buffer)

I think only allowing streaming access makes the most sense:

• I don't see much (if any need) for the kernel to access a dma_buf -

in all current usecases it just contains pixel data and no hw-specific things (like sg tables, command buffers, ..). At most I see the need for the kernel to access the buffer for dma bounce buffers, but that is internal to the dma subsystem (and hence does not need to be exposed).

There are a few situations where the kernel might actually access a dma_buf:

First of all there are some sensors that add meta data before the actual pixel data, and a kernel driver might well want to read out that data and process it. Secondly (and really very similar), video frames sent to/from an FPGA can also contain meta data (Cisco does that on some of our products) that the kernel may need to inspect.

I admit that these use-cases aren't very common, but they do exist.

• Userspace can still access the contents through the exporting

subsystem (e.g. use some gem mmap support). For efficiency reason gpu drivers are already messing around with cache coherency in a platform specific way (and hence violated the dma api a bit), so we could stuff the mmap coherency in there, too. When we later on extend dma_buf support so that other drivers than the gpu can export dma_bufs, we can then extend the official dma api with already a few drivers with use-patterns around.

But I still think that the kernel must not be required to enforce correct access ordering for the reasons outlined in my other mail.

I agree with Daniel on this.

BTW, the V4L2 subsystem has a clear concept of passing bufffer ownership: the VIDIOC_QBUF and VIDIOC_DQBUF ioctls deal with that. Pretty much all V4L2 apps request the buffers, then mmap them, then call QBUF to give the ownership of those buffers to the kernel. While the kernel owns those buffers any access to the mmap'ped memory leads to undefined results. Only after calling DQBUF can userspace actually safely access that memory.

Allowing mmap() on the dma_buf's fd would actually make things easier for V4L2. It's an elegant way of mapping the memory.

Regards,

Hans

On Mon, Dec 5, 2011 at 4:09 PM, Arnd Bergmann arnd@arndb.de wrote:

...

https://github.com/robclark/kernel-omap4/commits/dmabuf

Ok, thanks. I think it would be good to post these for reference in v3, with a clear indication that they are not being submitted for discussion/inclusion yet.

btw, don't look at this too closely at that tree yet.. where the attach/detach is done in videobuf2 code isn't really correct. But I was going to get something functioning first.

BR, -R

On Wed, Dec 7, 2011 at 3:41 PM, Arnd Bergmann arnd@arndb.de wrote:

On Wednesday 07 December 2011, Semwal, Sumit wrote:

...

...

Do you have a use case for making the interface compile-time disabled? I had assumed that any code using it would make no sense if it's not available so you don't actually need this.

Ok. Though if we keep the interface compile-time disabled, the users can actually check and fail or fall-back gracefully when the API is not available; If I remove it, anyways the users would need to do the same compile time check whether API is available or not, right?

If you have to do a compile-time check for the config symbol, it's better to do it the way you did here than in the caller.

My guess was that no caller would actually require this, because when you write a part of a subsystem to interact with the dma-buf infrastructure, you would always disable compilation of an extire file that deals with everything related to struct dma_buf, not just stub out the calls.

Right; that would be ideal, but we may not be able to ask each user to do so - especially when the sharing part might be interspersed in existing buffer handling code. So for now, I would like to keep it as it-is.

Arnd

BR, ~Sumit.

--

On Mon, Dec 05, 2011 at 09:18:48AM -0800, Arnd Bergmann wrote:

On Friday 02 December 2011, Sumit Semwal wrote:

...

This is the first step in defining a dma buffer sharing mechanism.

[...]

...

• return dmabuf;

+} +EXPORT_SYMBOL(dma_buf_export);

I agree with Konrad, this should definitely be EXPORT_SYMBOL_GPL, because it's really a low-level function that I would expect to get used by in-kernel subsystems providing the feature to users and having back-end drivers, but it's not the kind of thing we want out-of-tree drivers to mess with.

Is this really necessary? If this is intended to be a lowest-common-denominator between many drivers to allow buffer sharing, it seems like it needs to be able to be usable by all drivers.

If the interface is not accessible then I fear many drivers will be forced to continue to roll their own buffer sharing mechanisms (which is exactly what we're trying to avoid here, needless to say).

- Robert

On Sat, Dec 10, 2011 at 03:13:06AM -0800, Mauro Carvalho Chehab wrote:

On 09-12-2011 20:50, Robert Morell wrote:

...

On Mon, Dec 05, 2011 at 09:18:48AM -0800, Arnd Bergmann wrote:

...

On Friday 02 December 2011, Sumit Semwal wrote:

...

This is the first step in defining a dma buffer sharing mechanism.

[...]

...

...

• return dmabuf;

+} +EXPORT_SYMBOL(dma_buf_export);

I agree with Konrad, this should definitely be EXPORT_SYMBOL_GPL, because it's really a low-level function that I would expect to get used by in-kernel subsystems providing the feature to users and having back-end drivers, but it's not the kind of thing we want out-of-tree drivers to mess with.

Is this really necessary? If this is intended to be a lowest-common-denominator between many drivers to allow buffer sharing, it seems like it needs to be able to be usable by all drivers.

If the interface is not accessible then I fear many drivers will be forced to continue to roll their own buffer sharing mechanisms (which is exactly what we're trying to avoid here, needless to say).

Doing a buffer sharing with something that is not GPL is not fun, as, if any issue rises there, it would be impossible to discover if the problem is either at the closed-source driver or at the open source one. At the time I was using the Nvidia proprietary driver, it was very common to have unexplained issues caused likely by bad code there at the buffer management code, causing X applications and extensions (like xv) to break.

We should really make this EXPORT_SYMBOL_GPL(), in order to be able to latter debug future share buffer issues, when needed.

Sorry, I don't buy this argument. Making these exports GPL-only is not likely to cause anybody to open-source their driver, but will rather just cause them to use yet more closed-source code that is even less debuggable than this would be, to those without access to the source.

Thanks, Robert

Regards, Mauro

On Tue, Dec 13, 2011 at 07:10:02AM -0800, Arnd Bergmann wrote:

On Monday 12 December 2011, Robert Morell wrote:

...

...

Doing a buffer sharing with something that is not GPL is not fun, as, if any issue rises there, it would be impossible to discover if the problem is either at the closed-source driver or at the open source one. At the time I was using the Nvidia proprietary driver, it was very common to have unexplained issues caused likely by bad code there at the buffer management code, causing X applications and extensions (like xv) to break.

We should really make this EXPORT_SYMBOL_GPL(), in order to be able to latter debug future share buffer issues, when needed.

Sorry, I don't buy this argument. Making these exports GPL-only is not likely to cause anybody to open-source their driver, but will rather just cause them to use yet more closed-source code that is even less debuggable than this would be, to those without access to the source.

But at least the broken module then won't be interacting with other modules because it cannot share any buffers.

One of the goals of this project is to unify the fragmented space of the ARM SoC memory managers so that each vendor doesn't implement their own, and they can all be closer to mainline.

I fear that restricting the use of this buffer sharing mechanism to GPL drivers only will prevent that goal from being achieved, if an SoC driver has to interact with modules that use a non-GPL license.

As a hypothetical example, consider laptops that have multiple GPUs. Today, these ship with onboard graphics (integrated to the CPU or chipset) along with a discrete GPU, where in many cases only the onboard graphics can actually display to the screen. In order for anything rendered by the discrete GPU to be displayed, it has to be copied to memory available for the smaller onboard graphics to texture from or display directly. Obviously, that's best done by sharing dma buffers rather than bouncing them through the GPU. It's not much of a stretch to imagine that we'll see such systems with a Tegra CPU/GPU plus a discrete GPU in the future; in that case, we'd want to be able to share memory between the discrete GPU and the Tegra system. In that scenario, if this interface is GPL-only, we'd be unable to adopt the dma_buffer sharing mechanism for Tegra.

(This isn't too pie-in-the-sky, either; people are already combining Tegra with discrete GPUs: http://blogs.nvidia.com/2011/11/world%e2%80%99s-first-arm-based-supercompute... )

Thanks, Robert

On Mon, Jan 9, 2012 at 8:10 AM, Daniel Vetter daniel@ffwll.ch wrote:

On Mon, Jan 09, 2012 at 03:20:48PM +0900, InKi Dae wrote:

...

I has test dmabuf based drm gem module for exynos and I found one problem. you can refer to this test repository: http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/exyn...

at this repository, I added some exception codes for resource release in addition to Dave's patch sets.

let's suppose we use dmabuf based vb2 and drm gem with physically continuous memory(no IOMMU) and we try to share allocated buffer between them(v4l2 and drm driver).

1. request memory allocation through drm gem interface.
2. request DRM_SET_PRIME ioctl with the gem handle to get a fd to the

gem object.

• internally, private gem based dmabuf moudle calls drm_buf_export()

to register allocated gem object to fd. 3. request qbuf with the fd(got from 2) and DMABUF type to set the buffer to v4l2 based device.

• internally, vb2 plug in module gets a buffer to the fd and then

calls dmabuf->ops->map_dmabuf() callback to get the sg table containing physical memory info to the gem object. and then the physical memory info would be copied to vb2_xx_buf object. for DMABUF feature for v4l2 and videobuf2 framework, you can refer to this repository: git://github.com/robclark/kernel-omap4.git drmplane-dmabuf

after that, if v4l2 driver want to release vb2_xx_buf object with allocated memory region by user request, how should we do?. refcount to vb2_xx_buf is dependent on videobuf2 framework. so when vb2_xx_buf object is released videobuf2 framework don't know who is using the physical memory region. so this physical memory region is released and when drm driver tries to access the region or to release it also, a problem would be induced.

for this problem, I added get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there may be better way. if there is any missing point, please let me know.

The dma_buf object needs to hold a reference on the underlying (necessarily reference-counted) buffer object when the exporter creates the dma_buf handle. This reference should then get dropped in the exporters dma_buf->ops->release() function, which is only getting called when the last reference to the dma_buf disappears.

If this doesn't work like that currently, we have a bug, and exporting the reference count or something similar can't fix that.

Yours, Daniel

PS: Please cut down the original mail when replying, otherwise it's pretty hard to find your response ;-)

And also the importer needs to realise it doesn't own the pages in the sg_table and when its freeing its backing memory it shouldn't free those pages. So for GEM objects we have to keep track if we allocated the pages or we got them from an dma buf.

Dave.

On Mon, Jan 09, 2012 at 07:10:25PM +0900, InKi Dae wrote:

2012/1/9 Daniel Vetter daniel@ffwll.ch:

...

On Mon, Jan 09, 2012 at 03:20:48PM +0900, InKi Dae wrote:

...

I has test dmabuf based drm gem module for exynos and I found one problem. you can refer to this test repository: http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/exyn...

at this repository, I added some exception codes for resource release in addition to Dave's patch sets.

let's suppose we use dmabuf based vb2 and drm gem with physically continuous memory(no IOMMU) and we try to share allocated buffer between them(v4l2 and drm driver).

1. request memory allocation through drm gem interface.
2. request DRM_SET_PRIME ioctl with the gem handle to get a fd to the

gem object.

• internally, private gem based dmabuf moudle calls drm_buf_export()

to register allocated gem object to fd. 3. request qbuf with the fd(got from 2) and DMABUF type to set the buffer to v4l2 based device.

• internally, vb2 plug in module gets a buffer to the fd and then

calls dmabuf->ops->map_dmabuf() callback to get the sg table containing physical memory info to the gem object. and then the physical memory info would be copied to vb2_xx_buf object. for DMABUF feature for v4l2 and videobuf2 framework, you can refer to this repository: git://github.com/robclark/kernel-omap4.git drmplane-dmabuf

after that, if v4l2 driver want to release vb2_xx_buf object with allocated memory region by user request, how should we do?. refcount to vb2_xx_buf is dependent on videobuf2 framework. so when vb2_xx_buf object is released videobuf2 framework don't know who is using the physical memory region. so this physical memory region is released and when drm driver tries to access the region or to release it also, a problem would be induced.

for this problem, I added get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there may be better way. if there is any missing point, please let me know.

The dma_buf object needs to hold a reference on the underlying (necessarily reference-counted) buffer object when the exporter creates the dma_buf handle. This reference should then get dropped in the exporters dma_buf->ops->release() function, which is only getting called when the last reference to the dma_buf disappears.

when the exporter creates the dma_buf handle(for example, gem -> fd), I think the refcount of gem object should be increased at this point, and decreased by dma_buf->ops->release() again because when the dma_buf is created and dma_buf_export() is called, this dma_buf refers to the gem object one time. and in case of inporter(fd -> gem), file->f_count of the dma_buf is increased and then when this gem object is released by user request such as drm close or drn_gem_close_ioctl, dma_buf_put() should be called by dma_buf->ops->detach() to decrease file->f_count again because the gem object refers to the dma_buf. for this, you can refer to my test repository I mentioned above. but the problem is that when a buffer is released by one side, another can't know whether the buffer already was released or not.

Nope, dma_buf_put should not be called by ->detach. The importer gets his reference when importing the dma_buf and needs to drop that reference himself when it's done using the buffer by calling dma_buf_put (i.e. after the last ->detach call). I think adding separate reference counting to ->attach and ->detach is a waste of time and only papers over buggy importers.

Additionally the importer does _not_ control the lifetime of an dma_buf object and it's underlying backing storage. It hence may _never_ free the backing storage itself, that's the job of the exporter.

With that cleared up, referencing the exporters underlying buffer object from the dma_buf will just do the right thing.

note : in case of sharing a buffer between v4l2 and drm driver, the memory info would be copied vb2_xx_buf to xx_gem or xx_gem to vb2_xx_buf through sg table. in this case, only memory info is used to share, not some objects.

Hm, maybe I need to take a look at the currently proposed v4l dma_buf patches ;-) atm I don't have an idea what exactly you're talking about.

Yours, Daniel

On Mon, Jan 09, 2012 at 09:06:56PM +0900, InKi Dae wrote:

2012/1/9 Daniel Vetter daniel@ffwll.ch:

...

On Mon, Jan 09, 2012 at 07:10:25PM +0900, InKi Dae wrote:

...

2012/1/9 Daniel Vetter daniel@ffwll.ch:

...

On Mon, Jan 09, 2012 at 03:20:48PM +0900, InKi Dae wrote:

...

I has test dmabuf based drm gem module for exynos and I found one problem. you can refer to this test repository: http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/exyn...

at this repository, I added some exception codes for resource release in addition to Dave's patch sets.

let's suppose we use dmabuf based vb2 and drm gem with physically continuous memory(no IOMMU) and we try to share allocated buffer between them(v4l2 and drm driver).

1. request memory allocation through drm gem interface.
2. request DRM_SET_PRIME ioctl with the gem handle to get a fd to the

gem object.

• internally, private gem based dmabuf moudle calls drm_buf_export()

to register allocated gem object to fd. 3. request qbuf with the fd(got from 2) and DMABUF type to set the buffer to v4l2 based device.

• internally, vb2 plug in module gets a buffer to the fd and then

calls dmabuf->ops->map_dmabuf() callback to get the sg table containing physical memory info to the gem object. and then the physical memory info would be copied to vb2_xx_buf object. for DMABUF feature for v4l2 and videobuf2 framework, you can refer to this repository: git://github.com/robclark/kernel-omap4.git drmplane-dmabuf

after that, if v4l2 driver want to release vb2_xx_buf object with allocated memory region by user request, how should we do?. refcount to vb2_xx_buf is dependent on videobuf2 framework. so when vb2_xx_buf object is released videobuf2 framework don't know who is using the physical memory region. so this physical memory region is released and when drm driver tries to access the region or to release it also, a problem would be induced.

for this problem, I added get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there may be better way. if there is any missing point, please let me know.

The dma_buf object needs to hold a reference on the underlying (necessarily reference-counted) buffer object when the exporter creates the dma_buf handle. This reference should then get dropped in the exporters dma_buf->ops->release() function, which is only getting called when the last reference to the dma_buf disappears.

when the exporter creates the dma_buf handle(for example, gem -> fd), I think the refcount of gem object should be increased at this point, and decreased by dma_buf->ops->release() again because when the dma_buf is created and dma_buf_export() is called, this dma_buf refers to the gem object one time. and in case of inporter(fd -> gem), file->f_count of the dma_buf is increased and then when this gem object is released by user request such as drm close or drn_gem_close_ioctl, dma_buf_put() should be called by dma_buf->ops->detach() to decrease file->f_count again because the gem object refers to the dma_buf. for this, you can refer to my test repository I mentioned above. but the problem is that when a buffer is released by one side, another can't know whether the buffer already was released or not.

Nope, dma_buf_put should not be called by ->detach. The importer gets his reference when importing the dma_buf and needs to drop that reference himself when it's done using the buffer by calling dma_buf_put (i.e. after the last ->detach call).

I'm afraid that there may be my missing points. I'm confusing. who is Importer and who is Exporter you think? Importer is fd goes to private buffer and Exporter is private buffer goes to fd? if so, yes, when the importer needs to drop that reference(the importer want to release that buffer), dma_buf_put() should be called somewhere and in my case, that function is called by drm_prime_gem_destory(). this function is included at Dave's patch sets and also dma_buf_detatch() is called there. and I just thought that here is right place. I didn't find the place dma_buf_put() is called anywhere. could you please tell me where dma_buf_put() should be called at you think?.

for this, you can refer to Dave's repository: http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf

I haven't really looked at Dave's latest prime patches, but he reported some reference counting issues last time around we chatted about it on irc. So maybe you're just right and the dma_buf_put is indeed missing from drm_prime_gem_destroy ;-) But as I've said, haven't really reviewed the code, so I'm likely completely wrong.

Cheers, Daniel

2012/1/10 Rob Clark rob@ti.com:

On Mon, Jan 9, 2012 at 4:10 AM, InKi Dae daeinki@gmail.com wrote:

...

note : in case of sharing a buffer between v4l2 and drm driver, the memory info would be copied vb2_xx_buf to xx_gem or xx_gem to vb2_xx_buf through sg table. in this case, only memory info is used to share, not some objects.

which v4l2/vb2 patches are you looking at?  The patches I was using, vb2 holds a reference to the 'struct dma_buf *' internally, not just keeping the sg_table

yes, not keeping the sg_table. I mean... see a example below please.

static void vb2_dma_contig_map_dmabuf(void *mem_priv) { struct sg_table *sg; ... sg = dma_buf_map_attachment(buf->db_attach, dir); ... buf->dma_addr = sg_dma_address(sg->sgl); ... }

at least with no IOMMU, the memory information(containing physical memory address) would be copied to vb2_xx_buf object if drm gem exported its own buffer and vb2 wants to use that buffer at this time, sg table is used to share that buffer. and the problem I pointed out is that this buffer(also physical memory region) could be released by vb2 framework(as you know, vb2_xx_buf object and the memory region for buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that so some problems would be induced once drm gem tries to release or access that buffer. and I have tried to resolve this issue adding get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there would be better way.

Thanks.

BR, -R

On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark rob@ti.com wrote:

On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae daeinki@gmail.com wrote:

...

2012/1/10 Rob Clark rob@ti.com: at least with no IOMMU, the memory information(containing physical memory address) would be copied to vb2_xx_buf object if drm gem exported its own buffer and vb2 wants to use that buffer at this time, sg table is used to share that buffer. and the problem I pointed out is that this buffer(also physical memory region) could be released by vb2 framework(as you know, vb2_xx_buf object and the memory region for buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that so some problems would be induced once drm gem tries to release or access that buffer. and I have tried to resolve this issue adding get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there would be better way.

Hi Inki, As also mentioned in the documentation patch, importer (the user of the buffer) - in this case for current RFC patches on v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory of the buffer directly - it should only use the dma-buf callbacks in the right sequence to let the exporter know that it is done using this buffer, so the exporter can release it if allowed and needed.

the exporter (in this case your driver's drm/gem bits) shouldn't release that mapping / sgtable until the importer (in this case v4l2) calls dma_buf_unmap fxn..

It would be an error if the importer did a dma_buf_put() without first calling dma_buf_unmap_attachment() (if currently mapped) and then dma_buf_detach() (if currently attached).  Perhaps somewhere there should be some sanity checking debug code which could be enabled to do a WARN_ON() if the importer does the wrong thing.  It shouldn't really be part of the API, I don't think, but it actually does seem like a good thing, esp. as new drivers start trying to use dmabuf, to have some debug options which could be enabled.

It is entirely possible that something was missed on the vb2 patches, but the way it is intended to work is like this: https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3...

where it does a detach() before the dma_buf_put(), and the vb2-contig backend checks here that it is also unmapped(): https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3...

The proposed RFC for V4L2 adaptation at [1] does exactly the same thing; detach() before dma_buf_put(), and check in vb2-contig backend for unmapped() as mentioned above.

BR, -R

BR, Sumit.

[1]: V4l2 as a dma-buf user RFC: http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/429...

2012/1/10 InKi Dae daeinki@gmail.com:

2012/1/10 Semwal, Sumit sumit.semwal@ti.com:

...

On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark rob@ti.com wrote:

...

On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae daeinki@gmail.com wrote:

...

2012/1/10 Rob Clark rob@ti.com: at least with no IOMMU, the memory information(containing physical memory address) would be copied to vb2_xx_buf object if drm gem exported its own buffer and vb2 wants to use that buffer at this time, sg table is used to share that buffer. and the problem I pointed out is that this buffer(also physical memory region) could be released by vb2 framework(as you know, vb2_xx_buf object and the memory region for buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that so some problems would be induced once drm gem tries to release or access that buffer. and I have tried to resolve this issue adding get_shared_cnt() callback to dma-buf.h but I'm not sure that this is good way. maybe there would be better way.

Hi Inki, As also mentioned in the documentation patch, importer (the user of the buffer) - in this case for current RFC patches on v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory of the buffer directly - it should only use the dma-buf callbacks in the right sequence to let the exporter know that it is done using this buffer, so the exporter can release it if allowed and needed.

thank you for your comments.:) and below are some tables about dmabuf operations with ideal use and these tables indicate reference count of when buffer is created, shared and released. so if there are any problems, please let me know. P.S. these are just simple cases so there would be others.

in case of using only drm gem and dmabuf,

operations                       gem refcount    file f_count   buf refcount

1. gem create                   A:1                                   A:0
2. export(handle A -> fd)    A:2                A:1              A:0
3. import(fd -> handle B)    A:2, B:1         A:2              A:1
4. file close(A)                  A:2, B:1         A:1              A:1
5. gem close(A)                A:1, B:1         A:1              A:1
6. gem close(B)                A:1, B:0         A:1              A:0
7. file close(A)                  A:0                A:0

---

3. handle B shares the buf of handle A.
4. release handle B but its buf.
5. release gem handle A and dmabuf of file A and also physical memory region.

and in case of using drm gem, vb2 and dmabuf,

operations                  gem, vb2 refcount    file f_count   buf refcount

1. gem create                   A:1                 A:0

(GEM side) 2. export(handle A -> fd)    A:2                 A:1              A:0   (GEM side) 3. import(fd -> handle B)    A:2, B:1          A:2              A:1   (VB2 side) 4. file close(A)                  A:2, B:1          A:1              A:1   (VB2 side) 5. vb2 close(B)                 A:2, B:0          A:1              A:0   (VB2 side) 6. gem close(A)                A:1                A:1              A:0   (GEM side) 7. file close(A)                  A:0                A:0   (GEM side)

---

3. vb2 handle B is shared with the buf of gem handle A.
4. release vb2 handle B and decrease refcount of the buf pointed by it.
5. release gem handle A and dmabuf of file A and also physical memory region.

Ah, sorry, it seems that file close shouldn't be called because file->f_count of the file would be dropped by Importer and if f_count is 0 then the file would be released by fput() so I'm not sure but again:

in case of using only drm gem and dmabuf,

operations gem refcount file f_count buf refcount -------------------------------------------------------------------------------------------------- 1. gem create(A) A:1 A:0 2. export(handle A -> fd) A:2 A:1 A:0 3. import(fd -> handle B) A:2, B:1 A:2 A:1 4. gem close(B) A:2, B:release A:1 A:0 5. gem close(A) A:1, A:1 A:0 6. gem close(A) A:release A:release A:release -------------------------------------------------------------------------------------------------

and in case of using drm gem, vb2 and dmabuf,

operations gem, vb2 refcount file f_count buf refcount ------------------------------------------------------------------------------------------------ 1. gem create A:1 A:0 A:0 (GEM side) 2. export(handle A -> fd) A:2 A:1 A:0 (GEM side) 3. import(fd -> handle B) A:2, B:1 A:2 A:1 (VB2 side) 5. vb2 close(B) A:2, B:release A:1 A:0 (VB2 side) 6. gem close(A) A:1 A:1 A:0 (GEM side) 6. gem close(A) A:release A:release A:release (GEM side) ------------------------------------------------------------------------------------------------

...

...

the exporter (in this case your driver's drm/gem bits) shouldn't release that mapping / sgtable until the importer (in this case v4l2) calls dma_buf_unmap fxn..

It would be an error if the importer did a dma_buf_put() without first calling dma_buf_unmap_attachment() (if currently mapped) and then dma_buf_detach() (if currently attached).  Perhaps somewhere there should be some sanity checking debug code which could be enabled to do a WARN_ON() if the importer does the wrong thing.  It shouldn't really be part of the API, I don't think, but it actually does seem like a good thing, esp. as new drivers start trying to use dmabuf, to have some debug options which could be enabled.

It is entirely possible that something was missed on the vb2 patches, but the way it is intended to work is like this: https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3...

where it does a detach() before the dma_buf_put(), and the vb2-contig backend checks here that it is also unmapped(): https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3...

The proposed RFC for V4L2 adaptation at [1] does exactly the same thing; detach() before dma_buf_put(), and check in vb2-contig backend for unmapped() as mentioned above.

...

BR, -R

BR, Sumit.

[1]: V4l2 as a dma-buf user RFC: http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/429...