Re: [PATCH 4/5] reservation: cross-device reservation support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I took a quick look on the fencing and added some thoughts on
shared fences:

On 09/28/2012 02:43 PM, Maarten Lankhorst wrote:
This adds support for a generic reservations framework that can be
hooked up to ttm and dma-buf and allows easy sharing of reservations
across devices.

The idea is that a dma-buf and ttm object both will get a pointer
to a struct reservation_object, which has to be reserved before
anything is done with the buffer.

Some followup patches are needed in ttm so the lru_lock is no longer
taken during the reservation step. This makes the lockdep annotation
patch a lot more useful, and the assumption that the lru lock protects
atomic removal off the lru list will fail soon, anyway.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx>

+
+/**
+ * ticket_commit - commit a reservation with a new fence
+ * @ticket:	[in]	the reservation_ticket returned by
+ * ticket_reserve
+ * @entries:	[in]	a linked list of struct reservation_entry
+ * @fence:	[in]	the fence that indicates completion
+ *
+ * This function will call reservation_ticket_fini, no need
+ * to do it manually.
+ *
+ * This function should be called after a hardware command submission is
+ * completed succesfully. The fence is used to indicate completion of
+ * those commands.
+ */
+void
+ticket_commit(struct reservation_ticket *ticket,
+		  struct list_head *entries, struct fence *fence)
+{
+	struct list_head *cur;
+
+	if (list_empty(entries))
+		return;
+
+	if (WARN_ON(!fence)) {
+		ticket_backoff(ticket, entries);
+		return;
+	}
+
+	list_for_each(cur, entries) {
+		struct reservation_object *bo;
+		bool shared;
+
+		reservation_entry_get(cur, &bo, &shared);
+
+		if (!shared) {
+			int i;
+			for (i = 0; i < bo->fence_shared_count; ++i) {
+				fence_put(bo->fence_shared[i]);
+				bo->fence_shared[i] = NULL;
+			}
+			bo->fence_shared_count = 0;
+			if (bo->fence_excl)
+				fence_put(bo->fence_excl);
+
+			bo->fence_excl = fence;
I assume here that the validation code has made sure that fences are
either ordered or expired so that "fence" signals *after* all other fences
have signaled.

+		} else {
+			if (WARN_ON(bo->fence_shared_count >=
+				    ARRAY_SIZE(bo->fence_shared))) {
+				continue;
+			}
This is bad. Failure to fence a buffer is a catastrophic error that can lead
to pages being reused for other stuff while still being read by the GPU,
and the caller must be informed with an error code and sync on the fence.

I guess this has been discussed previously, but I think it might be more
appropriate with a list of pointers to fences. There is an allocation overhead,
for this, but allocation from a mem cache should really be fast enough, and
the list entries can be allocated during ticket_reserve to avoid errors in
the commit code.

+
+			bo->fence_shared[bo->fence_shared_count++] = fence;

It might be good if this function had access to a light version of a cross-device
struct fence * order_fences(struct fence *a, struct fence *b)
function that can quickly check two fences and determine whether signaling one means that the other one also is signaled. In that case one or more of the shared fences can be unreferenced,
putting less pressure on the fence_shared array.
The lightweight version of order_fences is allowed to fail if there is no simple
and quick way of ordering them. Could perhaps be added to the fence API.

And (even though not part of the reservation API)
There is a heavyweight version of that cross-device function
int order_fence(struct fence *a, int gpu_engine) needed for the validation code
exclusive fencing that
*makes sure* fence a has signaled before the current gpu_engine executes its commands. For some gpu - fence pairs the ordering is done implicitly since they share the same command stream, for some it's possible to insert barriers in the gpu_engine command stream (radeon and nouveau is doing that), and if there is no other way of doing it, the code will need to
wait for the fence.

+		}
+		fence_get(fence);

Hmm. Perhaps a fence_get(fence, NUM) to avoid a huge number of atomic incs?

+
+		object_unreserve(bo, ticket);
+	}
+	reservation_ticket_fini(ticket);
+}
+EXPORT_SYMBOL(ticket_commit);


Thomas

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux