[PATCH v2 00/28] Replace seqno values with request structures

[PATCH v2 00/28] Replace seqno values with request structures

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

There is a general feeling that it is better to move away from using a simple
integer 'seqno' value to track batch buffer completion. Instead, the request
structure should be used. That provides for much more flexibility going
forwards. Especially which things like a GPU scheduler (which can re-order batch
buffers and hence seqnos after submission to the hardware), Android sync points
and other such features which potentially make seqno usage more and more
complex.

This patch set does the work of converting most of the driver to use request
structures in preference to seqno values. The only place left that still uses
seqnos is the semaphore code. It was decided to leave that alone for the time
being as the semaphores are hardware based and the hardware only understands
seqno values.

v2: Rebased to newer nightly tree which significantly changed some of the
display MMIO flip code (including making __wait_request public and renaming
it).

[Patches against drm-intel-nightly tree fetched 13/11/2014]

John Harrison (28):
  drm/i915: Ensure OLS & PLR are always in sync
  drm/i915: Add reference count to request structure
  drm/i915: Add helper functions to aid seqno -> request transition
  drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
  drm/i915: Convert i915_gem_ring_throttle to use requests
  drm/i915: Ensure requests stick around during waits
  drm/i915: Remove 'outstanding_lazy_seqno'
  drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
  drm/i915: Convert 'last_flip_req' to be a request not a seqno
  drm/i915: Convert i915_wait_seqno to i915_wait_request
  drm/i915: Add IRQ friendly request deference facility
  drm/i915: Convert mmio_flip::seqno to struct request
  drm/i915: Convert __wait_seqno() to __wait_request()
  drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
  drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
  drm/i915: Convert trace functions from seqno to request
  drm/i915: Convert 'trace_irq' to use requests rather than seqnos
  drm/i915: Convert 'ring_idle()' to use requests not seqnos
  drm/i915: Connect requests to rings at creation not submission
  drm/i915: Convert 'i915_seqno_passed' calls into 'i915_gem_request_completed'
  drm/i915: Remove the now redundant 'obj->ring'
  drm/i915: Cache request completion status
  drm/i915: Zero fill the request structure
  drm/i915: Spinlock protection for request list
  drm/i915: Interrupt driven request completion
  drm/i915: Remove obsolete parameter to i915_gem_request_completed()
  drm/i915: Add unique id to the request structure for debugging
  drm/i915: Additional request structure tracing

 drivers/gpu/drm/i915/i915_debugfs.c          |   20 +-
 drivers/gpu/drm/i915/i915_drv.h              |   97 ++++++--
 drivers/gpu/drm/i915/i915_gem.c              |  319 ++++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_context.c      |    3 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |   10 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h          |    4 +-
 drivers/gpu/drm/i915/i915_gem_render_state.c |    2 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c       |    2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c        |    7 +-
 drivers/gpu/drm/i915/i915_irq.c              |   16 +-
 drivers/gpu/drm/i915/i915_trace.h            |   69 +++---
 drivers/gpu/drm/i915/intel_display.c         |   47 ++--
 drivers/gpu/drm/i915/intel_drv.h             |    5 +-
 drivers/gpu/drm/i915/intel_lrc.c             |   61 ++---
 drivers/gpu/drm/i915/intel_overlay.c         |   26 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.c      |   82 ++++---
 drivers/gpu/drm/i915/intel_ringbuffer.h      |   25 +-
 17 files changed, 502 insertions(+), 293 deletions(-)

-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 01/28] drm/i915: Ensure OLS & PLR are always in sync

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The aim is to replace seqno values with request structures. A step along the way
is to switch to using the PLR in preference to the OLS. That requires the PLR to
only be valid when and only when the OLS is also valid. I.e., the two must be
kept in lock step. Then, code which was using the OLS can be safely switched
over to using the PLR instead.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |   42 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   29 +++++++++++++++------
 2 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2a1a719..be0178c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -794,27 +794,39 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
 static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
 				    struct intel_context *ctx)
 {
-	if (ring->outstanding_lazy_seqno)
-		return 0;
+	struct drm_i915_gem_request *request;
+	int ret;
 
-	if (ring->preallocated_lazy_request == NULL) {
-		struct drm_i915_gem_request *request;
+	/* XXX: The aim is to replace seqno values with request structures.
+	 * A step along the way is to switch to using the PLR in preference
+	 * to the OLS. That requires the PLR to only be valid when the OLS is
+	 * also valid. I.e., the two must be kept in step. */
 
-		request = kmalloc(sizeof(*request), GFP_KERNEL);
-		if (request == NULL)
-			return -ENOMEM;
+	if (ring->outstanding_lazy_seqno) {
+		WARN_ON(ring->preallocated_lazy_request == NULL);
+		return 0;
+	}
+	WARN_ON(ring->preallocated_lazy_request != NULL);
 
-		/* Hold a reference to the context this request belongs to
-		 * (we will need it when the time comes to emit/retire the
-		 * request).
-		 */
-		request->ctx = ctx;
-		i915_gem_context_reference(request->ctx);
+	request = kmalloc(sizeof(*request), GFP_KERNEL);
+	if (request == NULL)
+		return -ENOMEM;
 
-		ring->preallocated_lazy_request = request;
+	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	if (ret) {
+		kfree(request);
+		return ret;
 	}
 
-	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	/* Hold a reference to the context this request belongs to
+	 * (we will need it when the time comes to emit/retire the
+	 * request).
+	 */
+	request->ctx = ctx;
+	i915_gem_context_reference(request->ctx);
+
+	ring->preallocated_lazy_request = request;
+	return 0;
 }
 
 static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ae09258..5592a64 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2020,20 +2020,33 @@ int intel_ring_idle(struct intel_engine_cs *ring)
 static int
 intel_ring_alloc_seqno(struct intel_engine_cs *ring)
 {
-	if (ring->outstanding_lazy_seqno)
+	int ret;
+	struct drm_i915_gem_request *request;
+
+	/* XXX: The aim is to replace seqno values with request structures.
+	 * A step along the way is to switch to using the PLR in preference
+	 * to the OLS. That requires the PLR to only be valid when the OLS
+	 * is also valid. I.e., the two must be kept in step. */
+
+	if (ring->outstanding_lazy_seqno) {
+		WARN_ON(ring->preallocated_lazy_request == NULL);
 		return 0;
+	}
 
-	if (ring->preallocated_lazy_request == NULL) {
-		struct drm_i915_gem_request *request;
+	WARN_ON(ring->preallocated_lazy_request != NULL);
 
-		request = kmalloc(sizeof(*request), GFP_KERNEL);
-		if (request == NULL)
-			return -ENOMEM;
+	request = kmalloc(sizeof(*request), GFP_KERNEL);
+	if (request == NULL)
+		return -ENOMEM;
 
-		ring->preallocated_lazy_request = request;
+	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	if (ret) {
+		kfree(request);
+		return ret;
 	}
 
-	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	ring->preallocated_lazy_request = request;
+	return 0;
 }
 
 static int __intel_ring_prepare(struct intel_engine_cs *ring,
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/28] drm/i915: Add reference count to request structure

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The plan is to use request structures everywhere that seqno values were
previously used. This means saving pointers to structures in places that used to
be simple integers. In turn, that means that the target structure now needs much
more stringent lifetime tracking. That is, it must not be freed while some other
random object still holds a pointer to it.

To achieve this tracking, a reference count needs to be added. Whenever a
pointer to the structure is saved away, the count must be incremented and the
free must only occur when all references have been released.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c         |   17 ++++++++++++-----
 drivers/gpu/drm/i915/intel_lrc.c        |    4 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 +++-
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3f3035c..47ae368 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1989,6 +1989,8 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
  * an emission time with seqnos for tracking how far ahead of the GPU we are.
  */
 struct drm_i915_gem_request {
+	struct kref ref;
+
 	/** On Which ring this request was generated */
 	struct intel_engine_cs *ring;
 
@@ -2018,6 +2020,32 @@ struct drm_i915_gem_request {
 	struct list_head client_list;
 };
 
+void i915_gem_request_free(struct kref *req_ref);
+
+static inline void
+i915_gem_request_reference(struct drm_i915_gem_request *req)
+{
+	kref_get(&req->ref);
+}
+
+static inline void
+i915_gem_request_unreference(struct drm_i915_gem_request *req)
+{
+	kref_put(&req->ref, i915_gem_request_free);
+}
+
+static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
+					   struct drm_i915_gem_request *src)
+{
+	if (src)
+		i915_gem_request_reference(src);
+
+	if (*pdst)
+		i915_gem_request_unreference(*pdst);
+
+	*pdst = src;
+}
+
 struct drm_i915_file_private {
 	struct drm_i915_private *dev_priv;
 	struct drm_file *file;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 86cf428..f0b6f5ba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2579,10 +2579,18 @@ static void i915_gem_free_request(struct drm_i915_gem_request *request)
 	list_del(&request->list);
 	i915_gem_request_remove_from_client(request);
 
-	if (request->ctx)
-		i915_gem_context_unreference(request->ctx);
+	i915_gem_request_unreference(request);
+}
+
+void i915_gem_request_free(struct kref *req_ref)
+{
+	struct drm_i915_gem_request *req = container_of(req_ref,
+						 typeof(*req), ref);
 
-	kfree(request);
+	if (req->ctx)
+		i915_gem_context_unreference(req->ctx);
+
+	kfree(req);
 }
 
 struct drm_i915_gem_request *
@@ -2665,8 +2673,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 	}
 
 	/* These may not have been flush before the reset, do so now */
-	kfree(ring->preallocated_lazy_request);
-	ring->preallocated_lazy_request = NULL;
+	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
 	ring->outstanding_lazy_seqno = 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index be0178c..0ae6925 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -812,6 +812,8 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
 	if (request == NULL)
 		return -ENOMEM;
 
+	kref_init(&request->ref);
+
 	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
 	if (ret) {
 		kfree(request);
@@ -1271,7 +1273,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 	intel_logical_ring_stop(ring);
 	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
-	ring->preallocated_lazy_request = NULL;
+	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
 	ring->outstanding_lazy_seqno = 0;
 
 	if (ring->cleanup)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5592a64..7752791 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1859,7 +1859,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
 	intel_destroy_ringbuffer_obj(ringbuf);
-	ring->preallocated_lazy_request = NULL;
+	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
 	ring->outstanding_lazy_seqno = 0;
 
 	if (ring->cleanup)
@@ -2039,6 +2039,8 @@ intel_ring_alloc_seqno(struct intel_engine_cs *ring)
 	if (request == NULL)
 		return -ENOMEM;
 
+	kref_init(&request->ref);
+
 	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
 	if (ret) {
 		kfree(request);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 03/28] drm/i915: Add helper functions to aid seqno -> request transition

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Added helper functions for retrieving the ring and seqno entries from a request
structure. This allows the internal workings of the request structure to be
hidden from code that is using these. It also allows for useful
workarounds/debug code to be added as or when necessary.

Note that it is intended that the majority (if not all) uses of the seqno
accessor will disappear eventually as code is updated to use the request
structure itself rather than working with seqno values.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   12 ++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47ae368..9071bb1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2022,6 +2022,18 @@ struct drm_i915_gem_request {
 
 void i915_gem_request_free(struct kref *req_ref);
 
+static inline uint32_t
+i915_gem_request_get_seqno(struct drm_i915_gem_request *req)
+{
+	return req ? req->seqno : 0;
+}
+
+static inline struct intel_engine_cs *
+i915_gem_request_get_ring(struct drm_i915_gem_request *req)
+{
+	return req ? req->ring : NULL;
+}
+
 static inline void
 i915_gem_request_reference(struct drm_i915_gem_request *req)
 {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index aab2e2f..eecd770 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -438,6 +438,13 @@ static inline u32 intel_ring_get_seqno(struct intel_engine_cs *ring)
 	return ring->outstanding_lazy_seqno;
 }
 
+static inline struct drm_i915_gem_request *
+intel_ring_get_request(struct intel_engine_cs *ring)
+{
+	BUG_ON(ring->preallocated_lazy_request == NULL);
+	return ring->preallocated_lazy_request;
+}
+
 static inline void i915_trace_irq_get(struct intel_engine_cs *ring, u32 seqno)
 {
 	if (ring->trace_irq_seqno == 0 && ring->irq_get(ring))
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 04/28] drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The object structure contains the last read, write and fenced seqno values for
use in syncrhonisation operations. These have now been replaced with their
request structure counterparts.

Note that to ensure that objects do not end up with dangling pointers, the
assignments of last_*_req include reference count updates. Thus a request cannot
be freed if an object is still hanging on to it for any reason.

v2: Corrected 'last_rendering_' to 'last_read_' in a number of comments that did
not get updated when 'last_rendering_seqno' became 'last_read|write_seqno'
several millenia ago.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |    6 +--
 drivers/gpu/drm/i915/i915_drv.h            |   13 ++---
 drivers/gpu/drm/i915/i915_gem.c            |   71 ++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +--
 drivers/gpu/drm/i915/i915_gem_gtt.h        |    4 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c     |    2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c      |    4 +-
 drivers/gpu/drm/i915/intel_display.c       |    6 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    2 +-
 9 files changed, 64 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 319da61..ca704af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -133,9 +133,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->base.size / 1024,
 		   obj->base.read_domains,
 		   obj->base.write_domain,
-		   obj->last_read_seqno,
-		   obj->last_write_seqno,
-		   obj->last_fenced_seqno,
+		   i915_gem_request_get_seqno(obj->last_read_req),
+		   i915_gem_request_get_seqno(obj->last_write_req),
+		   i915_gem_request_get_seqno(obj->last_fenced_req),
 		   i915_cache_level_str(to_i915(obj->base.dev), obj->cache_level),
 		   obj->dirty ? " dirty" : "",
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9071bb1..31ba496 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1938,10 +1938,10 @@ struct drm_i915_gem_object {
 	struct intel_engine_cs *ring;
 
 	/** Breadcrumb of last rendering to the buffer. */
-	uint32_t last_read_seqno;
-	uint32_t last_write_seqno;
+	struct drm_i915_gem_request *last_read_req;
+	struct drm_i915_gem_request *last_write_req;
 	/** Breadcrumb of last fenced GPU access to the buffer. */
-	uint32_t last_fenced_seqno;
+	struct drm_i915_gem_request *last_fenced_req;
 
 	/** Current tiling stride for the object, if it's tiled. */
 	uint32_t stride;
@@ -1984,9 +1984,10 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
  * The request queue allows us to note sequence numbers that have been emitted
  * and may be associated with active buffers to be retired.
  *
- * By keeping this list, we can avoid having to do questionable
- * sequence-number comparisons on buffer last_rendering_seqnos, and associate
- * an emission time with seqnos for tracking how far ahead of the GPU we are.
+ * By keeping this list, we can avoid having to do questionable sequence
+ * number comparisons on buffer last_read|write_seqno. It also allows an
+ * emission time to be associated with the request for tracking how far ahead
+ * of the GPU the submission is.
  */
 struct drm_i915_gem_request {
 	struct kref ref;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f0b6f5ba..a8c93c4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1371,11 +1371,11 @@ i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj)
 	/* Manually manage the write flush as we may have not yet
 	 * retired the buffer.
 	 *
-	 * Note that the last_write_seqno is always the earlier of
-	 * the two (read/write) seqno, so if we haved successfully waited,
+	 * Note that the last_write_req is always the earlier of
+	 * the two (read/write) requests, so if we haved successfully waited,
 	 * we know we have passed the last write.
 	 */
-	obj->last_write_seqno = 0;
+	i915_gem_request_assign(&obj->last_write_req, NULL);
 
 	return 0;
 }
@@ -1388,14 +1388,18 @@ static __must_check int
 i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
 			       bool readonly)
 {
+	struct drm_i915_gem_request *req;
 	struct intel_engine_cs *ring = obj->ring;
 	u32 seqno;
 	int ret;
 
-	seqno = readonly ? obj->last_write_seqno : obj->last_read_seqno;
-	if (seqno == 0)
+	req = readonly ? obj->last_write_req : obj->last_read_req;
+	if (!req)
 		return 0;
 
+	seqno = i915_gem_request_get_seqno(req);
+	WARN_ON(seqno == 0);
+
 	ret = i915_wait_seqno(ring, seqno);
 	if (ret)
 		return ret;
@@ -1411,6 +1415,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 					    struct drm_i915_file_private *file_priv,
 					    bool readonly)
 {
+	struct drm_i915_gem_request *req;
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = obj->ring;
@@ -1421,10 +1426,13 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 	BUG_ON(!dev_priv->mm.interruptible);
 
-	seqno = readonly ? obj->last_write_seqno : obj->last_read_seqno;
-	if (seqno == 0)
+	req = readonly ? obj->last_write_req : obj->last_read_req;
+	if (!req)
 		return 0;
 
+	seqno = i915_gem_request_get_seqno(req);
+	WARN_ON(seqno == 0);
+
 	ret = i915_gem_check_wedge(&dev_priv->gpu_error, true);
 	if (ret)
 		return ret;
@@ -2262,12 +2270,12 @@ static void
 i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 			       struct intel_engine_cs *ring)
 {
-	u32 seqno = intel_ring_get_seqno(ring);
+	struct drm_i915_gem_request *req = intel_ring_get_request(ring);
 
 	BUG_ON(ring == NULL);
-	if (obj->ring != ring && obj->last_write_seqno) {
-		/* Keep the seqno relative to the current ring */
-		obj->last_write_seqno = seqno;
+	if (obj->ring != ring && obj->last_write_req) {
+		/* Keep the request relative to the current ring */
+		i915_gem_request_assign(&obj->last_write_req, req);
 	}
 	obj->ring = ring;
 
@@ -2279,7 +2287,7 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 
 	list_move_tail(&obj->ring_list, &ring->active_list);
 
-	obj->last_read_seqno = seqno;
+	i915_gem_request_assign(&obj->last_read_req, req);
 }
 
 void i915_vma_move_to_active(struct i915_vma *vma,
@@ -2310,11 +2318,11 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	list_del_init(&obj->ring_list);
 	obj->ring = NULL;
 
-	obj->last_read_seqno = 0;
-	obj->last_write_seqno = 0;
+	i915_gem_request_assign(&obj->last_read_req, NULL);
+	i915_gem_request_assign(&obj->last_write_req, NULL);
 	obj->base.write_domain = 0;
 
-	obj->last_fenced_seqno = 0;
+	i915_gem_request_assign(&obj->last_fenced_req, NULL);
 
 	obj->active = 0;
 	drm_gem_object_unreference(&obj->base);
@@ -2331,7 +2339,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj)
 		return;
 
 	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      obj->last_read_seqno))
+			      i915_gem_request_get_seqno(obj->last_read_req)))
 		i915_gem_object_move_to_inactive(obj);
 }
 
@@ -2746,7 +2754,8 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 				      struct drm_i915_gem_object,
 				      ring_list);
 
-		if (!i915_seqno_passed(seqno, obj->last_read_seqno))
+		if (!i915_seqno_passed(seqno,
+			     i915_gem_request_get_seqno(obj->last_read_req)))
 			break;
 
 		i915_gem_object_move_to_inactive(obj);
@@ -2856,7 +2865,8 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 	int ret;
 
 	if (obj->active) {
-		ret = i915_gem_check_olr(obj->ring, obj->last_read_seqno);
+		ret = i915_gem_check_olr(obj->ring,
+			     i915_gem_request_get_seqno(obj->last_read_req));
 		if (ret)
 			return ret;
 
@@ -2917,13 +2927,12 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	if (ret)
 		goto out;
 
-	if (obj->active) {
-		seqno = obj->last_read_seqno;
-		ring = obj->ring;
-	}
+	if (!obj->active || !obj->last_read_req)
+		goto out;
 
-	if (seqno == 0)
-		 goto out;
+	seqno = i915_gem_request_get_seqno(obj->last_read_req);
+	WARN_ON(seqno == 0);
+	ring = obj->ring;
 
 	/* Do this after OLR check to make sure we make forward progress polling
 	 * on this IOCTL with a timeout <=0 (like busy ioctl)
@@ -2974,7 +2983,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 
 	idx = intel_ring_sync_index(from, to);
 
-	seqno = obj->last_read_seqno;
+	seqno = i915_gem_request_get_seqno(obj->last_read_req);
 	/* Optimization: Avoid semaphore sync when we are sure we already
 	 * waited for an object with higher seqno */
 	if (seqno <= from->semaphore.sync_seqno[idx])
@@ -2987,11 +2996,12 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	trace_i915_gem_ring_sync_to(from, to, seqno);
 	ret = to->semaphore.sync_to(to, from, seqno);
 	if (!ret)
-		/* We use last_read_seqno because sync_to()
+		/* We use last_read_req because sync_to()
 		 * might have just caused seqno wrap under
 		 * the radar.
 		 */
-		from->semaphore.sync_seqno[idx] = obj->last_read_seqno;
+		from->semaphore.sync_seqno[idx] =
+				i915_gem_request_get_seqno(obj->last_read_req);
 
 	return ret;
 }
