On 07/04/2015 10:18, Maarten Lankhorst wrote:
Hey,
Op 20-03-15 om 18:48 schreef John.C.Harrison@xxxxxxxxx:
From: John Harrison <John.C.Harrison@xxxxxxxxx>
There is a construct in the linux kernel called 'struct fence' that is intended
to keep track of work that is executed on hardware. I.e. it solves the basic
problem that the drivers 'struct drm_i915_gem_request' is trying to address. The
request structure does quite a lot more than simply track the execution progress
so is very definitely still required. However, the basic completion status side
could be updated to use the ready made fence implementation and gain all the
advantages that provides.
This patch makes the first step of integrating a struct fence into the request.
It replaces the explicit reference count with that of the fence. It also
replaces the 'is completed' test with the fence's equivalent. Currently, that
simply chains on to the original request implementation. A future patch will
improve this.
For: VIZ-5190
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 37 +++++++++------------
drivers/gpu/drm/i915/i915_gem.c | 55 ++++++++++++++++++++++++++++---
drivers/gpu/drm/i915/intel_lrc.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++
5 files changed, 70 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ce3a536..7dcaf8c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -50,6 +50,7 @@
#include <linux/intel-iommu.h>
#include <linux/kref.h>
#include <linux/pm_qos.h>
+#include <linux/fence.h>
/* General customization:
*/
@@ -2048,7 +2049,11 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
* initial reference taken using kref_init
*/
struct drm_i915_gem_request {
- struct kref ref;
+ /** Underlying object for implementing the signal/wait stuff.
+ * NB: Never call fence_later()! Due to lazy allocation, scheduler
+ * re-ordering, pre-emption, etc., there is no guarantee at all
+ * about the validity or sequentialiaty of the fence's seqno! */
+ struct fence fence;
Set fence.context differently for each per context timeline. :-)
Yeah, I didn't like the way the description for fence_later() says
'returns NULL if both fences are signaled' and then also returns null on
a context mismatch. I was also not entirely sure what the fence context
thing is meant to be for. AFAICT, the expectation is that there is only
supposed to be a finite and small number of contexts as there is no
management of them. They are simply an incrementing number with no way
to 'release' a previously allocated context. Whereas, the i915 context
is per application in an execlist enabled system. Potentially, multiple
contexts per application even. So there is an unbounded and large number
of them about. That sounds like a bad idea for the fence context
implementation!
+static bool i915_gem_request_enable_signaling(struct fence *req_fence)
+{
+ WARN(true, "Is this required?");
+ return true;
+}
Yes, try calling fence_wait() on the fence. :-) This function should call irq_get and add itself to ring->irq_queue.
See for an example radeon_fence_enable_signaling.
See patch three in the series :). The above warning should really say
'This should not be required yet.' but I didn't get around to updating it.
@@ -2557,6 +2596,8 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
return ret;
}
+ fence_init(&request->fence, &i915_gem_request_fops, &ring->fence_lock, ring->fence_context, request->seqno);
+
/*
* Reserve space in the ring buffer for all the commands required to
* eventually emit this request. This is to guarantee that the
Use ring->irq_queue.lock instead of making a new lock? This will make implementing enable_signaling easier too.
Is that definitely safe? It won't cause conflicts or unnecessary
complications? Indeed, is one supposed to play around with the implicit
lock inside a wait queue?
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
