| Message ID | 5277777E.5060904@canonical.com |
|---|---|
| State | New |
| Headers | show |
op 07-11-13 22:11, Rom Lemarchand schreef: > Hi Maarten, I tested your changes and needed the attached patch: behavior > now seems equivalent as android sync. I haven't tested performance. > > The issue resolved by this patch happens when i_b < b->num_fences and > i_a >= a->num_fences (or vice versa). Then, pt_a is invalid and so > dereferencing pt_a->context causes a crash. Oops, thinko. :) Originally I had it correct by doing this: + /* + * Assume sync_fence a and b are both ordered and have no + * duplicates with the same context. + * + * If a sync_fence can only be created with sync_fence_merge + * and sync_fence_create, this is a reasonable assumption. + */ + for (i = i_a = i_b = 0; i_a < a->num_fences && i_b < b->num_fences; ) { + struct fence *pt_a = a->cbs[i_a].sync_pt; + struct fence *pt_b = b->cbs[i_b].sync_pt; + + if (pt_a->context < pt_b->context) { + sync_fence_add_pt(fence, &i, pt_a); + + i_a++; + } else if (pt_a->context > pt_b->context) { + sync_fence_add_pt(fence, &i, pt_b); + + i_b++; + } else { + if (pt_a->seqno - pt_b->seqno <= INT_MAX) + sync_fence_add_pt(fence, &i, pt_a); + else + sync_fence_add_pt(fence, &i, pt_b); + + i_a++; + i_b++; + } + } + + /* Add remaining fences from a or b*/ + for (; i_a < a->num_fences; i_a++) + sync_fence_add_pt(fence, &i, a->cbs[i_a].sync_pt); + + for (; i_b < b->num_fences; i_b++) + sync_fence_add_pt(fence, &i, b->cbs[i_b].sync_pt); Then I thought I could clean it up by merging it, but that ended up being more unreadable and crashing... so I guess I'll revert back to this version. :)
op 07-11-13 22:11, Rom Lemarchand schreef: > Hi Maarten, I tested your changes and needed the attached patch: behavior > now seems equivalent as android sync. I haven't tested performance. > > The issue resolved by this patch happens when i_b < b->num_fences and i_a >> = a->num_fences (or vice versa). Then, pt_a is invalid and so > dereferencing pt_a->context causes a crash. > Yeah, I pushed my original fix. I intended to keep android userspace behavior the same, and I tried to keep the kernelspace the api same as much as I could. If peformance is the same, or not noticeably worse, would there be any objections on the android side about renaming dma-fence to syncpoint, and getting it in mainline? ~Maarten
I ran some benchmarks and things seem to be running about the same. No one on our graphics team seemed concerned about the change. The only concern I heard was about the increased complexity of the new sync code as opposed to the old sync framework which tried to keep things straightforward. On Fri, Nov 8, 2013 at 3:43 AM, Maarten Lankhorst < maarten.lankhorst@canonical.com> wrote: > op 07-11-13 22:11, Rom Lemarchand schreef: > > Hi Maarten, I tested your changes and needed the attached patch: behavior > > now seems equivalent as android sync. I haven't tested performance. > > > > The issue resolved by this patch happens when i_b < b->num_fences and i_a > >> = a->num_fences (or vice versa). Then, pt_a is invalid and so > > dereferencing pt_a->context causes a crash. > > > Yeah, I pushed my original fix. I intended to keep android userspace > behavior the same, and I tried to keep the kernelspace the api same as much > as I could. If peformance is the same, or not noticeably worse, would there > be any objections on the android side about renaming dma-fence to > syncpoint, and getting it in mainline? > > ~Maarten >
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index 2c7fd3f2ab23..d1d89f1f8553 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -232,39 +232,62 @@ void sync_fence_install(struct sync_fence *fence, int fd) } EXPORT_SYMBOL(sync_fence_install); +static void sync_fence_add_pt(struct sync_fence *fence, int *i, struct fence *pt) { + fence->cbs[*i].sync_pt = pt; + fence->cbs[*i].fence = fence; + + if (!fence_add_callback(pt, &fence->cbs[*i].cb, fence_check_cb_func)) { + fence_get(pt); + (*i)++; + } +} + struct sync_fence *sync_fence_merge(const char *name, struct sync_fence *a, struct sync_fence *b) { int num_fences = a->num_fences + b->num_fences; struct sync_fence *fence; - int i; + int i, i_a, i_b; fence = sync_fence_alloc(offsetof(struct sync_fence, cbs[num_fences]), name); if (fence == NULL) return NULL; - fence->num_fences = num_fences; atomic_set(&fence->status, num_fences); - for (i = 0; i < a->num_fences; ++i) { - struct fence *pt = a->cbs[i].sync_pt; - - fence_get(pt); - fence->cbs[i].sync_pt = pt; - fence->cbs[i].fence = fence; - if (fence_add_callback(pt, &fence->cbs[i].cb, fence_check_cb_func)) - atomic_dec(&fence->status); + /* + * Assume sync_fence a and b are both ordered and have no + * duplicates with the same context. + * + * If a sync_fence can only be created with sync_fence_merge + * and sync_fence_create, this is a reasonable assumption. + */ + for (i = i_a = i_b = 0; i_a < a->num_fences || i_b < b->num_fences; ) { + struct fence *pt_a = i_a < a->num_fences ? a->cbs[i_a].sync_pt : NULL; + struct fence *pt_b = i_b < b->num_fences ? b->cbs[i_b].sync_pt : NULL; + + if (!pt_b || pt_a->context < pt_b->context) { + sync_fence_add_pt(fence, &i, pt_a); + + i_a++; + } else if (!pt_a || pt_a->context > pt_b->context) { + sync_fence_add_pt(fence, &i, pt_b); + + i_b++; + } else { + if (pt_a->seqno - pt_b->seqno <= INT_MAX) + sync_fence_add_pt(fence, &i, pt_a); + else + sync_fence_add_pt(fence, &i, pt_b); + + i_a++; + i_b++; + } } - for (i = 0; i < b->num_fences; ++i) { - struct fence *pt = b->cbs[i].sync_pt; - - fence_get(pt); - fence->cbs[a->num_fences + i].sync_pt = pt; - fence->cbs[a->num_fences + i].fence = fence; - if (fence_add_callback(pt, &fence->cbs[a->num_fences + i].cb, fence_check_cb_func)) - atomic_dec(&fence->status); - } + if (num_fences > i) + atomic_sub(num_fences - i, &fence->status); + fence->num_fences = i; sync_fence_debug_add(fence); return fence;