@@ -3305,12 +3315,13 @@ static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
 static int
 i915_gem_object_wait_fence(struct drm_i915_gem_object *obj)
 {
-	if (obj->last_fenced_seqno) {
-		int ret = i915_wait_seqno(obj->ring, obj->last_fenced_seqno);
+	if (obj->last_fenced_req) {
+		int ret = i915_wait_seqno(obj->ring,
+			   i915_gem_request_get_seqno(obj->last_fenced_req));
 		if (ret)
 			return ret;
 
-		obj->last_fenced_seqno = 0;
+		i915_gem_request_assign(&obj->last_fenced_req, NULL);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index e1ed85a..094db18 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -943,7 +943,7 @@ void
 i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 				   struct intel_engine_cs *ring)
 {
-	u32 seqno = intel_ring_get_seqno(ring);
+	struct drm_i915_gem_request *req = intel_ring_get_request(ring);
 	struct i915_vma *vma;
 
 	list_for_each_entry(vma, vmas, exec_list) {
@@ -960,7 +960,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 		i915_vma_move_to_active(vma, ring);
 		if (obj->base.write_domain) {
 			obj->dirty = 1;
-			obj->last_write_seqno = seqno;
+			i915_gem_request_assign(&obj->last_write_req, req);
 
 			intel_fb_obj_invalidate(obj, ring);
 
@@ -968,7 +968,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
 		}
 		if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
-			obj->last_fenced_seqno = seqno;
+			i915_gem_request_assign(&obj->last_fenced_req, req);
 			if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
 				struct drm_i915_private *dev_priv = to_i915(ring->dev);
 				list_move_tail(&dev_priv->fence_regs[obj->fence_reg].lru_list,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d0562d0..81de7cc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -182,7 +182,7 @@ struct i915_address_space {
 	 * List of objects currently involved in rendering.
 	 *
 	 * Includes buffers having the contents of their GPU caches
-	 * flushed, not necessarily primitives.  last_rendering_seqno
+	 * flushed, not necessarily primitives. last_read_req
 	 * represents when the rendering involved will be completed.
 	 *
 	 * A reference is held on the buffer while on this list.
@@ -193,7 +193,7 @@ struct i915_address_space {
 	 * LRU list of objects which are not in the ringbuffer and
 	 * are ready to unbind, but are still in the GTT.
 	 *
-	 * last_rendering_seqno is 0 while an object is in this list.
+	 * last_read_req is NULL while an object is in this list.
 	 *
 	 * A reference is not held on the buffer while on this list,
 	 * as merely being GTT-bound shouldn't prevent its being
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index cd7f473..0a0a56f 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -381,7 +381,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 
 		if (ret == 0) {
 			obj->fence_dirty =
-				obj->last_fenced_seqno ||
+				obj->last_fenced_req ||
 				obj->fence_reg != I915_FENCE_REG_NONE;
 
 			obj->tiling_mode = args->tiling_mode;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 89a2f3d..99433d9 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -668,8 +668,8 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 
 	err->size = obj->base.size;
 	err->name = obj->base.name;
-	err->rseqno = obj->last_read_seqno;
-	err->wseqno = obj->last_write_seqno;
+	err->rseqno = i915_gem_request_get_seqno(obj->last_read_req);
+	err->wseqno = i915_gem_request_get_seqno(obj->last_write_req);
 	err->gtt_offset = vma->node.start;
 	err->read_domains = obj->base.read_domains;
 	err->write_domain = obj->base.write_domain;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4706856..60b662a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9451,7 +9451,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	intel_crtc->mmio_flip.seqno = obj->last_write_seqno;
+	intel_crtc->mmio_flip.seqno =
+			     i915_gem_request_get_seqno(obj->last_write_req);
 	intel_crtc->mmio_flip.ring = obj->ring;
 
 	schedule_work(&intel_crtc->mmio_flip.work);
@@ -9651,7 +9652,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		if (ret)
 			goto cleanup_unpin;
 
-		work->flip_queued_seqno = obj->last_write_seqno;
+		work->flip_queued_seqno =
+			     i915_gem_request_get_seqno(obj->last_write_req);
 		work->flip_queued_ring = obj->ring;
 	} else {
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index eecd770..8140ea4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -250,7 +250,7 @@ struct  intel_engine_cs {
 	 * ringbuffer.
 	 *
 	 * Includes buffers having the contents of their GPU caches
-	 * flushed, not necessarily primitives.  last_rendering_seqno
+	 * flushed, not necessarily primitives.  last_read_req
 	 * represents when the rendering involved will be completed.
 	 *
 	 * A reference is held on the buffer while on this list.
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/28] drm/i915: Convert i915_gem_ring_throttle to use requests

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Convert the throttle code to use the request structure rather than extracting a 
ring/seqno pair from it and using those. This is in preparation for
__wait_seqno() becoming __wait_request().

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a8c93c4..86c7aa0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4086,10 +4086,8 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	unsigned long recent_enough = jiffies - msecs_to_jiffies(20);
-	struct drm_i915_gem_request *request;
-	struct intel_engine_cs *ring = NULL;
+	struct drm_i915_gem_request *request, *target = NULL;
 	unsigned reset_counter;
-	u32 seqno = 0;
 	int ret;
 
 	ret = i915_gem_wait_for_error(&dev_priv->gpu_error);
@@ -4105,16 +4103,17 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 		if (time_after_eq(request->emitted_jiffies, recent_enough))
 			break;
 
-		ring = request->ring;
-		seqno = request->seqno;
+		target = request;
 	}
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	spin_unlock(&file_priv->mm.lock);
 
-	if (seqno == 0)
+	if (target == NULL)
 		return 0;
 
-	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, NULL, NULL);
+	ret = __i915_wait_seqno(i915_gem_request_get_ring(target),
+				i915_gem_request_get_seqno(target),
+				reset_counter, true, NULL, NULL);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 06/28] drm/i915: Ensure requests stick around during waits

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Added reference counting of the request structure around __wait_seqno() calls.
This is a precursor to updating the wait code itself to take the request rather
than a seqno. At that point, it would be a Bad Idea for a request object to be
retired and freed while the wait code is still using it.

Note that i915_wait_request() is safe without reference counting as the driver
mutex lock is held for the duration. Thus it is not possible for a free to occur
asynchronously.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 86c7aa0..4dbfee3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1442,10 +1442,12 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 		return ret;
 
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+	i915_gem_request_reference(req);
 	mutex_unlock(&dev->struct_mutex);
 	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, NULL,
 				file_priv);
 	mutex_lock(&dev->struct_mutex);
+	i915_gem_request_unreference(req);
 	if (ret)
 		return ret;
 
@@ -2904,6 +2906,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_wait *args = data;
 	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_request *req;
 	struct intel_engine_cs *ring = NULL;
 	unsigned reset_counter;
 	u32 seqno = 0;
@@ -2930,7 +2933,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	if (!obj->active || !obj->last_read_req)
 		goto out;
 
-	seqno = i915_gem_request_get_seqno(obj->last_read_req);
+	req = obj->last_read_req;
+	seqno = i915_gem_request_get_seqno(req);
 	WARN_ON(seqno == 0);
 	ring = obj->ring;
 
@@ -2944,10 +2948,15 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 
 	drm_gem_object_unreference(&obj->base);
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+	i915_gem_request_reference(req);
 	mutex_unlock(&dev->struct_mutex);
 
-	return __i915_wait_seqno(ring, seqno, reset_counter, true,
-				 &args->timeout_ns, file->driver_priv);
+	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, &args->timeout_ns,
+			   file->driver_priv);
+	mutex_lock(&dev->struct_mutex);
+	i915_gem_request_unreference(req);
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
 
 out:
 	drm_gem_object_unreference(&obj->base);
@@ -4106,6 +4115,8 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 		target = request;
 	}
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+	if (target)
+		i915_gem_request_reference(target);
 	spin_unlock(&file_priv->mm.lock);
 
 	if (target == NULL)
@@ -4117,6 +4128,10 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
+	mutex_lock(&dev->struct_mutex);
+	i915_gem_request_unreference(target);
+	mutex_unlock(&dev->struct_mutex);
+
 	return ret;
 }
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 07/28] drm/i915: Remove 'outstanding_lazy_seqno'

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The OLS value is now obsolete. Exactly the same value is guarateed to be always
available as PLR->seqno. Thus it is safe to remove the OLS completely. And also
to rename the PLR to OLR to keep the 'outstanding lazy ...' naming convention
valid.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   13 +++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++-
 drivers/gpu/drm/i915/intel_display.c       |    3 +-
 drivers/gpu/drm/i915/intel_lrc.c           |   26 +++++---------
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   52 +++++++++++++++-------------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   13 ++-----
 6 files changed, 49 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4dbfee3..fc03e21 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1189,7 +1189,7 @@ i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno)
 	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
 
 	ret = 0;
-	if (seqno == ring->outstanding_lazy_seqno)
+	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
 		ret = i915_add_request(ring, NULL);
 
 	return ret;
@@ -2426,7 +2426,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 	u32 request_ring_position, request_start;
 	int ret;
 
-	request = ring->preallocated_lazy_request;
+	request = ring->outstanding_lazy_request;
 	if (WARN_ON(request == NULL))
 		return -ENOMEM;
 
@@ -2471,7 +2471,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
 			return ret;
 	}
 
-	request->seqno = intel_ring_get_seqno(ring);
 	request->ring = ring;
 	request->head = request_start;
 	request->tail = request_ring_position;
@@ -2508,8 +2507,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 	}
 
 	trace_i915_gem_request_add(ring, request->seqno);
-	ring->outstanding_lazy_seqno = 0;
-	ring->preallocated_lazy_request = NULL;
+	ring->outstanding_lazy_request = NULL;
 
 	if (!dev_priv->ums.mm_suspended) {
 		i915_queue_hangcheck(ring->dev);
@@ -2682,9 +2680,8 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
 		kfree(submit_req);
 	}
 
-	/* These may not have been flush before the reset, do so now */
-	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
-	ring->outstanding_lazy_seqno = 0;
+	/* This may not have been flushed before the reset, so clean it now */
+	i915_gem_request_assign(&ring->outstanding_lazy_request, NULL);
 }
 
 void i915_gem_restore_fences(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 094db18..8123cb5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1167,7 +1167,9 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 			return ret;
 	}
 
-	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags);
+	trace_i915_gem_ring_dispatch(ring,
+		    i915_gem_request_get_seqno(intel_ring_get_request(ring)),
+		    flags);
 
 	i915_gem_execbuffer_move_to_active(vmas, ring);
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60b662a..9b7e993 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9661,7 +9661,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		if (ret)
 			goto cleanup_unpin;
 
-		work->flip_queued_seqno = intel_ring_get_seqno(ring);
+		work->flip_queued_seqno =
+		    i915_gem_request_get_seqno(intel_ring_get_request(ring));
 		work->flip_queued_ring = ring;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0ae6925..b55dcb0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -791,22 +791,14 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
 	execlists_context_queue(ring, ctx, ringbuf->tail);
 }
 
-static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
-				    struct intel_context *ctx)
+static int logical_ring_alloc_request(struct intel_engine_cs *ring,
+				      struct intel_context *ctx)
 {
 	struct drm_i915_gem_request *request;
 	int ret;
 
-	/* XXX: The aim is to replace seqno values with request structures.
-	 * A step along the way is to switch to using the PLR in preference
-	 * to the OLS. That requires the PLR to only be valid when the OLS is
-	 * also valid. I.e., the two must be kept in step. */
-
-	if (ring->outstanding_lazy_seqno) {
-		WARN_ON(ring->preallocated_lazy_request == NULL);
+	if (ring->outstanding_lazy_request)
 		return 0;
-	}
-	WARN_ON(ring->preallocated_lazy_request != NULL);
 
 	request = kmalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
@@ -814,7 +806,7 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
 
 	kref_init(&request->ref);
 
-	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
 		kfree(request);
 		return ret;
@@ -827,7 +819,7 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
 	request->ctx = ctx;
 	i915_gem_context_reference(request->ctx);
 
-	ring->preallocated_lazy_request = request;
+	ring->outstanding_lazy_request = request;
 	return 0;
 }
 
@@ -995,7 +987,7 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords)
 		return ret;
 
 	/* Preallocate the olr before touching the ring */
-	ret = logical_ring_alloc_seqno(ring, ringbuf->FIXME_lrc_ctx);
+	ret = logical_ring_alloc_request(ring, ringbuf->FIXME_lrc_ctx);
 	if (ret)
 		return ret;
 
@@ -1248,7 +1240,8 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf)
 				(ring->status_page.gfx_addr +
 				(I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT)));
 	intel_logical_ring_emit(ringbuf, 0);
-	intel_logical_ring_emit(ringbuf, ring->outstanding_lazy_seqno);
+	intel_logical_ring_emit(ringbuf,
+		i915_gem_request_get_seqno(ring->outstanding_lazy_request));
 	intel_logical_ring_emit(ringbuf, MI_USER_INTERRUPT);
 	intel_logical_ring_emit(ringbuf, MI_NOOP);
 	intel_logical_ring_advance_and_submit(ringbuf);
@@ -1273,8 +1266,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 	intel_logical_ring_stop(ring);
 	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
-	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
-	ring->outstanding_lazy_seqno = 0;
+	i915_gem_request_assign(&ring->outstanding_lazy_request, NULL);
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7752791..f3725da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -915,17 +915,20 @@ static int gen8_rcs_signal(struct intel_engine_cs *signaller,
 		return ret;
 
 	for_each_ring(waiter, dev_priv, i) {
+		u32 seqno;
 		u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
 		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
 			continue;
 
+		seqno = i915_gem_request_get_seqno(
+					   signaller->outstanding_lazy_request);
 		intel_ring_emit(signaller, GFX_OP_PIPE_CONTROL(6));
 		intel_ring_emit(signaller, PIPE_CONTROL_GLOBAL_GTT_IVB |
 					   PIPE_CONTROL_QW_WRITE |
 					   PIPE_CONTROL_FLUSH_ENABLE);
 		intel_ring_emit(signaller, lower_32_bits(gtt_offset));
 		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
-		intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
+		intel_ring_emit(signaller, seqno);
 		intel_ring_emit(signaller, 0);
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
 					   MI_SEMAPHORE_TARGET(waiter->id));
@@ -953,16 +956,19 @@ static int gen8_xcs_signal(struct intel_engine_cs *signaller,
 		return ret;
 
 	for_each_ring(waiter, dev_priv, i) {
+		u32 seqno;
 		u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
 		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
 			continue;
 
+		seqno = i915_gem_request_get_seqno(
+					   signaller->outstanding_lazy_request);
 		intel_ring_emit(signaller, (MI_FLUSH_DW + 1) |
 					   MI_FLUSH_DW_OP_STOREDW);
 		intel_ring_emit(signaller, lower_32_bits(gtt_offset) |
 					   MI_FLUSH_DW_USE_GTT);
 		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
-		intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
+		intel_ring_emit(signaller, seqno);
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
 					   MI_SEMAPHORE_TARGET(waiter->id));
 		intel_ring_emit(signaller, 0);
@@ -991,9 +997,11 @@ static int gen6_signal(struct intel_engine_cs *signaller,
 	for_each_ring(useless, dev_priv, i) {
 		u32 mbox_reg = signaller->semaphore.mbox.signal[i];
 		if (mbox_reg != GEN6_NOSYNC) {
+			u32 seqno = i915_gem_request_get_seqno(
+					   signaller->outstanding_lazy_request);
 			intel_ring_emit(signaller, MI_LOAD_REGISTER_IMM(1));
 			intel_ring_emit(signaller, mbox_reg);
-			intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
+			intel_ring_emit(signaller, seqno);
 		}
 	}
 
@@ -1028,7 +1036,8 @@ gen6_add_request(struct intel_engine_cs *ring)
 
 	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
-	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
+	intel_ring_emit(ring,
+		    i915_gem_request_get_seqno(ring->outstanding_lazy_request));
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	__intel_ring_advance(ring);
 
@@ -1146,7 +1155,8 @@ pc_render_add_request(struct intel_engine_cs *ring)
 			PIPE_CONTROL_WRITE_FLUSH |
 			PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE);
 	intel_ring_emit(ring, ring->scratch.gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
-	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
+	intel_ring_emit(ring,
+		    i915_gem_request_get_seqno(ring->outstanding_lazy_request));
 	intel_ring_emit(ring, 0);
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
 	scratch_addr += 2 * CACHELINE_BYTES; /* write to separate cachelines */
@@ -1165,7 +1175,8 @@ pc_render_add_request(struct intel_engine_cs *ring)
 			PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
 			PIPE_CONTROL_NOTIFY);
 	intel_ring_emit(ring, ring->scratch.gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
-	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
+	intel_ring_emit(ring,
+		    i915_gem_request_get_seqno(ring->outstanding_lazy_request));
 	intel_ring_emit(ring, 0);
 	__intel_ring_advance(ring);
 
@@ -1405,7 +1416,8 @@ i9xx_add_request(struct intel_engine_cs *ring)
 
 	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
-	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
+	intel_ring_emit(ring,
+		    i915_gem_request_get_seqno(ring->outstanding_lazy_request));
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	__intel_ring_advance(ring);
 
@@ -1859,8 +1871,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
 	intel_destroy_ringbuffer_obj(ringbuf);
-	i915_gem_request_assign(&ring->preallocated_lazy_request, NULL);
-	ring->outstanding_lazy_seqno = 0;
+	i915_gem_request_assign(&ring->outstanding_lazy_request, NULL);
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
@@ -2000,7 +2011,7 @@ int intel_ring_idle(struct intel_engine_cs *ring)
 	int ret;
 
 	/* We need to add any requests required to flush the objects and ring */
-	if (ring->outstanding_lazy_seqno) {
+	if (ring->outstanding_lazy_request) {
 		ret = i915_add_request(ring, NULL);
 		if (ret)
 			return ret;
@@ -2018,22 +2029,13 @@ int intel_ring_idle(struct intel_engine_cs *ring)
 }
 
 static int
-intel_ring_alloc_seqno(struct intel_engine_cs *ring)
+intel_ring_alloc_request(struct intel_engine_cs *ring)
 {
 	int ret;
 	struct drm_i915_gem_request *request;
 
-	/* XXX: The aim is to replace seqno values with request structures.
-	 * A step along the way is to switch to using the PLR in preference
-	 * to the OLS. That requires the PLR to only be valid when the OLS
-	 * is also valid. I.e., the two must be kept in step. */
-
-	if (ring->outstanding_lazy_seqno) {
-		WARN_ON(ring->preallocated_lazy_request == NULL);
+	if (ring->outstanding_lazy_request)
 		return 0;
-	}
-
-	WARN_ON(ring->preallocated_lazy_request != NULL);
 
 	request = kmalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
@@ -2041,13 +2043,13 @@ intel_ring_alloc_seqno(struct intel_engine_cs *ring)
 
 	kref_init(&request->ref);
 
-	ret = i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
+	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
 		kfree(request);
 		return ret;
 	}
 
-	ring->preallocated_lazy_request = request;
+	ring->outstanding_lazy_request = request;
 	return 0;
 }
 
@@ -2088,7 +2090,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
 		return ret;
 
 	/* Preallocate the olr before touching the ring */
-	ret = intel_ring_alloc_seqno(ring);
+	ret = intel_ring_alloc_request(ring);
 	if (ret)
 		return ret;
 
@@ -2123,7 +2125,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno)
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	BUG_ON(ring->outstanding_lazy_seqno);
+	BUG_ON(ring->outstanding_lazy_request);
 
 	if (INTEL_INFO(dev)->gen == 6 || INTEL_INFO(dev)->gen == 7) {
 		I915_WRITE(RING_SYNC_0(ring->mmio_base), 0);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 8140ea4..824f5884 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -266,8 +266,7 @@ struct  intel_engine_cs {
 	/**
 	 * Do we have some not yet emitted requests outstanding?
 	 */
-	struct drm_i915_gem_request *preallocated_lazy_request;
-	u32 outstanding_lazy_seqno;
+	struct drm_i915_gem_request *outstanding_lazy_request;
 	bool gpu_caches_dirty;
 	bool fbc_dirty;
 
@@ -432,17 +431,11 @@ static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
 	return ringbuf->tail;
 }
 
-static inline u32 intel_ring_get_seqno(struct intel_engine_cs *ring)
-{
-	BUG_ON(ring->outstanding_lazy_seqno == 0);
-	return ring->outstanding_lazy_seqno;
-}
-
 static inline struct drm_i915_gem_request *
 intel_ring_get_request(struct intel_engine_cs *ring)
 {
-	BUG_ON(ring->preallocated_lazy_request == NULL);
-	return ring->preallocated_lazy_request;
+	BUG_ON(ring->outstanding_lazy_request == NULL);
+	return ring->outstanding_lazy_request;
 }
 
 static inline void i915_trace_irq_get(struct intel_engine_cs *ring, u32 seqno)
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 08/28] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Updated the _check_olr() function to actually take a request object and compare
it to the OLR rather than extracting seqnos and comparing those.

Note that there is one use case where the request object being processed is no
longer available at that point in the call stack. Hence a temporary copy of the
original function is still present (but called _check_ols() instead). This will
be removed in a subsequent patch.

Also, downgraded a BUG_ON to a WARN_ON as apparently the former is frowned upon
for shipping code.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   18 +++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem.c |   28 +++++++++++-----------------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 31ba496..08aad6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2589,7 +2589,7 @@ bool i915_gem_retire_requests(struct drm_device *dev);
 void i915_gem_retire_requests_ring(struct intel_engine_cs *ring);
 int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
 				      bool interruptible);
-int __must_check i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno);
+int __must_check i915_gem_check_olr(struct drm_i915_gem_request *req);
 
 static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
 {
@@ -3129,4 +3129,20 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	}
 }
 
+/* XXX: Temporary solution to be removed later in patch series. */
+static inline int __must_check i915_gem_check_ols(
+				     struct intel_engine_cs *ring, u32 seqno)
+{
+	int ret;
+
+	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+
+	ret = 0;
+	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
+		ret = i915_add_request(ring, NULL);
+
+	return ret;
+}
+/* XXX: Temporary solution to be removed later in patch series. */
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc03e21..b9a6f9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1178,19 +1178,18 @@ i915_gem_check_wedge(struct i915_gpu_error *error,
 }
 
 /*
- * Compare seqno against outstanding lazy request. Emit a request if they are
- * equal.
+ * Compare arbitrary request against outstanding lazy request. Emit on match.
  */
 int
-i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno)
+i915_gem_check_olr(struct drm_i915_gem_request *req)
 {
 	int ret;
 
-	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
 
 	ret = 0;
-	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
-		ret = i915_add_request(ring, NULL);
+	if (req == req->ring->outstanding_lazy_request)
+		ret = i915_add_request(req->ring, NULL);
 
 	return ret;
 }
@@ -1353,7 +1352,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno)
 	if (ret)
 		return ret;
 
-	ret = i915_gem_check_olr(ring, seqno);
+	ret = i915_gem_check_ols(ring, seqno);
 	if (ret)
 		return ret;
 
@@ -1420,7 +1419,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = obj->ring;
 	unsigned reset_counter;
