Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
2016-06-24 Christian König <deathsimple@xxxxxxxxxxx>:
Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
Hi Christian,
2016-06-24 Christian König <christian.koenig@xxxxxxx>:
Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
Hi all,
This is an attempt to improve fence support on Sync File. The basic idea
is to have only sync_file->fence and store all fences there, either as
normal fences or fence_arrays. That way we can remove some potential
duplication when using fence_array with sync_file: the duplication of the array
of fences and the duplication of fence_add_callback() for all fences.
Now when creating a new sync_file during the merge process sync_file_set_fence()
will set sync_file->fence based on the number of fences for that sync_file. If
there is more than one fence a fence_array is created. One important advantage
approach is that we only add one fence callback now, no matter how many fences
there are in a sync_file - the individual callbacks are added by fence_array.
Two fence ops had to be created to help abstract the difference between handling
fences and fences_arrays: .teardown() and .get_fences(). The former run needed
on fence_array, and the latter just return a copy of all fences in the fence.
I'm not so sure about adding those two, speacially .get_fences(). What do you
think?
Clearly not a good idea to add this a fence ops, cause those are specialized
functions for only a certain fence implementation (the fence_array).
Are you refering only to .get_fences()?
That comment was only for the get_fences() operation, but the teardown()
callback looks very suspicious to me as well.
Can you explain once more why that should be necessary?
When the sync_file owner exits we need to clean up it and that means releasing
the fence too, however with fence_array we can't just call fence_put()
as a extra reference to array->base for each fence is held when enabling
signalling. Thus we need a prior step, that I called teardown(), to
remove the callback for not signaled fences and put the extra
references.
Another way to do this would be:
if (fence_is_array(sync_file->fence))
fence_array_destroy(to_fence_array(sync_file->fence));
else
fence_put(sync_file_fence);
This would avoid the extra ops, maybe we should go this way.
NAK on both approaches. The fence array grabs another reference on
itself for each callback it registers, so this isn't necessary:
for (i = 0; i < array->num_fences; ++i) {
cb[i].array = array;
/*
* As we may report that the fence is signaled before all
* callbacks are complete, we need to take an additional
* reference count on the array so that we do not free
it too
* early. The core fence handling will only hold the
reference
* until we signal the array as complete (but that is now
* insufficient).
*/
fence_get(&array->base);
if (fence_add_callback(array->fences[i], &cb[i].cb,
fence_array_cb_func)) {
fence_put(&array->base);
if (atomic_dec_and_test(&array->num_pending))
return false;
}
}
So you can just use fence_remove_callback() and then fence_put() without
worrying about the reference.
Regards,
Christian.
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
