diff mbox

[6/8] drm/i915: Introduce execlist_port_* accessors

Message ID 20170920143705.11277-7-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Sept. 20, 2017, 2:37 p.m. UTC 
Instead of trusting that first available port is at index 0,
use accessor to hide this. This is a preparation for a
following patches where head can be at arbitrary location
in the port array.

v2: improved commit message, elsp_ready readability (Chris)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 16 +++++++----
 drivers/gpu/drm/i915/i915_gpu_error.c      |  4 +--
 drivers/gpu/drm/i915/i915_guc_submission.c | 17 ++++++-----
 drivers/gpu/drm/i915/i915_irq.c            |  2 +-
 drivers/gpu/drm/i915/intel_engine_cs.c     |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c           | 42 +++++++++++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.h    | 46 ++++++++++++++++++++++++++----
 7 files changed, 87 insertions(+), 42 deletions(-)

Comments

Chris Wilson Sept. 21, 2017, 12:26 p.m. UTC | #1 
Quoting Mika Kuoppala (2017-09-20 15:37:03)
> Instead of trusting that first available port is at index 0,
> use accessor to hide this. This is a preparation for a
> following patches where head can be at arbitrary location
> in the port array.
> 
> v2: improved commit message, elsp_ready readability (Chris)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        | 16 +++++++----
>  drivers/gpu/drm/i915/i915_gpu_error.c      |  4 +--
>  drivers/gpu/drm/i915/i915_guc_submission.c | 17 ++++++-----
>  drivers/gpu/drm/i915/i915_irq.c            |  2 +-
>  drivers/gpu/drm/i915/intel_engine_cs.c     |  2 +-
>  drivers/gpu/drm/i915/intel_lrc.c           | 42 +++++++++++++++------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    | 46 ++++++++++++++++++++++++++----
>  7 files changed, 87 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index dbeb6f08ab79..af8cc2eab1b1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3348,16 +3348,20 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>  
>                         rcu_read_lock();
>                         for (idx = 0; idx < execlist_num_ports(el); idx++) {
> -                               unsigned int count;
> +                               const struct execlist_port *port;
> +                               unsigned int count, n;
>  
> -                               rq = port_unpack(&el->port[idx], &count);
> +                               port = execlist_port_index(el, idx);
> +                               n = port_index(port, el);

Bah, execlist_port_index() implies to me that it should return the
index, not the port. I would just call it execlist_port(). How does that
look?

> -static inline void
> +#define __port_idx(start, index, mask) (((start) + (index)) & (mask))
> +
> +static inline struct execlist_port *
> +execlist_port_head(struct intel_engine_execlist * const el)
> +{
> +       return &el->port[el->port_head];
> +}
> +
> +/* Index starting from port_head */
> +static inline struct execlist_port *
> +execlist_port_index(struct intel_engine_execlist * const el,
> +                   const unsigned int n)
> +{
> +       return &el->port[__port_idx(el->port_head, n, el->port_mask)];
> +}
> +
> +static inline struct execlist_port *
> +execlist_port_tail(struct intel_engine_execlist * const el)
> +{
> +       return &el->port[__port_idx(el->port_head, -1, el->port_mask)];
> +}

Hmm, I was expecting

execlist_port_head() { return execlist_port(el, 0); }
execlist_port_tail() { return execlist_port(el, -1); }

What's the impact on object size? (As a quick guide to how much the
compiler can keep the code in check.)
-Chris
Mika Kuoppala Sept. 21, 2017, 2:45 p.m. UTC | #2 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2017-09-20 15:37:03)
>> Instead of trusting that first available port is at index 0,
>> use accessor to hide this. This is a preparation for a
>> following patches where head can be at arbitrary location
>> in the port array.
>> 
>> v2: improved commit message, elsp_ready readability (Chris)
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c        | 16 +++++++----
>>  drivers/gpu/drm/i915/i915_gpu_error.c      |  4 +--
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 17 ++++++-----
>>  drivers/gpu/drm/i915/i915_irq.c            |  2 +-
>>  drivers/gpu/drm/i915/intel_engine_cs.c     |  2 +-
>>  drivers/gpu/drm/i915/intel_lrc.c           | 42 +++++++++++++++------------
>>  drivers/gpu/drm/i915/intel_ringbuffer.h    | 46 ++++++++++++++++++++++++++----
>>  7 files changed, 87 insertions(+), 42 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index dbeb6f08ab79..af8cc2eab1b1 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3348,16 +3348,20 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>>  
>>                         rcu_read_lock();
>>                         for (idx = 0; idx < execlist_num_ports(el); idx++) {
>> -                               unsigned int count;
>> +                               const struct execlist_port *port;
>> +                               unsigned int count, n;
>>  
>> -                               rq = port_unpack(&el->port[idx], &count);
>> +                               port = execlist_port_index(el, idx);
>> +                               n = port_index(port, el);
>
> Bah, execlist_port_index() implies to me that it should return the
> index, not the port. I would just call it execlist_port(). How does that
> look?

It looks much better.

>
>> -static inline void
>> +#define __port_idx(start, index, mask) (((start) + (index)) & (mask))
>> +
>> +static inline struct execlist_port *
>> +execlist_port_head(struct intel_engine_execlist * const el)
>> +{
>> +       return &el->port[el->port_head];
>> +}
>> +
>> +/* Index starting from port_head */
>> +static inline struct execlist_port *
>> +execlist_port_index(struct intel_engine_execlist * const el,
>> +                   const unsigned int n)
>> +{
>> +       return &el->port[__port_idx(el->port_head, n, el->port_mask)];
>> +}
>> +
>> +static inline struct execlist_port *
>> +execlist_port_tail(struct intel_engine_execlist * const el)
>> +{
>> +       return &el->port[__port_idx(el->port_head, -1, el->port_mask)];
>> +}
>
> Hmm, I was expecting
>
> execlist_port_head() { return execlist_port(el, 0); }
> execlist_port_tail() { return execlist_port(el, -1); }

Seems that I did these on the next patch, moved to here.

>
> What's the impact on object size? (As a quick guide to how much the
> compiler can keep the code in check.)