-	u32 seqno;
 	int ret;
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -1430,22 +1428,19 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	if (!req)
 		return 0;
 
-	seqno = i915_gem_request_get_seqno(req);
-	WARN_ON(seqno == 0);
-
 	ret = i915_gem_check_wedge(&dev_priv->gpu_error, true);
 	if (ret)
 		return ret;
 
-	ret = i915_gem_check_olr(ring, seqno);
+	ret = i915_gem_check_olr(req);
 	if (ret)
 		return ret;
 
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	i915_gem_request_reference(req);
 	mutex_unlock(&dev->struct_mutex);
-	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, NULL,
-				file_priv);
+	ret = __i915_wait_seqno(ring, i915_gem_request_get_seqno(req),
+				reset_counter, true, NULL, file_priv);
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_request_unreference(req);
 	if (ret)
@@ -2864,8 +2859,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 	int ret;
 
 	if (obj->active) {
-		ret = i915_gem_check_olr(obj->ring,
-			     i915_gem_request_get_seqno(obj->last_read_req));
+		ret = i915_gem_check_olr(obj->last_read_req);
 		if (ret)
 			return ret;
 
@@ -2995,7 +2989,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	if (seqno <= from->semaphore.sync_seqno[idx])
 		return 0;
 
-	ret = i915_gem_check_olr(obj->ring, seqno);
+	ret = i915_gem_check_olr(obj->last_read_req);
 	if (ret)
 		return ret;
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 09/28] drm/i915: Convert 'last_flip_req' to be a request not a seqno

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Converted 'last_flip_req' to be an actual request rather than a seqno value as
part of the on going seqno to request changes. This includes reference counting
the request being saved away to ensure it can not be retired and freed while the
overlay code is still waiting on it.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_overlay.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index dc2f4f2..5defc37 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -182,7 +182,7 @@ struct intel_overlay {
 	u32 flip_addr;
 	struct drm_i915_gem_object *reg_bo;
 	/* flip handling */
-	uint32_t last_flip_req;
+	struct drm_i915_gem_request *last_flip_req;
 	void (*flip_tail)(struct intel_overlay *);
 };
 
@@ -217,17 +217,20 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	int ret;
 
 	BUG_ON(overlay->last_flip_req);
-	ret = i915_add_request(ring, &overlay->last_flip_req);
+	i915_gem_request_assign(&overlay->last_flip_req,
+					     ring->outstanding_lazy_request);
+	ret = i915_add_request(ring, NULL);
 	if (ret)
 		return ret;
 
 	overlay->flip_tail = tail;
-	ret = i915_wait_seqno(ring, overlay->last_flip_req);
+	ret = i915_wait_seqno(ring,
+			 i915_gem_request_get_seqno(overlay->last_flip_req));
 	if (ret)
 		return ret;
 	i915_gem_retire_requests(dev);
 
-	overlay->last_flip_req = 0;
+	i915_gem_request_assign(&overlay->last_flip_req, NULL);
 	return 0;
 }
 
@@ -286,7 +289,10 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
 	intel_ring_emit(ring, flip_addr);
 	intel_ring_advance(ring);
 
-	return i915_add_request(ring, &overlay->last_flip_req);
+	WARN_ON(overlay->last_flip_req);
+	i915_gem_request_assign(&overlay->last_flip_req,
+					     ring->outstanding_lazy_request);
+	return i915_add_request(ring, NULL);
 }
 
 static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
@@ -366,10 +372,11 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	if (overlay->last_flip_req == 0)
+	if (overlay->last_flip_req == NULL)
 		return 0;
 
-	ret = i915_wait_seqno(ring, overlay->last_flip_req);
+	ret = i915_wait_seqno(ring,
+			 i915_gem_request_get_seqno(overlay->last_flip_req));
 	if (ret)
 		return ret;
 	i915_gem_retire_requests(dev);
@@ -377,7 +384,7 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 	if (overlay->flip_tail)
 		overlay->flip_tail(overlay);
 
-	overlay->last_flip_req = 0;
+	i915_gem_request_assign(&overlay->last_flip_req, NULL);
 	return 0;
 }
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 10/28] drm/i915: Convert i915_wait_seqno to i915_wait_request

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Updated i915_wait_seqno() to take a request structure instead of a seqno value
and renamed it accordingly. Internally, it just pulls the seqno out of the
request and calls on to __wait_seqno() as before. However, all the code further
up the stack is now simplified as it can just pass the request object straight
through without having to peek inside.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   19 +-----------------
 drivers/gpu/drm/i915/i915_gem.c         |   33 +++++++++++++++----------------
 drivers/gpu/drm/i915/intel_lrc.c        |    6 ++----
 drivers/gpu/drm/i915/intel_overlay.c    |   11 +++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   14 ++++++-------
 5 files changed, 28 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 08aad6b..180b674 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2641,8 +2641,7 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 			bool interruptible,
 			s64 *timeout,
 			struct drm_i915_file_private *file_priv);
-int __must_check i915_wait_seqno(struct intel_engine_cs *ring,
-				 uint32_t seqno);
+int __must_check i915_wait_request(struct drm_i915_gem_request *req);
 int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
 int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
@@ -3129,20 +3128,4 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	}
 }
 
-/* XXX: Temporary solution to be removed later in patch series. */
-static inline int __must_check i915_gem_check_ols(
-				     struct intel_engine_cs *ring, u32 seqno)
-{
-	int ret;
-
-	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
-
-	ret = 0;
-	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
-		ret = i915_add_request(ring, NULL);
-
-	return ret;
-}
-/* XXX: Temporary solution to be removed later in patch series. */
-
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b9a6f9c..60e5eec 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1333,32 +1333,37 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 }
 
 /**
- * Waits for a sequence number to be signaled, and cleans up the
+ * Waits for a request to be signaled, and cleans up the
  * request and object lists appropriately for that event.
  */
 int
-i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno)
+i915_wait_request(struct drm_i915_gem_request *req)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool interruptible = dev_priv->mm.interruptible;
+	struct drm_device *dev;
+	struct drm_i915_private *dev_priv;
+	bool interruptible;
 	unsigned reset_counter;
 	int ret;
 
+	BUG_ON(req == NULL);
+
+	dev = req->ring->dev;
+	dev_priv = dev->dev_private;
+	interruptible = dev_priv->mm.interruptible;
+
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
-	BUG_ON(seqno == 0);
 
 	ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible);
 	if (ret)
 		return ret;
 
-	ret = i915_gem_check_ols(ring, seqno);
+	ret = i915_gem_check_olr(req);
 	if (ret)
 		return ret;
 
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
-	return __i915_wait_seqno(ring, seqno, reset_counter, interruptible,
-				 NULL, NULL);
+	return __i915_wait_seqno(req->ring, i915_gem_request_get_seqno(req),
+				 reset_counter, interruptible, NULL, NULL);
 }
 
 static int
@@ -1388,18 +1393,13 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
 			       bool readonly)
 {
 	struct drm_i915_gem_request *req;
-	struct intel_engine_cs *ring = obj->ring;
-	u32 seqno;
 	int ret;
 
 	req = readonly ? obj->last_write_req : obj->last_read_req;
 	if (!req)
 		return 0;
 
-	seqno = i915_gem_request_get_seqno(req);
-	WARN_ON(seqno == 0);
-
-	ret = i915_wait_seqno(ring, seqno);
+	ret = i915_wait_request(req);
 	if (ret)
 		return ret;
 
@@ -3316,8 +3316,7 @@ static int
 i915_gem_object_wait_fence(struct drm_i915_gem_object *obj)
 {
 	if (obj->last_fenced_req) {
-		int ret = i915_wait_seqno(obj->ring,
-			   i915_gem_request_get_seqno(obj->last_fenced_req));
+		int ret = i915_wait_request(obj->last_fenced_req);
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b55dcb0..eba0acd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -828,7 +828,6 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 {
 	struct intel_engine_cs *ring = ringbuf->ring;
 	struct drm_i915_gem_request *request;
-	u32 seqno = 0;
 	int ret;
 
 	if (ringbuf->last_retired_head != -1) {
@@ -843,15 +842,14 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 	list_for_each_entry(request, &ring->request_list, list) {
 		if (__intel_ring_space(request->tail, ringbuf->tail,
 				       ringbuf->size) >= bytes) {
-			seqno = request->seqno;
 			break;
 		}
 	}
 
-	if (seqno == 0)
+	if (&request->list == &ring->request_list)
 		return -ENOSPC;
 
-	ret = i915_wait_seqno(ring, seqno);
+	ret = i915_wait_request(request);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 5defc37..6c530e2 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -224,8 +224,7 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 		return ret;
 
 	overlay->flip_tail = tail;
-	ret = i915_wait_seqno(ring,
-			 i915_gem_request_get_seqno(overlay->last_flip_req));
+	ret = i915_wait_request(overlay->last_flip_req);
 	if (ret)
 		return ret;
 	i915_gem_retire_requests(dev);
@@ -367,19 +366,15 @@ static int intel_overlay_off(struct intel_overlay *overlay)
  * We have to be careful not to repeat work forever an make forward progess. */
 static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
 {
-	struct drm_device *dev = overlay->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
 	int ret;
 
 	if (overlay->last_flip_req == NULL)
 		return 0;
 
-	ret = i915_wait_seqno(ring,
-			 i915_gem_request_get_seqno(overlay->last_flip_req));
+	ret = i915_wait_request(overlay->last_flip_req);
 	if (ret)
 		return ret;
-	i915_gem_retire_requests(dev);
+	i915_gem_retire_requests(overlay->dev);
 
 	if (overlay->flip_tail)
 		overlay->flip_tail(overlay);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f3725da..74c48ed 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1888,7 +1888,6 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
 	struct drm_i915_gem_request *request;
-	u32 seqno = 0;
 	int ret;
 
 	if (ringbuf->last_retired_head != -1) {
@@ -1903,15 +1902,14 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 	list_for_each_entry(request, &ring->request_list, list) {
 		if (__intel_ring_space(request->tail, ringbuf->tail,
 				       ringbuf->size) >= n) {
-			seqno = request->seqno;
 			break;
 		}
 	}
 
-	if (seqno == 0)
+	if (&request->list == &ring->request_list)
 		return -ENOSPC;
 
-	ret = i915_wait_seqno(ring, seqno);
+	ret = i915_wait_request(request);
 	if (ret)
 		return ret;
 
@@ -2007,7 +2005,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
 
 int intel_ring_idle(struct intel_engine_cs *ring)
 {
-	u32 seqno;
+	struct drm_i915_gem_request *req;
 	int ret;
 
 	/* We need to add any requests required to flush the objects and ring */
@@ -2021,11 +2019,11 @@ int intel_ring_idle(struct intel_engine_cs *ring)
 	if (list_empty(&ring->request_list))
 		return 0;
 
-	seqno = list_entry(ring->request_list.prev,
+	req = list_entry(ring->request_list.prev,
 			   struct drm_i915_gem_request,
-			   list)->seqno;
+			   list);
 
-	return i915_wait_seqno(ring, seqno);
+	return i915_wait_request(req);
 }
 
 static int
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The next patches in the series convert some display related seqno usage to
request structure usage. However, the request dereference introduced must be
done from interrupt context. As the dereference potentially involves freeing the
request structure and thus calling lots of non-interrupt friendly code, this
poses a problem.

The solution is to add an IRQ friendly version of the dereference function. All
this does is to add the request structure to a 'delayed free' list and return.
The retire code, which is run periodically, then processes this list and does
the actual dereferencing of the request structures.

v2: Added a count to allow for multiple IRQ dereferences of the same request at
a time.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
 drivers/gpu/drm/i915/i915_gem.c         |   33 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
 5 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 180b674..87cb355 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
 	struct drm_i915_file_private *file_priv;
 	/** file_priv list entry for this request */
 	struct list_head client_list;
+
+	/** deferred free list for dereferencing from IRQ context */
+	struct list_head delay_free_list;
+	uint32_t delay_free_count;
 };
 
 void i915_gem_request_free(struct kref *req_ref);
@@ -2044,9 +2048,12 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
 static inline void
 i915_gem_request_unreference(struct drm_i915_gem_request *req)
 {
+	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
 	kref_put(&req->ref, i915_gem_request_free);
 }
 
+void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req);
+
 static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
 					   struct drm_i915_gem_request *src)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 60e5eec..8453bbd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
 	i915_gem_restore_fences(dev);
 }
 
+void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req)
+{
+	struct intel_engine_cs *ring = req->ring;
+	unsigned long flags;
+
+	/* Need to add the req to a deferred dereference list to be processed
+	 * outside of interrupt time */
+	spin_lock_irqsave(&ring->reqlist_lock, flags);
+	if (req->delay_free_count++ == 0)
+		list_add_tail(&req->delay_free_list, &ring->delayed_free_list);
+	spin_unlock_irqrestore(&ring->reqlist_lock, flags);
+}
+
 /**
  * This function clears the request list as sequence numbers are passed.
  */
@@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 		ring->trace_irq_seqno = 0;
 	}
 
+	while (!list_empty(&ring->delayed_free_list)) {
+		struct drm_i915_gem_request *request;
+		unsigned long flags;
+		uint32_t count;
+
+		request = list_first_entry(&ring->delayed_free_list,
+					   struct drm_i915_gem_request,
+					   delay_free_list);
+
+		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
+		list_del(&request->delay_free_list);
+		count = request->delay_free_count;
+		request->delay_free_count = 0;
+		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
+
+		while (count-- > 0)
+			i915_gem_request_unreference(request);
+	}
+
 	WARN_ON(i915_verify_lists(ring->dev));
 }
 
@@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)
 {
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
 }
 
 void i915_init_vm(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index eba0acd..db8efaa 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
+	spin_lock_init(&ring->reqlist_lock);
 	init_waitqueue_head(&ring->irq_queue);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
 
 	INIT_LIST_HEAD(&ring->execlist_queue);
 	spin_lock_init(&ring->execlist_lock);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 74c48ed..4338132 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
+	spin_lock_init(&ring->reqlist_lock);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
 	INIT_LIST_HEAD(&ring->execlist_queue);
 	ringbuf->size = 32 * PAGE_SIZE;
 	ringbuf->ring = ring;
@@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
 
 	ringbuf->size = size;
 	ringbuf->effective_size = ringbuf->size;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 824f5884..3af7b7c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -262,6 +262,8 @@ struct  intel_engine_cs {
 	 * outstanding.
 	 */
 	struct list_head request_list;
+	spinlock_t reqlist_lock;
+	struct list_head delayed_free_list;
 
 	/**
 	 * Do we have some not yet emitted requests outstanding?
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 12/28] drm/i915: Convert mmio_flip::seqno to struct request

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Converted the mmio_flip 'seqno' value to be a request structure as part of the
on going seqno to request changes. This includes reference counting the request
being saved away to ensure it can not be retired and freed while the flip code
is still waiting on it.

v2: Used the IRQ friendly request dereference call in the notify handler as that
code is called asynchronously without holding any useful mutex locks.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |    3 +--
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b7e993..5e18d49 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9428,18 +9428,19 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
 {
 	struct intel_crtc *intel_crtc =
 		container_of(work, struct intel_crtc, mmio_flip.work);
-	struct intel_engine_cs *ring;
-	uint32_t seqno;
-
-	seqno = intel_crtc->mmio_flip.seqno;
-	ring = intel_crtc->mmio_flip.ring;
+	struct intel_mmio_flip *mmio_flip;
 
-	if (seqno)
-		WARN_ON(__i915_wait_seqno(ring, seqno,
+	mmio_flip = &intel_crtc->mmio_flip;
+	if (mmio_flip->req)
+		WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req),
+					  i915_gem_request_get_seqno(mmio_flip->req),
 					  intel_crtc->reset_counter,
 					  false, NULL, NULL) != 0);
 
 	intel_do_mmio_flip(intel_crtc);
+	if (mmio_flip->req)
+		i915_gem_request_unreference_irq(mmio_flip->req);
+	mmio_flip->req = NULL;
 }
 
 static int intel_queue_mmio_flip(struct drm_device *dev,
@@ -9451,9 +9452,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	intel_crtc->mmio_flip.seqno =
-			     i915_gem_request_get_seqno(obj->last_write_req);
-	intel_crtc->mmio_flip.ring = obj->ring;
+	i915_gem_request_assign(&intel_crtc->mmio_flip.req,
+				obj->last_write_req);
 
 	schedule_work(&intel_crtc->mmio_flip.work);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8432ae2..fbd1cab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,8 +403,7 @@ struct intel_pipe_wm {
 };
 
 struct intel_mmio_flip {
-	u32 seqno;
-	struct intel_engine_cs *ring;
+	struct drm_i915_gem_request *req;
 	struct work_struct work;
 };
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 13/28] drm/i915: Convert __wait_seqno() to __wait_request()

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Now that all code above is using request structures instead of seqno values, it
is possible to convert  __wait_seqno() itself. Internally, it is still calling
i915_seqno_passed(), this will be updated later in the series. This step is just
changing the parameter list and function name.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/i915_gem.c      |   45 +++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_display.c |    7 +++---
 3 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 87cb355..fded90b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2643,7 +2643,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 		       u32 *seqno);
 #define i915_add_request(ring, seqno) \
 	__i915_add_request(ring, NULL, NULL, seqno)
-int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+int __i915_wait_request(struct drm_i915_gem_request *req,
 			unsigned reset_counter,
 			bool interruptible,
 			s64 *timeout,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8453bbd..53ea0e1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1214,10 +1214,9 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
 }
 
 /**
- * __i915_wait_seqno - wait until execution of seqno has finished
- * @ring: the ring expected to report seqno
- * @seqno: duh!
- * @reset_counter: reset sequence associated with the given seqno
+ * __i915_wait_request - wait until execution of request has finished
+ * @req: duh!
+ * @reset_counter: reset sequence associated with the given request
  * @interruptible: do an interruptible wait (normally yes)
  * @timeout: in - how long to wait (NULL forever); out - how much time remaining
  *
@@ -1228,15 +1227,16 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
  * reset_counter _must_ be read before, and an appropriate smp_rmb must be
  * inserted.
  *
- * Returns 0 if the seqno was found within the alloted time. Else returns the
+ * Returns 0 if the request was found within the alloted time. Else returns the
  * errno with remaining time filled in timeout argument.
  */
-int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+int __i915_wait_request(struct drm_i915_gem_request *req,
 			unsigned reset_counter,
 			bool interruptible,
 			s64 *timeout,
 			struct drm_i915_file_private *file_priv)
 {
+	struct intel_engine_cs *ring = i915_gem_request_get_ring(req);
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const bool irq_test_in_progress =
@@ -1248,7 +1248,8 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 
 	WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
 
-	if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
+	if (i915_seqno_passed(ring->get_seqno(ring, true),
+			      i915_gem_request_get_seqno(req)))
 		return 0;
 
 	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
@@ -1265,7 +1266,8 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 		return -ENODEV;
 
 	/* Record current time in case interrupted by signal, or wedged */
-	trace_i915_gem_request_wait_begin(ring, seqno);
+	trace_i915_gem_request_wait_begin(i915_gem_request_get_ring(req),
+					  i915_gem_request_get_seqno(req));
 	before = ktime_get_raw_ns();
 	for (;;) {
 		struct timer_list timer;
@@ -1284,7 +1286,8 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 			break;
 		}
 
-		if (i915_seqno_passed(ring->get_seqno(ring, false), seqno)) {
+		if (i915_seqno_passed(ring->get_seqno(ring, false),
+				      i915_gem_request_get_seqno(req))) {
 			ret = 0;
 			break;
 		}
@@ -1316,7 +1319,8 @@ int __i915_wait_seqno(struct intel_engine_cs *ring, u32 seqno,
 		}
 	}
 	now = ktime_get_raw_ns();
-	trace_i915_gem_request_wait_end(ring, seqno);
+	trace_i915_gem_request_wait_end(i915_gem_request_get_ring(req),
+					i915_gem_request_get_seqno(req));
 
 	if (!irq_test_in_progress)
 		ring->irq_put(ring);
@@ -1362,8 +1366,8 @@ i915_wait_request(struct drm_i915_gem_request *req)
 		return ret;
 
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
-	return __i915_wait_seqno(req->ring, i915_gem_request_get_seqno(req),
-				 reset_counter, interruptible, NULL, NULL);
+	return __i915_wait_request(req, reset_counter,
+				   interruptible, NULL, NULL);
 }
 
 static int
@@ -1417,7 +1421,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	struct drm_i915_gem_request *req;
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *ring = obj->ring;
 	unsigned reset_counter;
 	int ret;
 
@@ -1439,8 +1442,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	i915_gem_request_reference(req);
 	mutex_unlock(&dev->struct_mutex);
-	ret = __i915_wait_seqno(ring, i915_gem_request_get_seqno(req),
-				reset_counter, true, NULL, file_priv);
+	ret = __i915_wait_request(req, reset_counter, true, NULL, file_priv);
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_request_unreference(req);
 	if (ret)
@@ -2930,9 +2932,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	struct drm_i915_gem_wait *args = data;
 	struct drm_i915_gem_object *obj;
 	struct drm_i915_gem_request *req;
-	struct intel_engine_cs *ring = NULL;
 	unsigned reset_counter;
-	u32 seqno = 0;
 	int ret = 0;
 
 	if (args->flags != 0)
@@ -2957,9 +2957,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 		goto out;
 
 	req = obj->last_read_req;
-	seqno = i915_gem_request_get_seqno(req);
-	WARN_ON(seqno == 0);
-	ring = obj->ring;
 
 	/* Do this after OLR check to make sure we make forward progress polling
 	 * on this IOCTL with a timeout <=0 (like busy ioctl)
@@ -2974,8 +2971,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	i915_gem_request_reference(req);
 	mutex_unlock(&dev->struct_mutex);
 
-	ret = __i915_wait_seqno(ring, seqno, reset_counter, true, &args->timeout_ns,
-			   file->driver_priv);
+	ret = __i915_wait_request(req, reset_counter, true, &args->timeout_ns,
+				  file->driver_priv);
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_request_unreference(req);
 	mutex_unlock(&dev->struct_mutex);
@@ -4144,9 +4141,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	if (target == NULL)
 		return 0;
 
-	ret = __i915_wait_seqno(i915_gem_request_get_ring(target),
-				i915_gem_request_get_seqno(target),
-				reset_counter, true, NULL, NULL);
+	ret = __i915_wait_request(target, reset_counter, true, NULL, NULL);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e18d49..457d968 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9432,10 +9432,9 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
 
 	mmio_flip = &intel_crtc->mmio_flip;
 	if (mmio_flip->req)
-		WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req),
-					  i915_gem_request_get_seqno(mmio_flip->req),
-					  intel_crtc->reset_counter,
-					  false, NULL, NULL) != 0);
+		WARN_ON(__i915_wait_request(mmio_flip->req,
+					    intel_crtc->reset_counter,
+					    false, NULL, NULL) != 0);
 
 	intel_do_mmio_flip(intel_crtc);
 	if (mmio_flip->req)
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 14/28] drm/i915: Remove obsolete seqno parameter from 'i915_add_request'

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

There is no longer any need to retrieve a seqno value from an i915_add_request()
call. The calling code already knows which request structure is being processed
(it can only be ring->OLR). And as the request itself is now used in preference
to the basic seqno value, the latter is now redundant in this situation.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |    7 +++----
 drivers/gpu/drm/i915/i915_gem.c              |    7 ++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |    2 +-
 drivers/gpu/drm/i915/i915_gem_render_state.c |    2 +-
 drivers/gpu/drm/i915/intel_lrc.c             |    2 +-
 drivers/gpu/drm/i915/intel_overlay.c         |    4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c      |    2 +-
 7 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fded90b..3e745bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2639,10 +2639,9 @@ int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 int __i915_add_request(struct intel_engine_cs *ring,
 		       struct drm_file *file,
-		       struct drm_i915_gem_object *batch_obj,
-		       u32 *seqno);
-#define i915_add_request(ring, seqno) \
-	__i915_add_request(ring, NULL, NULL, seqno)
+		       struct drm_i915_gem_object *batch_obj);
+#define i915_add_request(ring) \
+	__i915_add_request(ring, NULL, NULL)
 int __i915_wait_request(struct drm_i915_gem_request *req,
 			unsigned reset_counter,
 			bool interruptible,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 53ea0e1..490b664 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1189,7 +1189,7 @@ i915_gem_check_olr(struct drm_i915_gem_request *req)
 
 	ret = 0;
 	if (req == req->ring->outstanding_lazy_request)
-		ret = i915_add_request(req->ring, NULL);
+		ret = i915_add_request(req->ring);
 
 	return ret;
 }
@@ -2414,8 +2414,7 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
 
 int __i915_add_request(struct intel_engine_cs *ring,
 		       struct drm_file *file,
-		       struct drm_i915_gem_object *obj,
-		       u32 *out_seqno)
+		       struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
@@ -2516,8 +2515,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
 		intel_mark_busy(dev_priv->dev);
 	}
 
-	if (out_seqno)
-		*out_seqno = request->seqno;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 8123cb5..233fff1 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -990,7 +990,7 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)__i915_add_request(ring, file, obj, NULL);
+	(void)__i915_add_request(ring, file, obj);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 98dcd94..521548a 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -173,7 +173,7 @@ int i915_gem_render_state_init(struct intel_engine_cs *ring)
 
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), ring);
 
-	ret = __i915_add_request(ring, NULL, so.obj, NULL);
+	ret = __i915_add_request(ring, NULL, so.obj);
 	/* __i915_add_request moves object to inactive if it fails */
 out:
 	i915_gem_render_state_fini(&so);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index db8efaa..b2ec8ab 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1526,7 +1526,7 @@ int intel_lr_context_render_state_init(struct intel_engine_cs *ring,
 
 	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), ring);
 
-	ret = __i915_add_request(ring, file, so.obj, NULL);
+	ret = __i915_add_request(ring, file, so.obj);
 	/* intel_logical_ring_add_request moves object to inactive if it
 	 * fails */
 out:
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6c530e2..c1abf49 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -219,7 +219,7 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
 	BUG_ON(overlay->last_flip_req);
 	i915_gem_request_assign(&overlay->last_flip_req,
 					     ring->outstanding_lazy_request);
-	ret = i915_add_request(ring, NULL);
+	ret = i915_add_request(ring);
 	if (ret)
 		return ret;
 
@@ -291,7 +291,7 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
 	WARN_ON(overlay->last_flip_req);
 	i915_gem_request_assign(&overlay->last_flip_req,
 					     ring->outstanding_lazy_request);
-	return i915_add_request(ring, NULL);
+	return i915_add_request(ring);
 }
 
 static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4338132..f86275d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2012,7 +2012,7 @@ int intel_ring_idle(struct intel_engine_cs *ring)
 
 	/* We need to add any requests required to flush the objects and ring */
 	if (ring->outstanding_lazy_request) {
-		ret = i915_add_request(ring, NULL);
+		ret = i915_add_request(ring);
 		if (ret)
 			return ret;
 	}
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 15/28] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Converted the flip_queued_seqno value to be a request structure as part of the
on going seqno to request changes. This includes reference counting the request
being saved away to ensure it can not be retired and freed while the flip code
is still waiting on it.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    4 ++--
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ca704af..d35e6e0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -546,11 +546,11 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 			if (work->flip_queued_ring) {
 				seq_printf(m, "Flip queued on %s at seqno %u, next seqno %u [current breadcrumb %u], completed? %d\n",
 					   work->flip_queued_ring->name,
-					   work->flip_queued_seqno,
+					   i915_gem_request_get_seqno(work->flip_queued_req),
 					   dev_priv->next_seqno,
 					   work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
 					   i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
-							     work->flip_queued_seqno));
+							     i915_gem_request_get_seqno(work->flip_queued_req)));
 			} else
 				seq_printf(m, "Flip not associated with any ring\n");
 			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 457d968..88f70db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9484,10 +9484,18 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
 		return false;
 
 	if (work->flip_ready_vblank == 0) {
-		if (work->flip_queued_ring &&
-		    !i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
-				       work->flip_queued_seqno))
-			return false;
+		if (work->flip_queued_ring) {
+			uint32_t s1 = work->flip_queued_ring->get_seqno(
+					       work->flip_queued_ring, true);
+			uint32_t s2 = i915_gem_request_get_seqno(
+						      work->flip_queued_req);
+			if (!i915_seqno_passed(s1, s2))
+				return false;
+
+			i915_gem_request_unreference_irq(work->flip_queued_req);
+			work->flip_queued_req  = NULL;
+			work->flip_queued_ring = NULL;
+		}
 
 		work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
 	}
