[PATCH 1/4] drm/i915: Move execlists setup out of common

[PATCH 1/4] drm/i915: Move execlists setup out of common

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Move the execlists specific setup out of intel_engine_setup_common. This
was supposed to be only for backend agnostic bits. At the same time rename
it to intel_engine_setup_execlist to follow the setup vs init naming
convetion we have.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 34 ----------------------------------
 drivers/gpu/drm/i915/intel_lrc.c       | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index fede62daf3e1..d27e124d826a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -25,7 +25,6 @@
 #include <drm/drm_print.h>
 
 #include "i915_drv.h"
-#include "i915_vgpu.h"
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
 
@@ -391,37 +390,6 @@ static void intel_engine_init_timeline(struct intel_engine_cs *engine)
 	engine->timeline = &engine->i915->gt.global_timeline.engine[engine->id];
 }
 
-static bool csb_force_mmio(struct drm_i915_private *i915)
-{
-	/*
-	 * IOMMU adds unpredictable latency causing the CSB write (from the
-	 * GPU into the HWSP) to only be visible some time after the interrupt
-	 * (missed breadcrumb syndrome).
-	 */
-	if (intel_vtd_active())
-		return true;
-
-	/* Older GVT emulation depends upon intercepting CSB mmio */
-	if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
-		return true;
-
-	return false;
-}
-
-static void intel_engine_init_execlist(struct intel_engine_cs *engine)
-{
-	struct intel_engine_execlists * const execlists = &engine->execlists;
-
-	execlists->csb_use_mmio = csb_force_mmio(engine->i915);
-
-	execlists->port_mask = 1;
-	BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
-	GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
-
-	execlists->queue = RB_ROOT;
-	execlists->first = NULL;
-}
-
 /**
  * intel_engines_setup_common - setup engine state not requiring hw access
  * @engine: Engine to setup.
@@ -433,8 +401,6 @@ static void intel_engine_init_execlist(struct intel_engine_cs *engine)
  */
 void intel_engine_setup_common(struct intel_engine_cs *engine)
 {
-	intel_engine_init_execlist(engine);
-
 	intel_engine_init_timeline(engine);
 	intel_engine_init_hangcheck(engine);
 	i915_gem_batch_pool_init(engine, &engine->batch_pool);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 570864583e28..479f880c0f2f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -137,6 +137,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "i915_gem_render_state.h"
+#include "i915_vgpu.h"
 #include "intel_mocs.h"
 
 #define RING_EXECLIST_QFULL		(1 << 0x2)
@@ -1952,6 +1953,38 @@ logical_ring_default_irqs(struct intel_engine_cs *engine)
 	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
 }
 
