lkml.org 
[lkml]   [2016]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file
    2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:

    > On Tue, Jun 28, 2016 at 11:25:00AM -0300, Gustavo Padovan wrote:
    > > 2016-06-28 Chris Wilson <chris@chris-wilson.co.uk>:
    > >
    > > > On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
    > > > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
    > > > >
    > > > > Create sync_file->fence to abstract the type of fence we are using for
    > > > > each sync_file. If only one fence is present we use a normal struct fence
    > > > > but if there is more fences to be added to the sync_file a fence_array
    > > > > is created.
    > > > >
    > > > > This change cleans up sync_file a bit. We don't need to have sync_file_cb
    > > > > array anymore. Instead, as we always have one fence, only one fence
    > > > > callback is registered per sync_file.
    > > > >
    > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
    > > > > Cc: Christian König <christian.koenig@amd.com>
    > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
    > > > > ---
    > > > > @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
    > > > > {
    > > > > struct sync_file *sync_file;
    > > > >
    > > > > - sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
    > > > > + sync_file = sync_file_alloc();
    > > > > if (!sync_file)
    > > > > return NULL;
    > > > >
    > > > > - sync_file->num_fences = 1;
    > > > > + sync_file->fence = fence;
    > > > > +
    > > > > atomic_set(&sync_file->status, 1);
    > > >
    > > > sync_file->status => fence_is_signaled(sync_file->fence);
    > > >
    > > > Both should just be an atomic read, except fence_is_signaled() will then
    > > > do a secondary poll.
    > >
    > > Not sure I follow. I set it to 1 here, but below when we call
    > > fence_add_callback() and the fence is already signalled atomic_dec sets
    > > sync_file->status to 0.
    >
    > I'm just saying that usage sync_file->status is equivalent to
    > fence_is_signaled(), i.e. we reduce the amount of bookkeeping local to
    > sync_file.

    Indeed, that makes sense, I'll remove status from sync_file.

    >
    > > > > snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
    > > > > fence->ops->get_driver_name(fence),
    > > > > fence->ops->get_timeline_name(fence), fence->context,
    > > > > fence->seqno);
    > > > >
    > > > > - sync_file->cbs[0].fence = fence;
    > > > > - sync_file->cbs[0].sync_file = sync_file;
    > > > > - if (fence_add_callback(fence, &sync_file->cbs[0].cb,
    > > > > - fence_check_cb_func))
    > > > > + if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
    > > > > atomic_dec(&sync_file->status);
    > > > >
    > > > > return sync_file;
    > > > > @@ -121,14 +119,42 @@ err:
    > > > > return NULL;
    > > > > }
    > > > >
    > > > > -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
    > > > > - struct fence *fence)
    > > > > +static int sync_file_set_fence(struct sync_file *sync_file,
    > > > > + struct fence **fences, int num_fences)
    > > > > {
    > > > > - sync_file->cbs[*i].fence = fence;
    > > > > - sync_file->cbs[*i].sync_file = sync_file;
    > > > > + struct fence_array *array;
    > > > > +
    > > > > + if (num_fences == 1) {
    > > > > + sync_file->fence = fences[0];
    > > >
    > > > This steals the references.
    > > >
    > > > > + } else {
    > > > > + array = fence_array_create(num_fences, fences,
    > > > > + fence_context_alloc(1), 1, false);
    > > >
    > > > This creates a reference.
    > > >
    > > > When we call fence_put(sync_fence->fence) we release a reference we
    > > > never owned if num_fences == 1.
    > >
    > > No, sync_file_merge() gets a new reference for each fence it is going to
    > > add to the new fence. So for num_fences == 1 when sync_file->fence is
    > > set we already hold a reference to it, so no matter if it is a fence or
    > > a array we own a reference.
    >
    > Ugh. Root cause appears to be that fence_array_create() does not behave
    > how I would expect, in that it borrows references to the fences and
    > not own a reference to the fences in its array. I beg for a comment as
    > this function is very counter-intuitive for me.

    There is this in fence_array_create():

    * The caller should allocte the fences array with num_fences size
    * and fill it with the fences it wants to add to the object. Ownership of this
    * array is take and fence_put() is used on each fence on release.

    Gustavo

    \
     
     \ /
      Last update: 2016-06-28 18:21    [W:3.201 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site