@@ -9651,8 +9659,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		if (ret)
 			goto cleanup_unpin;
 
-		work->flip_queued_seqno =
-			     i915_gem_request_get_seqno(obj->last_write_req);
+		i915_gem_request_assign(&work->flip_queued_req,
+					obj->last_write_req);
 		work->flip_queued_ring = obj->ring;
 	} else {
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
@@ -9660,8 +9668,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		if (ret)
 			goto cleanup_unpin;
 
-		work->flip_queued_seqno =
-		    i915_gem_request_get_seqno(intel_ring_get_request(ring));
+		i915_gem_request_assign(&work->flip_queued_req,
+					intel_ring_get_request(ring));
 		work->flip_queued_ring = ring;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fbd1cab..ba4cfbb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -705,7 +705,7 @@ struct intel_unpin_work {
 	u32 flip_count;
 	u32 gtt_offset;
 	struct intel_engine_cs *flip_queued_ring;
-	u32 flip_queued_seqno;
+	struct drm_i915_gem_request *flip_queued_req;
 	int flip_queued_vblank;
 	int flip_ready_vblank;
 	bool enable_stall_check;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 16/28] drm/i915: Convert trace functions from seqno to request

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

All the code above is now using requests not seqnos so it is possible to convert
the trace functions across. Note that rather than get into problematic reference
counting issues, the trace code only saves the seqno and ring values from the
request structure not the structure pointer itself.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   12 +++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 +--
 drivers/gpu/drm/i915/i915_trace.h          |   47 ++++++++++++++++------------
 3 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 490b664..2fab638 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1266,8 +1266,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 		return -ENODEV;
 
 	/* Record current time in case interrupted by signal, or wedged */
-	trace_i915_gem_request_wait_begin(i915_gem_request_get_ring(req),
-					  i915_gem_request_get_seqno(req));
+	trace_i915_gem_request_wait_begin(req);
 	before = ktime_get_raw_ns();
 	for (;;) {
 		struct timer_list timer;
@@ -1319,8 +1318,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 		}
 	}
 	now = ktime_get_raw_ns();
-	trace_i915_gem_request_wait_end(i915_gem_request_get_ring(req),
-					i915_gem_request_get_seqno(req));
+	trace_i915_gem_request_wait_end(req);
 
 	if (!irq_test_in_progress)
 		ring->irq_put(ring);
@@ -2502,7 +2500,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
 		spin_unlock(&file_priv->mm.lock);
 	}
 
-	trace_i915_gem_request_add(ring, request->seqno);
+	trace_i915_gem_request_add(request);
 	ring->outstanding_lazy_request = NULL;
 
 	if (!dev_priv->ums.mm_suspended) {
@@ -2779,7 +2777,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 		if (!i915_seqno_passed(seqno, request->seqno))
 			break;
 
-		trace_i915_gem_request_retire(ring, request->seqno);
+		trace_i915_gem_request_retire(request);
 
 		/* This is one of the few common intersection points
 		 * between legacy ringbuffer submission and execlists:
@@ -3019,7 +3017,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	if (ret)
 		return ret;
 
-	trace_i915_gem_ring_sync_to(from, to, seqno);
+	trace_i915_gem_ring_sync_to(from, to, obj->last_read_req);
 	ret = to->semaphore.sync_to(to, from, seqno);
 	if (!ret)
 		/* We use last_read_req because sync_to()
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 233fff1..10744db 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1167,9 +1167,7 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 			return ret;
 	}
 
-	trace_i915_gem_ring_dispatch(ring,
-		    i915_gem_request_get_seqno(intel_ring_get_request(ring)),
-		    flags);
+	trace_i915_gem_ring_dispatch(intel_ring_get_request(ring), flags);
 
 	i915_gem_execbuffer_move_to_active(vmas, ring);
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 751d4ad..2c0327b 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -328,8 +328,8 @@ TRACE_EVENT(i915_gem_evict_vm,
 TRACE_EVENT(i915_gem_ring_sync_to,
 	    TP_PROTO(struct intel_engine_cs *from,
 		     struct intel_engine_cs *to,
-		     u32 seqno),
-	    TP_ARGS(from, to, seqno),
+		     struct drm_i915_gem_request *req),
+	    TP_ARGS(from, to, req),
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
@@ -342,7 +342,7 @@ TRACE_EVENT(i915_gem_ring_sync_to,
 			   __entry->dev = from->dev->primary->index;
 			   __entry->sync_from = from->id;
 			   __entry->sync_to = to->id;
-			   __entry->seqno = seqno;
+			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   ),
 
 	    TP_printk("dev=%u, sync-from=%u, sync-to=%u, seqno=%u",
@@ -352,8 +352,8 @@ TRACE_EVENT(i915_gem_ring_sync_to,
 );
 
 TRACE_EVENT(i915_gem_ring_dispatch,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno, u32 flags),
-	    TP_ARGS(ring, seqno, flags),
+	    TP_PROTO(struct drm_i915_gem_request *req, u32 flags),
+	    TP_ARGS(req, flags),
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
@@ -363,11 +363,13 @@ TRACE_EVENT(i915_gem_ring_dispatch,
 			     ),
 
 	    TP_fast_assign(
+			   struct intel_engine_cs *ring =
+						i915_gem_request_get_ring(req);
 			   __entry->dev = ring->dev->primary->index;
 			   __entry->ring = ring->id;
-			   __entry->seqno = seqno;
+			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   __entry->flags = flags;
-			   i915_trace_irq_get(ring, seqno);
+			   i915_trace_irq_get(ring, __entry->seqno);
 			   ),
 
 	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
@@ -398,8 +400,8 @@ TRACE_EVENT(i915_gem_ring_flush,
 );
 
 DECLARE_EVENT_CLASS(i915_gem_request,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno),
-	    TP_ARGS(ring, seqno),
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req),
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
@@ -408,9 +410,11 @@ DECLARE_EVENT_CLASS(i915_gem_request,
 			     ),
 
 	    TP_fast_assign(
+			   struct intel_engine_cs *ring =
+						i915_gem_request_get_ring(req);
 			   __entry->dev = ring->dev->primary->index;
 			   __entry->ring = ring->id;
-			   __entry->seqno = seqno;
+			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   ),
 
 	    TP_printk("dev=%u, ring=%u, seqno=%u",
@@ -418,8 +422,8 @@ DECLARE_EVENT_CLASS(i915_gem_request,
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req)
 );
 
 TRACE_EVENT(i915_gem_request_complete,
@@ -443,13 +447,13 @@ TRACE_EVENT(i915_gem_request_complete,
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_retire,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req)
 );
 
 TRACE_EVENT(i915_gem_request_wait_begin,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno),
-	    TP_ARGS(ring, seqno),
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req),
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
@@ -465,10 +469,13 @@ TRACE_EVENT(i915_gem_request_wait_begin,
 	     * less desirable.
 	     */
 	    TP_fast_assign(
+			   struct intel_engine_cs *ring =
+						i915_gem_request_get_ring(req);
 			   __entry->dev = ring->dev->primary->index;
 			   __entry->ring = ring->id;
-			   __entry->seqno = seqno;
-			   __entry->blocking = mutex_is_locked(&ring->dev->struct_mutex);
+			   __entry->seqno = i915_gem_request_get_seqno(req);
+			   __entry->blocking =
+				     mutex_is_locked(&ring->dev->struct_mutex);
 			   ),
 
 	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
@@ -477,8 +484,8 @@ TRACE_EVENT(i915_gem_request_wait_begin,
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_end,
-	    TP_PROTO(struct intel_engine_cs *ring, u32 seqno),
-	    TP_ARGS(ring, seqno)
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req)
 );
 
 DECLARE_EVENT_CLASS(i915_ring,
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 17/28] drm/i915: Convert 'trace_irq' to use requests rather than seqnos

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Updated the trace_irq code to use requests instead of seqnos. This includes
reference counting the request object to ensure it sticks around when required.
Note that getting access to the reference counting functions means moving the
inline i915_trace_irq_get() function from intel_ringbuffer.h to i915_drv.h.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
 drivers/gpu/drm/i915/i915_gem.c         |    7 ++++---
 drivers/gpu/drm/i915/i915_trace.h       |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h |    8 +-------
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3e745bb..2aa897c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3134,4 +3134,11 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	}
 }
 
+static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
+				      struct drm_i915_gem_request *req)
+{
+	if (ring->trace_irq_req == NULL && ring->irq_get(ring))
+		i915_gem_request_assign(&ring->trace_irq_req, req);
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2fab638..30fb486 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2800,10 +2800,11 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 		i915_gem_free_request(request);
 	}
 
-	if (unlikely(ring->trace_irq_seqno &&
-		     i915_seqno_passed(seqno, ring->trace_irq_seqno))) {
+	if (unlikely(ring->trace_irq_req &&
+		     i915_seqno_passed(seqno,
+			 i915_gem_request_get_seqno(ring->trace_irq_req)))) {
 		ring->irq_put(ring);
-		ring->trace_irq_seqno = 0;
+		i915_gem_request_assign(&ring->trace_irq_req, NULL);
 	}
 
 	while (!list_empty(&ring->delayed_free_list)) {
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 2c0327b..2ade958 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -369,7 +369,7 @@ TRACE_EVENT(i915_gem_ring_dispatch,
 			   __entry->ring = ring->id;
 			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   __entry->flags = flags;
-			   i915_trace_irq_get(ring, __entry->seqno);
+			   i915_trace_irq_get(ring, req);
 			   ),
 
 	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 3af7b7c..15ec7c4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -142,7 +142,7 @@ struct  intel_engine_cs {
 
 	unsigned irq_refcount; /* protected by dev_priv->irq_lock */
 	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
-	u32		trace_irq_seqno;
+	struct drm_i915_gem_request *trace_irq_req;
 	bool __must_check (*irq_get)(struct intel_engine_cs *ring);
 	void		(*irq_put)(struct intel_engine_cs *ring);
 
@@ -440,12 +440,6 @@ intel_ring_get_request(struct intel_engine_cs *ring)
 	return ring->outstanding_lazy_request;
 }
 
-static inline void i915_trace_irq_get(struct intel_engine_cs *ring, u32 seqno)
-{
-	if (ring->trace_irq_seqno == 0 && ring->irq_get(ring))
-		ring->trace_irq_seqno = seqno;
-}
-
 /* DRI warts */
 int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 18/28] drm/i915: Convert 'ring_idle()' to use requests not seqnos

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

More seqno value to request structure conversions. Note, this change temporarily
moves the 'get_seqno()' call inside ring_idle() but this will disappear again in
a later patch when i915_seqno_passed() itself is converted.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 729e9a3..0cb996a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2712,18 +2712,19 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-static u32
-ring_last_seqno(struct intel_engine_cs *ring)
+static struct drm_i915_gem_request *
+ring_last_request(struct intel_engine_cs *ring)
 {
 	return list_entry(ring->request_list.prev,
-			  struct drm_i915_gem_request, list)->seqno;
+			  struct drm_i915_gem_request, list);
 }
 
 static bool
-ring_idle(struct intel_engine_cs *ring, u32 seqno)
+ring_idle(struct intel_engine_cs *ring)
 {
 	return (list_empty(&ring->request_list) ||
-		i915_seqno_passed(seqno, ring_last_seqno(ring)));
+		i915_seqno_passed(ring->get_seqno(ring, false),
+			i915_gem_request_get_seqno(ring_last_request(ring))));
 }
 
 static bool
@@ -2943,7 +2944,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
 		acthd = intel_ring_get_active_head(ring);
 
 		if (ring->hangcheck.seqno == seqno) {
-			if (ring_idle(ring, seqno)) {
+			if (ring_idle(ring)) {
 				ring->hangcheck.action = HANGCHECK_IDLE;
 
 				if (waitqueue_active(&ring->irq_queue)) {
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 19/28] drm/i915: Connect requests to rings at creation not submission

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

It makes a lot more sense (and makes future seqno -> request conversion patches
simpler) to fill in the 'ring' field of the request structure at the point of
creation rather than submission. Given that the request structure is assigned by
ring specific code and thus is locked to a ring from the start, there really is
no reason to defer this assignment.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |    1 -
 drivers/gpu/drm/i915/intel_lrc.c        |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 30fb486..49dfb63 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2465,7 +2465,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
 			return ret;
 	}
 
-	request->ring = ring;
 	request->head = request_start;
 	request->tail = request_ring_position;
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b2ec8ab..a583cf3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -805,6 +805,7 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 		return -ENOMEM;
 
 	kref_init(&request->ref);
+	request->ring = ring;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f86275d..f29c31e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2042,6 +2042,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 		return -ENOMEM;
 
 	kref_init(&request->ref);
+	request->ring = ring;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 20/28] drm/i915: Convert 'i915_seqno_passed' calls into 'i915_gem_request_completed'

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Almost everywhere that caled i915_seqno_passed() was really asking 'has the
given seqno popped out of the hardware yet?'. Thus it had to query the current
hardware seqno and then do a signed delta comparison (which copes with wrapping
around zero but not with seqno values more than 2GB apart, although the latter
is unlikely!).

Now that the majority of seqno instances have been replaced with request
structures, it is possible to convert this test to be request based as well.
There is now a 'i915_gem_request_completed()' function which takes a request and
returns true or false as appropriate. Note that this currently just wraps up the
original _passed() test but a later patch in the series will reduce this to
simply returning a cached internal value, i.e.:
  _completed(req) { return req->completed; }'

This checkin converts almost all _seqno_passed() calls. The only one left is in
the semaphore code which still requires seqnos not request structures.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    3 +--
 drivers/gpu/drm/i915/i915_drv.h      |   18 ++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      |   26 +++++++-------------------
 drivers/gpu/drm/i915/i915_irq.c      |    3 +--
 drivers/gpu/drm/i915/intel_display.c |    6 +-----
 5 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d35e6e0..f4d138e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -549,8 +549,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 					   i915_gem_request_get_seqno(work->flip_queued_req),
 					   dev_priv->next_seqno,
 					   work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
-					   i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
-							     i915_gem_request_get_seqno(work->flip_queued_req)));
+					   i915_gem_request_completed(work->flip_queued_req, true));
 			} else
 				seq_printf(m, "Flip not associated with any ring\n");
 			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2aa897c..5ca23dc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2066,6 +2066,12 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
 	*pdst = src;
 }
 
+/*
+ * XXX: i915_gem_request_completed should be here but currently needs the
+ * definition of i915_seqno_passed() which is below. It will be moved in
+ * a later patch when the call to i915_seqno_passed() is obsoleted...
+ */
+
 struct drm_i915_file_private {
 	struct drm_i915_private *dev_priv;
 	struct drm_file *file;
@@ -3134,6 +3140,18 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	}
 }
 
+static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
+					      bool lazy_coherency)
+{
+	u32 seqno;
+
+	BUG_ON(req == NULL);
+
+	seqno = req->ring->get_seqno(req->ring, lazy_coherency);
+
+	return i915_seqno_passed(seqno, req->seqno);
+}
+
 static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
 				      struct drm_i915_gem_request *req)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 49dfb63..3d6894c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1248,8 +1248,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 
 	WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
 
-	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      i915_gem_request_get_seqno(req)))
+	if (i915_gem_request_completed(req, true))
 		return 0;
 
 	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
@@ -1285,8 +1284,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 			break;
 		}
 
-		if (i915_seqno_passed(ring->get_seqno(ring, false),
-				      i915_gem_request_get_seqno(req))) {
+		if (i915_gem_request_completed(req, false)) {
 			ret = 0;
 			break;
 		}
@@ -2335,8 +2333,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj)
 	if (ring == NULL)
 		return;
 
