Am 17.09.2018 um 11:33 schrieb zhoucm1:
[SNIP]
+static struct dma_fence *
+drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64
flags)
+{
+ struct dma_fence *fence;
+ int ret = 0;
+
+ if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ ret = wait_event_timeout(syncobj->syncobj_timeline.wq,
+ point <= syncobj->syncobj_timeline.signal_point,
+ msecs_to_jiffies(10000)); /* wait 10s */
Either don't use a timeout or provide the timeout explicitely, but
don't hard code it here.
Additional to that we probably need an incorruptible wait.
Initially, I used interruptible wait, I found it sometimes often
return -ERESTARTSYS without waiting any second when I wrote unit test.
Any ideas for that?
Yeah, that is perfectly normal. The application just received a signal
and we need to abort the IOCTL ASAP.
still need to interruptible wait?
Yes, that should certainly be an interruptible wait.
Otherwise an application could block on this forever when somebody
forgets to supply a signal point.
- for (i = 0; i < args->count_handles; i++)
- drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
-
+ for (i = 0; i < args->count_handles; i++) {
+ if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
+ DRM_ERROR("timeline syncobj cannot reset!\n");
Why not? I mean that should still work or do I miss anything?
timeline semaphore spec doesn't require reset interface, it says the
timeline value only can be changed by signal operations.
Yeah, but we don't care about the timeline spec in the kernel.
Question is rather if that still makes sense to support that and as far
as I can see it should be trivial to reinitialize the object.
+ ret = -EINVAL;
+ goto out;
+ }
+ drm_syncobj_timeline_fini(syncobjs[i],
+ &syncobjs[i]->syncobj_timeline);
+ drm_syncobj_timeline_init(syncobjs[i],
+ &syncobjs[i]->syncobj_timeline);
+ }
+out:
drm_syncobj_array_free(syncobjs, args->count_handles);
- return 0;
+ return ret;
}
int
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0a8d2d64f380..579e91a5858b 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2137,7 +2137,9 @@ await_fence_array(struct i915_execbuffer *eb,
if (!(flags & I915_EXEC_FENCE_WAIT))
continue;
- fence = drm_syncobj_fence_get(syncobj);
+ drm_syncobj_search_fence(syncobj, 0,
+ DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
Not sure if we can make DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT the
default here.
Maybe better if drivers explicitly need to ask for it?
if you don't like the flag used in specific driver, I can wrap it to a
new function.
Yeah, that is probably a good idea.
+ &fence);
if (!fence)
return -EINVAL;
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 425432b85a87..9535521e6623 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -30,6 +30,25 @@
struct drm_syncobj_cb;
+enum drm_syncobj_type {
+ DRM_SYNCOBJ_TYPE_NORMAL,
Does anybody have a better suggestion for _TYPE_NORMAL? That sounds
like timeline would be special.
How about _TYPE_INDIVIDUAL?
Oh, really good idea.
btw: I encounter a problem after removing advanced wait pt, I debug it
two days, but still don't find reason, from log, it's timeount when
wait_event_timeout, that means it don't receive signal operation.
Good question. Maybe a race condition? Have you tried to trace both
threads to the trace buffer, e.g. use trace_printk?
"./deqp-vk -n dEQP-VK*semaphore*" will fail on
'dEQP-VK.synchronization.basic.semaphore.chain' case, but can pass if
only run that one case like "./deqp-vk -n
dEQP-VK.synchronization.basic.semaphore.chain".
Both old and my previous implementation can pass and have no this
problem.
Can you reproduce that as well with the unit tests in libdrm?
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel