Am 2022-06-09 um 03:18 schrieb Christian König:
Am 09.06.22 um 02:05 schrieb Felix Kuehling:
[SNIP]
+ *
+ * ret = drm_exec_lock(&exec, boB, 1);
Where is drm_exec_lock? It's not in this patch.
I've renamed this function to drm_exec_prepare_obj() but forgot to
update the documentation. Going to fix this, thanks.
+ * drm_exec_continue_on_contention(&exec);
+ * if (ret)
+ * goto error;
+ * }
+ *
+ * drm_exec_for_each_locked_object(&exec, index, obj) {
+ * dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
+ * ...
+ * }
+ * drm_exec_fini(&exec);
+ *
+ * See struct dma_exec for more details.
+ */
+
+/* Dummy value used to initially enter the retry loop */
+#define DRM_EXEC_DUMMY (void*)~0
+
+/* Initialize the drm_exec_objects container */
+static void drm_exec_objects_init(struct drm_exec_objects *container)
+{
+ container->objects = kmalloc(PAGE_SIZE, GFP_KERNEL);
Should this be kvmalloc? You're using kvrealloc and kvfree elsewhere.
The initial number of objects tracked should be small enough so that
we can use kmalloc() directly.
Ok. It just seems weird to allocate with kmalloc and then kvrealloc the
same pointer. But if that's safe it doesn't matter.
BTW, if the caller already knows the number of BOs it wants to add,
would is make sense to add that as a parameter to drm_exec_objects_init
to save you all the reallocs?
[SNIP]
+/**
+ * drm_exec_prepare_obj - prepare a GEM object for use
+ * @exec: the drm_exec object with the state
+ * @obj: the GEM object to prepare
+ * @num_fences: how many fences to reserve
+ *
+ * Prepare a GEM object for use by locking it and reserving fence
slots. All
+ * succesfully locked objects are put into the locked container.
Duplicates
+ * detected as well and automatically moved into the duplicates
container.
+ *
+ * Returns: -EDEADLK if a contention is detected, -ENOMEM when memory
+ * allocation failed and zero for success.
+ */
+int drm_exec_prepare_obj(struct drm_exec *exec, struct
drm_gem_object *obj,
+ unsigned int num_fences)
+{
+ int ret;
+
+ ret = drm_exec_lock_contended(exec);
If this succeeds, it won't reserve any fence slots for object. Is
that a problem?
No, the contended object should be re-presented later on and then we
allocate the fence slots.
Ok. Along with all other objects, because drm_exec_cleanup will have
cleaned up the lists.
+ if (unlikely(ret == -EALREADY)) {
+ struct drm_exec_objects *container = &exec->duplicates;
+
+ /*
+ * If this is the first locked GEM object it was most likely
+ * just contended. So don't add it to the duplicates, just
+ * reserve the fence slots.
I don't understand this. Seems a bit arbitrary. Is it even legal to
try to add the same object twice? I thought duplicates was for
different objects that share the same reservation, not actually the
same object on the same list twice.
Yes, it's legal to try the same object twice. That can easily happen
when userspace specifies the same handle twice for a submission.
The other possibility is that we had a contention, backed off and then
locked the contention first and then re-tried everything else once more.
Maybe you meant to compare with
exec->locked.objects[exec->locked.num_objects-1], assuming that
drm_exec_lock_contended just succeeded locking a previously contended
object, and the caller retried locking that same object again?
Yes, that's pretty much the idea. But then exec->locked.num_objects is
just 1 so that's equal to checking exec->locked.num_objects[0].
Ok. I missed that after drm_exec_continue_on_contention the
drm_exec_cleanup will clean up the lists and we start over locking all
the objects. Why can't drm_exec_cleanup just lock the contended object
itself? I think that would make it more obvious that the contended
object is always the first one that gets locked and added to the list.
Thanks,
Felix
On the other hand we would also catch cases where we lock the same
object multiple times directly behind each other. Need to think about
that a bit.
+ */
+ if (exec->locked.num_objects && exec->locked.objects[0] ==
obj)
+ container = NULL;
+
+ ret = drm_exec_obj_locked(container, obj, num_fences);
+ if (ret)
+ return ret;
+
+ } else if (unlikely(ret)) {
+ return ret;
+
+ } else {
+ ret = drm_exec_obj_locked(&exec->locked, obj, num_fences);
+ if (ret)
+ goto error_unlock;
+ }
+
+ drm_gem_object_get(obj);
The container already gets a reference to obj. What is this extra
reference for? And where does it get dropped?
Need to double check this, might be that it's just a leftover.
Thanks,
Christian.
Regards,
Felix