-	if (i915_seqno_passed(ring->get_seqno(ring, true),
-			      i915_gem_request_get_seqno(obj->last_read_req)))
+	if (i915_gem_request_completed(obj->last_read_req, true))
 		i915_gem_object_move_to_inactive(obj);
 }
 
@@ -2596,12 +2593,9 @@ struct drm_i915_gem_request *
 i915_gem_find_active_request(struct intel_engine_cs *ring)
 {
 	struct drm_i915_gem_request *request;
-	u32 completed_seqno;
-
-	completed_seqno = ring->get_seqno(ring, false);
 
 	list_for_each_entry(request, &ring->request_list, list) {
-		if (i915_seqno_passed(completed_seqno, request->seqno))
+		if (i915_gem_request_completed(request, false))
 			continue;
 
 		return request;
@@ -2737,15 +2731,11 @@ void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req)
 void
 i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 {
-	uint32_t seqno;
-
 	if (list_empty(&ring->request_list))
 		return;
 
 	WARN_ON(i915_verify_lists(ring->dev));
 
-	seqno = ring->get_seqno(ring, true);
-
 	/* Move any buffers on the active list that are no longer referenced
 	 * by the ringbuffer to the flushing/inactive lists as appropriate,
 	 * before we free the context associated with the requests.
@@ -2757,8 +2747,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 				      struct drm_i915_gem_object,
 				      ring_list);
 
-		if (!i915_seqno_passed(seqno,
-			     i915_gem_request_get_seqno(obj->last_read_req)))
+		if (!i915_gem_request_completed(obj->last_read_req, true))
 			break;
 
 		i915_gem_object_move_to_inactive(obj);
@@ -2773,7 +2762,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 					   struct drm_i915_gem_request,
 					   list);
 
-		if (!i915_seqno_passed(seqno, request->seqno))
+		if (!i915_gem_request_completed(request, true))
 			break;
 
 		trace_i915_gem_request_retire(request);
@@ -2800,8 +2789,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 	}
 
 	if (unlikely(ring->trace_irq_req &&
-		     i915_seqno_passed(seqno,
-			 i915_gem_request_get_seqno(ring->trace_irq_req)))) {
+		     i915_gem_request_completed(ring->trace_irq_req, true))) {
 		ring->irq_put(ring);
 		i915_gem_request_assign(&ring->trace_irq_req, NULL);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0cb996a..198bbc6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2723,8 +2723,7 @@ static bool
 ring_idle(struct intel_engine_cs *ring)
 {
 	return (list_empty(&ring->request_list) ||
-		i915_seqno_passed(ring->get_seqno(ring, false),
-			i915_gem_request_get_seqno(ring_last_request(ring))));
+		i915_gem_request_completed(ring_last_request(ring), false));
 }
 
 static bool
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88f70db..35202ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9485,11 +9485,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
 
 	if (work->flip_ready_vblank == 0) {
 		if (work->flip_queued_ring) {
-			uint32_t s1 = work->flip_queued_ring->get_seqno(
-					       work->flip_queued_ring, true);
-			uint32_t s2 = i915_gem_request_get_seqno(
-						      work->flip_queued_req);
-			if (!i915_seqno_passed(s1, s2))
+			if (!i915_gem_request_completed(work->flip_queued_req, true))
 				return false;
 
 			i915_gem_request_unreference_irq(work->flip_queued_req);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 21/28] drm/i915: Remove the now redundant 'obj->ring'

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The ring member of the object structure was always updated with the
last_read_seqno member. Thus with the conversion to last_read_req, obj->ring is
now a direct copy of obj->last_read_req->ring. This makes it somewhat redundant
and potentially misleading (especially as there was no comment to explain its
purpose).

This checkin removes the redundant field. Many uses were simply testing for
non-null to see if the object is active on the GPU. Some of these have been
converted to check 'obj->active' instead. Others (where the last_read_req is
about to be used anyway) have been changed to check obj->last_read_req. The rest
simply pull the ring out from the request structure and proceed as before.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |    9 +++++----
 drivers/gpu/drm/i915/i915_drv.h         |    2 --
 drivers/gpu/drm/i915/i915_gem.c         |   32 +++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_context.c |    3 ++-
 drivers/gpu/drm/i915/i915_gpu_error.c   |    3 ++-
 drivers/gpu/drm/i915/intel_display.c    |    7 ++++---
 6 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f4d138e..8cbf32c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -168,8 +168,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		*t = '\0';
 		seq_printf(m, " (%s mappable)", s);
 	}
-	if (obj->ring != NULL)
-		seq_printf(m, " (%s)", obj->ring->name);
+	if (obj->last_read_req != NULL)
+		seq_printf(m, " (%s)",
+			   i915_gem_request_get_ring(obj->last_read_req)->name);
 	if (obj->frontbuffer_bits)
 		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
 }
@@ -336,7 +337,7 @@ static int per_file_stats(int id, void *ptr, void *data)
 			if (ppgtt->file_priv != stats->file_priv)
 				continue;
 
-			if (obj->ring) /* XXX per-vma statistic */
+			if (obj->active) /* XXX per-vma statistic */
 				stats->active += obj->base.size;
 			else
 				stats->inactive += obj->base.size;
@@ -346,7 +347,7 @@ static int per_file_stats(int id, void *ptr, void *data)
 	} else {
 		if (i915_gem_obj_ggtt_bound(obj)) {
 			stats->global += obj->base.size;
-			if (obj->ring)
+			if (obj->active)
 				stats->active += obj->base.size;
 			else
 				stats->inactive += obj->base.size;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5ca23dc..918d61b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1935,8 +1935,6 @@ struct drm_i915_gem_object {
 	void *dma_buf_vmapping;
 	int vmapping_count;
 
-	struct intel_engine_cs *ring;
-
 	/** Breadcrumb of last rendering to the buffer. */
 	struct drm_i915_gem_request *last_read_req;
 	struct drm_i915_gem_request *last_write_req;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3d6894c..1d07a14 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2265,14 +2265,18 @@ static void
 i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
 			       struct intel_engine_cs *ring)
 {
-	struct drm_i915_gem_request *req = intel_ring_get_request(ring);
+	struct drm_i915_gem_request *req;
+	struct intel_engine_cs *old_ring;
 
 	BUG_ON(ring == NULL);
-	if (obj->ring != ring && obj->last_write_req) {
+
+	req = intel_ring_get_request(ring);
+	old_ring = i915_gem_request_get_ring(obj->last_read_req);
+
+	if (old_ring != ring && obj->last_write_req) {
 		/* Keep the request relative to the current ring */
 		i915_gem_request_assign(&obj->last_write_req, req);
 	}
-	obj->ring = ring;
 
 	/* Add a reference if we're newly entering the active list. */
 	if (!obj->active) {
@@ -2311,7 +2315,6 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	intel_fb_obj_flush(obj, true);
 
 	list_del_init(&obj->ring_list);
-	obj->ring = NULL;
 
 	i915_gem_request_assign(&obj->last_read_req, NULL);
 	i915_gem_request_assign(&obj->last_write_req, NULL);
@@ -2328,9 +2331,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 static void
 i915_gem_object_retire(struct drm_i915_gem_object *obj)
 {
-	struct intel_engine_cs *ring = obj->ring;
-
-	if (ring == NULL)
+	if (obj->last_read_req == NULL)
 		return;
 
 	if (i915_gem_request_completed(obj->last_read_req, true))
@@ -2873,14 +2874,17 @@ i915_gem_idle_work_handler(struct work_struct *work)
 static int
 i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 {
+	struct intel_engine_cs *ring;
 	int ret;
 
 	if (obj->active) {
+		ring = i915_gem_request_get_ring(obj->last_read_req);
+
 		ret = i915_gem_check_olr(obj->last_read_req);
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests_ring(obj->ring);
+		i915_gem_retire_requests_ring(ring);
 	}
 
 	return 0;
@@ -2983,10 +2987,12 @@ int
 i915_gem_object_sync(struct drm_i915_gem_object *obj,
 		     struct intel_engine_cs *to)
 {
-	struct intel_engine_cs *from = obj->ring;
+	struct intel_engine_cs *from;
 	u32 seqno;
 	int ret, idx;
 
+	from = i915_gem_request_get_ring(obj->last_read_req);
+
 	if (from == NULL || to == from)
 		return 0;
 
@@ -3945,7 +3951,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	bool was_pin_display;
 	int ret;
 
-	if (pipelined != obj->ring) {
+	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
 		ret = i915_gem_object_sync(obj, pipelined);
 		if (ret)
 			return ret;
@@ -4393,9 +4399,11 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	ret = i915_gem_object_flush_active(obj);
 
 	args->busy = obj->active;
-	if (obj->ring) {
+	if (obj->last_read_req) {
+		struct intel_engine_cs *ring;
 		BUILD_BUG_ON(I915_NUM_RINGS > 16);
-		args->busy |= intel_ring_flag(obj->ring) << 16;
+		ring = i915_gem_request_get_ring(obj->last_read_req);
+		args->busy |= intel_ring_flag(ring) << 16;
 	}
 
 	drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index d17ff43..3c3a9ff 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -619,7 +619,8 @@ static int do_switch(struct intel_engine_cs *ring,
 		 * swapped, but there is no way to do that yet.
 		 */
 		from->legacy_hw_ctx.rcs_state->dirty = 1;
-		BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring);
+		BUG_ON(i915_gem_request_get_ring(
+			from->legacy_hw_ctx.rcs_state->last_read_req) != ring);
 
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 99433d9..415abbe 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -683,7 +683,8 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 	err->dirty = obj->dirty;
 	err->purgeable = obj->madv != I915_MADV_WILLNEED;
 	err->userptr = obj->userptr.mm != NULL;
-	err->ring = obj->ring ? obj->ring->id : -1;
+	err->ring = obj->last_read_req ?
+			i915_gem_request_get_ring(obj->last_read_req)->id : -1;
 	err->cache_level = obj->cache_level;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 35202ed..00cd579 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9387,7 +9387,7 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
 	else if (i915.enable_execlists)
 		return true;
 	else
-		return ring != obj->ring;
+		return ring != i915_gem_request_get_ring(obj->last_read_req);
 }
 
 static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
@@ -9635,7 +9635,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	} else if (IS_IVYBRIDGE(dev)) {
 		ring = &dev_priv->ring[BCS];
 	} else if (INTEL_INFO(dev)->gen >= 7) {
-		ring = obj->ring;
+		ring = i915_gem_request_get_ring(obj->last_read_req);
 		if (ring == NULL || ring->id != RCS)
 			ring = &dev_priv->ring[BCS];
 	} else {
@@ -9657,7 +9657,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 		i915_gem_request_assign(&work->flip_queued_req,
 					obj->last_write_req);
-		work->flip_queued_ring = obj->ring;
+		work->flip_queued_ring =
+				i915_gem_request_get_ring(obj->last_write_req);
 	} else {
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
 						   page_flip_flags);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 22/28] drm/i915: Cache request completion status

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Continuing the removal of seqno based operations - updated the request
completion query to not simply chain on to i915_seqno_passed(). Instead, it now
returns a pre-cached completion flag in the fast case. In the slow case it reads
the hardware seqno and, only if it has moved on since the last scan, looks
through the outstanding request list to see which requests can be marked as
completed.

Later in the patch series, this will be optimised further by only doing the
completion scan when an interrupt is raised to say that a request has actually
completed on the hardware. Thus the call to test the completion status of an
arbitrary request simply becomes 'return req->completed'.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   32 +++++++++++++++----------------
 drivers/gpu/drm/i915/i915_gem.c         |   24 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c        |    1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    3 +++
 5 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 918d61b..8531e0f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1990,6 +1990,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
 struct drm_i915_gem_request {
 	struct kref ref;
 
+	/** Is this request known to be complete? */
+	bool complete;
+
 	/** On Which ring this request was generated */
 	struct intel_engine_cs *ring;
 
@@ -2024,6 +2027,8 @@ struct drm_i915_gem_request {
 };
 
 void i915_gem_request_free(struct kref *req_ref);
+void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
+				     bool lazy_coherency);
 
 static inline uint32_t
 i915_gem_request_get_seqno(struct drm_i915_gem_request *req)
@@ -2064,11 +2069,16 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
 	*pdst = src;
 }
 
-/*
- * XXX: i915_gem_request_completed should be here but currently needs the
- * definition of i915_seqno_passed() which is below. It will be moved in
- * a later patch when the call to i915_seqno_passed() is obsoleted...
- */
+static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
+					      bool lazy_coherency)
+{
+	if (req->complete)
+		return true;
+
+	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
+
+	return req->complete;
+}
 
 struct drm_i915_file_private {
 	struct drm_i915_private *dev_priv;
@@ -3138,18 +3148,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	}
 }
 
-static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
-					      bool lazy_coherency)
-{
-	u32 seqno;
-
-	BUG_ON(req == NULL);
-
-	seqno = req->ring->get_seqno(req->ring, lazy_coherency);
-
-	return i915_seqno_passed(seqno, req->seqno);
-}
-
 static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
 				      struct drm_i915_gem_request *req)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1d07a14..b6c75b0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2726,6 +2726,30 @@ void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req)
 	spin_unlock_irqrestore(&ring->reqlist_lock, flags);
 }
 
+void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
+				     bool lazy_coherency)
+{
+	struct drm_i915_gem_request *req;
+	u32 seqno;
+
+	seqno = ring->get_seqno(ring, lazy_coherency);
+	if (!seqno)
+		return;
+
+	if (seqno == ring->last_read_seqno)
+		return;
+
+	list_for_each_entry(req, &ring->request_list, list) {
+		if (req->complete)
+			continue;
+
+		if (i915_seqno_passed(seqno, req->seqno))
+			req->complete = true;
+	}
+
+	ring->last_read_seqno = seqno;
+}
+
 /**
  * This function clears the request list as sequence numbers are passed.
  */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a583cf3..960cb9f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -806,6 +806,7 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 
 	kref_init(&request->ref);
 	request->ring = ring;
+	request->complete = false;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f29c31e..5cbc5c2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2043,6 +2043,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 
 	kref_init(&request->ref);
 	request->ring = ring;
+	request->complete = false;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
@@ -2135,6 +2136,7 @@ void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno)
 			I915_WRITE(RING_SYNC_2(ring->mmio_base), 0);
 	}
 
+	ring->last_read_seqno = 0;
 	ring->set_seqno(ring, seqno);
 	ring->hangcheck.seqno = seqno;
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 15ec7c4..4957c35 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -272,6 +272,9 @@ struct  intel_engine_cs {
 	bool gpu_caches_dirty;
 	bool fbc_dirty;
 
+	/* For optimising request completion events */
+	u32 last_read_seqno;
+
 	wait_queue_head_t irq_queue;
 
 	struct intel_context *default_context;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 23/28] drm/i915: Zero fill the request structure

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

There is a general theory that kzmalloc is better/safer than kmalloc, especially
for interesting data structures. This change updates the request structure
allocation to be zero filled. That also means it is no longer necessary to
explicitly clear the 'complete' field.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    3 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 960cb9f..802388d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -800,13 +800,12 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 	if (ring->outstanding_lazy_request)
 		return 0;
 
-	request = kmalloc(sizeof(*request), GFP_KERNEL);
+	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
 
 	kref_init(&request->ref);
 	request->ring = ring;
-	request->complete = false;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5cbc5c2..a898440 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2037,13 +2037,12 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 	if (ring->outstanding_lazy_request)
 		return 0;
 
-	request = kmalloc(sizeof(*request), GFP_KERNEL);
+	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
 
 	kref_init(&request->ref);
 	request->ring = ring;
-	request->complete = false;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 24/28] drm/i915: Spinlock protection for request list

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The completion status for all entries in the request list is updated on demand.
This occurs whenever the code queries the completion status of a given request
and a new seqno value has popped out of the hardware. Unfortuntately, not all
such queries are done with the driver mutex lock held. Therefore there is a race
condition between the query processing and the retired request removal code
which can result in a dodgy pointer dereference.

The solution is to spinlock around those two points - the actual list entry
removal and the potentially asynchronous query. This ensures that the query will
never see a node that is in the process of being destroyed.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b6c75b0..edf712b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2573,7 +2573,12 @@ static void i915_set_reset_status(struct drm_i915_private *dev_priv,
 
 static void i915_gem_free_request(struct drm_i915_gem_request *request)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&request->ring->reqlist_lock, flags);
 	list_del(&request->list);
+	spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
+
 	i915_gem_request_remove_from_client(request);
 
 	i915_gem_request_unreference(request);
@@ -2730,6 +2735,7 @@ void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
 				     bool lazy_coherency)
 {
 	struct drm_i915_gem_request *req;
+	unsigned long flags;
 	u32 seqno;
 
 	seqno = ring->get_seqno(ring, lazy_coherency);
@@ -2739,6 +2745,7 @@ void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
 	if (seqno == ring->last_read_seqno)
 		return;
 
+	spin_lock_irqsave(&ring->reqlist_lock, flags);
 	list_for_each_entry(req, &ring->request_list, list) {
 		if (req->complete)
 			continue;
@@ -2746,6 +2753,7 @@ void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
 		if (i915_seqno_passed(seqno, req->seqno))
 			req->complete = true;
 	}
+	spin_unlock_irqrestore(&ring->reqlist_lock, flags);
 
 	ring->last_read_seqno = seqno;
 }
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 25/28] drm/i915: Interrupt driven request completion

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Added a hook to the ring noftification code to process request completion. This
means that there is no longer a need to explicitly process request completions
every time a request object is tested. Instead, the test code simply becomes
'return req->completed'. Obviously, this only works if ring interrupts are
enabled, however, this is already the case for the duration of __wait_request()
which is the point where the driver really needs to know.

To prevent stale requests floating around indefinitely, the retire work handler
also now performs a completion check periodically.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    5 -----
 drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
 drivers/gpu/drm/i915/i915_irq.c |    2 ++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8531e0f..66219b5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2072,11 +2072,6 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
 static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
 					      bool lazy_coherency)
 {
-	if (req->complete)
-		return true;
-
-	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
-
 	return req->complete;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index edf712b..039dbb8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1264,6 +1264,11 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 	if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring)))
 		return -ENODEV;
 
+	/* Completion status should be interrupt driven but it is possible
+	 * the request popped out before the interrupt was enabled. So do an
+	 * explicit check now... */
+	i915_gem_complete_requests_ring(req->ring, false);
+
 	/* Record current time in case interrupted by signal, or wedged */
 	trace_i915_gem_request_wait_begin(req);
 	before = ktime_get_raw_ns();
@@ -2487,6 +2492,10 @@ int __i915_add_request(struct intel_engine_cs *ring,
 	list_add_tail(&request->list, &ring->request_list);
 	request->file_priv = NULL;
 
+	/* Avoid race condition where the request completes before it has
+	 * been added to the list. */
+	ring->last_read_seqno = 0;
+
 	if (file) {
 		struct drm_i915_file_private *file_priv = file->driver_priv;
 
@@ -2858,6 +2867,7 @@ i915_gem_retire_requests(struct drm_device *dev)
 	int i;
 
 	for_each_ring(ring, dev_priv, i) {
+		i915_gem_complete_requests_ring(ring, false);
 		i915_gem_retire_requests_ring(ring);
 		idle &= list_empty(&ring->request_list);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 198bbc6..4f63966 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -987,6 +987,8 @@ static void notify_ring(struct drm_device *dev,
 
 	trace_i915_gem_request_complete(ring);
 
+	i915_gem_complete_requests_ring(ring, false);
+
 	wake_up_all(&ring->irq_queue);
 	i915_queue_hangcheck(dev);
 }
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 26/28] drm/i915: Remove obsolete parameter to i915_gem_request_completed()

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The request completion test no longer chains on to the request completion
processing code. Thus it no longer needs to pass a 'lazy coherency' flag through
to the seqno query call. Hence that parameter can be removed.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    2 +-
 drivers/gpu/drm/i915/i915_drv.h      |    3 +--
 drivers/gpu/drm/i915/i915_gem.c      |   14 +++++++-------
 drivers/gpu/drm/i915/i915_irq.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 5 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8cbf32c..0d6af1c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -550,7 +550,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 					   i915_gem_request_get_seqno(work->flip_queued_req),
 					   dev_priv->next_seqno,
 					   work->flip_queued_ring->get_seqno(work->flip_queued_ring, true),
-					   i915_gem_request_completed(work->flip_queued_req, true));
+					   i915_gem_request_completed(work->flip_queued_req));
 			} else
 				seq_printf(m, "Flip not associated with any ring\n");
 			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 66219b5..b124c86 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2069,8 +2069,7 @@ static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
 	*pdst = src;
 }
 
-static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
-					      bool lazy_coherency)
+static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req)
 {
 	return req->complete;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 039dbb8..fec8ab7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1248,7 +1248,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 
 	WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
 
-	if (i915_gem_request_completed(req, true))
+	if (i915_gem_request_completed(req))
 		return 0;
 
 	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
@@ -1289,7 +1289,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 			break;
 		}
 
-		if (i915_gem_request_completed(req, false)) {
+		if (i915_gem_request_completed(req)) {
 			ret = 0;
 			break;
 		}
@@ -2339,7 +2339,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj)
 	if (obj->last_read_req == NULL)
 		return;
 
-	if (i915_gem_request_completed(obj->last_read_req, true))
+	if (i915_gem_request_completed(obj->last_read_req))
 		i915_gem_object_move_to_inactive(obj);
 }
 
@@ -2610,7 +2610,7 @@ i915_gem_find_active_request(struct intel_engine_cs *ring)
 	struct drm_i915_gem_request *request;
 
 	list_for_each_entry(request, &ring->request_list, list) {
-		if (i915_gem_request_completed(request, false))
+		if (i915_gem_request_completed(request))
 			continue;
 
 		return request;
@@ -2789,7 +2789,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 				      struct drm_i915_gem_object,
 				      ring_list);
 
-		if (!i915_gem_request_completed(obj->last_read_req, true))
+		if (!i915_gem_request_completed(obj->last_read_req))
 			break;
 
 		i915_gem_object_move_to_inactive(obj);
@@ -2804,7 +2804,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 					   struct drm_i915_gem_request,
 					   list);
 