+static bool csb_force_mmio(struct drm_i915_private *i915)
+{
+	/*
+	 * IOMMU adds unpredictable latency causing the CSB write (from the
+	 * GPU into the HWSP) to only be visible some time after the interrupt
+	 * (missed breadcrumb syndrome).
+	 */
+	if (intel_vtd_active())
+		return true;
+
+	/* Older GVT emulation depends upon intercepting CSB mmio */
+	if (intel_vgpu_active(i915) && !intel_vgpu_has_hwsp_emulation(i915))
+		return true;
+
+	return false;
+}
+
+static void
+intel_engine_setup_execlist(struct intel_engine_cs *engine)
+{
+	struct intel_engine_execlists * const execlists = &engine->execlists;
+
+	execlists->csb_use_mmio = csb_force_mmio(engine->i915);
+
+	execlists->port_mask = 1;
+	BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
+	GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
+
+	execlists->queue = RB_ROOT;
+	execlists->first = NULL;
+}
+
 static void
 logical_ring_setup(struct intel_engine_cs *engine)
 {
@@ -1959,6 +1992,7 @@ logical_ring_setup(struct intel_engine_cs *engine)
 	enum forcewake_domains fw_domains;
 
 	intel_engine_setup_common(engine);
+	intel_engine_setup_execlist(engine);
 
 	/* Intentionally left blank. */
 	engine->buffer = NULL;
-- 
2.14.1

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

[PATCH 2/4] drm/i915: Move engine->needs_cmd_parser to engine->flags

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Will be adding a new per-engine flags shortly so it makes sense
to consolidate.

v2: Keep the original code flow in intel_engine_cleanup_cmd_parser.
    (Joonas Lahtinen)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c     | 7 ++++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h    | 8 +++++++-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index b11629beeb63..ccb5ba043b63 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -26,6 +26,7 @@
  */
 
 #include "i915_drv.h"
+#include "intel_ringbuffer.h"
 
 /**
  * DOC: batch buffer command parser
@@ -940,7 +941,7 @@ void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
 		return;
 	}
 
-	engine->needs_cmd_parser = true;
+	engine->flags |= I915_ENGINE_NEEDS_CMD_PARSER;
 }
 
 /**
@@ -952,7 +953,7 @@ void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
  */
 void intel_engine_cleanup_cmd_parser(struct intel_engine_cs *engine)
 {
-	if (!engine->needs_cmd_parser)
+	if (!intel_engine_needs_cmd_parser(engine))
 		return;
 
 	fini_hash_table(engine);
@@ -1350,7 +1351,7 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv)
 
 	/* If the command parser is not enabled, report 0 - unsupported */
 	for_each_engine(engine, dev_priv, id) {
-		if (engine->needs_cmd_parser) {
+		if (intel_engine_needs_cmd_parser(engine)) {
 			active = true;
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 14d9e61a1e06..70ccd63cbf8e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -271,7 +271,7 @@ static inline u64 gen8_noncanonical_addr(u64 address)
 
 static inline bool eb_use_cmdparser(const struct i915_execbuffer *eb)
 {
-	return eb->engine->needs_cmd_parser && eb->batch_len;
+	return intel_engine_needs_cmd_parser(eb->engine) && eb->batch_len;
 }
 
 static int eb_create(struct i915_execbuffer *eb)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0f38d7b43f31..a91ce63b88b6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -539,7 +539,8 @@ struct intel_engine_cs {
 
 	struct intel_engine_hangcheck hangcheck;
 
-	bool needs_cmd_parser;
+#define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
+	unsigned int flags;
 
 	/*
 	 * Table of commands the command parser needs to know about
@@ -598,6 +599,11 @@ struct intel_engine_cs {
 	} stats;
 };
 
+static inline bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
+{
+	return engine->flags & I915_ENGINE_NEEDS_CMD_PARSER;
+}
+
 static inline void
 execlists_set_active(struct intel_engine_execlists *execlists,
 		     unsigned int bit)
-- 
2.14.1

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

[PATCH 3/4] drm/i915: Consolidate checks for engine stats availability

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Sagar noticed the check can be consolidated between the engine stats
implementation and the PMU.

My first choice was a static inline helper but that got into include
ordering mess quickly fast so I went with a macro instead. At some point
we should perhaps looking into taking out the non-ringubffer bits from
intel_ringbuffer.h into a new intel_engine.h or something.

v2: Use engine->flags. (Chris Wilson)
v3: Rebase and mark GuC as not yet supported. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
---
 drivers/gpu/drm/i915/i915_pmu.c             | 11 ++++-------
 drivers/gpu/drm/i915/intel_engine_cs.c      |  4 ++--
 drivers/gpu/drm/i915/intel_guc_submission.c |  7 +++++++
 drivers/gpu/drm/i915/intel_lrc.c            |  2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h     |  6 ++++++
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 3357b690ce90..c3c641ec962b 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -90,11 +90,6 @@ static unsigned int event_enabled_bit(struct perf_event *event)
 	return config_enabled_bit(event->attr.config);
 }
 
-static bool supports_busy_stats(struct drm_i915_private *i915)
-{
-	return INTEL_GEN(i915) >= 8;
-}
-
 static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
 {
 	u64 enable;
@@ -123,8 +118,10 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
 	/*
 	 * Also there is software busyness tracking available we do not
 	 * need the timer for I915_SAMPLE_BUSY counter.
+	 *
+	 * Use RCS as proxy for all engines.
 	 */
-	else if (supports_busy_stats(i915))
+	else if (intel_engine_supports_stats(i915->engine[RCS]))
 		enable &= ~BIT(I915_SAMPLE_BUSY);
 
 	/*
@@ -447,7 +444,7 @@ static void i915_pmu_event_read(struct perf_event *event)
 
 static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
 {
-	return supports_busy_stats(engine->i915) &&
+	return intel_engine_supports_stats(engine) &&
 	       (engine->pmu.enable & BIT(I915_SAMPLE_BUSY));
 }
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d27e124d826a..b58915ea7557 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1829,7 +1829,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
 {
 	unsigned long flags;
 
-	if (INTEL_GEN(engine->i915) < 8)
+	if (!intel_engine_supports_stats(engine))
 		return -ENODEV;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
@@ -1890,7 +1890,7 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
 {
 	unsigned long flags;
 
-	if (INTEL_GEN(engine->i915) < 8)
+	if (!intel_engine_supports_stats(engine))
 		return;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index cf1cc2cb6722..a8e63779de79 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1453,6 +1453,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 		execlists->tasklet.func = guc_submission_tasklet;
 		engine->park = guc_submission_park;
 		engine->unpark = guc_submission_unpark;
+
+		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
 	}
 
 	return 0;
@@ -1465,6 +1467,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 void intel_guc_submission_disable(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
 
 	GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
 
@@ -1473,6 +1477,9 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 	/* Revert back to manual ELSP submission */
 	intel_engines_reset_default_submission(dev_priv);
 
+	for_each_engine(engine, dev_priv, id)
+		engine->flags |= I915_ENGINE_SUPPORTS_STATS;
+
 	guc_clients_destroy(guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 479f880c0f2f..b811f7cddd7e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1983,6 +1983,8 @@ intel_engine_setup_execlist(struct intel_engine_cs *engine)
 
 	execlists->queue = RB_ROOT;
 	execlists->first = NULL;
+
+	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a91ce63b88b6..c68ab3ead83c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -540,6 +540,7 @@ struct intel_engine_cs {
 	struct intel_engine_hangcheck hangcheck;
 
 #define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
+#define I915_ENGINE_SUPPORTS_STATS   BIT(1)
 	unsigned int flags;
 
 	/*
@@ -604,6 +605,11 @@ static inline bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
 	return engine->flags & I915_ENGINE_NEEDS_CMD_PARSER;
 }
 
+static inline bool intel_engine_supports_stats(struct intel_engine_cs *engine)
+{
+	return engine->flags & I915_ENGINE_SUPPORTS_STATS;
+}
+
 static inline void
 execlists_set_active(struct intel_engine_execlists *execlists,
 		     unsigned int bit)
-- 
2.14.1

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

[PATCH 4/4] drm/i915: Add GuC support for engine busy stats

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Wire up the engine busy stats accounting to the GuC submission backend.

Since there is not context out interrupt we need to place the accounting
callbacks per-request.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index a8e63779de79..d80d2a3214da 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -781,6 +781,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
 			INIT_LIST_HEAD(&rq->priotree.link);
 
 			__i915_gem_request_submit(rq);
+			intel_engine_context_in(rq->engine);
 			trace_i915_gem_request_in(rq,
 						  port_index(port, execlists));
 			last = rq;
@@ -813,6 +814,7 @@ static void guc_submission_tasklet(unsigned long data)
 
 	rq = port_request(&port[0]);
 	while (rq && i915_gem_request_completed(rq)) {
+		intel_engine_context_out(rq->engine);
 		trace_i915_gem_request_out(rq);
 		i915_gem_request_put(rq);
 
@@ -1453,8 +1455,6 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 		execlists->tasklet.func = guc_submission_tasklet;
 		engine->park = guc_submission_park;
 		engine->unpark = guc_submission_unpark;
-
-		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
 	}
 
 	return 0;
@@ -1467,8 +1467,6 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 void intel_guc_submission_disable(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
 
 	GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
 
@@ -1477,9 +1475,6 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 	/* Revert back to manual ELSP submission */
 	intel_engines_reset_default_submission(dev_priv);
 
-	for_each_engine(engine, dev_priv, id)
-		engine->flags |= I915_ENGINE_SUPPORTS_STATS;
-
 	guc_clients_destroy(guc);
 }
 
-- 
2.14.1

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

Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Move the execlists specific setup out of intel_engine_setup_common. This
> was supposed to be only for backend agnostic bits. At the same time rename
> it to intel_engine_setup_execlist to follow the setup vs init naming
> convetion we have.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> +static void
> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
> +{
> +       struct intel_engine_execlists * const execlists = &engine->execlists;
> +
> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
> +
> +       execlists->port_mask = 1;
> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
> +
> +       execlists->queue = RB_ROOT;
> +       execlists->first = NULL;
> +}

The only problem here was that we wanted to be sure that some fields
were initialised for the common paths, i.e. so we could iterate over the
queue without worrying first if it was execlists (if it wasn't execlists
the queue would be empty).

Now, I think we could just rely on zero initialisation, but that was the
rationale for it ending up early. Now we could split it between
setup_execlists and init_execlists if we want the pedantry.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Consolidate checks for engine stats availability

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-28 12:41:29)
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index cf1cc2cb6722..a8e63779de79 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -1453,6 +1453,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>                 execlists->tasklet.func = guc_submission_tasklet;
>                 engine->park = guc_submission_park;
>                 engine->unpark = guc_submission_unpark;
> +
> +               engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
>         }
>  
>         return 0;
> @@ -1465,6 +1467,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>  void intel_guc_submission_disable(struct intel_guc *guc)
>  {
>         struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
>  
>         GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */
>  
> @@ -1473,6 +1477,9 @@ void intel_guc_submission_disable(struct intel_guc *guc)
>         /* Revert back to manual ELSP submission */
>         intel_engines_reset_default_submission(dev_priv);
>  
> +       for_each_engine(engine, dev_priv, id)
> +               engine->flags |= I915_ENGINE_SUPPORTS_STATS;

Push this into engine->reset_default_submission, then the guc isn't
making an assumption about the other end. We unconditionally clear when
the guc takes over because the guc doesn't provide the stats.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/4] drm/i915: Consolidate checks for engine stats availability

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Sagar noticed the check can be consolidated between the engine stats
implementation and the PMU.

My first choice was a static inline helper but that got into include
ordering mess quickly fast so I went with a macro instead. At some point
we should perhaps looking into taking out the non-ringubffer bits from
intel_ringbuffer.h into a new intel_engine.h or something.

v2: Use engine->flags. (Chris Wilson)
v3: Rebase and mark GuC as not yet supported. (Chris Wilson)
v4: Move flag setting to intel_engines_reset_default_submission.
    (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
---
 drivers/gpu/drm/i915/i915_pmu.c             | 11 ++++-------
 drivers/gpu/drm/i915/intel_engine_cs.c      |  8 +++++---
 drivers/gpu/drm/i915/intel_guc_submission.c |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c            |  2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.h     |  6 ++++++
 5 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 3357b690ce90..c3c641ec962b 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -90,11 +90,6 @@ static unsigned int event_enabled_bit(struct perf_event *event)
 	return config_enabled_bit(event->attr.config);
 }
 
-static bool supports_busy_stats(struct drm_i915_private *i915)
-{
-	return INTEL_GEN(i915) >= 8;
-}
-
 static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
 {
 	u64 enable;
@@ -123,8 +118,10 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
 	/*
 	 * Also there is software busyness tracking available we do not
 	 * need the timer for I915_SAMPLE_BUSY counter.
+	 *
+	 * Use RCS as proxy for all engines.
 	 */
-	else if (supports_busy_stats(i915))
+	else if (intel_engine_supports_stats(i915->engine[RCS]))
 		enable &= ~BIT(I915_SAMPLE_BUSY);
 
 	/*
@@ -447,7 +444,7 @@ static void i915_pmu_event_read(struct perf_event *event)
 
 static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
 {
-	return supports_busy_stats(engine->i915) &&
+	return intel_engine_supports_stats(engine) &&
 	       (engine->pmu.enable & BIT(I915_SAMPLE_BUSY));
 }
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d27e124d826a..a35f8d3a55fd 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1527,8 +1527,10 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
-	for_each_engine(engine, i915, id)
+	for_each_engine(engine, i915, id) {
 		engine->set_default_submission(engine);
+		engine->flags |= I915_ENGINE_SUPPORTS_STATS;
+	}
 }
 
 /**
@@ -1829,7 +1831,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
 {
 	unsigned long flags;
 
-	if (INTEL_GEN(engine->i915) < 8)
+	if (!intel_engine_supports_stats(engine))
 		return -ENODEV;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
@@ -1890,7 +1892,7 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
 {
 	unsigned long flags;
 
-	if (INTEL_GEN(engine->i915) < 8)
+	if (!intel_engine_supports_stats(engine))
 		return;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index cf1cc2cb6722..912ff143d531 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1453,6 +1453,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 		execlists->tasklet.func = guc_submission_tasklet;
 		engine->park = guc_submission_park;
 		engine->unpark = guc_submission_unpark;
+
+		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 479f880c0f2f..b811f7cddd7e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1983,6 +1983,8 @@ intel_engine_setup_execlist(struct intel_engine_cs *engine)
 
 	execlists->queue = RB_ROOT;
 	execlists->first = NULL;
+
+	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a91ce63b88b6..c68ab3ead83c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -540,6 +540,7 @@ struct intel_engine_cs {
 	struct intel_engine_hangcheck hangcheck;
 
 #define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
+#define I915_ENGINE_SUPPORTS_STATS   BIT(1)
 	unsigned int flags;
 
 	/*
@@ -604,6 +605,11 @@ static inline bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
 	return engine->flags & I915_ENGINE_NEEDS_CMD_PARSER;
 }
 
+static inline bool intel_engine_supports_stats(struct intel_engine_cs *engine)
+{
+	return engine->flags & I915_ENGINE_SUPPORTS_STATS;
+}
+
 static inline void
 execlists_set_active(struct intel_engine_execlists *execlists,
 		     unsigned int bit)
-- 
2.14.1

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

[PATCH v2 4/4] drm/i915: Add GuC support for engine busy stats

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Wire up the engine busy stats accounting to the GuC submission backend.

Since there is not context out interrupt we need to place the accounting
callbacks per-request.

v2: Rebase.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 912ff143d531..d80d2a3214da 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -781,6 +781,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
 			INIT_LIST_HEAD(&rq->priotree.link);
 
 			__i915_gem_request_submit(rq);
+			intel_engine_context_in(rq->engine);
 			trace_i915_gem_request_in(rq,
 						  port_index(port, execlists));
 			last = rq;
@@ -813,6 +814,7 @@ static void guc_submission_tasklet(unsigned long data)
 
 	rq = port_request(&port[0]);
 	while (rq && i915_gem_request_completed(rq)) {
+		intel_engine_context_out(rq->engine);
 		trace_i915_gem_request_out(rq);
 		i915_gem_request_put(rq);
 
@@ -1453,8 +1455,6 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 		execlists->tasklet.func = guc_submission_tasklet;
 		engine->park = guc_submission_park;
 		engine->unpark = guc_submission_unpark;
-
-		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
 	}
 
 	return 0;
-- 
2.14.1

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

Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Tvrtko Ursulin]

On 28/11/2017 12:48, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Move the execlists specific setup out of intel_engine_setup_common. This
>> was supposed to be only for backend agnostic bits. At the same time rename
>> it to intel_engine_setup_execlist to follow the setup vs init naming
>> convetion we have.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>> +static void
>> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
>> +{
>> +       struct intel_engine_execlists * const execlists = &engine->execlists;
>> +
>> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
>> +
>> +       execlists->port_mask = 1;
>> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
>> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
>> +
>> +       execlists->queue = RB_ROOT;
>> +       execlists->first = NULL;
>> +}
> 
> The only problem here was that we wanted to be sure that some fields
> were initialised for the common paths, i.e. so we could iterate over the
> queue without worrying first if it was execlists (if it wasn't execlists
> the queue would be empty).
> 
> Now, I think we could just rely on zero initialisation, but that was the
> rationale for it ending up early. Now we could split it between
> setup_execlists and init_execlists if we want the pedantry.

Common paths as in ringbuffer submission? I grepped around and don't see 
it used there.

Then about setup vs init, we said init is for hw access so I don't 
follow how you would split the above?

Regards,

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Move execlists setup out of common (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Move execlists setup out of common (rev3)
URL   : https://patchwork.freedesktop.org/series/34541/
State : success

== Summary ==

Series 34541v3 series starting with [1/4] drm/i915: Move execlists setup out of common
https://patchwork.freedesktop.org/api/1.0/series/34541/revisions/3/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:448s
fi-bdw-gvtdvm    total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:500s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:387s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:546s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:281s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:513s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:512s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:497s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:486s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:427s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:269s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:543s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:425s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:435s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:478s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:458s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:534s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:482s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:534s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:543s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:572s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:515s
fi-skl-gvtdvm    total:289  pass:265  dwarn:1   dfail:0   fail:0   skip:23  time:498s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:562s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:423s
Blacklisted hosts:
fi-cfl-s2        total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:602s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:496s

9cf8a68eee7db5e7ded75187aac4222c07a808dd drm-tip: 2017y-11m-28d-09h-02m-29s UTC integration manifest
cdb78f3945a3 drm/i915: Add GuC support for engine busy stats
6038dd010082 drm/i915: Consolidate checks for engine stats availability
430b227e71f8 drm/i915: Move engine->needs_cmd_parser to engine->flags
d6e3dc5eec5c drm/i915: Move execlists setup out of common

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7323/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915: Move execlists setup out of common (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Move execlists setup out of common (rev3)
URL   : https://patchwork.freedesktop.org/series/34541/
State : success

== Summary ==

Blacklisted hosts:
shard-hsw        total:2643 pass:1514 dwarn:6   dfail:2   fail:17  skip:1103 time:8953s
shard-kbl        total:2350 pass:1568 dwarn:21  dfail:12  fail:12  skip:729 time:8553s
shard-snb        total:2530 pass:1229 dwarn:14  dfail:4   fail:18  skip:1262 time:7625s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7323/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-28 13:07:54)
> 
> On 28/11/2017 12:48, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Move the execlists specific setup out of intel_engine_setup_common. This
> >> was supposed to be only for backend agnostic bits. At the same time rename
> >> it to intel_engine_setup_execlist to follow the setup vs init naming
> >> convetion we have.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> ---
> >> +static void
> >> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
> >> +{
> >> +       struct intel_engine_execlists * const execlists = &engine->execlists;
> >> +
> >> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
> >> +
> >> +       execlists->port_mask = 1;
> >> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
> >> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
> >> +
> >> +       execlists->queue = RB_ROOT;
> >> +       execlists->first = NULL;
> >> +}
> > 
> > The only problem here was that we wanted to be sure that some fields
> > were initialised for the common paths, i.e. so we could iterate over the
> > queue without worrying first if it was execlists (if it wasn't execlists
> > the queue would be empty).
> > 
> > Now, I think we could just rely on zero initialisation, but that was the
> > rationale for it ending up early. Now we could split it between
> > setup_execlists and init_execlists if we want the pedantry.
> 
> Common paths as in ringbuffer submission? I grepped around and don't see 
> it used there.

See the reset code, the debug code, etc; in the common layer, above the
backends, we want to be neutral.

> Then about setup vs init, we said init is for hw access so I don't 
> follow how you would split the above?

init_hw is for initialising hw. Better names for the phases is still
open :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Tvrtko Ursulin]

On 28/11/2017 16:04, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-28 13:07:54)
>>
>> On 28/11/2017 12:48, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Move the execlists specific setup out of intel_engine_setup_common. This
>>>> was supposed to be only for backend agnostic bits. At the same time rename
>>>> it to intel_engine_setup_execlist to follow the setup vs init naming
>>>> convetion we have.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>> +static void
>>>> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
>>>> +{
>>>> +       struct intel_engine_execlists * const execlists = &engine->execlists;
>>>> +
>>>> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
>>>> +
>>>> +       execlists->port_mask = 1;
>>>> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
>>>> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
>>>> +
>>>> +       execlists->queue = RB_ROOT;
>>>> +       execlists->first = NULL;
>>>> +}
>>>
>>> The only problem here was that we wanted to be sure that some fields
>>> were initialised for the common paths, i.e. so we could iterate over the
>>> queue without worrying first if it was execlists (if it wasn't execlists
>>> the queue would be empty).
>>>
>>> Now, I think we could just rely on zero initialisation, but that was the
>>> rationale for it ending up early. Now we could split it between
>>> setup_execlists and init_execlists if we want the pedantry.
>>
>> Common paths as in ringbuffer submission? I grepped around and don't see
>> it used there.
> 
> See the reset code, the debug code, etc; in the common layer, above the
> backends, we want to be neutral.
> 
>> Then about setup vs init, we said init is for hw access so I don't
>> follow how you would split the above?
> 
> init_hw is for initialising hw. Better names for the phases is still
> open :)

Okay I found execlists->first usage in intel_engine_dump, so one so far. 
That could be made conditional, or if there are other places abstracted 
out to the backend implementation. It could be that I just did not find 
more due too much context switching?

This way or the other, I did not want to put a code like "if 
(HAS_EXECLISTS(i915)) ..."  in the function called 
intel_engine_init_execlists. That's just wrong.

And I'd say it's equally wrong to call intel_engine_init_execlists from 
_intel_engine_setup_common_, because the spiritual starting point for 
this common setup refactoring was to only put there bits _common_ to 
both backends.

If you want to keep this approach of letting the higher layers just 
assume they can access backend specific parts then simplest would be I 
just drop this patch and put that ugly "if HAS_EXECLISTS" where I don't 
want to put it. Can view it as interim and fixup later?

Regards,

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

Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-28 17:29:25)
> 
> On 28/11/2017 16:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-11-28 13:07:54)
> >>
> >> On 28/11/2017 12:48, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>> Move the execlists specific setup out of intel_engine_setup_common. This
> >>>> was supposed to be only for backend agnostic bits. At the same time rename
> >>>> it to intel_engine_setup_execlist to follow the setup vs init naming
> >>>> convetion we have.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>> ---
> >>>> +static void
> >>>> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
> >>>> +{
> >>>> +       struct intel_engine_execlists * const execlists = &engine->execlists;
> >>>> +
> >>>> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
> >>>> +
> >>>> +       execlists->port_mask = 1;
> >>>> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
> >>>> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
> >>>> +
> >>>> +       execlists->queue = RB_ROOT;
> >>>> +       execlists->first = NULL;
> >>>> +}
> >>>
> >>> The only problem here was that we wanted to be sure that some fields
> >>> were initialised for the common paths, i.e. so we could iterate over the
> >>> queue without worrying first if it was execlists (if it wasn't execlists
> >>> the queue would be empty).
> >>>
> >>> Now, I think we could just rely on zero initialisation, but that was the
> >>> rationale for it ending up early. Now we could split it between
> >>> setup_execlists and init_execlists if we want the pedantry.
> >>
> >> Common paths as in ringbuffer submission? I grepped around and don't see
> >> it used there.
> > 
> > See the reset code, the debug code, etc; in the common layer, above the
> > backends, we want to be neutral.
> > 
> >> Then about setup vs init, we said init is for hw access so I don't
> >> follow how you would split the above?
> > 
> > init_hw is for initialising hw. Better names for the phases is still
> > open :)
> 
> Okay I found execlists->first usage in intel_engine_dump, so one so far. 

Keep looking.

> That could be made conditional,

No. It already is conditional on the derived state.

or if there are other places abstracted 

> out to the backend implementation. It could be that I just did not find 
> more due too much context switching?
> 
> This way or the other, I did not want to put a code like "if 
> (HAS_EXECLISTS(i915)) ..."  in the function called 
> intel_engine_init_execlists. That's just wrong.
> 
> And I'd say it's equally wrong to call intel_engine_init_execlists

It's wrong to initialised the shared structure?

> from 
> _intel_engine_setup_common_, because the spiritual starting point for 
> this common setup refactoring was to only put there bits _common_ to 
> both backends.
> 
> If you want to keep this approach of letting the higher layers just 
> assume they can access backend specific parts then simplest would be I 
> just drop this patch and put that ugly "if HAS_EXECLISTS" where I don't 
> want to put it. Can view it as interim and fixup later?

Don't break it in this patch, and you won't have to do either?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/4] drm/i915: Consolidate checks for engine stats availability

[Sagar Arun Kamble]

On 11/28/2017 6:34 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Sagar noticed the check can be consolidated between the engine stats
> implementation and the PMU.
>
> My first choice was a static inline helper but that got into include
> ordering mess quickly fast so I went with a macro instead. At some point
> we should perhaps looking into taking out the non-ringubffer bits from
> intel_ringbuffer.h into a new intel_engine.h or something.
>
> v2: Use engine->flags. (Chris Wilson)
> v3: Rebase and mark GuC as not yet supported. (Chris Wilson)
> v4: Move flag setting to intel_engines_reset_default_submission.
>      (Chris Wilson)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Suggested-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
> ---
>   drivers/gpu/drm/i915/i915_pmu.c             | 11 ++++-------
>   drivers/gpu/drm/i915/intel_engine_cs.c      |  8 +++++---
>   drivers/gpu/drm/i915/intel_guc_submission.c |  2 ++
>   drivers/gpu/drm/i915/intel_lrc.c            |  2 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.h     |  6 ++++++
>   5 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 3357b690ce90..c3c641ec962b 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -90,11 +90,6 @@ static unsigned int event_enabled_bit(struct perf_event *event)
>   	return config_enabled_bit(event->attr.config);
>   }
>   
> -static bool supports_busy_stats(struct drm_i915_private *i915)
> -{
> -	return INTEL_GEN(i915) >= 8;
> -}
> -
>   static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
>   {
>   	u64 enable;
> @@ -123,8 +118,10 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
>   	/*
>   	 * Also there is software busyness tracking available we do not
>   	 * need the timer for I915_SAMPLE_BUSY counter.
> +	 *
> +	 * Use RCS as proxy for all engines.
>   	 */
> -	else if (supports_busy_stats(i915))
> +	else if (intel_engine_supports_stats(i915->engine[RCS]))
>   		enable &= ~BIT(I915_SAMPLE_BUSY);
>   
>   	/*
> @@ -447,7 +444,7 @@ static void i915_pmu_event_read(struct perf_event *event)
>   
>   static bool engine_needs_busy_stats(struct intel_engine_cs *engine)
>   {
> -	return supports_busy_stats(engine->i915) &&
> +	return intel_engine_supports_stats(engine) &&
>   	       (engine->pmu.enable & BIT(I915_SAMPLE_BUSY));
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index d27e124d826a..a35f8d3a55fd 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1527,8 +1527,10 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
>   	struct intel_engine_cs *engine;
>   	enum intel_engine_id id;
>   
> -	for_each_engine(engine, i915, id)
> +	for_each_engine(engine, i915, id) {
>   		engine->set_default_submission(engine);
> +		engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> +	}
>   }
>   
>   /**
> @@ -1829,7 +1831,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>   {
>   	unsigned long flags;
>   
> -	if (INTEL_GEN(engine->i915) < 8)
> +	if (!intel_engine_supports_stats(engine))
>   		return -ENODEV;
>   
>   	spin_lock_irqsave(&engine->stats.lock, flags);
> @@ -1890,7 +1892,7 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
>   {
>   	unsigned long flags;
>   
> -	if (INTEL_GEN(engine->i915) < 8)
> +	if (!intel_engine_supports_stats(engine))
>   		return;
>   
>   	spin_lock_irqsave(&engine->stats.lock, flags);
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index cf1cc2cb6722..912ff143d531 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -1453,6 +1453,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>   		execlists->tasklet.func = guc_submission_tasklet;
>   		engine->park = guc_submission_park;
>   		engine->unpark = guc_submission_unpark;
> +
> +		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 479f880c0f2f..b811f7cddd7e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1983,6 +1983,8 @@ intel_engine_setup_execlist(struct intel_engine_cs *engine)
I am seeing intel_engine_init_execlist function in drm-tip. and that 
will get called for < gen 8 too so this
may not be right place. Setting it in logical_ring_setup and clearing in 
intel_init_ring_buffer is more clearer I guess.
>   
>   	execlists->queue = RB_ROOT;
>   	execlists->first = NULL;
> +
> +	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
>   }
>   
>   static void
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index a91ce63b88b6..c68ab3ead83c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -540,6 +540,7 @@ struct intel_engine_cs {
>   	struct intel_engine_hangcheck hangcheck;
>   
>   #define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
> +#define I915_ENGINE_SUPPORTS_STATS   BIT(1)
>   	unsigned int flags;
>   
>   	/*
> @@ -604,6 +605,11 @@ static inline bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
>   	return engine->flags & I915_ENGINE_NEEDS_CMD_PARSER;
>   }
>   
> +static inline bool intel_engine_supports_stats(struct intel_engine_cs *engine)
> +{
> +	return engine->flags & I915_ENGINE_SUPPORTS_STATS;
> +}
> +
>   static inline void
>   execlists_set_active(struct intel_engine_execlists *execlists,
>   		     unsigned int bit)

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