Re: [RFC PATCH] fence: dma-buf cross-device synchronization (v12)

[Rob Clark]

On Thu, Aug 15, 2013 at 7:16 AM, Maarten Lankhorst
<maarten.lankhorst@xxxxxxxxxxxxx> wrote:
> Op 12-08-13 17:43, Rob Clark schreef:
>> On Mon, Jul 29, 2013 at 10:05 AM, Maarten Lankhorst
>> <maarten.lankhorst@xxxxxxxxxxxxx> wrote:
>>> +
[snip]
>>> +/**
>>> + * fence_add_callback - add a callback to be called when the fence
>>> + * is signaled
>>> + * @fence:     [in]    the fence to wait on
>>> + * @cb:                [in]    the callback to register
>>> + * @func:      [in]    the function to call
>>> + * @priv:      [in]    the argument to pass to function
>>> + *
>>> + * cb will be initialized by fence_add_callback, no initialization
>>> + * by the caller is required. Any number of callbacks can be registered
>>> + * to a fence, but a callback can only be registered to one 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)
>>> + *
>>> + * Add a software callback to the fence. Same restrictions apply to
>>> + * refcount as it does to fence_wait, however the caller doesn't need to
>>> + * keep a refcount to fence afterwards: when software access is enabled,
>>> + * the creator of the fence is required to keep the fence alive until
>>> + * after it signals with fence_signal. The callback itself can be called
>>> + * from irq context.
>>> + *
>>> + */
>>> +int fence_add_callback(struct fence *fence, struct fence_cb *cb,
>>> +                      fence_func_t func, void *priv)
>>> +{
>>> +       unsigned long flags;
>>> +       int ret = 0;
>>> +       bool was_set;
>>> +
>>> +       if (WARN_ON(!fence || !func))
>>> +               return -EINVAL;
>>> +
>>> +       if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +               return -ENOENT;
>>> +
>>> +       spin_lock_irqsave(fence->lock, flags);
>>> +
>>> +       was_set = test_and_set_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags);
>>> +
>>> +       if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +               ret = -ENOENT;
>>> +       else if (!was_set && !fence->ops->enable_signaling(fence)) {
>>> +               __fence_signal(fence);
>>> +               ret = -ENOENT;
>>> +       }
>>> +
>>> +       if (!ret) {
>>> +               cb->func = func;
>>> +               cb->priv = priv;
>>> +               list_add_tail(&cb->node, &fence->cb_list);
>> since the user is providing the 'struct fence_cb', why not drop the
>> priv & func args, and have some cb-initialize macro, ie.
>>
>> INIT_FENCE_CB(&foo->fence, cbfxn);
>>
>> and I guess we can just drop priv and let the user embed fence in
>> whatever structure they like.  Ie. make it look a bit how work_struct
>> works.
> I don't mind killing priv. But a INIT_FENCE_CB macro is silly, when all it would do is set cb->func.
> So passing it as  an argument to fence_add_callback is fine, unless you have a better reason to
> do so.
>
> INIT_WORK seems to have a bit more initialization than us, it seems work can be more complicated
> than callbacks, because the callbacks can only be called once and work can be rescheduled multiple times.

yeah, INIT_WORK does more.. although maybe some day we want
INIT_FENCE_CB to do more (ie. if we add some debug features to help
catch misuse of fence/fence-cb's).  And if nothing else, having it
look a bit like other constructs that we have in the kernel seems
useful.  And with my point below, you'd want INIT_FENCE_CB to do a
INIT_LIST_HEAD(), so it is (very) slightly more than just setting the
fxn ptr.

>> maybe also, if (!list_empty(&cb->node) return -EBUSY?
> I think checking for list_empty(cb->node) is a terrible idea. This is no different from any other list corruption,
> and it's a programming error. Not a runtime error. :-)

I was thinking for crtc and page-flip, embed the fence_cb in the crtc.
 You should only use the cb once at a time, but in this case you might
want to re-use it for the next page flip.  Having something to catch
cb mis-use in this sort of scenario seems useful.

maybe how I am thinking to use fence_cb is not quite what you had in
mind.  I'm not sure.  I was trying to think how I could just directly
use fence/fence_cb in msm for everything (imported dmabuf or just
regular 'ol gem buffers).

> cb->node.next/prev may be NULL, which would fail with this check. The contents of cb->node are undefined
> before fence_add_callback is called. Calling fence_remove_callback on a fence that hasn't been added is
> undefined too. Calling fence_remove_callback works, but I'm thinking of changing the list_del_init to list_del,
> which would make calling fence_remove_callback twice a fatal error if CONFIG_DEBUG_LIST is enabled,
> and a possible memory corruption otherwise.
>>> ...
>>> +
[snip]
>>> +
>>> +/**
>>> + * fence context counter: each execution context should have its own
>>> + * fence context, this allows checking if fences belong to the same
>>> + * context or not. One device can have multiple separate contexts,
>>> + * and they're used if some engine can run independently of another.
>>> + */
>>> +extern atomic_t fence_context_counter;
>> context-alloc should not be in the critical path.. I'd think probably
>> drop the extern and inline, and make fence_context_counter static
>> inside the .c
> Shrug, your bikeshed. I'll fix it shortly.
>>> +
>>> +static inline unsigned fence_context_alloc(unsigned num)
>> well, this is actually allocating 'num' contexts, so
>> 'fence_context_alloc()' sounds a bit funny.. or at least to me it
>> sounds from the name like it allocates a single context
> Sorry, max number of bikesheds reached. :P

well, names are important to convey meaning, and not confusing users
of the API..  but fence_context*s*_alloc() also sounds a bit funny.
So I could live w/ just some kerneldoc.  Ie. move the doc about
fence_counter_contex down and make it doc about the function.  That
was a bit my point about moving the function into the .c and making
fence_context_counter static.. ie. I don't think it was your intention
that anyone accesses fence_counter_context directly, so better to
document the function and make fence_counter_context the internal
implementation detail.

Anyways, some of this is a bit nit-picky, but since fence is going to
be something used by many different drivers/subsystems, I guess it is
worthwhile to nit-pick over the API.

BR,
-R

> ~Maarten
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/