-		if (!i915_gem_request_completed(request, true))
+		if (!i915_gem_request_completed(request))
 			break;
 
 		trace_i915_gem_request_retire(request);
@@ -2831,7 +2831,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 	}
 
 	if (unlikely(ring->trace_irq_req &&
-		     i915_gem_request_completed(ring->trace_irq_req, true))) {
+		     i915_gem_request_completed(ring->trace_irq_req))) {
 		ring->irq_put(ring);
 		i915_gem_request_assign(&ring->trace_irq_req, NULL);
 	}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4f63966..415f890 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2725,7 +2725,7 @@ static bool
 ring_idle(struct intel_engine_cs *ring)
 {
 	return (list_empty(&ring->request_list) ||
-		i915_gem_request_completed(ring_last_request(ring), false));
+		i915_gem_request_completed(ring_last_request(ring)));
 }
 
 static bool
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00cd579..f0041391 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9485,7 +9485,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
 
 	if (work->flip_ready_vblank == 0) {
 		if (work->flip_queued_ring) {
-			if (!i915_gem_request_completed(work->flip_queued_req, true))
+			if (!i915_gem_request_completed(work->flip_queued_req))
 				return false;
 
 			i915_gem_request_unreference_irq(work->flip_queued_req);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 27/28] drm/i915: Add unique id to the request structure for debugging

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

For debugging purposes, it is useful to be able to uniquely identify a given
request structure as it works its way through the system. This becomes
especially tricky once the seqno value is lazily allocated as then the request
has nothing but its pointer to identify it for much of its life.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |    4 ++++
 drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 3 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b124c86..8e7d9b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1781,6 +1781,8 @@ struct drm_i915_private {
 		void (*stop_ring)(struct intel_engine_cs *ring);
 	} gt;
 
+	uint32_t request_uniq;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
@@ -2024,6 +2026,8 @@ struct drm_i915_gem_request {
 	/** deferred free list for dereferencing from IRQ context */
 	struct list_head delay_free_list;
 	uint32_t delay_free_count;
+
+	uint32_t uniq;
 };
 
 void i915_gem_request_free(struct kref *req_ref);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 802388d..73ac3ce 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -795,6 +795,7 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 				      struct intel_context *ctx)
 {
 	struct drm_i915_gem_request *request;
+	struct drm_i915_private *dev_private = ring->dev->dev_private;
 	int ret;
 
 	if (ring->outstanding_lazy_request)
@@ -806,6 +807,7 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
 
 	kref_init(&request->ref);
 	request->ring = ring;
+	request->uniq = dev_private->request_uniq++;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a898440..0c24d4e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2033,6 +2033,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 {
 	int ret;
 	struct drm_i915_gem_request *request;
+	struct drm_i915_private *dev_private = ring->dev->dev_private;
 
 	if (ring->outstanding_lazy_request)
 		return 0;
@@ -2043,6 +2044,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
 
 	kref_init(&request->ref);
 	request->ring = ring;
+	request->uniq = dev_private->request_uniq++;
 
 	ret = i915_gem_get_seqno(ring->dev, &request->seqno);
 	if (ret) {
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 28/28] drm/i915: Additional request structure tracing

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Added the request structure's 'uniq' identifier to the trace information. Also
renamed the '_complete' trace event to '_notify' as it actually happens in the
IRQ 'notify_ring()' function. Additionally there is now a new '_complete' trace
event which occurs when a request structure is marked as complete.

v2: New patch added to series.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c   |    4 +++-
 drivers/gpu/drm/i915/i915_irq.c   |    2 +-
 drivers/gpu/drm/i915/i915_trace.h |   22 ++++++++++++++++------
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fec8ab7..c35f526 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2759,8 +2759,10 @@ void i915_gem_complete_requests_ring(struct intel_engine_cs *ring,
 		if (req->complete)
 			continue;
 
-		if (i915_seqno_passed(seqno, req->seqno))
+		if (i915_seqno_passed(seqno, req->seqno)) {
 			req->complete = true;
+			trace_i915_gem_request_complete(req);
+		}
 	}
 	spin_unlock_irqrestore(&ring->reqlist_lock, flags);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 415f890..868beb2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -985,7 +985,7 @@ static void notify_ring(struct drm_device *dev,
 	if (!intel_ring_initialized(ring))
 		return;
 
-	trace_i915_gem_request_complete(ring);
+	trace_i915_gem_request_notify(ring);
 
 	i915_gem_complete_requests_ring(ring, false);
 
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 2ade958..6058a01 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -406,6 +406,7 @@ DECLARE_EVENT_CLASS(i915_gem_request,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, ring)
+			     __field(u32, uniq)
 			     __field(u32, seqno)
 			     ),
 
@@ -414,11 +415,13 @@ DECLARE_EVENT_CLASS(i915_gem_request,
 						i915_gem_request_get_ring(req);
 			   __entry->dev = ring->dev->primary->index;
 			   __entry->ring = ring->id;
+			   __entry->uniq = req ? req->uniq : 0;
 			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   ),
 
-	    TP_printk("dev=%u, ring=%u, seqno=%u",
-		      __entry->dev, __entry->ring, __entry->seqno)
+	    TP_printk("dev=%u, ring=%u, uniq=%u, seqno=%u",
+		      __entry->dev, __entry->ring, __entry->uniq,
+		      __entry->seqno)
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
@@ -426,7 +429,7 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
 	    TP_ARGS(req)
 );
 
-TRACE_EVENT(i915_gem_request_complete,
+TRACE_EVENT(i915_gem_request_notify,
 	    TP_PROTO(struct intel_engine_cs *ring),
 	    TP_ARGS(ring),
 
@@ -451,6 +454,11 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_retire,
 	    TP_ARGS(req)
 );
 
+DEFINE_EVENT(i915_gem_request, i915_gem_request_complete,
+	    TP_PROTO(struct drm_i915_gem_request *req),
+	    TP_ARGS(req)
+);
+
 TRACE_EVENT(i915_gem_request_wait_begin,
 	    TP_PROTO(struct drm_i915_gem_request *req),
 	    TP_ARGS(req),
@@ -458,6 +466,7 @@ TRACE_EVENT(i915_gem_request_wait_begin,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, ring)
+			     __field(u32, uniq)
 			     __field(u32, seqno)
 			     __field(bool, blocking)
 			     ),
@@ -473,14 +482,15 @@ TRACE_EVENT(i915_gem_request_wait_begin,
 						i915_gem_request_get_ring(req);
 			   __entry->dev = ring->dev->primary->index;
 			   __entry->ring = ring->id;
+			   __entry->uniq = req ? req->uniq : 0;
 			   __entry->seqno = i915_gem_request_get_seqno(req);
 			   __entry->blocking =
 				     mutex_is_locked(&ring->dev->struct_mutex);
 			   ),
 
-	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
-		      __entry->dev, __entry->ring, __entry->seqno,
-		      __entry->blocking ?  "yes (NB)" : "no")
+	    TP_printk("dev=%u, ring=%u, uniq=%u, seqno=%u, blocking=%s",
+		      __entry->dev, __entry->ring, __entry->uniq,
+		      __entry->seqno, __entry->blocking ?  "yes (NB)" : "no")
 );
 
 DEFINE_EVENT(i915_gem_request, i915_gem_request_wait_end,
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 28/28] drm/i915: Additional request structure

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/291->290/291
PNV: pass/total=351/356->356/356
ILK: pass/total=371/372->365/372
IVB: pass/total=545/546->545/546
SNB: pass/total=424/425->423/425
HSW: pass/total=579/579->576/579
BDW: pass/total=434/435->434/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
PNV: Intel_gpu_tools, igt_gen3_mixed_blits, CRASH(1, M23) -> PASS(4, M7)
PNV: Intel_gpu_tools, igt_gen3_render_linear_blits, CRASH(1, M23) -> PASS(1, M7)
PNV: Intel_gpu_tools, igt_gen3_render_mixed_blits, CRASH(1, M23) -> PASS(1, M7)
PNV: Intel_gpu_tools, igt_gen3_render_tiledx_blits, CRASH(2, M23)DMESG_WARN(1, M23)NRUN(1, M23) -> PASS(1, M7)
PNV: Intel_gpu_tools, igt_gen3_render_tiledy_blits, CRASH(1, M23) -> PASS(1, M7)
ILK: Intel_gpu_tools, igt_drv_missed_irq_hang, PASS(1, M6) -> DMESG_FAIL(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_bcs-flip-vs-modeset-interruptible, PASS(1, M6) -> DMESG_WARN(2, M26)PASS(2, M26)
ILK: Intel_gpu_tools, igt_kms_flip_bcs-wf_vblank-vs-dpms-interruptible, PASS(1, M6) -> DMESG_WARN(2, M26)PASS(2, M26)
ILK: Intel_gpu_tools, igt_kms_flip_blocking-wf_vblank, PASS(1, M6) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-wf_vblank-interruptible, PASS(1, M6) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_wf_vblank-vs-modeset-interruptible, PASS(1, M6) -> DMESG_WARN(2, M26)PASS(2, M26)
SNB: Intel_gpu_tools, igt_pm_rpm_gem-idle, PASS(1, M22) -> NO_RESULT(1, M22)PASS(3, M22)
HSW: Intel_gpu_tools, igt_drv_missed_irq_hang, PASS(1, M20) -> DMESG_FAIL(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_gem_bad_reloc_negative-reloc-lut, PASS(1, M20) -> NSPT(1, M19)PASS(3, M19)
HSW: Intel_gpu_tools, igt_pm_rpm_gem-idle, PASS(1, M20) -> NO_RESULT(1, M19)PASS(3, M19)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of John.C.Harrison@Intel.com
> Sent: Friday, November 14, 2014 12:19 PM
> To: Intel-GFX@Lists.FreeDesktop.Org
> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
> deference facility
> 
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The next patches in the series convert some display related seqno usage to
> request structure usage. However, the request dereference introduced
> must be done from interrupt context. As the dereference potentially
> involves freeing the request structure and thus calling lots of non-interrupt
> friendly code, this poses a problem.
> 
> The solution is to add an IRQ friendly version of the dereference function. All
> this does is to add the request structure to a 'delayed free' list and return.
> The retire code, which is run periodically, then processes this list and does
> the actual dereferencing of the request structures.
> 
> v2: Added a count to allow for multiple IRQ dereferences of the same
> request at a time.
> 
> For: VIZ-4377
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
>  drivers/gpu/drm/i915/i915_gem.c         |   33
> +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
>  5 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
>  	struct drm_i915_file_private *file_priv;
>  	/** file_priv list entry for this request */
>  	struct list_head client_list;
> +
> +	/** deferred free list for dereferencing from IRQ context */
> +	struct list_head delay_free_list;
> +	uint32_t delay_free_count;
>  };
> 
>  void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12
> @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
> static inline void  i915_gem_request_unreference(struct
> drm_i915_gem_request *req)  {
> +	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>  	kref_put(&req->ref, i915_gem_request_free);  }
> 
> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request
> +*req);
> +
>  static inline void i915_gem_request_assign(struct drm_i915_gem_request
> **pdst,
>  					   struct drm_i915_gem_request *src)
> { diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
>  	i915_gem_restore_fences(dev);
>  }
> 
> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request
> *req)
> +{
> +	struct intel_engine_cs *ring = req->ring;
> +	unsigned long flags;
> +
> +	/* Need to add the req to a deferred dereference list to be
> processed
> +	 * outside of interrupt time */
> +	spin_lock_irqsave(&ring->reqlist_lock, flags);
> +	if (req->delay_free_count++ == 0)
> +		list_add_tail(&req->delay_free_list, &ring-
> >delayed_free_list);
> +	spin_unlock_irqrestore(&ring->reqlist_lock, flags); }
> +
>  /**
>   * This function clears the request list as sequence numbers are passed.
>   */
> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
> intel_engine_cs *ring)
>  		ring->trace_irq_seqno = 0;
>  	}
> 
> +	while (!list_empty(&ring->delayed_free_list)) {
> +		struct drm_i915_gem_request *request;
> +		unsigned long flags;
> +		uint32_t count;
> +
> +		request = list_first_entry(&ring->delayed_free_list,
> +					   struct drm_i915_gem_request,
> +					   delay_free_list);
> +
> +		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
> +		list_del(&request->delay_free_list);
> +		count = request->delay_free_count;
> +		request->delay_free_count = 0;
> +		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
> +
> +		while (count-- > 0)
> +			i915_gem_request_unreference(request);
> +	}
> +
>  	WARN_ON(i915_verify_lists(ring->dev));
>  }
> 
> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
> +	INIT_LIST_HEAD(&ring->delayed_free_list);

Same comment as before - multiple init points for this list.

Thomas.

>  }
> 
>  void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index eba0acd..db8efaa 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev,
> struct intel_engine_cs *rin
>  	ring->dev = dev;
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
> +	spin_lock_init(&ring->reqlist_lock);
>  	init_waitqueue_head(&ring->irq_queue);
> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> 
>  	INIT_LIST_HEAD(&ring->execlist_queue);
>  	spin_lock_init(&ring->execlist_lock);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 74c48ed..4338132 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device
> *dev,
>  	ring->dev = dev;
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
> +	spin_lock_init(&ring->reqlist_lock);
> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>  	INIT_LIST_HEAD(&ring->execlist_queue);
>  	ringbuf->size = 32 * PAGE_SIZE;
>  	ringbuf->ring = ring;
> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device
> *dev, u64 start, u32 size)
>  	ring->dev = dev;
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> 
>  	ringbuf->size = size;
>  	ringbuf->effective_size = ringbuf->size; diff --git
> a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 824f5884..3af7b7c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -262,6 +262,8 @@ struct  intel_engine_cs {
>  	 * outstanding.
>  	 */
>  	struct list_head request_list;
> +	spinlock_t reqlist_lock;
> +	struct list_head delayed_free_list;
> 
>  	/**
>  	 * Do we have some not yet emitted requests outstanding?
> --
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 25/28] drm/i915: Interrupt driven request completion

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of John.C.Harrison@Intel.com
> Sent: Friday, November 14, 2014 12:19 PM
> To: Intel-GFX@Lists.FreeDesktop.Org
> Subject: [Intel-gfx] [PATCH v2 25/28] drm/i915: Interrupt driven request
> completion
> 
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Added a hook to the ring noftification code to process request completion.
> This means that there is no longer a need to explicitly process request
> completions every time a request object is tested. Instead, the test code
> simply becomes 'return req->completed'. Obviously, this only works if ring
> interrupts are enabled, however, this is already the case for the duration of
> __wait_request() which is the point where the driver really needs to know.
> 
> To prevent stale requests floating around indefinitely, the retire work
> handler also now performs a completion check periodically.
> 
> For: VIZ-4377
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    5 -----
>  drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
>  drivers/gpu/drm/i915/i915_irq.c |    2 ++
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 8531e0f..66219b5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2072,11 +2072,6 @@ static inline void i915_gem_request_assign(struct
> drm_i915_gem_request **pdst,  static inline bool
> i915_gem_request_completed(struct drm_i915_gem_request *req,
>  					      bool lazy_coherency)
>  {
> -	if (req->complete)
> -		return true;
> -
> -	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
> -
>  	return req->complete;
>  }
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index edf712b..039dbb8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1264,6 +1264,11 @@ int __i915_wait_request(struct
> drm_i915_gem_request *req,
>  	if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring)))
>  		return -ENODEV;
> 
> +	/* Completion status should be interrupt driven but it is possible
> +	 * the request popped out before the interrupt was enabled. So do
> an
> +	 * explicit check now... */
> +	i915_gem_complete_requests_ring(req->ring, false);
> +
>  	/* Record current time in case interrupted by signal, or wedged */
>  	trace_i915_gem_request_wait_begin(req);
>  	before = ktime_get_raw_ns();
> @@ -2487,6 +2492,10 @@ int __i915_add_request(struct intel_engine_cs
> *ring,
>  	list_add_tail(&request->list, &ring->request_list);
>  	request->file_priv = NULL;
> 
> +	/* Avoid race condition where the request completes before it has
> +	 * been added to the list. */
> +	ring->last_read_seqno = 0;
> +
>  	if (file) {
>  		struct drm_i915_file_private *file_priv = file->driver_priv;
> 
> @@ -2858,6 +2867,7 @@ i915_gem_retire_requests(struct drm_device
> *dev)
>  	int i;
> 
>  	for_each_ring(ring, dev_priv, i) {
> +		i915_gem_complete_requests_ring(ring, false);
>  		i915_gem_retire_requests_ring(ring);
>  		idle &= list_empty(&ring->request_list);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c index 198bbc6..4f63966 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -987,6 +987,8 @@ static void notify_ring(struct drm_device *dev,
> 
>  	trace_i915_gem_request_complete(ring);
> 
> +	i915_gem_complete_requests_ring(ring, false);
I915_gem_complete_requests_ring takes the request list lock and iterates over the entire list.  Are you sure this is safe to do during the interrupt handler?

Thomas.

> +
>  	wake_up_all(&ring->irq_queue);
>  	i915_queue_hangcheck(dev);
>  }
> --
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[John Harrison]

On 18/11/2014 09:34, Daniel, Thomas wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of John.C.Harrison@Intel.com
>> Sent: Friday, November 14, 2014 12:19 PM
>> To: Intel-GFX@Lists.FreeDesktop.Org
>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
>> deference facility
>>
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The next patches in the series convert some display related seqno usage to
>> request structure usage. However, the request dereference introduced
>> must be done from interrupt context. As the dereference potentially
>> involves freeing the request structure and thus calling lots of non-interrupt
>> friendly code, this poses a problem.
>>
>> The solution is to add an IRQ friendly version of the dereference function. All
>> this does is to add the request structure to a 'delayed free' list and return.
>> The retire code, which is run periodically, then processes this list and does
>> the actual dereferencing of the request structures.
>>
>> v2: Added a count to allow for multiple IRQ dereferences of the same
>> request at a time.
>>
>> For: VIZ-4377
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
>>   drivers/gpu/drm/i915/i915_gem.c         |   33
>> +++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
>>   5 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
>>   	struct drm_i915_file_private *file_priv;
>>   	/** file_priv list entry for this request */
>>   	struct list_head client_list;
>> +
>> +	/** deferred free list for dereferencing from IRQ context */
>> +	struct list_head delay_free_list;
>> +	uint32_t delay_free_count;
>>   };
>>
>>   void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12
>> @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
>> static inline void  i915_gem_request_unreference(struct
>> drm_i915_gem_request *req)  {
>> +	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>>   	kref_put(&req->ref, i915_gem_request_free);  }
>>
>> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request
>> +*req);
>> +
>>   static inline void i915_gem_request_assign(struct drm_i915_gem_request
>> **pdst,
>>   					   struct drm_i915_gem_request *src)
>> { diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
>>   	i915_gem_restore_fences(dev);
>>   }
>>
>> +void i915_gem_request_unreference_irq(struct drm_i915_gem_request
>> *req)
>> +{
>> +	struct intel_engine_cs *ring = req->ring;
>> +	unsigned long flags;
>> +
>> +	/* Need to add the req to a deferred dereference list to be
>> processed
>> +	 * outside of interrupt time */
>> +	spin_lock_irqsave(&ring->reqlist_lock, flags);
>> +	if (req->delay_free_count++ == 0)
>> +		list_add_tail(&req->delay_free_list, &ring-
>>> delayed_free_list);
>> +	spin_unlock_irqrestore(&ring->reqlist_lock, flags); }
>> +
>>   /**
>>    * This function clears the request list as sequence numbers are passed.
>>    */
>> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
>> intel_engine_cs *ring)
>>   		ring->trace_irq_seqno = 0;
>>   	}
>>
>> +	while (!list_empty(&ring->delayed_free_list)) {
>> +		struct drm_i915_gem_request *request;
>> +		unsigned long flags;
>> +		uint32_t count;
>> +
>> +		request = list_first_entry(&ring->delayed_free_list,
>> +					   struct drm_i915_gem_request,
>> +					   delay_free_list);
>> +
>> +		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
>> +		list_del(&request->delay_free_list);
>> +		count = request->delay_free_count;
>> +		request->delay_free_count = 0;
>> +		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
>> +
>> +		while (count-- > 0)
>> +			i915_gem_request_unreference(request);
>> +	}
>> +
>>   	WARN_ON(i915_verify_lists(ring->dev));
>>   }
>>
>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
>>   	INIT_LIST_HEAD(&ring->active_list);
>>   	INIT_LIST_HEAD(&ring->request_list);
>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> Same comment as before - multiple init points for this list.
>
> Thomas.
Same reply as before: the function already exists and is already 
initialising other lists therefore it seems sensible to initialise the 
new list as well. Whether the function as a whole is redundant or not is 
unclear. The same duplicated initialisation also happens in 
intel_render_ring_init_dri(). If someone thinks these can all be 
simplified and wants to only do the initialisation once in one place 
then that should be another patch.

