lkml.org 
[lkml]   [2012]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] dma-fence: dma-buf synchronization (v2)
    Hey Rob,

    Op 13-07-12 17:38, Rob Clark schreef:
    > ...
    > +/**
    > + * dma_buf_attach_fence - Attach a fence to a dma-buf.
    > + *
    > + * @buf: the dma-buf to attach to
    > + * @fence: the fence to attach
    > + *
    > + * A fence can only be attached to a single dma-buf. The dma-buf takes
    > + * ownership of the fence, which is unref'd when the fence is signaled.
    > + * The fence takes a reference to the dma-buf so the buffer will not be
    > + * freed while there is a pending fence.
    > + */
    > +int dma_buf_attach_fence(struct dma_buf *buf, struct dma_fence *fence)
    > +{
    > + unsigned long flags;
    > + int ret = -EINVAL;
    > +
    > + if (WARN_ON(!buf || !fence))
    > + return -EINVAL;
    > +
    > + spin_lock_irqsave(&fence->event_queue.lock, flags);
    > + if (!fence->attached) {
    > + get_dma_buf(buf);
    > + fence->attached = true;
    > + list_add(&fence->list_node, &buf->fence_list);
    > + ret = 0;
    > + }
    > + spin_unlock_irqrestore(&fence->event_queue.lock, flags);
    > +
    > + return ret;
    > +}
    > +EXPORT_SYMBOL_GPL(dma_buf_attach_fence);
    This design that a fence can only be attached to 1 dmabuf?
    Wouldn't it be better to kill the fence_list and just create an array
    of pointers to all the fences attached to current dmabuf?
    Or some other design that would allow multiple fences to be
    attached to a single dmabuf, and a single fence to multiple
    dma-bufs without being attached to all and without too many
    memory allocations. Maybe we should add a limit in a #define
    to how many fences can be attached to a single dmabuf?
    More than 4 fences on a single dma-buf is likely overkill, but I don't
    want to place a limit yet on how many dma-bufs can attach to a
    single fence.
    > +/**
    > + * dma_buf_get_fence - Get the most recent pending fence attached to the
    > + * dma-buf.
    > + *
    > + * @buf: the dma-buf whose fence to get
    > + *
    > + * If this returns NULL, there are no pending fences. Otherwise this
    > + * takes a reference to the returned fence, so the caller must later
    > + * call dma_fence_put() to release the reference.
    > + */
    > +struct dma_fence *dma_buf_get_fence(struct dma_buf *buf)
    > +{
    > + struct dma_fence *fence = NULL;
    > + unsigned long flags;
    > +
    > + if (WARN_ON(!buf))
    > + return ERR_PTR(-EINVAL);
    > +
    > + spin_lock_irqsave(&fence->event_queue.lock, flags);
    > + if (!list_empty(&buf->fence_list)) {
    > + fence = list_first_entry(&buf->fence_list,
    > + struct dma_fence, list_node);
    > + dma_fence_get(fence);
    > + }
    > + spin_unlock_irqrestore(&fence->event_queue.lock, flags);
    > +
    > + return fence;
    > +}
    > +EXPORT_SYMBOL_GPL(dma_buf_get_fence);
    Would mean obsoleting this function, since there's
    no longer a single fence.

    > + * dma_fence_put - Release a reference to the fence.
    > + */
    > +void dma_fence_put(struct dma_fence *fence)
    > +{
    > + WARN_ON(!fence);
    > + kref_put(&fence->refcount, release_fence);
    > +}
    > +EXPORT_SYMBOL_GPL(dma_fence_put);
    Make this inline?

    > +/**
    > + * dma_fence_get - Take a reference to the fence.
    > + *
    > + * In most cases this is used only internally by dma-fence.
    > + */
    > +void dma_fence_get(struct dma_fence *fence)
    > +{
    > + WARN_ON(!fence);
    > + kref_get(&fence->refcount);
    > +}
    > +EXPORT_SYMBOL_GPL(dma_fence_get);
    Same.

    > +/**
    > + * dma_fence_add_callback - Add a callback to be called when the fence
    > + * is signaled.
    > + *
    > + * @fence: The fence to wait on
    > + * @cb: The callback to register
    > + *
    > + * Any number of callbacks can be registered to a fence, but a callback
    > + * can only be registered to once fence at a time.
    > + *
    > + * Note that the callback can be called from an atomic context. If
    > + * fence is already signaled, this function will return -ENOENT (and
    > + * *not* call the callback)
    > + */
    > +int dma_fence_add_callback(struct dma_fence *fence,
    > + struct dma_fence_cb *cb)
    > +{
    > + unsigned long flags;
    > + int ret;
    > +
    > + if (WARN_ON(!fence || !cb))
    > + return -EINVAL;
    > +
    > + ret = check_signaling(fence);
    > +
    > + spin_lock_irqsave(&fence->event_queue.lock, flags);
    > + if (ret == -ENOENT) {
    > + /* if state changed while we dropped the lock, dispatch now */
    > + signal_fence(fence);
    > + } else if (!fence->signaled && !ret) {
    > + dma_fence_get(fence);
    > + cb->fence = fence;
    > + __add_wait_queue(&fence->event_queue, &cb->base);
    > + ret = 0;
    > + } else {
    > + ret = -EINVAL;
    > + }
    > + spin_unlock_irqrestore(&fence->event_queue.lock, flags);
    Unconditionally taking same spinlock twice seems a bit overkill,
    maybe just drop it in check_signalling if needed?

    Some standardized base for hardware dma-buf fence objects would
    also be nice, it will make implementing it for drm a lot easier.

    ~Maarten


    \
     
     \ /
      Last update: 2012-07-16 13:01    [W:0.061 / U:0.040 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site