I can't say what would constitute as a still in check, but things
grow:

add/remove: 0/0 grow/shrink: 6/1 up/down: 277/-26 (251)
function                                     old     new   delta
intel_lrc_irq_handler                       1591    1700    +109
i915_guc_irq_handler                        1041    1110     +69
i915_engine_info                            1983    2031     +48
insert_request                               127     152     +25
intel_engine_is_idle                         304     317     +13
gen8_cs_irq_handler                          113     126     +13
capture                                     5454    5428     -26
Total: Before=1144612, After=1144863, chg +0.02%

-Mika
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index dbeb6f08ab79..af8cc2eab1b1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3348,16 +3348,20 @@  static int i915_engine_info(struct seq_file *m, void *unused)
 
 			rcu_read_lock();
 			for (idx = 0; idx < execlist_num_ports(el); idx++) {
-				unsigned int count;
+				const struct execlist_port *port;
+				unsigned int count, n;
 
-				rq = port_unpack(&el->port[idx], &count);
+				port = execlist_port_index(el, idx);
+				n = port_index(port, el);
+
+				rq = port_unpack(port, &count);
 				if (rq) {
-					seq_printf(m, "\t\tELSP[%d] count=%d, ",
-						   idx, count);
+					seq_printf(m, "\t\tELSP[%d:%d] count=%d, ",
+						   idx, n, count);
 					print_request(m, rq, "rq: ");
 				} else {
-					seq_printf(m, "\t\tELSP[%d] idle\n",
-						   idx);
+					seq_printf(m, "\t\tELSP[%d:%d] idle\n",
+						   idx, n);
 				}
 			}
 			rcu_read_unlock();
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 0a803d76256b..19e4c297c857 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1332,11 +1332,11 @@  static void engine_record_requests(struct intel_engine_cs *engine,
 static void error_record_engine_execlists(struct intel_engine_cs *engine,
 					  struct drm_i915_error_engine *ee)
 {
-	const struct intel_engine_execlist * const el = &engine->execlist;
+	struct intel_engine_execlist * const el = &engine->execlist;
 	unsigned int n;
 
 	for (n = 0; n < execlist_num_ports(el); n++) {
-		struct drm_i915_gem_request *rq = port_request(&el->port[n]);
+		struct drm_i915_gem_request *rq = port_request(execlist_port_index(el, n));
 
 		if (!rq)
 			break;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3a4f875d5930..25c9bac94c39 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -562,8 +562,7 @@  static void i915_guc_dequeue(struct intel_engine_cs *engine)
 	struct intel_engine_execlist * const el = &engine->execlist;
 	struct execlist_port *port = el->port;
 	struct drm_i915_gem_request *last = NULL;
-	const struct execlist_port * const last_port =
-		&el->port[el->port_mask];
+	const struct execlist_port * const last_port = execlist_port_tail(el);
 	bool submit = false;
 	struct rb_node *rb;
 
@@ -587,7 +586,8 @@  static void i915_guc_dequeue(struct intel_engine_cs *engine)
 
 				if (submit)
 					port_assign(port, last);
-				port++;
+
+				port = execlist_port_next(el, port);
 			}
 
 			INIT_LIST_HEAD(&rq->priotree.link);
@@ -618,19 +618,18 @@  static void i915_guc_irq_handler(unsigned long data)
 {
 	struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
 	struct intel_engine_execlist * const el = &engine->execlist;
-	struct execlist_port *port = el->port;
-	const struct execlist_port * const last_port =
-		&el->port[el->port_mask];
+	struct execlist_port *port = execlist_port_head(el);
+	const struct execlist_port * const last_port = execlist_port_tail(el);
 	struct drm_i915_gem_request *rq;
 
-	rq = port_request(&port[0]);
+	rq = port_request(port);
 	while (rq && i915_gem_request_completed(rq)) {
 		trace_i915_gem_request_out(rq);
 		i915_gem_request_put(rq);
 
-		execlist_port_complete(el, port);
+		port = execlist_port_complete(el, port);
 
-		rq = port_request(&port[0]);
+		rq = port_request(port);
 	}
 
 	if (!port_isset(last_port))
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index de703c26e124..ac5a95439393 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1342,7 +1342,7 @@  gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 	bool tasklet = false;
 
 	if (iir & (GT_CONTEXT_SWITCH_INTERRUPT << test_shift)) {
-		if (port_count(&el->port[0])) {
+		if (port_count(execlist_port_head(el))) {
 			__set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
 			tasklet = true;
 		}
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 523d56084870..b0d702063a50 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1502,7 +1502,7 @@  bool intel_engine_is_idle(struct intel_engine_cs *engine)
 		return false;
 
 	/* Both ports drained, no more ELSP submission? */
-	if (port_request(&engine->execlist.port[0]))
+	if (port_request(execlist_port_head(&engine->execlist)))
 		return false;
 
 	/* ELSP is empty, but there are ready requests? */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c659745e6b7b..8550cd6635c9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -394,24 +394,27 @@  static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 
 static void execlists_submit_ports(struct intel_engine_cs *engine)
 {
-	struct execlist_port *port = engine->execlist.port;
+	struct intel_engine_execlist * const el = &engine->execlist;
 	u32 __iomem *elsp =
 		engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
 	unsigned int n;
 
-	for (n = execlist_num_ports(&engine->execlist); n--; ) {
+	for (n = execlist_num_ports(el); n--; ) {
+		struct execlist_port *port;
 		struct drm_i915_gem_request *rq;
 		unsigned int count;
 		u64 desc;
 
-		rq = port_unpack(&port[n], &count);
+		port = execlist_port_index(el, n);
+
+		rq = port_unpack(port, &count);
 		if (rq) {
 			GEM_BUG_ON(count > !n);
 			if (!count++)
 				execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
-			port_set(&port[n], port_pack(rq, count));
+			port_set(port, port_pack(rq, count));
 			desc = execlists_update_context(rq);
-			GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
+			GEM_DEBUG_EXEC(port->context_id = upper_32_bits(desc));
 		} else {
 			GEM_BUG_ON(!n);
 			desc = 0;
@@ -455,9 +458,8 @@  static void execlists_dequeue(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *last;
 	struct intel_engine_execlist * const el = &engine->execlist;
-	struct execlist_port *port = el->port;
-	const struct execlist_port * const last_port =
-		&el->port[el->port_mask];
+	struct execlist_port *port = execlist_port_head(el);
+	const struct execlist_port * const last_port = execlist_port_tail(el);
 	struct rb_node *rb;
 	bool submit = false;
 
@@ -541,7 +543,8 @@  static void execlists_dequeue(struct intel_engine_cs *engine)
 
 				if (submit)
 					port_assign(port, last);
-				port++;
+
+				port = execlist_port_next(el, port);
 
 				GEM_BUG_ON(port_isset(port));
 			}
@@ -638,11 +641,12 @@  static void execlists_cancel_requests(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
 }
 
-static bool execlists_elsp_ready(const struct intel_engine_cs *engine)
+static bool execlists_elsp_ready(struct intel_engine_execlist * const el)
 {
-	const struct execlist_port *port = engine->execlist.port;
+	struct execlist_port * const port0 = execlist_port_head(el);
+	struct execlist_port * const port1 = execlist_port_next(el, port0);
 
-	return port_count(&port[0]) + port_count(&port[1]) < 2;
+	return port_count(port0) + port_count(port1) < 2;
 }
 
 /*
@@ -653,7 +657,7 @@  static void intel_lrc_irq_handler(unsigned long data)
 {
 	struct intel_engine_cs * const engine = (struct intel_engine_cs *)data;
 	struct intel_engine_execlist * const el = &engine->execlist;
-	struct execlist_port *port = el->port;
+	struct execlist_port *port = execlist_port_head(el);
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	/* We can skip acquiring intel_runtime_pm_get() here as it was taken
@@ -751,7 +755,7 @@  static void intel_lrc_irq_handler(unsigned long data)
 				trace_i915_gem_request_out(rq);
 				i915_gem_request_put(rq);
 
-				execlist_port_complete(el, port);
+				port = execlist_port_complete(el, port);
 			} else {
 				port_set(port, port_pack(rq, count));
 			}
@@ -768,7 +772,7 @@  static void intel_lrc_irq_handler(unsigned long data)
 		}
 	}
 
-	if (execlists_elsp_ready(engine))
+	if (execlists_elsp_ready(el))
 		execlists_dequeue(engine);
 
 	intel_uncore_forcewake_put(dev_priv, el->fw_domains);
@@ -778,16 +782,18 @@  static void insert_request(struct intel_engine_cs *engine,
 			   struct i915_priotree *pt,
 			   int prio)
 {
+	struct intel_engine_execlist * const el = &engine->execlist;
 	struct i915_priolist *p = lookup_priolist(engine, pt, prio);
 
 	list_add_tail(&pt->link, &ptr_mask_bits(p, 1)->requests);
-	if (ptr_unmask_bits(p, 1) && execlists_elsp_ready(engine))
-		tasklet_hi_schedule(&engine->execlist.irq_tasklet);
+	if (ptr_unmask_bits(p, 1) && execlists_elsp_ready(el))
+		tasklet_hi_schedule(&el->irq_tasklet);
 }
 
 static void execlists_submit_request(struct drm_i915_gem_request *request)
 {
 	struct intel_engine_cs *engine = request->engine;
+	struct intel_engine_execlist * const el = &engine->execlist;
 	unsigned long flags;
 
 	/* Will be called from irq-context when using foreign fences. */
@@ -795,7 +801,7 @@  static void execlists_submit_request(struct drm_i915_gem_request *request)
 
 	insert_request(engine, &request->priotree, request->priotree.priority);
 
-	GEM_BUG_ON(!engine->execlist.first);
+	GEM_BUG_ON(!el->first);
 	GEM_BUG_ON(list_empty(&request->priotree.link));
 
 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 94e6c2a38fb7..991f6c0bd6c2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -244,6 +244,11 @@  struct intel_engine_execlist {
 	unsigned int port_mask;
 
 	/**
+	 * @port_head: first used execlist port
+	 */
+	unsigned int port_head;
+
+	/**
 	 * @queue: queue of requests, in priority lists
 	 */
 	struct rb_root queue;
@@ -524,16 +529,47 @@  execlist_num_ports(const struct intel_engine_execlist * const el)
 	return el->port_mask + 1;
 }
 
-static inline void
+#define __port_idx(start, index, mask) (((start) + (index)) & (mask))
+
+static inline struct execlist_port *
+execlist_port_head(struct intel_engine_execlist * const el)
+{
+	return &el->port[el->port_head];
+}
+
+/* Index starting from port_head */
+static inline struct execlist_port *
+execlist_port_index(struct intel_engine_execlist * const el,
+		    const unsigned int n)
+{
+	return &el->port[__port_idx(el->port_head, n, el->port_mask)];
+}
+
+static inline struct execlist_port *
+execlist_port_tail(struct intel_engine_execlist * const el)
+{
+	return &el->port[__port_idx(el->port_head, -1, el->port_mask)];
+}
+
+static inline struct execlist_port *
+execlist_port_next(struct intel_engine_execlist * const el,
+		   const struct execlist_port * const port)
+{
+	const unsigned int i = port_index(port, el);
+
+	return &el->port[__port_idx(i, 1, el->port_mask)];
+}
+
+static inline struct execlist_port *
 execlist_port_complete(struct intel_engine_execlist * const el,
 		       struct execlist_port * const port)
 {
-	const unsigned int m = el->port_mask;
+	GEM_BUG_ON(port_index(port, el) != el->port_head);
 
-	GEM_BUG_ON(port_index(port, el) != 0);
+	memset(port, 0, sizeof(struct execlist_port));
+	el->port_head = __port_idx(el->port_head, 1, el->port_mask);
 
-	memmove(port, port + 1, m * sizeof(struct execlist_port));
-	memset(port + m, 0, sizeof(struct execlist_port));
+	return execlist_port_head(el);
 }
 
 static inline unsigned int