>>   }
>>
>>   void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
>> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index eba0acd..db8efaa 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev,
>> struct intel_engine_cs *rin
>>   	ring->dev = dev;
>>   	INIT_LIST_HEAD(&ring->active_list);
>>   	INIT_LIST_HEAD(&ring->request_list);
>> +	spin_lock_init(&ring->reqlist_lock);
>>   	init_waitqueue_head(&ring->irq_queue);
>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>
>>   	INIT_LIST_HEAD(&ring->execlist_queue);
>>   	spin_lock_init(&ring->execlist_lock);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 74c48ed..4338132 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device
>> *dev,
>>   	ring->dev = dev;
>>   	INIT_LIST_HEAD(&ring->active_list);
>>   	INIT_LIST_HEAD(&ring->request_list);
>> +	spin_lock_init(&ring->reqlist_lock);
>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>   	INIT_LIST_HEAD(&ring->execlist_queue);
>>   	ringbuf->size = 32 * PAGE_SIZE;
>>   	ringbuf->ring = ring;
>> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device
>> *dev, u64 start, u32 size)
>>   	ring->dev = dev;
>>   	INIT_LIST_HEAD(&ring->active_list);
>>   	INIT_LIST_HEAD(&ring->request_list);
>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>
>>   	ringbuf->size = size;
>>   	ringbuf->effective_size = ringbuf->size; diff --git
>> a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 824f5884..3af7b7c 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -262,6 +262,8 @@ struct  intel_engine_cs {
>>   	 * outstanding.
>>   	 */
>>   	struct list_head request_list;
>> +	spinlock_t reqlist_lock;
>> +	struct list_head delayed_free_list;
>>
>>   	/**
>>   	 * Do we have some not yet emitted requests outstanding?
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 25/28] drm/i915: Interrupt driven request completion

[John Harrison]

On 18/11/2014 09:40, Daniel, Thomas wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of John.C.Harrison@Intel.com
>> Sent: Friday, November 14, 2014 12:19 PM
>> To: Intel-GFX@Lists.FreeDesktop.Org
>> Subject: [Intel-gfx] [PATCH v2 25/28] drm/i915: Interrupt driven request
>> completion
>>
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Added a hook to the ring noftification code to process request completion.
>> This means that there is no longer a need to explicitly process request
>> completions every time a request object is tested. Instead, the test code
>> simply becomes 'return req->completed'. Obviously, this only works if ring
>> interrupts are enabled, however, this is already the case for the duration of
>> __wait_request() which is the point where the driver really needs to know.
>>
>> To prevent stale requests floating around indefinitely, the retire work
>> handler also now performs a completion check periodically.
>>
>> For: VIZ-4377
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h |    5 -----
>>   drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
>>   drivers/gpu/drm/i915/i915_irq.c |    2 ++
>>   3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index 8531e0f..66219b5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2072,11 +2072,6 @@ static inline void i915_gem_request_assign(struct
>> drm_i915_gem_request **pdst,  static inline bool
>> i915_gem_request_completed(struct drm_i915_gem_request *req,
>>   					      bool lazy_coherency)
>>   {
>> -	if (req->complete)
>> -		return true;
>> -
>> -	i915_gem_complete_requests_ring(req->ring, lazy_coherency);
>> -
>>   	return req->complete;
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c index edf712b..039dbb8 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1264,6 +1264,11 @@ int __i915_wait_request(struct
>> drm_i915_gem_request *req,
>>   	if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring)))
>>   		return -ENODEV;
>>
>> +	/* Completion status should be interrupt driven but it is possible
>> +	 * the request popped out before the interrupt was enabled. So do
>> an
>> +	 * explicit check now... */
>> +	i915_gem_complete_requests_ring(req->ring, false);
>> +
>>   	/* Record current time in case interrupted by signal, or wedged */
>>   	trace_i915_gem_request_wait_begin(req);
>>   	before = ktime_get_raw_ns();
>> @@ -2487,6 +2492,10 @@ int __i915_add_request(struct intel_engine_cs
>> *ring,
>>   	list_add_tail(&request->list, &ring->request_list);
>>   	request->file_priv = NULL;
>>
>> +	/* Avoid race condition where the request completes before it has
>> +	 * been added to the list. */
>> +	ring->last_read_seqno = 0;
>> +
>>   	if (file) {
>>   		struct drm_i915_file_private *file_priv = file->driver_priv;
>>
>> @@ -2858,6 +2867,7 @@ i915_gem_retire_requests(struct drm_device
>> *dev)
>>   	int i;
>>
>>   	for_each_ring(ring, dev_priv, i) {
>> +		i915_gem_complete_requests_ring(ring, false);
>>   		i915_gem_retire_requests_ring(ring);
>>   		idle &= list_empty(&ring->request_list);
>>   	}
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c index 198bbc6..4f63966 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -987,6 +987,8 @@ static void notify_ring(struct drm_device *dev,
>>
>>   	trace_i915_gem_request_complete(ring);
>>
>> +	i915_gem_complete_requests_ring(ring, false);
> I915_gem_complete_requests_ring takes the request list lock and iterates over the entire list.  Are you sure this is safe to do during the interrupt handler?
>
> Thomas.
The list is generally fairly small (max tens of items when running a 
desktop with a bunch of OGL apps/benchmarks running concurrently, 
usually single digit counts). Also, the processing is pretty minimal - 
do an integer comparison and set a flag if true. The overhead really is 
not great.

>> +
>>   	wake_up_all(&ring->irq_queue);
>>   	i915_queue_hangcheck(dev);
>>   }
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Daniel, Thomas]

> -----Original Message-----
> From: Harrison, John C
> Sent: Wednesday, November 19, 2014 12:26 PM
> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org
> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
> deference facility
> 
> On 18/11/2014 09:34, Daniel, Thomas wrote:
> >> -----Original Message-----
> >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> >> Behalf Of John.C.Harrison@Intel.com
> >> Sent: Friday, November 14, 2014 12:19 PM
> >> To: Intel-GFX@Lists.FreeDesktop.Org
> >> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
> >> request deference facility
> >>
> >> From: John Harrison <John.C.Harrison@Intel.com>
> >>
> >> The next patches in the series convert some display related seqno
> >> usage to request structure usage. However, the request dereference
> >> introduced must be done from interrupt context. As the dereference
> >> potentially involves freeing the request structure and thus calling
> >> lots of non-interrupt friendly code, this poses a problem.
> >>
> >> The solution is to add an IRQ friendly version of the dereference
> >> function. All this does is to add the request structure to a 'delayed free'
> list and return.
> >> The retire code, which is run periodically, then processes this list
> >> and does the actual dereferencing of the request structures.
> >>
> >> v2: Added a count to allow for multiple IRQ dereferences of the same
> >> request at a time.
> >>
> >> For: VIZ-4377
> >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
> >>   drivers/gpu/drm/i915/i915_gem.c         |   33
> >> +++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
> >>   drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
> >>   drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
> >>   5 files changed, 47 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
> >>   	struct drm_i915_file_private *file_priv;
> >>   	/** file_priv list entry for this request */
> >>   	struct list_head client_list;
> >> +
> >> +	/** deferred free list for dereferencing from IRQ context */
> >> +	struct list_head delay_free_list;
> >> +	uint32_t delay_free_count;
> >>   };
> >>
> >>   void i915_gem_request_free(struct kref *req_ref); @@ -2044,9
> >> +2048,12 @@ i915_gem_request_reference(struct
> drm_i915_gem_request
> >> *req) static inline void  i915_gem_request_unreference(struct
> >> drm_i915_gem_request *req)  {
> >> +	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
> >>   	kref_put(&req->ref, i915_gem_request_free);  }
> >>
> >> +void i915_gem_request_unreference_irq(struct
> drm_i915_gem_request
> >> +*req);
> >> +
> >>   static inline void i915_gem_request_assign(struct
> >> drm_i915_gem_request **pdst,
> >>   					   struct drm_i915_gem_request *src)
> { diff --git
> >> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index 60e5eec..8453bbd 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
> >>   	i915_gem_restore_fences(dev);
> >>   }
> >>
> >> +void i915_gem_request_unreference_irq(struct
> drm_i915_gem_request
> >> *req)
> >> +{
> >> +	struct intel_engine_cs *ring = req->ring;
> >> +	unsigned long flags;
> >> +
> >> +	/* Need to add the req to a deferred dereference list to be
> >> processed
> >> +	 * outside of interrupt time */
> >> +	spin_lock_irqsave(&ring->reqlist_lock, flags);
> >> +	if (req->delay_free_count++ == 0)
> >> +		list_add_tail(&req->delay_free_list, &ring-
> >>> delayed_free_list);
> >> +	spin_unlock_irqrestore(&ring->reqlist_lock, flags); }
> >> +
> >>   /**
> >>    * This function clears the request list as sequence numbers are passed.
> >>    */
> >> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
> >> intel_engine_cs *ring)
> >>   		ring->trace_irq_seqno = 0;
> >>   	}
> >>
> >> +	while (!list_empty(&ring->delayed_free_list)) {
> >> +		struct drm_i915_gem_request *request;
> >> +		unsigned long flags;
> >> +		uint32_t count;
> >> +
> >> +		request = list_first_entry(&ring->delayed_free_list,
> >> +					   struct drm_i915_gem_request,
> >> +					   delay_free_list);
> >> +
> >> +		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
> >> +		list_del(&request->delay_free_list);
> >> +		count = request->delay_free_count;
> >> +		request->delay_free_count = 0;
> >> +		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
> >> +
> >> +		while (count-- > 0)
> >> +			i915_gem_request_unreference(request);
> >> +	}
> >> +
> >>   	WARN_ON(i915_verify_lists(ring->dev));
> >>   }
> >>
> >> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
> >>   	INIT_LIST_HEAD(&ring->active_list);
> >>   	INIT_LIST_HEAD(&ring->request_list);
> >> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> > Same comment as before - multiple init points for this list.
> >
> > Thomas.
> Same reply as before: the function already exists and is already initialising 
You forgot to reply to the mailing list last time.

> other lists therefore it seems sensible to initialise the new list as well.
> Whether the function as a whole is redundant or not is unclear. The same
You've introduced a new list - surely it's your responsibility to know where it should be initialised?

> duplicated initialisation also happens in intel_render_ring_init_dri(). If
> someone thinks these can all be simplified and wants to only do the
> initialisation once in one place then that should be another patch.
Agree that the other duplicate inits should also be fixed up in another patch but that's no reason to add another dupe.

Thomas.

> >>   }
> >>
> >>   void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
> >> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >> index eba0acd..db8efaa 100644
> >> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device
> >> *dev, struct intel_engine_cs *rin
> >>   	ring->dev = dev;
> >>   	INIT_LIST_HEAD(&ring->active_list);
> >>   	INIT_LIST_HEAD(&ring->request_list);
> >> +	spin_lock_init(&ring->reqlist_lock);
> >>   	init_waitqueue_head(&ring->irq_queue);
> >> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>
> >>   	INIT_LIST_HEAD(&ring->execlist_queue);
> >>   	spin_lock_init(&ring->execlist_lock);
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 74c48ed..4338132 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct
> >> drm_device *dev,
> >>   	ring->dev = dev;
> >>   	INIT_LIST_HEAD(&ring->active_list);
> >>   	INIT_LIST_HEAD(&ring->request_list);
> >> +	spin_lock_init(&ring->reqlist_lock);
> >> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>   	INIT_LIST_HEAD(&ring->execlist_queue);
> >>   	ringbuf->size = 32 * PAGE_SIZE;
> >>   	ringbuf->ring = ring;
> >> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct
> >> drm_device *dev, u64 start, u32 size)
> >>   	ring->dev = dev;
> >>   	INIT_LIST_HEAD(&ring->active_list);
> >>   	INIT_LIST_HEAD(&ring->request_list);
> >> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>
> >>   	ringbuf->size = size;
> >>   	ringbuf->effective_size = ringbuf->size; diff --git
> >> a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> index 824f5884..3af7b7c 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> @@ -262,6 +262,8 @@ struct  intel_engine_cs {
> >>   	 * outstanding.
> >>   	 */
> >>   	struct list_head request_list;
> >> +	spinlock_t reqlist_lock;
> >> +	struct list_head delayed_free_list;
> >>
> >>   	/**
> >>   	 * Do we have some not yet emitted requests outstanding?
> >> --
> >> 1.7.9.5
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[John Harrison]

On 19/11/2014 13:28, Daniel, Thomas wrote:
>> -----Original Message-----
>> From: Harrison, John C
>> Sent: Wednesday, November 19, 2014 12:26 PM
>> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org
>> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
>> deference facility
>>
>> On 18/11/2014 09:34, Daniel, Thomas wrote:
>>>> -----Original Message-----
>>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>>>> Behalf Of John.C.Harrison@Intel.com
>>>> Sent: Friday, November 14, 2014 12:19 PM
>>>> To: Intel-GFX@Lists.FreeDesktop.Org
>>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
>>>> request deference facility
>>>>
>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>> The next patches in the series convert some display related seqno
>>>> usage to request structure usage. However, the request dereference
>>>> introduced must be done from interrupt context. As the dereference
>>>> potentially involves freeing the request structure and thus calling
>>>> lots of non-interrupt friendly code, this poses a problem.
>>>>
>>>> The solution is to add an IRQ friendly version of the dereference
>>>> function. All this does is to add the request structure to a 'delayed free'
>> list and return.
>>>> The retire code, which is run periodically, then processes this list
>>>> and does the actual dereferencing of the request structures.
>>>>
>>>> v2: Added a count to allow for multiple IRQ dereferences of the same
>>>> request at a time.
>>>>
>>>> For: VIZ-4377
>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
>>>>    drivers/gpu/drm/i915/i915_gem.c         |   33
>>>> +++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
>>>>    drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
>>>>    drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
>>>>    5 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
>>>>    	struct drm_i915_file_private *file_priv;
>>>>    	/** file_priv list entry for this request */
>>>>    	struct list_head client_list;
>>>> +
>>>> +	/** deferred free list for dereferencing from IRQ context */
>>>> +	struct list_head delay_free_list;
>>>> +	uint32_t delay_free_count;
>>>>    };
>>>>
>>>>    void i915_gem_request_free(struct kref *req_ref); @@ -2044,9
>>>> +2048,12 @@ i915_gem_request_reference(struct
>> drm_i915_gem_request
>>>> *req) static inline void  i915_gem_request_unreference(struct
>>>> drm_i915_gem_request *req)  {
>>>> +	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>>>>    	kref_put(&req->ref, i915_gem_request_free);  }
>>>>
>>>> +void i915_gem_request_unreference_irq(struct
>> drm_i915_gem_request
>>>> +*req);
>>>> +
>>>>    static inline void i915_gem_request_assign(struct
>>>> drm_i915_gem_request **pdst,
>>>>    					   struct drm_i915_gem_request *src)
>> { diff --git
>>>> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 60e5eec..8453bbd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
>>>>    	i915_gem_restore_fences(dev);
>>>>    }
>>>>
>>>> +void i915_gem_request_unreference_irq(struct
>> drm_i915_gem_request
>>>> *req)
>>>> +{
>>>> +	struct intel_engine_cs *ring = req->ring;
>>>> +	unsigned long flags;
>>>> +
>>>> +	/* Need to add the req to a deferred dereference list to be
>>>> processed
>>>> +	 * outside of interrupt time */
>>>> +	spin_lock_irqsave(&ring->reqlist_lock, flags);
>>>> +	if (req->delay_free_count++ == 0)
>>>> +		list_add_tail(&req->delay_free_list, &ring-
>>>>> delayed_free_list);
>>>> +	spin_unlock_irqrestore(&ring->reqlist_lock, flags); }
>>>> +
>>>>    /**
>>>>     * This function clears the request list as sequence numbers are passed.
>>>>     */
>>>> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
>>>> intel_engine_cs *ring)
>>>>    		ring->trace_irq_seqno = 0;
>>>>    	}
>>>>
>>>> +	while (!list_empty(&ring->delayed_free_list)) {
>>>> +		struct drm_i915_gem_request *request;
>>>> +		unsigned long flags;
>>>> +		uint32_t count;
>>>> +
>>>> +		request = list_first_entry(&ring->delayed_free_list,
>>>> +					   struct drm_i915_gem_request,
>>>> +					   delay_free_list);
>>>> +
>>>> +		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
>>>> +		list_del(&request->delay_free_list);
>>>> +		count = request->delay_free_count;
>>>> +		request->delay_free_count = 0;
>>>> +		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
>>>> +
>>>> +		while (count-- > 0)
>>>> +			i915_gem_request_unreference(request);
>>>> +	}
>>>> +
>>>>    	WARN_ON(i915_verify_lists(ring->dev));
>>>>    }
>>>>
>>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
>>>>    	INIT_LIST_HEAD(&ring->active_list);
>>>>    	INIT_LIST_HEAD(&ring->request_list);
>>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>> Same comment as before - multiple init points for this list.
>>>
>>> Thomas.
>> Same reply as before: the function already exists and is already initialising
> You forgot to reply to the mailing list last time.
>
>> other lists therefore it seems sensible to initialise the new list as well.
>> Whether the function as a whole is redundant or not is unclear. The same
> You've introduced a new list - surely it's your responsibility to know where it should be initialised?
My list is an extension of the existing 'request_list'. Therefore is 
seems prudent to initialise it wherever better minds than me have deemed 
it necessary to initialise request_list itself.

>> duplicated initialisation also happens in intel_render_ring_init_dri(). If
>> someone thinks these can all be simplified and wants to only do the
>> initialisation once in one place then that should be another patch.
> Agree that the other duplicate inits should also be fixed up in another patch but that's no reason to add another dupe.
>
> Thomas.

Only if it is definitely a redundant duplication. Currently, it is not 
obvious that it is therefore I am being safe/sensible by following the 
existing convention.


