* [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
2014-10-06 14:15 ` [RFC 08/21] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno John.C.Harrison
2014-10-19 12:55 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno Daniel Vetter
2014-10-10 11:38 ` [RFC 08/25] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
2014-10-19 12:48 ` [RFC 08/21] " Daniel Vetter
2 siblings, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++-
drivers/gpu/drm/i915/i915_gem.c | 25 ++++++++++---------------
drivers/gpu/drm/i915/intel_display.c | 2 +-
3 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1401266..9504206 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2483,7 +2483,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)
{
@@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
}
}
+/************************* Deprecated *************************/
+static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno)
+{
+ int ret;
+
+ BUG_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;
+}
+/************************* Deprecated *************************/
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0d0eb26..2c7deca 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1097,19 +1097,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));
+ BUG_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;
}
@@ -1271,7 +1270,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;
@@ -1338,7 +1337,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));
@@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
if (!req)
return 0;
- seqno = i915_gem_request_get_seqno(req);
- BUG_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 = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv);
+ ret = __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)
@@ -2786,7 +2781,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;
@@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */
return 0;
- ret = i915_gem_check_olr(obj->ring, seqno);
+ ret = i915_gem_check_olr(obj->last_read_req);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6da18c5..2af421e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
i915_gem_request_get_seqno(obj->last_write_req)))
return 0;
- ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req));
+ ret = i915_gem_check_olr(obj->last_write_req);
if (ret)
return ret;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno
2014-10-06 14:15 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 11/21] drm/i915: Convert i915_wait_seqno to i915_wait_request John.C.Harrison
2014-10-19 12:57 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno Daniel Vetter
2014-10-19 12:55 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno Daniel Vetter
1 sibling, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/intel_overlay.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index dc2f4f2..ccd5732 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,17 @@ 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);
+ ret = i915_add_request(ring, &overlay->last_flip_req->seqno);
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;
+ overlay->last_flip_req = NULL;
return 0;
}
@@ -286,7 +286,7 @@ 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);
+ return i915_add_request(ring, &overlay->last_flip_req->seqno);
}
static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
@@ -366,10 +366,10 @@ 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 +377,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;
+ overlay->last_flip_req = NULL;
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 11/21] drm/i915: Convert i915_wait_seqno to i915_wait_request
2014-10-06 14:15 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 12/21] drm/i915: Convert 'i915_add_request' to take a request not a seqno John.C.Harrison
2014-10-19 12:57 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno Daniel Vetter
1 sibling, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 18 +-----------------
drivers/gpu/drm/i915/i915_gem.c | 30 +++++++++++++++---------------
drivers/gpu/drm/i915/intel_lrc.c | 6 ++----
drivers/gpu/drm/i915/intel_overlay.c | 9 +++------
drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++--------
5 files changed, 27 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9504206..4ce4ae6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2530,8 +2530,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 __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,
@@ -3020,19 +3019,4 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
}
}
-/************************* Deprecated *************************/
-static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno)
-{
- int ret;
-
- BUG_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;
-}
-/************************* Deprecated *************************/
-
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c7deca..30731d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1252,29 +1252,34 @@ static int __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;
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;
- return __wait_seqno(ring, seqno,
+ return __wait_seqno(req->ring, i915_gem_request_get_seqno(req),
atomic_read(&dev_priv->gpu_error.reset_counter),
interruptible, NULL, NULL);
}
@@ -1306,18 +1311,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);
- BUG_ON(seqno == 0);
-
- ret = i915_wait_seqno(ring, seqno);
+ ret = i915_wait_request(req);
if (ret)
return ret;
@@ -3234,7 +3234,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 7005745..0b68fc7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -830,7 +830,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) {
@@ -845,15 +844,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 ccd5732..27ff4a2 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -222,7 +222,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);
@@ -361,18 +361,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 dd59691..1be1934 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1868,7 +1868,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) {
@@ -1883,15 +1882,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;
@@ -1987,7 +1985,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 */
@@ -2001,11 +1999,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
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 12/21] drm/i915: Convert 'i915_add_request' to take a request not a seqno
2014-10-06 14:15 ` [RFC 11/21] drm/i915: Convert i915_wait_seqno to i915_wait_request John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request John.C.Harrison
0 siblings, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 6 +++---
drivers/gpu/drm/i915/i915_gem.c | 6 +++---
drivers/gpu/drm/i915/intel_overlay.c | 4 ++--
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ce4ae6..6fe3be0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2527,9 +2527,9 @@ 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_request **out_req);
+#define i915_add_request(ring, out_req) \
+ __i915_add_request(ring, NULL, NULL, out_req)
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
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 30731d4..b28e39b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2331,7 +2331,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_request **out_req)
{
struct drm_i915_private *dev_priv = ring->dev->dev_private;
struct drm_i915_gem_request *request;
@@ -2434,8 +2434,8 @@ int __i915_add_request(struct intel_engine_cs *ring,
intel_mark_busy(dev_priv->dev);
}
- if (out_seqno)
- *out_seqno = request->seqno;
+ if (out_req)
+ *out_req = request;
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 27ff4a2..942f91a 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -217,7 +217,7 @@ 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->seqno);
+ ret = i915_add_request(ring, &overlay->last_flip_req);
if (ret)
return ret;
@@ -286,7 +286,7 @@ 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->seqno);
+ return i915_add_request(ring, &overlay->last_flip_req);
}
static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request
2014-10-06 14:15 ` [RFC 12/21] drm/i915: Convert 'i915_add_request' to take a request not a seqno John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' John.C.Harrison
2014-10-19 13:07 ` [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request Daniel Vetter
0 siblings, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
drivers/gpu/drm/i915/intel_drv.h | 2 +-
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2af421e..f13bc30 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9799,15 +9799,16 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
struct intel_mmio_flip *mmio_flip;
mmio_flip = &intel_crtc->mmio_flip;
- if (mmio_flip->seqno == 0)
+ if (mmio_flip->req == NULL)
continue;
if (ring->id != mmio_flip->ring_id)
continue;
- if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
+ if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
intel_do_mmio_flip(intel_crtc);
- mmio_flip->seqno = 0;
+ i915_gem_request_unreference(mmio_flip->req);
+ mmio_flip->req = NULL;
ring->irq_put(ring);
}
}
@@ -9826,7 +9827,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
unsigned long irq_flags;
int ret;
- if (WARN_ON(intel_crtc->mmio_flip.seqno))
+ if (WARN_ON(intel_crtc->mmio_flip.req))
return -EBUSY;
ret = intel_postpone_flip(obj);
@@ -9838,7 +9839,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
}
spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
- intel_crtc->mmio_flip.seqno = i915_gem_request_get_seqno(obj->last_write_req);
+ intel_crtc->mmio_flip.req = obj->last_write_req;
+ i915_gem_request_reference(intel_crtc->mmio_flip.req);
intel_crtc->mmio_flip.ring_id = obj->ring->id;
spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dd5e0f1..418ac13 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -395,7 +395,7 @@ struct intel_pipe_wm {
};
struct intel_mmio_flip {
- u32 seqno;
+ struct drm_i915_gem_request *req;
u32 ring_id;
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
2014-10-06 14:15 ` [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 15/21] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
2014-10-19 13:11 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' Daniel Vetter
2014-10-19 13:07 ` [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request Daniel Vetter
1 sibling, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/i915/intel_display.c | 9 ++++++---
drivers/gpu/drm/i915/intel_drv.h | 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 726a8f0..52ddd19 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -547,11 +547,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 f13bc30..26fdd96 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9879,7 +9879,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
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))
+ i915_gem_request_get_seqno(work->flip_queued_req)))
return false;
work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
@@ -10047,7 +10047,7 @@ 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);
+ 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,
@@ -10055,10 +10055,13 @@ 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));
+ work->flip_queued_req = intel_ring_get_request(ring);
work->flip_queued_ring = ring;
}
+ if (work->flip_queued_req)
+ i915_gem_request_reference(work->flip_queued_req);
+
work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
work->enable_stall_check = true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 418ac13..cb5af63 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -687,7 +687,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
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 15/21] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
2014-10-06 14:15 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 16/21] drm/i915: Convert __wait_seqno() to __wait_request() John.C.Harrison
2014-10-10 11:39 ` [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
2014-10-19 13:11 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' Daniel Vetter
1 sibling, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +--
drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++++++++++
drivers/gpu/drm/i915/i915_gem.c | 12 ++++--------
drivers/gpu/drm/i915/intel_display.c | 11 +++--------
drivers/gpu/drm/i915/intel_lrc.c | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
6 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 52ddd19..e764af9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -550,8 +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_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 6fe3be0..0790593 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1970,6 +1970,8 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req)
kref_put(&req->ref, i915_gem_request_free);
}
+/* ??? i915_gem_request_completed should be here ??? */
+
struct drm_i915_file_private {
struct drm_i915_private *dev_priv;
struct drm_file *file;
@@ -3019,4 +3021,19 @@ 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);
+
+ if (req->ring == NULL)
+ return false;
+
+ seqno = req->ring->get_seqno(req->ring, lazy_coherency);
+
+ return i915_seqno_passed(seqno, req->seqno);
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b28e39b..d4657e7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2253,8 +2253,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);
}
@@ -2522,12 +2521,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;
@@ -2671,7 +2667,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);
@@ -2686,7 +2682,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(ring, request->seqno);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26fdd96..9ca8f94 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
ring = obj->ring;
- if (i915_seqno_passed(ring->get_seqno(ring, true),
- i915_gem_request_get_seqno(obj->last_write_req)))
+ if (i915_gem_request_completed(obj->last_write_req, true))
return 0;
ret = i915_gem_check_olr(obj->last_write_req);
@@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
struct drm_i915_private *dev_priv = to_i915(ring->dev);
struct intel_crtc *intel_crtc;
unsigned long irq_flags;
- u32 seqno;
-
- seqno = ring->get_seqno(ring, false);
spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
for_each_intel_crtc(ring->dev, intel_crtc) {
@@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
if (ring->id != mmio_flip->ring_id)
continue;
- if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
+ if (i915_gem_request_completed(mmio_flip->req, false)) {
intel_do_mmio_flip(intel_crtc);
i915_gem_request_unreference(mmio_flip->req);
mmio_flip->req = NULL;
@@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
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),
- i915_gem_request_get_seqno(work->flip_queued_req)))
+ !i915_gem_request_completed(work->flip_queued_req, true))
return false;
work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0b68fc7..744684a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -807,6 +807,7 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
return -ENOMEM;
kref_init(&request->ref);
+ request->ring = NULL;
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 1be1934..4bbccdf 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2020,6 +2020,7 @@ intel_ring_alloc_seqno(struct intel_engine_cs *ring)
return -ENOMEM;
kref_init(&request->ref);
+ request->ring = NULL;
ret = i915_gem_get_seqno(ring->dev, &request->seqno);
if (ret) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 16/21] drm/i915: Convert __wait_seqno() to __wait_request()
2014-10-06 14:15 ` [RFC 15/21] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 17/21] drm/i915: Convert trace functions from seqno to request John.C.Harrison
2014-10-10 11:39 ` [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
1 sibling, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 57 +++++++++++++++++----------------------
1 file changed, 24 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d4657e7..f1e64d8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1133,10 +1133,9 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
}
/**
- * __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
+ * __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
*
@@ -1147,15 +1146,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.
*/
-static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
- unsigned reset_counter,
- bool interruptible,
- s64 *timeout,
- struct drm_i915_file_private *file_priv)
+static int __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 =
@@ -1167,7 +1167,7 @@ static int __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_gem_request_completed(req, true))
return 0;
timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
@@ -1184,7 +1184,7 @@ static int __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;
@@ -1203,7 +1203,7 @@ static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
break;
}
- if (i915_seqno_passed(ring->get_seqno(ring, false), seqno)) {
+ if (i915_gem_request_completed(req, false)) {
ret = 0;
break;
}
@@ -1235,7 +1235,7 @@ static int __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);
@@ -1279,9 +1279,9 @@ i915_wait_request(struct drm_i915_gem_request *req)
if (ret)
return ret;
- return __wait_seqno(req->ring, i915_gem_request_get_seqno(req),
- atomic_read(&dev_priv->gpu_error.reset_counter),
- interruptible, NULL, NULL);
+ return __wait_request(req,
+ atomic_read(&dev_priv->gpu_error.reset_counter),
+ interruptible, NULL, NULL);
}
static int
@@ -1335,7 +1335,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;
@@ -1357,7 +1356,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 = __wait_seqno(ring, i915_gem_request_get_seqno(req), reset_counter, true, NULL, file_priv);
+ ret = __wait_request(req, reset_counter, true, NULL, file_priv);
mutex_lock(&dev->struct_mutex);
i915_gem_request_unreference(req);
if (ret)
@@ -2816,9 +2815,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;
ret = i915_mutex_lock_interruptible(dev);
@@ -2840,9 +2837,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);
- BUG_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)
@@ -2857,8 +2851,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 = __wait_seqno(ring, seqno, reset_counter, true, &args->timeout_ns,
- file->driver_priv);
+ ret = __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);
@@ -4053,10 +4047,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);
@@ -4072,16 +4064,15 @@ 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 = __wait_seqno(ring, seqno, reset_counter, true, NULL, NULL);
+ ret = __wait_request(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
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 17/21] drm/i915: Convert trace functions from seqno to request
2014-10-06 14:15 ` [RFC 16/21] drm/i915: Convert __wait_seqno() to __wait_request() John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 18/21] drm/i915: Convert 'trace_irq' to use requests rather than seqnos John.C.Harrison
0 siblings, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 10 +++---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_trace.h | 47 ++++++++++++++++------------
3 files changed, 33 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f1e64d8..18f3605 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1184,7 +1184,7 @@ static int __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;
@@ -1235,7 +1235,7 @@ static int __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);
@@ -2419,7 +2419,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) {
@@ -2684,7 +2684,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
if (!i915_gem_request_completed(request, true))
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:
@@ -2902,7 +2902,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 e0fdfae..9bb8415 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1170,7 +1170,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 f5aa006..66616f7 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
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 18/21] drm/i915: Convert 'trace_irq' to use requests rather than seqnos
2014-10-06 14:15 ` [RFC 17/21] drm/i915: Convert trace functions from seqno to request John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos John.C.Harrison
0 siblings, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
Note: i915_trace_irq_get is no longer inline as it requires accessing the
internals of the request structure. As i915_drv.h includes intel_ringbuffer.h,
an inline within the former is never going to be able to use details from the
latter.
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 10 +++-------
drivers/gpu/drm/i915/i915_trace.h | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 8 ++------
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 18f3605..2e1ebad 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2646,15 +2646,11 @@ void i915_gem_reset(struct drm_device *dev)
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.
@@ -2707,10 +2703,10 @@ 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_gem_request_completed(ring->trace_irq_req, true))) {
ring->irq_put(ring);
- ring->trace_irq_seqno = 0;
+ ring->trace_irq_req = NULL;
}
WARN_ON(i915_verify_lists(ring->dev));
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 66616f7..c8e515d 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(req);
),
TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4bbccdf..c8ec78c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2791,3 +2791,11 @@ intel_stop_ring_buffer(struct intel_engine_cs *ring)
stop_ring(ring);
}
+
+void i915_trace_irq_get(struct drm_i915_gem_request *req)
+{
+ struct intel_engine_cs *ring = i915_gem_request_get_ring(req);
+
+ if (ring->trace_irq_req == NULL && ring->irq_get(ring))
+ ring->trace_irq_req = req;
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 98b219d..5475046 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);
@@ -435,11 +435,7 @@ 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;
-}
+void i915_trace_irq_get(struct drm_i915_gem_request *req);
/* DRI warts */
int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos
2014-10-06 14:15 ` [RFC 18/21] drm/i915: Convert 'trace_irq' to use requests rather than seqnos John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use " John.C.Harrison
2014-10-19 14:08 ` [RFC 19/21] drm/i915: Convert semaphores to handle " Daniel Vetter
0 siblings, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 ++-
drivers/gpu/drm/i915/i915_gem.c | 14 ++++++--------
drivers/gpu/drm/i915/i915_gpu_error.c | 12 ++++++++----
drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++----
drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++--
5 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e764af9..df53515 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2604,7 +2604,8 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
seq_puts(m, "\nSync seqno:\n");
for_each_ring(ring, dev_priv, i) {
for (j = 0; j < num_rings; j++) {
- seq_printf(m, " 0x%08x ", ring->semaphore.sync_seqno[j]);
+ seq_printf(m, " 0x%08x ",
+ i915_gem_request_get_seqno(ring->semaphore.sync_req[j]));
}
seq_putc(m, '\n');
}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2e1ebad..d40dad7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2275,8 +2275,8 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
for_each_ring(ring, dev_priv, i) {
intel_ring_init_seqno(ring, seqno);
- for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_seqno); j++)
- ring->semaphore.sync_seqno[j] = 0;
+ for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_req); j++)
+ ring->semaphore.sync_req[j] = NULL;
}
return 0;
@@ -2877,7 +2877,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
struct intel_engine_cs *to)
{
struct intel_engine_cs *from = obj->ring;
- u32 seqno;
int ret, idx;
if (from == NULL || to == from)
@@ -2888,10 +2887,10 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
idx = intel_ring_sync_index(from, to);
- 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]) /* <--- broken?! needs to use i915_seqno_passed()??? */
+ if (i915_gem_request_get_seqno(obj->last_read_req) <=
+ i915_gem_request_get_seqno(from->semaphore.sync_req[idx])) /* <--- broken?! needs to use i915_seqno_passed()??? */
return 0;
ret = i915_gem_check_olr(obj->last_read_req);
@@ -2899,14 +2898,13 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
return ret;
trace_i915_gem_ring_sync_to(from, to, obj->last_read_req);
- ret = to->semaphore.sync_to(to, from, seqno);
+ ret = to->semaphore.sync_to(to, from, obj->last_read_req);
if (!ret)
/* We use last_read_req because sync_to()
* might have just caused seqno wrap under
* the radar.
*/
- from->semaphore.sync_seqno[idx] =
- i915_gem_request_get_seqno(obj->last_read_req);
+ from->semaphore.sync_req[idx] = obj->last_read_req;
return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 1b58390..9545d96 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -822,7 +822,8 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
idx = intel_ring_sync_index(ring, to);
ering->semaphore_mboxes[idx] = tmp[signal_offset];
- ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
+ ering->semaphore_seqno[idx] =
+ i915_gem_request_get_seqno(ring->semaphore.sync_req[idx]);
}
}
@@ -832,13 +833,16 @@ static void gen6_record_semaphore_state(struct drm_i915_private *dev_priv,
{
ering->semaphore_mboxes[0] = I915_READ(RING_SYNC_0(ring->mmio_base));
ering->semaphore_mboxes[1] = I915_READ(RING_SYNC_1(ring->mmio_base));
- ering->semaphore_seqno[0] = ring->semaphore.sync_seqno[0];
- ering->semaphore_seqno[1] = ring->semaphore.sync_seqno[1];
+ ering->semaphore_seqno[0] =
+ i915_gem_request_get_seqno(ring->semaphore.sync_req[0]);
+ ering->semaphore_seqno[1] =
+ i915_gem_request_get_seqno(ring->semaphore.sync_req[1]);
if (HAS_VEBOX(dev_priv->dev)) {
ering->semaphore_mboxes[2] =
I915_READ(RING_SYNC_2(ring->mmio_base));
- ering->semaphore_seqno[2] = ring->semaphore.sync_seqno[2];
+ ering->semaphore_seqno[2] =
+ i915_gem_request_get_seqno(ring->semaphore.sync_req[2]);
}
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c8ec78c..0a3c24a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1042,7 +1042,7 @@ static inline bool i915_gem_has_seqno_wrapped(struct drm_device *dev,
static int
gen8_ring_sync(struct intel_engine_cs *waiter,
struct intel_engine_cs *signaller,
- u32 seqno)
+ struct drm_i915_gem_request *req)
{
struct drm_i915_private *dev_priv = waiter->dev->dev_private;
int ret;
@@ -1055,7 +1055,7 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
MI_SEMAPHORE_GLOBAL_GTT |
MI_SEMAPHORE_POLL |
MI_SEMAPHORE_SAD_GTE_SDD);
- intel_ring_emit(waiter, seqno);
+ intel_ring_emit(waiter, i915_gem_request_get_seqno(req));
intel_ring_emit(waiter,
lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
intel_ring_emit(waiter,
@@ -1067,18 +1067,20 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
static int
gen6_ring_sync(struct intel_engine_cs *waiter,
struct intel_engine_cs *signaller,
- u32 seqno)
+ struct drm_i915_gem_request *req)
{
u32 dw1 = MI_SEMAPHORE_MBOX |
MI_SEMAPHORE_COMPARE |
MI_SEMAPHORE_REGISTER;
u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id];
int ret;
+ u32 seqno;
/* Throughout all of the GEM code, seqno passed implies our current
* seqno is >= the last seqno executed. However for hardware the
* comparison is strictly greater than.
*/
+ seqno = i915_gem_request_get_seqno(req);
seqno -= 1;
WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
@@ -1794,7 +1796,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
INIT_LIST_HEAD(&ring->execlist_queue);
ringbuf->size = 32 * PAGE_SIZE;
ringbuf->ring = ring;
- memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno));
+ memset(ring->semaphore.sync_req, 0, sizeof(ring->semaphore.sync_req));
init_waitqueue_head(&ring->irq_queue);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5475046..64a4346 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -211,7 +211,7 @@ struct intel_engine_cs {
* ie. transpose of f(x, y)
*/
struct {
- u32 sync_seqno[I915_NUM_RINGS-1];
+ struct drm_i915_gem_request *sync_req[I915_NUM_RINGS-1];
union {
struct {
@@ -226,7 +226,7 @@ struct intel_engine_cs {
/* AKA wait() */
int (*sync_to)(struct intel_engine_cs *ring,
struct intel_engine_cs *to,
- u32 seqno);
+ struct drm_i915_gem_request *req);
int (*signal)(struct intel_engine_cs *signaller,
/* num_dwords needed by caller */
unsigned int num_dwords);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos
2014-10-06 14:15 ` [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-06 14:15 ` [RFC 21/21] drm/i915: Remove 'obj->ring' John.C.Harrison
2014-10-19 14:09 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos Daniel Vetter
2014-10-19 14:08 ` [RFC 19/21] drm/i915: Convert semaphores to handle " Daniel Vetter
1 sibling, 2 replies; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4847ed5..c2a7127 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3030,18 +3030,18 @@ 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_gem_request_completed(ring_last_request(ring), false));
}
static bool
@@ -3261,7 +3261,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
^ permalink raw reply related [flat|nested] 72+ messages in thread* [RFC 21/21] drm/i915: Remove 'obj->ring'
2014-10-06 14:15 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use " John.C.Harrison
@ 2014-10-06 14:15 ` John.C.Harrison
2014-10-19 14:12 ` Daniel Vetter
2014-10-19 14:09 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos Daniel Vetter
1 sibling, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-06 14:15 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: 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 | 12 +++++++++++-
drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
6 files changed, 46 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index df53515..b1d989f 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->last_read_req) /* 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->last_read_req)
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 0790593..cdbbdeb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1859,8 +1859,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 d40dad7..561fb81 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2184,14 +2184,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 */
obj->last_write_req = req;
}
- obj->ring = ring;
/* Add a reference if we're newly entering the active list. */
if (!obj->active) {
@@ -2230,7 +2234,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;
obj->last_read_req = NULL;
obj->last_write_req = NULL;
@@ -2247,9 +2250,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))
@@ -2769,14 +2770,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;
@@ -2876,9 +2880,11 @@ 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;
int ret, idx;
+ from = i915_gem_request_get_ring(obj->last_read_req);
+
if (from == NULL || to == from)
return 0;
@@ -3889,7 +3895,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;
@@ -4303,9 +4309,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 a5221d8..8f24831 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -613,7 +613,17 @@ 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); */
+ /* NB: last_read_req has already been updated to the current
+ * request however, that request has not yet been submitted.
+ * Thus last_read_req->ring is guaranteed to be null!
+ * NB2: Doing the check before the update of last_read_req
+ * (which happens in i915_vma_move_to_active() just above),
+ * also fails because last_read_req is almost always null on
+ * entry!
+ */
/* 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 9545d96..b9ecbd9 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -681,7 +681,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 9ca8f94..8238aac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9728,7 +9728,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)
@@ -9769,8 +9769,6 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
if (!obj->last_write_req)
return 0;
- ring = obj->ring;
-
if (i915_gem_request_completed(obj->last_write_req, true))
return 0;
@@ -9778,6 +9776,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
if (ret)
return ret;
+ ring = i915_gem_request_get_ring(obj->last_write_req);
if (WARN_ON(!ring->irq_get(ring)))
return 0;
@@ -9837,14 +9836,15 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
intel_crtc->mmio_flip.req = obj->last_write_req;
i915_gem_request_reference(intel_crtc->mmio_flip.req);
- intel_crtc->mmio_flip.ring_id = obj->ring->id;
+ BUG_ON(ring != i915_gem_request_get_ring(intel_crtc->mmio_flip.req));
+ intel_crtc->mmio_flip.ring_id = ring->id;
spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
/*
* Double check to catch cases where irq fired before
* mmio flip data was ready
*/
- intel_notify_mmio_flip(obj->ring);
+ intel_notify_mmio_flip(ring);
return 0;
}
@@ -10022,7 +10022,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 {
@@ -10043,7 +10043,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
goto cleanup_unpin;
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
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 21/21] drm/i915: Remove 'obj->ring'
2014-10-06 14:15 ` [RFC 21/21] drm/i915: Remove 'obj->ring' John.C.Harrison
@ 2014-10-19 14:12 ` Daniel Vetter
2014-10-28 15:09 ` John Harrison
0 siblings, 1 reply; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 14:12 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
I think this should be split up into the different parts:
- s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
the current ring.
- s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
would actually be more readable if we'd check for obj->active instead.
- All the oddball special cases probably deserve their own commit + nice
explanation in the commit message about why the change is correct.
Cheers, Daniel
> ---
> 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 | 12 +++++++++++-
> drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
> 6 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index df53515..b1d989f 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->last_read_req) /* 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->last_read_req)
> 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 0790593..cdbbdeb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1859,8 +1859,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 d40dad7..561fb81 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2184,14 +2184,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 */
> obj->last_write_req = req;
> }
> - obj->ring = ring;
>
> /* Add a reference if we're newly entering the active list. */
> if (!obj->active) {
> @@ -2230,7 +2234,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;
>
> obj->last_read_req = NULL;
> obj->last_write_req = NULL;
> @@ -2247,9 +2250,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))
> @@ -2769,14 +2770,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;
> @@ -2876,9 +2880,11 @@ 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;
> int ret, idx;
>
> + from = i915_gem_request_get_ring(obj->last_read_req);
> +
> if (from == NULL || to == from)
> return 0;
>
> @@ -3889,7 +3895,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;
> @@ -4303,9 +4309,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 a5221d8..8f24831 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -613,7 +613,17 @@ 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); */
> + /* NB: last_read_req has already been updated to the current
> + * request however, that request has not yet been submitted.
> + * Thus last_read_req->ring is guaranteed to be null!
> + * NB2: Doing the check before the update of last_read_req
> + * (which happens in i915_vma_move_to_active() just above),
> + * also fails because last_read_req is almost always null on
> + * entry!
> + */
>
> /* 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 9545d96..b9ecbd9 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -681,7 +681,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 9ca8f94..8238aac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9728,7 +9728,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)
> @@ -9769,8 +9769,6 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> if (!obj->last_write_req)
> return 0;
>
> - ring = obj->ring;
> -
> if (i915_gem_request_completed(obj->last_write_req, true))
> return 0;
>
> @@ -9778,6 +9776,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> if (ret)
> return ret;
>
> + ring = i915_gem_request_get_ring(obj->last_write_req);
> if (WARN_ON(!ring->irq_get(ring)))
> return 0;
>
> @@ -9837,14 +9836,15 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
> intel_crtc->mmio_flip.req = obj->last_write_req;
> i915_gem_request_reference(intel_crtc->mmio_flip.req);
> - intel_crtc->mmio_flip.ring_id = obj->ring->id;
> + BUG_ON(ring != i915_gem_request_get_ring(intel_crtc->mmio_flip.req));
> + intel_crtc->mmio_flip.ring_id = ring->id;
> spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
>
> /*
> * Double check to catch cases where irq fired before
> * mmio flip data was ready
> */
> - intel_notify_mmio_flip(obj->ring);
> + intel_notify_mmio_flip(ring);
> return 0;
> }
>
> @@ -10022,7 +10022,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 {
> @@ -10043,7 +10043,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> goto cleanup_unpin;
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 21/21] drm/i915: Remove 'obj->ring'
2014-10-19 14:12 ` Daniel Vetter
@ 2014-10-28 15:09 ` John Harrison
2014-11-03 10:38 ` Daniel Vetter
0 siblings, 1 reply; 72+ messages in thread
From: John Harrison @ 2014-10-28 15:09 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-GFX
On 19/10/2014 15:12, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> For: VIZ-4377
>> Signed-off-by: John.C.Harrison@Intel.com
> I think this should be split up into the different parts:
>
> - s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
> the current ring.
> - s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
> would actually be more readable if we'd check for obj->active instead.
> - All the oddball special cases probably deserve their own commit + nice
> explanation in the commit message about why the change is correct.
Can you explain which is which? As for why the change is correct, it is
not a functional change. obj->ring was assigned with
obj->last_read_seqno and cleared with obj->last_read_seqno. Thus
querying obj->ring is equivalent to querying obj->last_read_req->ring in
all cases. Given that the ring is now available from obj->lrr, it seemed
redundant to also have it explicitly saved in obj->ring, hence the patch
to remove it.
AFAICT, there are two examples in debugfs that should just be querying
obj->active. The rest are pretty much wanting the currently in use ring
and/or are about to use last_read_req anyway. It seems more sensible to
say 'if(obj->lrr) { do_stuff(obj->lrr); }' than 'if(obj->active) {
do_stuff(obj->lrr); }'. Nothing looks particularly 'oddball' to me!
> Cheers, Daniel
>> ---
>> 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 | 12 +++++++++++-
>> drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
>> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>> 6 files changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index df53515..b1d989f 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->last_read_req) /* 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->last_read_req)
>> 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 0790593..cdbbdeb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1859,8 +1859,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 d40dad7..561fb81 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2184,14 +2184,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 */
>> obj->last_write_req = req;
>> }
>> - obj->ring = ring;
>>
>> /* Add a reference if we're newly entering the active list. */
>> if (!obj->active) {
>> @@ -2230,7 +2234,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;
>>
>> obj->last_read_req = NULL;
>> obj->last_write_req = NULL;
>> @@ -2247,9 +2250,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))
>> @@ -2769,14 +2770,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;
>> @@ -2876,9 +2880,11 @@ 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;
>> int ret, idx;
>>
>> + from = i915_gem_request_get_ring(obj->last_read_req);
>> +
>> if (from == NULL || to == from)
>> return 0;
>>
>> @@ -3889,7 +3895,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;
>> @@ -4303,9 +4309,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 a5221d8..8f24831 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -613,7 +613,17 @@ 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); */
>> + /* NB: last_read_req has already been updated to the current
>> + * request however, that request has not yet been submitted.
>> + * Thus last_read_req->ring is guaranteed to be null!
>> + * NB2: Doing the check before the update of last_read_req
>> + * (which happens in i915_vma_move_to_active() just above),
>> + * also fails because last_read_req is almost always null on
>> + * entry!
>> + */
>>
>> /* 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 9545d96..b9ecbd9 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -681,7 +681,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 9ca8f94..8238aac 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9728,7 +9728,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)
>> @@ -9769,8 +9769,6 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>> if (!obj->last_write_req)
>> return 0;
>>
>> - ring = obj->ring;
>> -
>> if (i915_gem_request_completed(obj->last_write_req, true))
>> return 0;
>>
>> @@ -9778,6 +9776,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>> if (ret)
>> return ret;
>>
>> + ring = i915_gem_request_get_ring(obj->last_write_req);
>> if (WARN_ON(!ring->irq_get(ring)))
>> return 0;
>>
>> @@ -9837,14 +9836,15 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
>> intel_crtc->mmio_flip.req = obj->last_write_req;
>> i915_gem_request_reference(intel_crtc->mmio_flip.req);
>> - intel_crtc->mmio_flip.ring_id = obj->ring->id;
>> + BUG_ON(ring != i915_gem_request_get_ring(intel_crtc->mmio_flip.req));
>> + intel_crtc->mmio_flip.ring_id = ring->id;
>> spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
>>
>> /*
>> * Double check to catch cases where irq fired before
>> * mmio flip data was ready
>> */
>> - intel_notify_mmio_flip(obj->ring);
>> + intel_notify_mmio_flip(ring);
>> return 0;
>> }
>>
>> @@ -10022,7 +10022,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 {
>> @@ -10043,7 +10043,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>> goto cleanup_unpin;
>>
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 21/21] drm/i915: Remove 'obj->ring'
2014-10-28 15:09 ` John Harrison
@ 2014-11-03 10:38 ` Daniel Vetter
0 siblings, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-11-03 10:38 UTC (permalink / raw)
To: John Harrison; +Cc: Intel-GFX
On Tue, Oct 28, 2014 at 03:09:07PM +0000, John Harrison wrote:
> On 19/10/2014 15:12, Daniel Vetter wrote:
> >On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@Intel.com wrote:
> >>From: John Harrison <John.C.Harrison@Intel.com>
> >>
> >>For: VIZ-4377
> >>Signed-off-by: John.C.Harrison@Intel.com
> >I think this should be split up into the different parts:
> >
> >- s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
> > the current ring.
> >- s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
> > would actually be more readable if we'd check for obj->active instead.
> >- All the oddball special cases probably deserve their own commit + nice
> > explanation in the commit message about why the change is correct.
>
> Can you explain which is which? As for why the change is correct, it is not
> a functional change. obj->ring was assigned with obj->last_read_seqno and
> cleared with obj->last_read_seqno. Thus querying obj->ring is equivalent to
> querying obj->last_read_req->ring in all cases. Given that the ring is now
> available from obj->lrr, it seemed redundant to also have it explicitly
> saved in obj->ring, hence the patch to remove it.
>
> AFAICT, there are two examples in debugfs that should just be querying
> obj->active. The rest are pretty much wanting the currently in use ring
> and/or are about to use last_read_req anyway. It seems more sensible to say
> 'if(obj->lrr) { do_stuff(obj->lrr); }' than 'if(obj->active) {
> do_stuff(obj->lrr); }'. Nothing looks particularly 'oddball' to me!
[snip]
> >>+ if (obj->last_read_req) /* XXX per-vma statistic */
> >> stats->active += obj->base.size;
Stuff like this here is what I've meant. Checking for obj->active here is
better, and you might as well ditch the XXX comment too.
You're right that it's all semantically equivalent and your change here is
the less risky since there's no way behaviour can change. But code is
mostly written so that other humans can understand it (when debugging it),
so conveying as much meaning as possible is important. And this looks like
a good opportunity to review all the various places and make sure the code
is sane. We have accumulated a bit of cruft from the per-vma rework and
there's more cruft on the horizon with potentially multiple read fences,
so I think this is worth it.
-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
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos
2014-10-06 14:15 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use " John.C.Harrison
2014-10-06 14:15 ` [RFC 21/21] drm/i915: Remove 'obj->ring' John.C.Harrison
@ 2014-10-19 14:09 ` Daniel Vetter
2014-10-28 14:03 ` John Harrison
1 sibling, 1 reply; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 14:09 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:24PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
We have places that shovel stuff onto the ring without an explicit
add_request. Or at least we've had, so this needs a full audit, and that
audit needs to be in the commit message.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_irq.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4847ed5..c2a7127 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3030,18 +3030,18 @@ 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_gem_request_completed(ring_last_request(ring), false));
> }
>
> static bool
> @@ -3261,7 +3261,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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos
2014-10-19 14:09 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos Daniel Vetter
@ 2014-10-28 14:03 ` John Harrison
2014-11-03 10:44 ` Daniel Vetter
0 siblings, 1 reply; 72+ messages in thread
From: John Harrison @ 2014-10-28 14:03 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-GFX
On 19/10/2014 15:09, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:24PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> For: VIZ-4377
>> Signed-off-by: John.C.Harrison@Intel.com
> We have places that shovel stuff onto the ring without an explicit
> add_request. Or at least we've had, so this needs a full audit, and that
> audit needs to be in the commit message.
> -Daniel
Not sure what you mean. There is no functional change here. The old
version pulled the seqno out of the last request entry in the list and
compared that to the hardware seqno value to check for completion. The
new version gets the same request entry and does the same completion
test, just in request parlance. If commands are being dumped on the ring
without an request being added to the request list then the old version
would still not have been checking for them. It would still only look at
the last piece of work that actually did call add_request and see if
that has completed or not.
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 4847ed5..c2a7127 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3030,18 +3030,18 @@ 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_gem_request_completed(ring_last_request(ring), false));
>> }
>>
>> static bool
>> @@ -3261,7 +3261,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos
2014-10-28 14:03 ` John Harrison
@ 2014-11-03 10:44 ` Daniel Vetter
0 siblings, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-11-03 10:44 UTC (permalink / raw)
To: John Harrison; +Cc: Intel-GFX
On Tue, Oct 28, 2014 at 02:03:14PM +0000, John Harrison wrote:
> On 19/10/2014 15:09, Daniel Vetter wrote:
> >We have places that shovel stuff onto the ring without an explicit
> >add_request. Or at least we've had, so this needs a full audit, and that
> >audit needs to be in the commit message.
> Not sure what you mean. There is no functional change here. The old version
> pulled the seqno out of the last request entry in the list and compared that
> to the hardware seqno value to check for completion. The new version gets
> the same request entry and does the same completion test, just in request
> parlance. If commands are being dumped on the ring without an request being
> added to the request list then the old version would still not have been
> checking for them. It would still only look at the last piece of work that
> actually did call add_request and see if that has completed or not.
Oh, I've pretty badly mixed stuff up ;-)
The issue I'm seeing here though is acessing the request list, from irq
context, without any locks. Pre-existing, but still deserves a FIXME
comment.
Wrt the change itself I'm not sure it's that useful, this is fairly
low-level code.
-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
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos
2014-10-06 14:15 ` [RFC 19/21] drm/i915: Convert semaphores to handle requests not seqnos John.C.Harrison
2014-10-06 14:15 ` [RFC 20/21] drm/i915: Convert 'ring_idle()' to use " John.C.Harrison
@ 2014-10-19 14:08 ` Daniel Vetter
1 sibling, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 14:08 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:23PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
This smells funky on a quick look. I'd expect some reference counting in
here, or at least an explanatation for why it's not needed.
But even without that I'm not sure we want to track requests at the
semaphore level. After all hw semaphores work with seqno numbers and not
requests, and depending upon how the scheduler materializes seqnos
shoveling requests through these functions makes no sense at all.
I'm maybe missing something big here, but with the thin commit message I
can't tell. So I vote to just drop this one for now.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 ++-
> drivers/gpu/drm/i915/i915_gem.c | 14 ++++++--------
> drivers/gpu/drm/i915/i915_gpu_error.c | 12 ++++++++----
> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++--
> 5 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e764af9..df53515 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2604,7 +2604,8 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
> seq_puts(m, "\nSync seqno:\n");
> for_each_ring(ring, dev_priv, i) {
> for (j = 0; j < num_rings; j++) {
> - seq_printf(m, " 0x%08x ", ring->semaphore.sync_seqno[j]);
> + seq_printf(m, " 0x%08x ",
> + i915_gem_request_get_seqno(ring->semaphore.sync_req[j]));
> }
> seq_putc(m, '\n');
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2e1ebad..d40dad7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2275,8 +2275,8 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
> for_each_ring(ring, dev_priv, i) {
> intel_ring_init_seqno(ring, seqno);
>
> - for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_seqno); j++)
> - ring->semaphore.sync_seqno[j] = 0;
> + for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_req); j++)
> + ring->semaphore.sync_req[j] = NULL;
> }
>
> return 0;
> @@ -2877,7 +2877,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
> struct intel_engine_cs *to)
> {
> struct intel_engine_cs *from = obj->ring;
> - u32 seqno;
> int ret, idx;
>
> if (from == NULL || to == from)
> @@ -2888,10 +2887,10 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>
> idx = intel_ring_sync_index(from, to);
>
> - 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]) /* <--- broken?! needs to use i915_seqno_passed()??? */
> + if (i915_gem_request_get_seqno(obj->last_read_req) <=
> + i915_gem_request_get_seqno(from->semaphore.sync_req[idx])) /* <--- broken?! needs to use i915_seqno_passed()??? */
> return 0;
>
> ret = i915_gem_check_olr(obj->last_read_req);
> @@ -2899,14 +2898,13 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
> return ret;
>
> trace_i915_gem_ring_sync_to(from, to, obj->last_read_req);
> - ret = to->semaphore.sync_to(to, from, seqno);
> + ret = to->semaphore.sync_to(to, from, obj->last_read_req);
> if (!ret)
> /* We use last_read_req because sync_to()
> * might have just caused seqno wrap under
> * the radar.
> */
> - from->semaphore.sync_seqno[idx] =
> - i915_gem_request_get_seqno(obj->last_read_req);
> + from->semaphore.sync_req[idx] = obj->last_read_req;
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 1b58390..9545d96 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -822,7 +822,8 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> idx = intel_ring_sync_index(ring, to);
>
> ering->semaphore_mboxes[idx] = tmp[signal_offset];
> - ering->semaphore_seqno[idx] = ring->semaphore.sync_seqno[idx];
> + ering->semaphore_seqno[idx] =
> + i915_gem_request_get_seqno(ring->semaphore.sync_req[idx]);
> }
> }
>
> @@ -832,13 +833,16 @@ static void gen6_record_semaphore_state(struct drm_i915_private *dev_priv,
> {
> ering->semaphore_mboxes[0] = I915_READ(RING_SYNC_0(ring->mmio_base));
> ering->semaphore_mboxes[1] = I915_READ(RING_SYNC_1(ring->mmio_base));
> - ering->semaphore_seqno[0] = ring->semaphore.sync_seqno[0];
> - ering->semaphore_seqno[1] = ring->semaphore.sync_seqno[1];
> + ering->semaphore_seqno[0] =
> + i915_gem_request_get_seqno(ring->semaphore.sync_req[0]);
> + ering->semaphore_seqno[1] =
> + i915_gem_request_get_seqno(ring->semaphore.sync_req[1]);
>
> if (HAS_VEBOX(dev_priv->dev)) {
> ering->semaphore_mboxes[2] =
> I915_READ(RING_SYNC_2(ring->mmio_base));
> - ering->semaphore_seqno[2] = ring->semaphore.sync_seqno[2];
> + ering->semaphore_seqno[2] =
> + i915_gem_request_get_seqno(ring->semaphore.sync_req[2]);
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c8ec78c..0a3c24a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1042,7 +1042,7 @@ static inline bool i915_gem_has_seqno_wrapped(struct drm_device *dev,
> static int
> gen8_ring_sync(struct intel_engine_cs *waiter,
> struct intel_engine_cs *signaller,
> - u32 seqno)
> + struct drm_i915_gem_request *req)
> {
> struct drm_i915_private *dev_priv = waiter->dev->dev_private;
> int ret;
> @@ -1055,7 +1055,7 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
> MI_SEMAPHORE_GLOBAL_GTT |
> MI_SEMAPHORE_POLL |
> MI_SEMAPHORE_SAD_GTE_SDD);
> - intel_ring_emit(waiter, seqno);
> + intel_ring_emit(waiter, i915_gem_request_get_seqno(req));
> intel_ring_emit(waiter,
> lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
> intel_ring_emit(waiter,
> @@ -1067,18 +1067,20 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
> static int
> gen6_ring_sync(struct intel_engine_cs *waiter,
> struct intel_engine_cs *signaller,
> - u32 seqno)
> + struct drm_i915_gem_request *req)
> {
> u32 dw1 = MI_SEMAPHORE_MBOX |
> MI_SEMAPHORE_COMPARE |
> MI_SEMAPHORE_REGISTER;
> u32 wait_mbox = signaller->semaphore.mbox.wait[waiter->id];
> int ret;
> + u32 seqno;
>
> /* Throughout all of the GEM code, seqno passed implies our current
> * seqno is >= the last seqno executed. However for hardware the
> * comparison is strictly greater than.
> */
> + seqno = i915_gem_request_get_seqno(req);
> seqno -= 1;
>
> WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
> @@ -1794,7 +1796,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
> INIT_LIST_HEAD(&ring->execlist_queue);
> ringbuf->size = 32 * PAGE_SIZE;
> ringbuf->ring = ring;
> - memset(ring->semaphore.sync_seqno, 0, sizeof(ring->semaphore.sync_seqno));
> + memset(ring->semaphore.sync_req, 0, sizeof(ring->semaphore.sync_req));
>
> init_waitqueue_head(&ring->irq_queue);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5475046..64a4346 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -211,7 +211,7 @@ struct intel_engine_cs {
> * ie. transpose of f(x, y)
> */
> struct {
> - u32 sync_seqno[I915_NUM_RINGS-1];
> + struct drm_i915_gem_request *sync_req[I915_NUM_RINGS-1];
>
> union {
> struct {
> @@ -226,7 +226,7 @@ struct intel_engine_cs {
> /* AKA wait() */
> int (*sync_to)(struct intel_engine_cs *ring,
> struct intel_engine_cs *to,
> - u32 seqno);
> + struct drm_i915_gem_request *req);
> int (*signal)(struct intel_engine_cs *signaller,
> /* num_dwords needed by caller */
> unsigned int num_dwords);
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
2014-10-06 14:15 ` [RFC 15/21] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
2014-10-06 14:15 ` [RFC 16/21] drm/i915: Convert __wait_seqno() to __wait_request() John.C.Harrison
@ 2014-10-10 11:39 ` John.C.Harrison
2014-10-19 14:04 ` Daniel Vetter
1 sibling, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-10 11:39 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: 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 | 12 ++++--------
drivers/gpu/drm/i915/intel_display.c | 11 +++--------
4 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 52ddd19..e764af9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -550,8 +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_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 6fe3be0..650f712 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req)
kref_put(&req->ref, i915_gem_request_free);
}
+/*
+ * NB: 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;
@@ -3019,4 +3025,16 @@ 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);
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e1a58f1..b602e8c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2253,8 +2253,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);
}
@@ -2521,12 +2520,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;
@@ -2670,7 +2666,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);
@@ -2685,7 +2681,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(ring, request->seqno);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26fdd96..9ca8f94 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
ring = obj->ring;
- if (i915_seqno_passed(ring->get_seqno(ring, true),
- i915_gem_request_get_seqno(obj->last_write_req)))
+ if (i915_gem_request_completed(obj->last_write_req, true))
return 0;
ret = i915_gem_check_olr(obj->last_write_req);
@@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
struct drm_i915_private *dev_priv = to_i915(ring->dev);
struct intel_crtc *intel_crtc;
unsigned long irq_flags;
- u32 seqno;
-
- seqno = ring->get_seqno(ring, false);
spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
for_each_intel_crtc(ring->dev, intel_crtc) {
@@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
if (ring->id != mmio_flip->ring_id)
continue;
- if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
+ if (i915_gem_request_completed(mmio_flip->req, false)) {
intel_do_mmio_flip(intel_crtc);
i915_gem_request_unreference(mmio_flip->req);
mmio_flip->req = NULL;
@@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
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),
- i915_gem_request_get_seqno(work->flip_queued_req)))
+ !i915_gem_request_completed(work->flip_queued_req, true))
return false;
work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
2014-10-10 11:39 ` [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
@ 2014-10-19 14:04 ` Daniel Vetter
2014-10-28 14:02 ` John Harrison
0 siblings, 1 reply; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 14:04 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Fri, Oct 10, 2014 at 12:39:49PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: 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 | 12 ++++--------
> drivers/gpu/drm/i915/intel_display.c | 11 +++--------
> 4 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 52ddd19..e764af9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -550,8 +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_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 6fe3be0..650f712 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req)
> kref_put(&req->ref, i915_gem_request_free);
> }
>
> +/*
> + * NB: 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;
> @@ -3019,4 +3025,16 @@ 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);
> +}
Tbh this thing looks a bit big for a static inline. Have you
double-checked that inlining this doesn't increase binary size? If it
does then we need performance data justifying the cost. Same probably
applies to some of the other static inline functions you've added.
-Daniel
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e1a58f1..b602e8c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2253,8 +2253,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);
> }
>
> @@ -2521,12 +2520,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;
> @@ -2670,7 +2666,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);
> @@ -2685,7 +2681,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(ring, request->seqno);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 26fdd96..9ca8f94 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>
> ring = obj->ring;
>
> - if (i915_seqno_passed(ring->get_seqno(ring, true),
> - i915_gem_request_get_seqno(obj->last_write_req)))
> + if (i915_gem_request_completed(obj->last_write_req, true))
> return 0;
>
> ret = i915_gem_check_olr(obj->last_write_req);
> @@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
> struct drm_i915_private *dev_priv = to_i915(ring->dev);
> struct intel_crtc *intel_crtc;
> unsigned long irq_flags;
> - u32 seqno;
> -
> - seqno = ring->get_seqno(ring, false);
>
> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
> for_each_intel_crtc(ring->dev, intel_crtc) {
> @@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
> if (ring->id != mmio_flip->ring_id)
> continue;
>
> - if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
> + if (i915_gem_request_completed(mmio_flip->req, false)) {
> intel_do_mmio_flip(intel_crtc);
> i915_gem_request_unreference(mmio_flip->req);
> mmio_flip->req = NULL;
> @@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
>
> 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),
> - i915_gem_request_get_seqno(work->flip_queued_req)))
> + !i915_gem_request_completed(work->flip_queued_req, true))
> return false;
>
> work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
2014-10-19 14:04 ` Daniel Vetter
@ 2014-10-28 14:02 ` John Harrison
0 siblings, 0 replies; 72+ messages in thread
From: John Harrison @ 2014-10-28 14:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-GFX
On 19/10/2014 15:04, Daniel Vetter wrote:
> On Fri, Oct 10, 2014 at 12:39:49PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> For: VIZ-4377
>> Signed-off-by: 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 | 12 ++++--------
>> drivers/gpu/drm/i915/intel_display.c | 11 +++--------
>> 4 files changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 52ddd19..e764af9 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -550,8 +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_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 6fe3be0..650f712 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req)
>> kref_put(&req->ref, i915_gem_request_free);
>> }
>>
>> +/*
>> + * NB: 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;
>> @@ -3019,4 +3025,16 @@ 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);
>> +}
> Tbh this thing looks a bit big for a static inline. Have you
> double-checked that inlining this doesn't increase binary size? If it
> does then we need performance data justifying the cost. Same probably
> applies to some of the other static inline functions you've added.
> -Daniel
It is a replacement for i915_seqno_passed() which is a static inline so
it seemed sensible that this should be too. Note that the ultimate aim
is for this to be a simple 'return req->completed'. At which point it
should certainly be worthy of inlining.
But no I haven't done any actual analysis of the benefit/cost yet. As
has been mentioned, this RFC was a quick knock up to get something to
discuss as soon as possible.
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index e1a58f1..b602e8c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2253,8 +2253,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);
>> }
>>
>> @@ -2521,12 +2520,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;
>> @@ -2670,7 +2666,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);
>> @@ -2685,7 +2681,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(ring, request->seqno);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 26fdd96..9ca8f94 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>>
>> ring = obj->ring;
>>
>> - if (i915_seqno_passed(ring->get_seqno(ring, true),
>> - i915_gem_request_get_seqno(obj->last_write_req)))
>> + if (i915_gem_request_completed(obj->last_write_req, true))
>> return 0;
>>
>> ret = i915_gem_check_olr(obj->last_write_req);
>> @@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
>> struct drm_i915_private *dev_priv = to_i915(ring->dev);
>> struct intel_crtc *intel_crtc;
>> unsigned long irq_flags;
>> - u32 seqno;
>> -
>> - seqno = ring->get_seqno(ring, false);
>>
>> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
>> for_each_intel_crtc(ring->dev, intel_crtc) {
>> @@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
>> if (ring->id != mmio_flip->ring_id)
>> continue;
>>
>> - if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
>> + if (i915_gem_request_completed(mmio_flip->req, false)) {
>> intel_do_mmio_flip(intel_crtc);
>> i915_gem_request_unreference(mmio_flip->req);
>> mmio_flip->req = NULL;
>> @@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
>>
>> 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),
>> - i915_gem_request_get_seqno(work->flip_queued_req)))
>> + !i915_gem_request_completed(work->flip_queued_req, true))
>> return false;
>>
>> work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
>> --
>> 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
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request'
2014-10-06 14:15 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' John.C.Harrison
2014-10-06 14:15 ` [RFC 15/21] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed' John.C.Harrison
@ 2014-10-19 13:11 ` Daniel Vetter
1 sibling, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 13:11 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:18PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
Again on the topic of thin commit message: Some additional words about why
we can noodle around in request structures from hardirq context (those
fields are invariant after add_request) would be good. Since with the
scheduler a lot of that stuff suddenly is a lot less invariant (if we
delay the seqno allocation until we emit requests, which I'm pretty sure
we have to for a bunch of reasons).
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/intel_display.c | 9 ++++++---
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 726a8f0..52ddd19 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -547,11 +547,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)));
Yeah it's no your patch, but this stuff has defintely fallen over the
edge. A prep patch to extract some helper functions to bring sanity to
i915_gem_pageflip_info's indentation levels is needed here imo.
-Daniel
> } 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 f13bc30..26fdd96 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9879,7 +9879,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
> 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))
> + i915_gem_request_get_seqno(work->flip_queued_req)))
> return false;
>
> work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> @@ -10047,7 +10047,7 @@ 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);
> + 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,
> @@ -10055,10 +10055,13 @@ 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));
> + work->flip_queued_req = intel_ring_get_request(ring);
> work->flip_queued_ring = ring;
> }
>
> + if (work->flip_queued_req)
> + i915_gem_request_reference(work->flip_queued_req);
> +
> work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> work->enable_stall_check = true;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 418ac13..cb5af63 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,7 +687,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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request
2014-10-06 14:15 ` [RFC 13/21] drm/i915: Convert mmio_flip::seqno to struct request John.C.Harrison
2014-10-06 14:15 ` [RFC 14/21] drm/i915: Convert 'flip_queued_seqno' into 'flip_queued_request' John.C.Harrison
@ 2014-10-19 13:07 ` Daniel Vetter
1 sibling, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 13:07 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:17PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
Again on the topic of thin commit messages: Beyond the boilerplate blabla
explaining why we do all the s/seqno/request/ stuff for the benefit of the
future reader this definitely needs a mention of the refcounting needed
for the async work.
-Daniel
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
> ---
> drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2af421e..f13bc30 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9799,15 +9799,16 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring)
> struct intel_mmio_flip *mmio_flip;
>
> mmio_flip = &intel_crtc->mmio_flip;
> - if (mmio_flip->seqno == 0)
> + if (mmio_flip->req == NULL)
> continue;
>
> if (ring->id != mmio_flip->ring_id)
> continue;
>
> - if (i915_seqno_passed(seqno, mmio_flip->seqno)) {
> + if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) {
> intel_do_mmio_flip(intel_crtc);
> - mmio_flip->seqno = 0;
> + i915_gem_request_unreference(mmio_flip->req);
> + mmio_flip->req = NULL;
> ring->irq_put(ring);
> }
> }
> @@ -9826,7 +9827,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> unsigned long irq_flags;
> int ret;
>
> - if (WARN_ON(intel_crtc->mmio_flip.seqno))
> + if (WARN_ON(intel_crtc->mmio_flip.req))
> return -EBUSY;
>
> ret = intel_postpone_flip(obj);
> @@ -9838,7 +9839,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> }
>
> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
> - intel_crtc->mmio_flip.seqno = i915_gem_request_get_seqno(obj->last_write_req);
> + intel_crtc->mmio_flip.req = obj->last_write_req;
> + i915_gem_request_reference(intel_crtc->mmio_flip.req);
> intel_crtc->mmio_flip.ring_id = obj->ring->id;
> spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dd5e0f1..418ac13 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -395,7 +395,7 @@ struct intel_pipe_wm {
> };
>
> struct intel_mmio_flip {
> - u32 seqno;
> + struct drm_i915_gem_request *req;
> u32 ring_id;
> };
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno
2014-10-06 14:15 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno John.C.Harrison
2014-10-06 14:15 ` [RFC 11/21] drm/i915: Convert i915_wait_seqno to i915_wait_request John.C.Harrison
@ 2014-10-19 12:57 ` Daniel Vetter
1 sibling, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 12:57 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:14PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
I know there's often not a lot to talk about for if you have a refactoring
step that needs to be applied n times. But even then a small commit
message to reiterate what is going on and why and a small note if there's
anything funky goes a long way.
Since in a few months someone will digg out your patch here using git
blame and git lock --pickaxe and will be totally lost without the context
of the entire series. So each patch really needs to be able to be
understood on its own.
-Daniel
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
> ---
> drivers/gpu/drm/i915/intel_overlay.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index dc2f4f2..ccd5732 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,17 @@ 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);
> + ret = i915_add_request(ring, &overlay->last_flip_req->seqno);
> 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;
> + overlay->last_flip_req = NULL;
> return 0;
> }
>
> @@ -286,7 +286,7 @@ 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);
> + return i915_add_request(ring, &overlay->last_flip_req->seqno);
> }
>
> static void intel_overlay_release_old_vid_tail(struct intel_overlay *overlay)
> @@ -366,10 +366,10 @@ 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 +377,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;
> + 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
2014-10-06 14:15 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno John.C.Harrison
2014-10-06 14:15 ` [RFC 10/21] drm/i915: Convert 'last_flip_req' to be a request not a seqno John.C.Harrison
@ 2014-10-19 12:55 ` Daniel Vetter
2014-10-28 14:01 ` John Harrison
1 sibling, 1 reply; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 12:55 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
To thin commit message.
Also I wonder whethere we should track the olr state more explicitly in
the request structure instead of jumping through all these hoops. And
explicit olr state for a request might also help to clarify control flow
and checks in a bunch of places.
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
> ---
> drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++-
> drivers/gpu/drm/i915/i915_gem.c | 25 ++++++++++---------------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 3 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1401266..9504206 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2483,7 +2483,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)
> {
> @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> }
> }
>
> +/************************* Deprecated *************************/
There's a gcc flag for functions if you want to mark this as depracated,
otherwise I'd just go with a "XXX: This temporary function will disappear
at the end of the patch series" somewhere.
> +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno)
> +{
> + int ret;
> +
> + BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
WARN_ON instead of BUG_ON for locking checks.
> +
> + ret = 0;
> + if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
> + ret = i915_add_request(ring, NULL);
> +
> + return ret;
> +}
> +/************************* Deprecated *************************/
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0d0eb26..2c7deca 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1097,19 +1097,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));
> + BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
Same here. Plus please a stern warning in the commit message and cc: to
whomever who managed to sneak this in (and failed to catch it in review).
>
> 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;
> }
> @@ -1271,7 +1270,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;
>
> @@ -1338,7 +1337,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));
> @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
> if (!req)
> return 0;
>
> - seqno = i915_gem_request_get_seqno(req);
> - BUG_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 = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv);
> + ret = __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)
> @@ -2786,7 +2781,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;
>
> @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
> if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */
> return 0;
>
> - ret = i915_gem_check_olr(obj->ring, seqno);
> + ret = i915_gem_check_olr(obj->last_read_req);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6da18c5..2af421e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> i915_gem_request_get_seqno(obj->last_write_req)))
> return 0;
>
> - ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req));
> + ret = i915_gem_check_olr(obj->last_write_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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
2014-10-19 12:55 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno Daniel Vetter
@ 2014-10-28 14:01 ` John Harrison
2014-11-03 10:51 ` Daniel Vetter
0 siblings, 1 reply; 72+ messages in thread
From: John Harrison @ 2014-10-28 14:01 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-GFX
On 19/10/2014 13:55, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
> To thin commit message.
>
> Also I wonder whethere we should track the olr state more explicitly in
> the request structure instead of jumping through all these hoops. And
> explicit olr state for a request might also help to clarify control flow
> and checks in a bunch of places.
What would be nicer would be to get rid of the OLR completely. If code
wants to write to the ring then it should request a request structure at
the start, pass that request in to all the ring writing code and submit
the same request structure at the end. The whole idea of some random
request floating around in the ether scooping up any ring accesses that
happen to occur with no idea about ownership, sequence, etc. just seems
highly dodgy. Indeed it definitely causes issues with the scheduler with
things like page flips being mixed up in completely unrelated batch
buffer submissions! I believe that is one of the things Chris did in his
re-write the universe patch.
>> For: VIZ-4377
>> Signed-off-by: John.C.Harrison@Intel.com
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++-
>> drivers/gpu/drm/i915/i915_gem.c | 25 ++++++++++---------------
>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>> 3 files changed, 27 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1401266..9504206 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2483,7 +2483,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)
>> {
>> @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>> }
>> }
>>
>> +/************************* Deprecated *************************/
> There's a gcc flag for functions if you want to mark this as depracated,
> otherwise I'd just go with a "XXX: This temporary function will disappear
> at the end of the patch series" somewhere.
>
>> +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno)
>> +{
>> + int ret;
>> +
>> + BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> WARN_ON instead of BUG_ON for locking checks.
>
>> +
>> + ret = 0;
>> + if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
>> + ret = i915_add_request(ring, NULL);
>> +
>> + return ret;
>> +}
>> +/************************* Deprecated *************************/
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 0d0eb26..2c7deca 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1097,19 +1097,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));
>> + BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
> Same here. Plus please a stern warning in the commit message and cc: to
> whomever who managed to sneak this in (and failed to catch it in review).
>>
>> 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;
>> }
>> @@ -1271,7 +1270,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;
>>
>> @@ -1338,7 +1337,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));
>> @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
>> if (!req)
>> return 0;
>>
>> - seqno = i915_gem_request_get_seqno(req);
>> - BUG_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 = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv);
>> + ret = __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)
>> @@ -2786,7 +2781,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;
>>
>> @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>> if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */
>> return 0;
>>
>> - ret = i915_gem_check_olr(obj->ring, seqno);
>> + ret = i915_gem_check_olr(obj->last_read_req);
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 6da18c5..2af421e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>> i915_gem_request_get_seqno(obj->last_write_req)))
>> return 0;
>>
>> - ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req));
>> + ret = i915_gem_check_olr(obj->last_write_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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
2014-10-28 14:01 ` John Harrison
@ 2014-11-03 10:51 ` Daniel Vetter
0 siblings, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-11-03 10:51 UTC (permalink / raw)
To: John Harrison; +Cc: Intel-GFX
On Tue, Oct 28, 2014 at 02:01:52PM +0000, John Harrison wrote:
> On 19/10/2014 13:55, Daniel Vetter wrote:
> >On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@Intel.com wrote:
> >>From: John Harrison <John.C.Harrison@Intel.com>
> >To thin commit message.
> >
> >Also I wonder whethere we should track the olr state more explicitly in
> >the request structure instead of jumping through all these hoops. And
> >explicit olr state for a request might also help to clarify control flow
> >and checks in a bunch of places.
>
> What would be nicer would be to get rid of the OLR completely. If code wants
> to write to the ring then it should request a request structure at the
> start, pass that request in to all the ring writing code and submit the same
> request structure at the end. The whole idea of some random request floating
> around in the ether scooping up any ring accesses that happen to occur with
> no idea about ownership, sequence, etc. just seems highly dodgy. Indeed it
> definitely causes issues with the scheduler with things like page flips
> being mixed up in completely unrelated batch buffer submissions! I believe
> that is one of the things Chris did in his re-write the universe patch.
Oh OLR is highly dodgy ;-)
It is unfortunately a bit of work to fix things up, especially without a
rewrite-the-universe patch. Perhaps:
1. Explicitly preallocate the request, still store it at the same place.
2. Pass the explicit request to add_request, add any missing add_requests
call where we currently relied on the olr to scoop up register writes.
3. Shovel the request pointer from the engine to the ring (since that's
where we need it it for execlist), that means we need to allocate the
request for a specific engine. Steps 1&3 might be better if switched,
dunno.
4. Sprinkle asserts over all ring functions to make sure the request is
there already.
5. Throw away olr logic.
6. Optional, refactoring over the next few years: Use requests as the
primary cmd submission object everywhere.
Comments?
-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
^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC 08/25] drm/i915: Remove 'outstanding_lazy_seqno'
2014-10-06 14:15 ` [RFC 08/21] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
2014-10-06 14:15 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno John.C.Harrison
@ 2014-10-10 11:38 ` John.C.Harrison
2014-10-19 13:05 ` Daniel Vetter
2014-10-19 12:48 ` [RFC 08/21] " Daniel Vetter
2 siblings, 1 reply; 72+ messages in thread
From: John.C.Harrison @ 2014-10-10 11:38 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 13 +++----
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 2 +-
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, 46 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9ce0808..c884188 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1108,7 +1108,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;
@@ -2344,7 +2344,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;
@@ -2391,7 +2391,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
i915_gem_request_reference(request);
- request->seqno = intel_ring_get_seqno(ring);
request->ring = ring;
request->head = request_start;
request->tail = request_ring_position;
@@ -2428,8 +2427,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);
@@ -2605,9 +2603,8 @@ 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;
- ring->outstanding_lazy_seqno = 0;
+ kfree(ring->outstanding_lazy_request);
+ 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 4250211..e0fdfae 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1170,7 +1170,7 @@ 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 5aae3d1e..6da18c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10053,7 +10053,7 @@ 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 b5b6430..55c607a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -793,22 +793,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;
- /* 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) {
- BUG_ON(ring->preallocated_lazy_request == NULL);
+ if (ring->outstanding_lazy_request)
return 0;
- }
- BUG_ON(ring->preallocated_lazy_request != NULL);
request = kmalloc(sizeof(*request), GFP_KERNEL);
if (request == NULL)
@@ -816,7 +808,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;
@@ -829,7 +821,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;
}
@@ -997,7 +989,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;
@@ -1212,7 +1204,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);
@@ -1235,8 +1228,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;
- ring->outstanding_lazy_seqno = 0;
+ 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 7350f78..99544bc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -895,17 +895,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));
@@ -933,16 +936,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);
@@ -971,9 +977,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);
}
}
@@ -1008,7 +1016,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);
@@ -1126,7 +1135,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 */
@@ -1145,7 +1155,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);
@@ -1385,7 +1396,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);
@@ -1839,8 +1851,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;
- ring->outstanding_lazy_seqno = 0;
+ ring->outstanding_lazy_request = NULL;
if (ring->cleanup)
ring->cleanup(ring);
@@ -1980,7 +1991,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;
@@ -1998,22 +2009,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;
- /* 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) {
- BUG_ON(ring->preallocated_lazy_request == NULL);
+ if (ring->outstanding_lazy_request)
return 0;
- }
-
- BUG_ON(ring->preallocated_lazy_request != NULL);
request = kmalloc(sizeof(*request), GFP_KERNEL);
if (request == NULL)
@@ -2021,13 +2023,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;
}
@@ -2068,7 +2070,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;
@@ -2103,7 +2105,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 d98b964..98b219d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -265,8 +265,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;
@@ -429,17 +428,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
^ permalink raw reply related [flat|nested] 72+ messages in thread* Re: [RFC 08/25] drm/i915: Remove 'outstanding_lazy_seqno'
2014-10-10 11:38 ` [RFC 08/25] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
@ 2014-10-19 13:05 ` Daniel Vetter
0 siblings, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 13:05 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Fri, Oct 10, 2014 at 12:38:22PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
This looks like a v2 of the previous patch 08, but the commit message
fails to mention what exactly changed and why.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 13 +++----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 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, 46 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9ce0808..c884188 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1108,7 +1108,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;
> @@ -2344,7 +2344,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;
>
> @@ -2391,7 +2391,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
>
> i915_gem_request_reference(request);
>
> - request->seqno = intel_ring_get_seqno(ring);
> request->ring = ring;
> request->head = request_start;
> request->tail = request_ring_position;
> @@ -2428,8 +2427,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);
> @@ -2605,9 +2603,8 @@ 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;
> - ring->outstanding_lazy_seqno = 0;
> + kfree(ring->outstanding_lazy_request);
> + 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 4250211..e0fdfae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1170,7 +1170,7 @@ 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 5aae3d1e..6da18c5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10053,7 +10053,7 @@ 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 b5b6430..55c607a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -793,22 +793,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;
>
> - /* 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) {
> - BUG_ON(ring->preallocated_lazy_request == NULL);
> + if (ring->outstanding_lazy_request)
> return 0;
> - }
> - BUG_ON(ring->preallocated_lazy_request != NULL);
>
> request = kmalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> @@ -816,7 +808,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;
> @@ -829,7 +821,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;
> }
>
> @@ -997,7 +989,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;
>
> @@ -1212,7 +1204,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);
> @@ -1235,8 +1228,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;
> - ring->outstanding_lazy_seqno = 0;
> + 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 7350f78..99544bc 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -895,17 +895,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));
> @@ -933,16 +936,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);
> @@ -971,9 +977,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);
> }
> }
>
> @@ -1008,7 +1016,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);
>
> @@ -1126,7 +1135,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 */
> @@ -1145,7 +1155,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);
>
> @@ -1385,7 +1396,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);
>
> @@ -1839,8 +1851,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;
> - ring->outstanding_lazy_seqno = 0;
> + ring->outstanding_lazy_request = NULL;
>
> if (ring->cleanup)
> ring->cleanup(ring);
> @@ -1980,7 +1991,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;
> @@ -1998,22 +2009,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;
>
> - /* 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) {
> - BUG_ON(ring->preallocated_lazy_request == NULL);
> + if (ring->outstanding_lazy_request)
> return 0;
> - }
> -
> - BUG_ON(ring->preallocated_lazy_request != NULL);
>
> request = kmalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> @@ -2021,13 +2023,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;
> }
>
> @@ -2068,7 +2070,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;
>
> @@ -2103,7 +2105,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 d98b964..98b219d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -265,8 +265,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;
>
> @@ -429,17 +428,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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC 08/21] drm/i915: Remove 'outstanding_lazy_seqno'
2014-10-06 14:15 ` [RFC 08/21] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
2014-10-06 14:15 ` [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno John.C.Harrison
2014-10-10 11:38 ` [RFC 08/25] drm/i915: Remove 'outstanding_lazy_seqno' John.C.Harrison
@ 2014-10-19 12:48 ` Daniel Vetter
2014-10-19 12:50 ` Daniel Vetter
2 siblings, 1 reply; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 12:48 UTC (permalink / raw)
To: John.C.Harrison; +Cc: Intel-GFX
On Mon, Oct 06, 2014 at 03:15:12PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com
given the scary commit message for patch 3 I'm confused that we can just
do this. Or maybe we should move patch 3 right before this one here and
instead of mentioning fun in combination with the scheduler as the
justification use this patch here. Since obviously if we dig out the ols
from the plr then they obviously need to be allocated together.
In any case this patch here should at least mention why this is suddenly
possible without fuzz. A few comments below.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 13 +++-----
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 20 ++++--------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 48 +++++++++++++++-------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 13 ++------
> 6 files changed, 41 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 53b48ad..0d0eb26 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1108,7 +1108,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;
> @@ -2344,7 +2344,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;
>
> @@ -2391,7 +2391,6 @@ int __i915_add_request(struct intel_engine_cs *ring,
>
> i915_gem_request_reference(request);
>
> - request->seqno = intel_ring_get_seqno(ring);
> request->ring = ring;
> request->head = request_start;
> request->tail = request_ring_position;
> @@ -2428,8 +2427,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);
> @@ -2605,9 +2603,8 @@ 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;
> - ring->outstanding_lazy_seqno = 0;
> + kfree(ring->outstanding_lazy_request);
> + ring->outstanding_lazy_request = NULL;
Somewhat unrelated, but shouldn't we move to refcount even the ring->plr?
If we allocate it then the ring->plr pointer would hold the ref, then it
gets transferred to the request list.
This isn't too terribly important yet, but sometimes (when we have
per-engine reset) we need to fix up all the request access the hangcheck
and error state capture code does. Which probably means we really need to
refcount requests super-carefully everywhere (because the hangcheck can
happen almost anywhere at all).
> }
>
> 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 4250211..e0fdfae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1170,7 +1170,7 @@ 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 5aae3d1e..6da18c5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10053,7 +10053,7 @@ 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 b5b6430..7005745 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -799,16 +799,8 @@ static int logical_ring_alloc_seqno(struct intel_engine_cs *ring,
> struct drm_i915_gem_request *request;
> int ret;
>
> - /* 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 you add a comment only to remove it later on in the series I usually
smash an "XXX: This will get cleaned up further" or similar on top of it.
it aids review if you can gauge this whitout peeking ahead.
> -
> - if (ring->outstanding_lazy_seqno) {
> - BUG_ON(ring->preallocated_lazy_request == NULL);
> + if (ring->outstanding_lazy_request)
> return 0;
> - }
> - BUG_ON(ring->preallocated_lazy_request != NULL);
>
> request = kmalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> @@ -816,7 +808,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;
> @@ -829,7 +821,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;
> }
>
> @@ -1212,7 +1204,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);
> @@ -1235,8 +1228,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;
> - ring->outstanding_lazy_seqno = 0;
> + 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 7350f78..dd59691 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -895,17 +895,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));
> @@ -933,16 +936,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);
> @@ -971,9 +977,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);
> }
> }
>
> @@ -1008,7 +1016,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);
>
> @@ -1126,7 +1135,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 */
> @@ -1145,7 +1155,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);
>
> @@ -1385,7 +1396,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);
>
> @@ -1839,8 +1851,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;
> - ring->outstanding_lazy_seqno = 0;
> + ring->outstanding_lazy_request = NULL;
>
> if (ring->cleanup)
> ring->cleanup(ring);
> @@ -1980,7 +1991,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;
> @@ -2003,17 +2014,8 @@ intel_ring_alloc_seqno(struct intel_engine_cs *ring)
> int ret;
> struct drm_i915_gem_request *request;
>
> - /* 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) {
> - BUG_ON(ring->preallocated_lazy_request == NULL);
> + if (ring->outstanding_lazy_request)
> return 0;
> - }
> -
> - BUG_ON(ring->preallocated_lazy_request != NULL);
>
> request = kmalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> @@ -2021,13 +2023,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;
> }
>
> @@ -2103,7 +2105,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 d98b964..98b219d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -265,8 +265,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;
>
> @@ -429,17 +428,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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread* Re: [RFC 08/21] drm/i915: Remove 'outstanding_lazy_seqno'
2014-10-19 12:48 ` [RFC 08/21] " Daniel Vetter
@ 2014-10-19 12:50 ` Daniel Vetter
0 siblings, 0 replies; 72+ messages in thread
From: Daniel Vetter @ 2014-10-19 12:50 UTC (permalink / raw)
To: John Harrison; +Cc: intel-gfx
On Sun, Oct 19, 2014 at 2:48 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> @@ -2605,9 +2603,8 @@ 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;
>> - ring->outstanding_lazy_seqno = 0;
>> + kfree(ring->outstanding_lazy_request);
>> + ring->outstanding_lazy_request = NULL;
>
> Somewhat unrelated, but shouldn't we move to refcount even the ring->plr?
> If we allocate it then the ring->plr pointer would hold the ref, then it
> gets transferred to the request list.
Actually this is 100% related to this patch, since it replaces the ol
seqno with the ol request. Would be good to clarify this in the commit
message, since this patch doesn't remove the logic around olr, but
really just does the switch from seqno to request.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 72+ messages in thread