>>>>    }
>>>>
>>>>    void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
>>>> a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index eba0acd..db8efaa 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device
>>>> *dev, struct intel_engine_cs *rin
>>>>    	ring->dev = dev;
>>>>    	INIT_LIST_HEAD(&ring->active_list);
>>>>    	INIT_LIST_HEAD(&ring->request_list);
>>>> +	spin_lock_init(&ring->reqlist_lock);
>>>>    	init_waitqueue_head(&ring->irq_queue);
>>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>>>
>>>>    	INIT_LIST_HEAD(&ring->execlist_queue);
>>>>    	spin_lock_init(&ring->execlist_lock);
>>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> index 74c48ed..4338132 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct
>>>> drm_device *dev,
>>>>    	ring->dev = dev;
>>>>    	INIT_LIST_HEAD(&ring->active_list);
>>>>    	INIT_LIST_HEAD(&ring->request_list);
>>>> +	spin_lock_init(&ring->reqlist_lock);
>>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>>>    	INIT_LIST_HEAD(&ring->execlist_queue);
>>>>    	ringbuf->size = 32 * PAGE_SIZE;
>>>>    	ringbuf->ring = ring;
>>>> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct
>>>> drm_device *dev, u64 start, u32 size)
>>>>    	ring->dev = dev;
>>>>    	INIT_LIST_HEAD(&ring->active_list);
>>>>    	INIT_LIST_HEAD(&ring->request_list);
>>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
>>>>
>>>>    	ringbuf->size = size;
>>>>    	ringbuf->effective_size = ringbuf->size; diff --git
>>>> a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>>> index 824f5884..3af7b7c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>>> @@ -262,6 +262,8 @@ struct  intel_engine_cs {
>>>>    	 * outstanding.
>>>>    	 */
>>>>    	struct list_head request_list;
>>>> +	spinlock_t reqlist_lock;
>>>> +	struct list_head delayed_free_list;
>>>>
>>>>    	/**
>>>>    	 * Do we have some not yet emitted requests outstanding?
>>>> --
>>>> 1.7.9.5
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Daniel, Thomas]

> -----Original Message-----
> From: Harrison, John C
> Sent: Wednesday, November 19, 2014 5:05 PM
> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org
> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly request
> deference facility
> 
> On 19/11/2014 13:28, Daniel, Thomas wrote:
> >> -----Original Message-----
> >> From: Harrison, John C
> >> Sent: Wednesday, November 19, 2014 12:26 PM
> >> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org
> >> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
> >> request deference facility
> >>
> >> On 18/11/2014 09:34, Daniel, Thomas wrote:
> >>>> -----Original Message-----
> >>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> >>>> Behalf Of John.C.Harrison@Intel.com
> >>>> Sent: Friday, November 14, 2014 12:19 PM
> >>>> To: Intel-GFX@Lists.FreeDesktop.Org
> >>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
> >>>> request deference facility
> >>>>
> >>>> From: John Harrison <John.C.Harrison@Intel.com>
> >>>>
> >>>> @@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
> >>>> intel_engine_cs *ring)
> >>>>    		ring->trace_irq_seqno = 0;
> >>>>    	}
> >>>>
> >>>> +	while (!list_empty(&ring->delayed_free_list)) {
> >>>> +		struct drm_i915_gem_request *request;
> >>>> +		unsigned long flags;
> >>>> +		uint32_t count;
> >>>> +
> >>>> +		request = list_first_entry(&ring->delayed_free_list,
> >>>> +					   struct drm_i915_gem_request,
> >>>> +					   delay_free_list);
> >>>> +
> >>>> +		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
> >>>> +		list_del(&request->delay_free_list);
> >>>> +		count = request->delay_free_count;
> >>>> +		request->delay_free_count = 0;
> >>>> +		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
> >>>> +
> >>>> +		while (count-- > 0)
> >>>> +			i915_gem_request_unreference(request);
> >>>> +	}
> >>>> +
> >>>>    	WARN_ON(i915_verify_lists(ring->dev));
> >>>>    }
> >>>>
> >>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
> >>>>    	INIT_LIST_HEAD(&ring->active_list);
> >>>>    	INIT_LIST_HEAD(&ring->request_list);
> >>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>> Same comment as before - multiple init points for this list.
> >>>
> >>> Thomas.
> >> Same reply as before: the function already exists and is already
> >> initialising
> > You forgot to reply to the mailing list last time.
> >
> >> other lists therefore it seems sensible to initialise the new list as well.
> >> Whether the function as a whole is redundant or not is unclear. The
> >> same
> > You've introduced a new list - surely it's your responsibility to know where
> it should be initialised?
> My list is an extension of the existing 'request_list'. Therefore is seems
> prudent to initialise it wherever better minds than me have deemed it
> necessary to initialise request_list itself.
> 
> >> duplicated initialisation also happens in
> >> intel_render_ring_init_dri(). If someone thinks these can all be
> >> simplified and wants to only do the initialisation once in one place then
> that should be another patch.
> > Agree that the other duplicate inits should also be fixed up in another patch
> but that's no reason to add another dupe.
> >
> > Thomas.
> 
> Only if it is definitely a redundant duplication. Currently, it is not obvious that
> it is therefore I am being safe/sensible by following the existing convention.
Git blame suggests that init_ring_lists is a vestigial init from before intel_ringbuffers existed.  Don’t you agree that either init_ring_buffer or logical_ring_init must be called before any requests can be put into the request_list?

Thomas.

> 
> >>>>    }
> >>>>
> >>>>    void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
> >>>> a/drivers/gpu/drm/i915/intel_lrc.c
> >>>> b/drivers/gpu/drm/i915/intel_lrc.c
> >>>> index eba0acd..db8efaa 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>> @@ -1287,7 +1287,9 @@ static int logical_ring_init(struct
> >>>> drm_device *dev, struct intel_engine_cs *rin
> >>>>    	ring->dev = dev;
> >>>>    	INIT_LIST_HEAD(&ring->active_list);
> >>>>    	INIT_LIST_HEAD(&ring->request_list);
> >>>> +	spin_lock_init(&ring->reqlist_lock);
> >>>>    	init_waitqueue_head(&ring->irq_queue);
> >>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>>>
> >>>>    	INIT_LIST_HEAD(&ring->execlist_queue);
> >>>>    	spin_lock_init(&ring->execlist_lock);
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>>> index 74c48ed..4338132 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>>> @@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct
> >>>> drm_device *dev,
> >>>>    	ring->dev = dev;
> >>>>    	INIT_LIST_HEAD(&ring->active_list);
> >>>>    	INIT_LIST_HEAD(&ring->request_list);
> >>>> +	spin_lock_init(&ring->reqlist_lock);
> >>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>>>    	INIT_LIST_HEAD(&ring->execlist_queue);
> >>>>    	ringbuf->size = 32 * PAGE_SIZE;
> >>>>    	ringbuf->ring = ring;
> >>>> @@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct
> >>>> drm_device *dev, u64 start, u32 size)
> >>>>    	ring->dev = dev;
> >>>>    	INIT_LIST_HEAD(&ring->active_list);
> >>>>    	INIT_LIST_HEAD(&ring->request_list);
> >>>> +	INIT_LIST_HEAD(&ring->delayed_free_list);
> >>>>
> >>>>    	ringbuf->size = size;
> >>>>    	ringbuf->effective_size = ringbuf->size; diff --git
> >>>> a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>> index 824f5884..3af7b7c 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>> @@ -262,6 +262,8 @@ struct  intel_engine_cs {
> >>>>    	 * outstanding.
> >>>>    	 */
> >>>>    	struct list_head request_list;
> >>>> +	spinlock_t reqlist_lock;
> >>>> +	struct list_head delayed_free_list;
> >>>>
> >>>>    	/**
> >>>>    	 * Do we have some not yet emitted requests outstanding?
> >>>> --
> >>>> 1.7.9.5
> >>>>
> >>>> _______________________________________________
> >>>> Intel-gfx mailing list
> >>>> Intel-gfx@lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Dave Gordon]

On 19/11/14 17:05, John Harrison wrote:
> On 19/11/2014 13:28, Daniel, Thomas wrote:
>>> -----Original Message-----
>>> From: Harrison, John C
>>> Sent: Wednesday, November 19, 2014 12:26 PM
>>> To: Daniel, Thomas; Intel-GFX@Lists.FreeDesktop.Org
>>> Subject: Re: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
>>> request
>>> deference facility
>>>
>>> On 18/11/2014 09:34, Daniel, Thomas wrote:
>>>>> -----Original Message-----
>>>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>>>>> Behalf Of John.C.Harrison@Intel.com
>>>>> Sent: Friday, November 14, 2014 12:19 PM
>>>>> To: Intel-GFX@Lists.FreeDesktop.Org
>>>>> Subject: [Intel-gfx] [PATCH v2 11/28] drm/i915: Add IRQ friendly
>>>>> request deference facility
>>>>>
>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>
>>>>> The next patches in the series convert some display related seqno
>>>>> usage to request structure usage. However, the request dereference
>>>>> introduced must be done from interrupt context. As the dereference
>>>>> potentially involves freeing the request structure and thus calling
>>>>> lots of non-interrupt friendly code, this poses a problem.
>>>>>
>>>>> The solution is to add an IRQ friendly version of the dereference
>>>>> function. All this does is to add the request structure to a
>>>>> 'delayed free'
>>> list and return.
>>>>> The retire code, which is run periodically, then processes this list
>>>>> and does the actual dereferencing of the request structures.
>>>>>
>>>>> v2: Added a count to allow for multiple IRQ dereferences of the same
>>>>> request at a time.
>>>>>
>>>>> For: VIZ-4377
>>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
>>>>>    drivers/gpu/drm/i915/i915_gem.c         |   33
>>>>> +++++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
>>>>>    drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
>>>>>    drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
>>>>>    5 files changed, 47 insertions(+)
>>>>>
[snip]
>>>>>
>>>>> @@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
>>>>>        INIT_LIST_HEAD(&ring->active_list);
>>>>>        INIT_LIST_HEAD(&ring->request_list);
>>>>> +    INIT_LIST_HEAD(&ring->delayed_free_list);
>>>> Same comment as before - multiple init points for this list.
>>>>
>>>> Thomas.
>>> Same reply as before: the function already exists and is already
>>> initialising
>> You forgot to reply to the mailing list last time.
>>
>>> other lists therefore it seems sensible to initialise the new list as
>>> well.
>>> Whether the function as a whole is redundant or not is unclear. The same
>> You've introduced a new list - surely it's your responsibility to know
>> where it should be initialised?
> My list is an extension of the existing 'request_list'. Therefore is
> seems prudent to initialise it wherever better minds than me have deemed
> it necessary to initialise request_list itself.
> 
>>> duplicated initialisation also happens in
>>> intel_render_ring_init_dri(). If
>>> someone thinks these can all be simplified and wants to only do the
>>> initialisation once in one place then that should be another patch.
>> Agree that the other duplicate inits should also be fixed up in
>> another patch but that's no reason to add another dupe.
>>
>> Thomas.
> 
> Only if it is definitely a redundant duplication. Currently, it is not
> obvious that it is therefore I am being safe/sensible by following the
> existing convention.

One initialisation of (ring->request_list) is in init_ring_lists()
which does nothing other than what it's name implies. It's called
only from i915_gem_load(), in turn called (early) from i915_driver_load().

The same list is also (re)initialised in i915_gem_init_rings() in legacy
mode or logical_ring_init() in LRC mode; these are called (indirectly)
from i915_gem_init_hw(). But this is called not only from
i915_gem_init() (which is called via i915_load_modeset_init() later in
i915_driver_load()), but also from i915_reset(), i915_drm_resume(),
and i915_gem_entervt_ioctl!

It is therefore entirely likely that it is necessary to REinitialise
these lists in various situations after driver loading is complete; but
conversely quite possible that they have to be set up very early, before
the modeset call -- and in any case there's the possibility that the
modeset may not be executed, on sufficiently ancient h/w :(

It should be possible to test whether the early initialisation is
necessary by poisoning the list {head,tail} values so that it triggers
an OOPS if they're dereferenced before they're reinitialised.

There's one more initialisation in intel_render_ring_init_dri(), but
this code is not reachable on post-gen5 h/w and therefore by definition
not when using execlists.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/28] Replace seqno values with request structures

[Daniel Vetter]

On Fri, Nov 14, 2014 at 12:18:51PM +0000, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> There is a general feeling that it is better to move away from using a simple
> integer 'seqno' value to track batch buffer completion. Instead, the request
> structure should be used. That provides for much more flexibility going
> forwards. Especially which things like a GPU scheduler (which can re-order batch
> buffers and hence seqnos after submission to the hardware), Android sync points
> and other such features which potentially make seqno usage more and more
> complex.
> 
> This patch set does the work of converting most of the driver to use request
> structures in preference to seqno values. The only place left that still uses
> seqnos is the semaphore code. It was decided to leave that alone for the time
> being as the semaphores are hardware based and the hardware only understands
> seqno values.
> 
> v2: Rebased to newer nightly tree which significantly changed some of the
> display MMIO flip code (including making __wait_request public and renaming
> it).
> 
> [Patches against drm-intel-nightly tree fetched 13/11/2014]
> 
> John Harrison (28):
>   drm/i915: Ensure OLS & PLR are always in sync
>   drm/i915: Add reference count to request structure
>   drm/i915: Add helper functions to aid seqno -> request transition
>   drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
>   drm/i915: Convert i915_gem_ring_throttle to use requests
>   drm/i915: Ensure requests stick around during waits
>   drm/i915: Remove 'outstanding_lazy_seqno'
>   drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
>   drm/i915: Convert 'last_flip_req' to be a request not a seqno
>   drm/i915: Convert i915_wait_seqno to i915_wait_request
>   drm/i915: Add IRQ friendly request deference facility
>   drm/i915: Convert mmio_flip::seqno to struct request
>   drm/i915: Convert __wait_seqno() to __wait_request()
>   drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
>   drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
>   drm/i915: Convert trace functions from seqno to request
>   drm/i915: Convert 'trace_irq' to use requests rather than seqnos
>   drm/i915: Convert 'ring_idle()' to use requests not seqnos
>   drm/i915: Connect requests to rings at creation not submission
>   drm/i915: Convert 'i915_seqno_passed' calls into 'i915_gem_request_completed'
>   drm/i915: Remove the now redundant 'obj->ring'
>   drm/i915: Cache request completion status
>   drm/i915: Zero fill the request structure
>   drm/i915: Spinlock protection for request list
>   drm/i915: Interrupt driven request completion
>   drm/i915: Remove obsolete parameter to i915_gem_request_completed()
>   drm/i915: Add unique id to the request structure for debugging
>   drm/i915: Additional request structure tracing

Erhm this stuff is blocking refactoring work since I don't want to wreak
major havoc with the codebase to avoid rebase hell for you. That means
reviewing and merging early, even if the tail of the patch series isn't
100% clear yet.

Imo everything past patch 21 is fairly optional icing on the cake here.
And if there early patches are non-controversial then I'd like them to
pull in even if there's some detail missing in a patch in the middle.

But I haven't seen r-b tags anywhere really :(

Or am I blind?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/28] Replace seqno values with request structures

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Wednesday, November 19, 2014 7:29 PM
> To: Harrison, John C
> Cc: Intel-GFX@Lists.FreeDesktop.Org
> Subject: Re: [Intel-gfx] [PATCH v2 00/28] Replace seqno values with request
> structures
> 
> On Fri, Nov 14, 2014 at 12:18:51PM +0000, John.C.Harrison@Intel.com wrote:
> > From: John Harrison <John.C.Harrison@Intel.com>
> >
> > There is a general feeling that it is better to move away from using a
> > simple integer 'seqno' value to track batch buffer completion.
> > Instead, the request structure should be used. That provides for much
> > more flexibility going forwards. Especially which things like a GPU
> > scheduler (which can re-order batch buffers and hence seqnos after
> > submission to the hardware), Android sync points and other such
> > features which potentially make seqno usage more and more complex.
> >
> > This patch set does the work of converting most of the driver to use
> > request structures in preference to seqno values. The only place left
> > that still uses seqnos is the semaphore code. It was decided to leave
> > that alone for the time being as the semaphores are hardware based and
> > the hardware only understands seqno values.
> >
> > v2: Rebased to newer nightly tree which significantly changed some of
> > the display MMIO flip code (including making __wait_request public and
> > renaming it).
> >
> > [Patches against drm-intel-nightly tree fetched 13/11/2014]
> >
> > John Harrison (28):
> >   drm/i915: Ensure OLS & PLR are always in sync
> >   drm/i915: Add reference count to request structure
> >   drm/i915: Add helper functions to aid seqno -> request transition
> >   drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
> >   drm/i915: Convert i915_gem_ring_throttle to use requests
> >   drm/i915: Ensure requests stick around during waits
> >   drm/i915: Remove 'outstanding_lazy_seqno'
> >   drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
> >   drm/i915: Convert 'last_flip_req' to be a request not a seqno
> >   drm/i915: Convert i915_wait_seqno to i915_wait_request
> >   drm/i915: Add IRQ friendly request deference facility
> >   drm/i915: Convert mmio_flip::seqno to struct request
> >   drm/i915: Convert __wait_seqno() to __wait_request()
> >   drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
> >   drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
> >   drm/i915: Convert trace functions from seqno to request
> >   drm/i915: Convert 'trace_irq' to use requests rather than seqnos
> >   drm/i915: Convert 'ring_idle()' to use requests not seqnos
> >   drm/i915: Connect requests to rings at creation not submission
> >   drm/i915: Convert 'i915_seqno_passed' calls into
> 'i915_gem_request_completed'
> >   drm/i915: Remove the now redundant 'obj->ring'
> >   drm/i915: Cache request completion status
> >   drm/i915: Zero fill the request structure
> >   drm/i915: Spinlock protection for request list
> >   drm/i915: Interrupt driven request completion
> >   drm/i915: Remove obsolete parameter to
> i915_gem_request_completed()
> >   drm/i915: Add unique id to the request structure for debugging
> >   drm/i915: Additional request structure tracing
> 
> Erhm this stuff is blocking refactoring work since I don't want to wreak major
> havoc with the codebase to avoid rebase hell for you. That means reviewing
> and merging early, even if the tail of the patch series isn't 100% clear yet.
> 
> Imo everything past patch 21 is fairly optional icing on the cake here.
> And if there early patches are non-controversial then I'd like them to pull in
> even if there's some detail missing in a patch in the middle.
> 
> But I haven't seen r-b tags anywhere really :(
Well there is some review discussion going on, but my remaining gripe about a double init for a new list isn't a showstopper.  So in the interests of unblocking other work I will give my r-b to this v2 patchset.

Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com>

Thomas.

> 
> Or am I blind?
> 
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/28] Replace seqno values with request structures

[Daniel Vetter]

On Thu, Nov 20, 2014 at 09:32:34AM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Daniel Vetter
> > Sent: Wednesday, November 19, 2014 7:29 PM
> > To: Harrison, John C
> > Cc: Intel-GFX@Lists.FreeDesktop.Org
> > Subject: Re: [Intel-gfx] [PATCH v2 00/28] Replace seqno values with request
> > structures
> > 
> > On Fri, Nov 14, 2014 at 12:18:51PM +0000, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > >
> > > There is a general feeling that it is better to move away from using a
> > > simple integer 'seqno' value to track batch buffer completion.
> > > Instead, the request structure should be used. That provides for much
> > > more flexibility going forwards. Especially which things like a GPU
> > > scheduler (which can re-order batch buffers and hence seqnos after
> > > submission to the hardware), Android sync points and other such
> > > features which potentially make seqno usage more and more complex.
> > >
> > > This patch set does the work of converting most of the driver to use
> > > request structures in preference to seqno values. The only place left
> > > that still uses seqnos is the semaphore code. It was decided to leave
> > > that alone for the time being as the semaphores are hardware based and
> > > the hardware only understands seqno values.
> > >
> > > v2: Rebased to newer nightly tree which significantly changed some of
> > > the display MMIO flip code (including making __wait_request public and
> > > renaming it).
> > >
> > > [Patches against drm-intel-nightly tree fetched 13/11/2014]
> > >
> > > John Harrison (28):
> > >   drm/i915: Ensure OLS & PLR are always in sync
> > >   drm/i915: Add reference count to request structure
> > >   drm/i915: Add helper functions to aid seqno -> request transition
> > >   drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
> > >   drm/i915: Convert i915_gem_ring_throttle to use requests
> > >   drm/i915: Ensure requests stick around during waits
> > >   drm/i915: Remove 'outstanding_lazy_seqno'
> > >   drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
> > >   drm/i915: Convert 'last_flip_req' to be a request not a seqno
> > >   drm/i915: Convert i915_wait_seqno to i915_wait_request
> > >   drm/i915: Add IRQ friendly request deference facility
> > >   drm/i915: Convert mmio_flip::seqno to struct request
> > >   drm/i915: Convert __wait_seqno() to __wait_request()
> > >   drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
> > >   drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
> > >   drm/i915: Convert trace functions from seqno to request
> > >   drm/i915: Convert 'trace_irq' to use requests rather than seqnos
> > >   drm/i915: Convert 'ring_idle()' to use requests not seqnos
> > >   drm/i915: Connect requests to rings at creation not submission
> > >   drm/i915: Convert 'i915_seqno_passed' calls into
> > 'i915_gem_request_completed'
> > >   drm/i915: Remove the now redundant 'obj->ring'
> > >   drm/i915: Cache request completion status
> > >   drm/i915: Zero fill the request structure
> > >   drm/i915: Spinlock protection for request list
> > >   drm/i915: Interrupt driven request completion
> > >   drm/i915: Remove obsolete parameter to
> > i915_gem_request_completed()
> > >   drm/i915: Add unique id to the request structure for debugging
> > >   drm/i915: Additional request structure tracing
> > 
> > Erhm this stuff is blocking refactoring work since I don't want to wreak major
> > havoc with the codebase to avoid rebase hell for you. That means reviewing
> > and merging early, even if the tail of the patch series isn't 100% clear yet.
> > 
> > Imo everything past patch 21 is fairly optional icing on the cake here.
> > And if there early patches are non-controversial then I'd like them to pull in
> > even if there's some detail missing in a patch in the middle.
> > 
> > But I haven't seen r-b tags anywhere really :(
> Well there is some review discussion going on, but my remaining gripe
> about a double init for a new list isn't a showstopper.  So in the
> interests of unblocking other work I will give my r-b to this v2
> patchset.
> 
> Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com>

Ok, I've tried to vacuum this up but it conflicts. And this kind of
large-scale refactoring isn't really the stuff I feel safe to fix up while
applying. So can you please rebase this (perhaps together with Thomas just
to make sure there's not last minute screwup and the r-b is still valid)?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 00/28] Replace seqno values with request structures

[Daniel Vetter]

On Fri, Nov 21, 2014 at 09:52:37PM +0100, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 09:32:34AM +0000, Daniel, Thomas wrote:
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > > Of Daniel Vetter
> > > Sent: Wednesday, November 19, 2014 7:29 PM
> > > To: Harrison, John C
> > > Cc: Intel-GFX@Lists.FreeDesktop.Org
> > > Subject: Re: [Intel-gfx] [PATCH v2 00/28] Replace seqno values with request
> > > structures
> > > 
> > > On Fri, Nov 14, 2014 at 12:18:51PM +0000, John.C.Harrison@Intel.com wrote:
> > > > From: John Harrison <John.C.Harrison@Intel.com>
> > > >
> > > > There is a general feeling that it is better to move away from using a
> > > > simple integer 'seqno' value to track batch buffer completion.
> > > > Instead, the request structure should be used. That provides for much
> > > > more flexibility going forwards. Especially which things like a GPU
> > > > scheduler (which can re-order batch buffers and hence seqnos after
> > > > submission to the hardware), Android sync points and other such
> > > > features which potentially make seqno usage more and more complex.
> > > >
> > > > This patch set does the work of converting most of the driver to use
> > > > request structures in preference to seqno values. The only place left
> > > > that still uses seqnos is the semaphore code. It was decided to leave
> > > > that alone for the time being as the semaphores are hardware based and
> > > > the hardware only understands seqno values.
> > > >
> > > > v2: Rebased to newer nightly tree which significantly changed some of
> > > > the display MMIO flip code (including making __wait_request public and
> > > > renaming it).
> > > >
> > > > [Patches against drm-intel-nightly tree fetched 13/11/2014]
> > > >
> > > > John Harrison (28):
> > > >   drm/i915: Ensure OLS & PLR are always in sync
> > > >   drm/i915: Add reference count to request structure
> > > >   drm/i915: Add helper functions to aid seqno -> request transition
> > > >   drm/i915: Replace last_[rwf]_seqno with last_[rwf]_req
> > > >   drm/i915: Convert i915_gem_ring_throttle to use requests
> > > >   drm/i915: Ensure requests stick around during waits
> > > >   drm/i915: Remove 'outstanding_lazy_seqno'
> > > >   drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
> > > >   drm/i915: Convert 'last_flip_req' to be a request not a seqno
> > > >   drm/i915: Convert i915_wait_seqno to i915_wait_request
> > > >   drm/i915: Add IRQ friendly request deference facility
> > > >   drm/i915: Convert mmio_flip::seqno to struct request
> > > >   drm/i915: Convert __wait_seqno() to __wait_request()
> > > >   drm/i915: Remove obsolete seqno parameter from 'i915_add_request'
> > > >   drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
> > > >   drm/i915: Convert trace functions from seqno to request
> > > >   drm/i915: Convert 'trace_irq' to use requests rather than seqnos
> > > >   drm/i915: Convert 'ring_idle()' to use requests not seqnos
> > > >   drm/i915: Connect requests to rings at creation not submission
> > > >   drm/i915: Convert 'i915_seqno_passed' calls into
> > > 'i915_gem_request_completed'
> > > >   drm/i915: Remove the now redundant 'obj->ring'
> > > >   drm/i915: Cache request completion status
> > > >   drm/i915: Zero fill the request structure
> > > >   drm/i915: Spinlock protection for request list
> > > >   drm/i915: Interrupt driven request completion
> > > >   drm/i915: Remove obsolete parameter to
> > > i915_gem_request_completed()
> > > >   drm/i915: Add unique id to the request structure for debugging
> > > >   drm/i915: Additional request structure tracing
> > > 
> > > Erhm this stuff is blocking refactoring work since I don't want to wreak major
> > > havoc with the codebase to avoid rebase hell for you. That means reviewing
> > > and merging early, even if the tail of the patch series isn't 100% clear yet.
> > > 
> > > Imo everything past patch 21 is fairly optional icing on the cake here.
> > > And if there early patches are non-controversial then I'd like them to pull in
> > > even if there's some detail missing in a patch in the middle.
> > > 
> > > But I haven't seen r-b tags anywhere really :(
> > Well there is some review discussion going on, but my remaining gripe
> > about a double init for a new list isn't a showstopper.  So in the
> > interests of unblocking other work I will give my r-b to this v2
> > patchset.
> > 
> > Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com>
> 
> Ok, I've tried to vacuum this up but it conflicts. And this kind of
> large-scale refactoring isn't really the stuff I feel safe to fix up while
> applying. So can you please rebase this (perhaps together with Thomas just
> to make sure there's not last minute screwup and the r-b is still valid)?

Aside, for rebasing just do the up to patch 21 since I'm kinda not yet
sold on the caching. I think we should directly go to struct fence (which
we need anyway for android syncpts) which already has state caching. And
even if we merge these patches I prefer to give the first batch a bit of
time on the line to hang out and dry.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx