All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
@ 2019-12-13 15:51 Venkata Sandeep Dhanalakota
  2019-12-13 15:51 ` [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing Venkata Sandeep Dhanalakota
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-13 15:51 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

We do not require to register the sysctl paths per instance,
so making registration global.

v2: make sysctl path register and unregister function driver
    specific (Tvrtko and Lucas).

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
 drivers/gpu/drm/i915/i915_perf.c       | 18 ++++++++++++++----
 drivers/gpu/drm/i915/i915_perf.h       |  2 ++
 drivers/gpu/drm/i915/i915_perf_types.h |  1 -
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index bba6b50e6beb..4b33128070da 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -30,6 +30,7 @@
 #include "display/intel_fbdev.h"
 
 #include "i915_drv.h"
+#include "i915_perf.h"
 #include "i915_globals.h"
 #include "i915_selftest.h"
 
@@ -1051,7 +1052,12 @@ static int __init i915_init(void)
 		return 0;
 	}
 
-	return pci_register_driver(&i915_pci_driver);
+	err = pci_register_driver(&i915_pci_driver);
+	if (err)
+		return err;
+
+	i915_perf_sysctl_register();
+	return 0;
 }
 
 static void __exit i915_exit(void)
@@ -1059,6 +1065,7 @@ static void __exit i915_exit(void)
 	if (!i915_pci_driver.driver.owner)
 		return;
 
+	i915_perf_sysctl_unregister();
 	pci_unregister_driver(&i915_pci_driver);
 	i915_globals_exit();
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 8d2e37949f46..4abd7623ef2d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
 	struct i915_vma *vma;
 };
 
+static struct ctl_table_header *sysctl_header;
+
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
 
 void i915_oa_config_release(struct kref *ref)
@@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
 };
 
 /**
- * i915_perf_init - initialize i915-perf state on module load
+ * i915_perf_init - initialize i915-perf state on module bind
  * @i915: i915 device instance
  *
  * Initializes i915-perf state without exposing anything to userspace.
@@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
 
 		oa_sample_rate_hard_limit = 1000 *
 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
-		perf->sysctl_header = register_sysctl_table(dev_root);
 
 		mutex_init(&perf->metrics_lock);
 		idr_init(&perf->metrics_idr);
@@ -4381,6 +4382,17 @@ static int destroy_config(int id, void *p, void *data)
 	return 0;
 }
 
+void i915_perf_sysctl_register(void)
+{
+	sysctl_header = register_sysctl_table(dev_root);
+}
+
+void i915_perf_sysctl_unregister(void)
+{
+	if (sysctl_header)
+		unregister_sysctl_table(sysctl_header);
+}
+
 /**
  * i915_perf_fini - Counter part to i915_perf_init()
  * @i915: i915 device instance
@@ -4395,8 +4407,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
 	idr_destroy(&perf->metrics_idr);
 
-	unregister_sysctl_table(perf->sysctl_header);
-
 	memset(&perf->ops, 0, sizeof(perf->ops));
 	perf->i915 = NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
index 4ceebce72060..882fdd0a7680 100644
--- a/drivers/gpu/drm/i915/i915_perf.h
+++ b/drivers/gpu/drm/i915/i915_perf.h
@@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
 void i915_perf_register(struct drm_i915_private *i915);
 void i915_perf_unregister(struct drm_i915_private *i915);
 int i915_perf_ioctl_version(void);
+void i915_perf_sysctl_register(void);
+void i915_perf_sysctl_unregister(void);
 
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
index 74ddc20a0d37..45e581455f5d 100644
--- a/drivers/gpu/drm/i915/i915_perf_types.h
+++ b/drivers/gpu/drm/i915/i915_perf_types.h
@@ -380,7 +380,6 @@ struct i915_perf {
 	struct drm_i915_private *i915;
 
 	struct kobject *metrics_kobj;
-	struct ctl_table_header *sysctl_header;
 
 	/*
 	 * Lock associated with adding/modifying/removing OA configs
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing
  2019-12-13 15:51 [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Venkata Sandeep Dhanalakota
@ 2019-12-13 15:51 ` Venkata Sandeep Dhanalakota
  2019-12-13 15:58   ` Chris Wilson
  2019-12-13 16:09 ` [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Chris Wilson
  2019-12-13 21:48 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] " Patchwork
  2 siblings, 1 reply; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-13 15:51 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

New macros ENGINE_TRACE(), CE_TRACE(), RQ_TRACE() and
GT_TRACE() are introduce to tag device name and engine
name with contexts and requests tracing in i915.

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pm.c        |   4 +-
 drivers/gpu/drm/i915/gt/intel_context.c       |   9 +-
 drivers/gpu/drm/i915/gt/intel_context.h       |   7 ++
 drivers/gpu/drm/i915/gt/intel_engine.h        |   8 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   6 +-
 drivers/gpu/drm/i915/gt/intel_engine_pm.c     |   6 +-
 drivers/gpu/drm/i915/gt/intel_gt_pm.c         |  15 ++-
 drivers/gpu/drm/i915/gt/intel_gt_pm.h         |   6 +
 drivers/gpu/drm/i915/gt/intel_lrc.c           | 106 ++++++++----------
 drivers/gpu/drm/i915/gt/intel_reset.c         |   2 +-
 .../gpu/drm/i915/gt/intel_ring_submission.c   |  11 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |   8 +-
 drivers/gpu/drm/i915/i915_request.c           |  23 +---
 drivers/gpu/drm/i915/i915_request.h           |   8 ++
 14 files changed, 110 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index f88ee1317bb4..3671a4e7e1cb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -13,7 +13,7 @@
 
 void i915_gem_suspend(struct drm_i915_private *i915)
 {
-	GEM_TRACE("\n");
+	GEM_TRACE("%s\n", dev_name(i915->drm.dev));
 
 	intel_wakeref_auto(&i915->ggtt.userfault_wakeref, 0);
 	flush_workqueue(i915->wq);
@@ -99,7 +99,7 @@ void i915_gem_suspend_late(struct drm_i915_private *i915)
 
 void i915_gem_resume(struct drm_i915_private *i915)
 {
-	GEM_TRACE("\n");
+	GEM_TRACE("%s\n", dev_name(i915->drm.dev));
 
 	intel_uncore_forcewake_get(&i915->uncore, FORCEWAKE_ALL);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 61c39e943f69..2deaa93045fe 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -68,8 +68,7 @@ int __intel_context_do_pin(struct intel_context *ce)
 		if (err)
 			goto err;
 
-		GEM_TRACE("%s context:%llx pin ring:{head:%04x, tail:%04x}\n",
-			  ce->engine->name, ce->timeline->fence_context,
+		CE_TRACE(ce, "pin ring:{head:%04x, tail:%04x}\n",
 			  ce->ring->head, ce->ring->tail);
 
 		i915_gem_context_get(ce->gem_context); /* for ctx->ppgtt */
@@ -98,8 +97,7 @@ void intel_context_unpin(struct intel_context *ce)
 	mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING);
 
 	if (likely(atomic_dec_and_test(&ce->pin_count))) {
-		GEM_TRACE("%s context:%llx retire\n",
-			  ce->engine->name, ce->timeline->fence_context);
+		CE_TRACE(ce, "retire\n");
 
 		ce->ops->unpin(ce);
 
@@ -141,8 +139,7 @@ static void __intel_context_retire(struct i915_active *active)
 {
 	struct intel_context *ce = container_of(active, typeof(*ce), active);
 
-	GEM_TRACE("%s context:%llx retire\n",
-		  ce->engine->name, ce->timeline->fence_context);
+	CE_TRACE(ce, "retire\n");
 
 	set_bit(CONTEXT_VALID_BIT, &ce->flags);
 	if (ce->state)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index 68b3d317d959..13cc9e0c98db 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -15,6 +15,13 @@
 #include "intel_ring_types.h"
 #include "intel_timeline_types.h"
 
+#define CE_TRACE(ce__, fmt, ...) do {					\
+	typecheck(struct intel_context, *(ce__));			\
+	ENGINE_TRACE((ce__)->engine, "context:%llx" fmt,		\
+		     (ce__)->timeline->fence_context,			\
+		     ##__VA_ARGS__);					\
+} while(0);
+
 void intel_context_init(struct intel_context *ce,
 			struct i915_gem_context *ctx,
 			struct intel_engine_cs *engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index c294ea80605e..d9c7121fa09e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -29,6 +29,14 @@ struct intel_gt;
 #define CACHELINE_BYTES 64
 #define CACHELINE_DWORDS (CACHELINE_BYTES / sizeof(u32))
 
+#define ENGINE_TRACE(e__, fmt, ...) do {				\
+	typecheck(struct intel_engine_cs, *(e__));			\
+	GEM_TRACE("%s %s: " fmt, 					\
+		  dev_name((e__)->i915->drm.dev),			\
+		  (e__)->name,						\
+		  ##__VA_ARGS__);					\
+} while (0)
+
 /*
  * The register defines to be used with the following macros need to accept a
  * base param, e.g:
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 49473c25916c..3d1d48bf90cf 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -912,7 +912,7 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine)
 	if (INTEL_GEN(engine->i915) < 3)
 		return -ENODEV;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	intel_uncore_write_fw(uncore, mode, _MASKED_BIT_ENABLE(STOP_RING));
 
@@ -921,7 +921,7 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine)
 					 mode, MODE_IDLE, MODE_IDLE,
 					 1000, stop_timeout(engine),
 					 NULL)) {
-		GEM_TRACE("%s: timed out on STOP_RING -> IDLE\n", engine->name);
+		ENGINE_TRACE(engine, "timed out on STOP_RING -> IDLE\n");
 		err = -ETIMEDOUT;
 	}
 
@@ -933,7 +933,7 @@ int intel_engine_stop_cs(struct intel_engine_cs *engine)
 
 void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine)
 {
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	ENGINE_WRITE_FW(engine, RING_MI_MODE, _MASKED_BIT_DISABLE(STOP_RING));
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index 889eb37e386a..bcbda8e52d41 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -21,7 +21,7 @@ static int __engine_unpark(struct intel_wakeref *wf)
 		container_of(wf, typeof(*engine), wakeref);
 	void *map;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	intel_gt_pm_get(engine->gt);
 
@@ -80,7 +80,7 @@ __queue_and_release_pm(struct i915_request *rq,
 {
 	struct intel_gt_timelines *timelines = &engine->gt->timelines;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	/*
 	 * We have to serialise all potential retirement paths with our
@@ -204,7 +204,7 @@ static int __engine_park(struct intel_wakeref *wf)
 	if (!switch_to_kernel_context(engine))
 		return -EBUSY;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	call_idle_barriers(engine); /* cleanup after wedging */
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index ecde67a75e32..bb57e3443a50 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -43,7 +43,7 @@ static int __gt_unpark(struct intel_wakeref *wf)
 	struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
 	struct drm_i915_private *i915 = gt->i915;
 
-	GEM_TRACE("\n");
+	GT_TRACE(gt, "\n");
 
 	i915_globals_unpark();
 
@@ -76,7 +76,7 @@ static int __gt_park(struct intel_wakeref *wf)
 	intel_wakeref_t wakeref = fetch_and_zero(&gt->awake);
 	struct drm_i915_private *i915 = gt->i915;
 
-	GEM_TRACE("\n");
+	GT_TRACE(gt, "\n");
 
 	intel_gt_park_requests(gt);
 
@@ -141,7 +141,7 @@ void intel_gt_sanitize(struct intel_gt *gt, bool force)
 	enum intel_engine_id id;
 	intel_wakeref_t wakeref;
 
-	GEM_TRACE("force:%s\n", yesno(force));
+	GT_TRACE(gt, "force:%s", yesno(force));
 
 	/* Use a raw wakeref to avoid calling intel_display_power_get early */
 	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
@@ -188,7 +188,7 @@ int intel_gt_resume(struct intel_gt *gt)
 	enum intel_engine_id id;
 	int err = 0;
 
-	GEM_TRACE("\n");
+	GT_TRACE(gt, "\n");
 
 	/*
 	 * After resume, we may need to poke into the pinned kernel
@@ -301,20 +301,19 @@ void intel_gt_suspend_late(struct intel_gt *gt)
 
 	intel_gt_sanitize(gt, false);
 
-	GEM_TRACE("\n");
+	GT_TRACE(gt, "\n");
 }
 
 void intel_gt_runtime_suspend(struct intel_gt *gt)
 {
 	intel_uc_runtime_suspend(&gt->uc);
 
-	GEM_TRACE("\n");
+	GT_TRACE(gt, "\n");
 }
 
 int intel_gt_runtime_resume(struct intel_gt *gt)
 {
-	GEM_TRACE("\n");
-
+	GT_TRACE(gt, "\n");
 	intel_gt_init_swizzling(gt);
 
 	return intel_uc_runtime_resume(&gt->uc);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index 4a9e48c12bd4..5cd5dd9f3e7a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -12,6 +12,12 @@
 #include "intel_gt_types.h"
 #include "intel_wakeref.h"
 
+#define GT_TRACE(gt__, fmt, ...) do {                                  \
+	typecheck(struct intel_gt, *(gt__));	                        \
+	GEM_TRACE("%s  " fmt, dev_name(gt->i915->drm.dev),		\
+		  ##__VA_ARGS__);					\
+} while(0)
+
 static inline bool intel_gt_pm_is_awake(const struct intel_gt *gt)
 {
 	return intel_wakeref_is_active(&gt->wakeref);
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 929f6bae4eba..e402b3b28150 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1069,8 +1069,8 @@ static void reset_active(struct i915_request *rq,
 	 * remain correctly ordered. And we defer to __i915_request_submit()
 	 * so that all asynchronous waits are correctly handled.
 	 */
-	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
-		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
+	ENGINE_TRACE(engine, "{ rq=%llx:%lld }\n",
+		     rq->fence.context, rq->fence.seqno);
 
 	/* On resubmission of the active request, payload will be scrubbed */
 	if (i915_request_completed(rq))
@@ -1274,15 +1274,14 @@ trace_ports(const struct intel_engine_execlists *execlists,
 	if (!ports[0])
 		return;
 
-	GEM_TRACE("%s: %s { %llx:%lld%s, %llx:%lld }\n",
-		  engine->name, msg,
-		  ports[0]->fence.context,
-		  ports[0]->fence.seqno,
-		  i915_request_completed(ports[0]) ? "!" :
-		  i915_request_started(ports[0]) ? "*" :
-		  "",
-		  ports[1] ? ports[1]->fence.context : 0,
-		  ports[1] ? ports[1]->fence.seqno : 0);
+	ENGINE_TRACE(engine, "%s { %llx:%lld%s, %llx:%lld }\n", msg,
+		     ports[0]->fence.context,
+		     ports[0]->fence.seqno,
+		     i915_request_completed(ports[0]) ? "!" :
+		     i915_request_started(ports[0]) ? "*" :
+		     "",
+		     ports[1] ? ports[1]->fence.context : 0,
+		     ports[1] ? ports[1]->fence.seqno : 0);
 }
 
 static __maybe_unused bool
@@ -1700,12 +1699,11 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	last = last_active(execlists);
 	if (last) {
 		if (need_preempt(engine, last, rb)) {
-			GEM_TRACE("%s: preempting last=%llx:%lld, prio=%d, hint=%d\n",
-				  engine->name,
-				  last->fence.context,
-				  last->fence.seqno,
-				  last->sched.attr.priority,
-				  execlists->queue_priority_hint);
+			ENGINE_TRACE(engine, "preempting last=%llx:%lld, prio=%d, hint=%d\n",
+				     last->fence.context,
+				     last->fence.seqno,
+				     last->sched.attr.priority,
+				     execlists->queue_priority_hint);
 			record_preemption(execlists);
 
 			/*
@@ -1735,12 +1733,12 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 			last = NULL;
 		} else if (need_timeslice(engine, last) &&
 			   timer_expired(&engine->execlists.timer)) {
-			GEM_TRACE("%s: expired last=%llx:%lld, prio=%d, hint=%d\n",
-				  engine->name,
-				  last->fence.context,
-				  last->fence.seqno,
-				  last->sched.attr.priority,
-				  execlists->queue_priority_hint);
+			ENGINE_TRACE(engine,
+				     "expired last=%llx:%lld, prio=%d, hint=%d\n",
+				     last->fence.context,
+				     last->fence.seqno,
+				     last->sched.attr.priority,
+				     execlists->queue_priority_hint);
 
 			ring_set_paused(engine, 1);
 			defer_active(engine);
@@ -1817,14 +1815,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 				return; /* leave this for another */
 			}
 
-			GEM_TRACE("%s: virtual rq=%llx:%lld%s, new engine? %s\n",
-				  engine->name,
-				  rq->fence.context,
-				  rq->fence.seqno,
-				  i915_request_completed(rq) ? "!" :
-				  i915_request_started(rq) ? "*" :
-				  "",
-				  yesno(engine != ve->siblings[0]));
+			ENGINE_TRACE(engine, "virtual rq=%llx:%lld%s, new engine? %s\n",
+				     rq->fence.context,
+				     rq->fence.seqno,
+				     i915_request_completed(rq) ? "!" :
+				     i915_request_started(rq) ? "*" :
+				     "",
+				     yesno(engine != ve->siblings[0]));
 
 			ve->request = NULL;
 			ve->base.execlists.queue_priority_hint = INT_MIN;
@@ -1980,9 +1977,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 	 * interrupt for secondary ports).
 	 */
 	execlists->queue_priority_hint = queue_prio(execlists);
-	GEM_TRACE("%s: queue_priority_hint:%d, submit:%s\n",
-		  engine->name, execlists->queue_priority_hint,
-		  yesno(submit));
 
 	if (submit) {
 		*port = execlists_schedule_in(last, port - execlists->pending);
@@ -2131,7 +2125,7 @@ static void process_csb(struct intel_engine_cs *engine)
 	 */
 	head = execlists->csb_head;
 	tail = READ_ONCE(*execlists->csb_write);
-	GEM_TRACE("%s cs-irq head=%d, tail=%d\n", engine->name, head, tail);
+	ENGINE_TRACE(engine, "cs-irq head=%d, tail=%d\n", head, tail);
 	if (unlikely(head == tail))
 		return;
 
@@ -2169,9 +2163,8 @@ static void process_csb(struct intel_engine_cs *engine)
 		 * status notifier.
 		 */
 
-		GEM_TRACE("%s csb[%d]: status=0x%08x:0x%08x\n",
-			  engine->name, head,
-			  buf[2 * head + 0], buf[2 * head + 1]);
+		ENGINE_TRACE(engine, "csb[%d]: status=0x%08x:0x%08x\n",
+			     head, buf[2 * head + 0], buf[2 * head + 1]);
 
 		if (INTEL_GEN(engine->i915) >= 12)
 			promote = gen12_csb_parse(execlists, buf + 2 * head);
@@ -2262,10 +2255,9 @@ static noinline void preempt_reset(struct intel_engine_cs *engine)
 	/* Mark this tasklet as disabled to avoid waiting for it to complete */
 	tasklet_disable_nosync(&engine->execlists.tasklet);
 
-	GEM_TRACE("%s: preempt timeout %lu+%ums\n",
-		  engine->name,
-		  READ_ONCE(engine->props.preempt_timeout_ms),
-		  jiffies_to_msecs(jiffies - engine->execlists.preempt.expires));
+	ENGINE_TRACE(engine, "preempt timeout %lu+%ums\n",
+		     READ_ONCE(engine->props.preempt_timeout_ms),
+		     jiffies_to_msecs(jiffies - engine->execlists.preempt.expires));
 	intel_engine_reset(engine, "preemption time out");
 
 	tasklet_enable(&engine->execlists.tasklet);
@@ -2971,8 +2963,8 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 	unsigned long flags;
 
-	GEM_TRACE("%s: depth<-%d\n", engine->name,
-		  atomic_read(&execlists->tasklet.count));
+	ENGINE_TRACE(engine, "depth<-%d\n",
+		     atomic_read(&execlists->tasklet.count));
 
 	/*
 	 * Prevent request submission to the hardware until we have
@@ -3134,8 +3126,8 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	restore_default_state(ce, engine);
 
 out_replay:
-	GEM_TRACE("%s replay {head:%04x, tail:%04x}\n",
-		  engine->name, ce->ring->head, ce->ring->tail);
+	ENGINE_TRACE(engine, "replay {head:%04x, tail:%04x}\n",
+		     ce->ring->head, ce->ring->tail);
 	intel_ring_update_space(ce->ring);
 	__execlists_reset_reg_state(ce, engine);
 	__execlists_update_reg_state(ce, engine);
@@ -3151,7 +3143,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 {
 	unsigned long flags;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	spin_lock_irqsave(&engine->active.lock, flags);
 
@@ -3172,7 +3164,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
 	struct rb_node *rb;
 	unsigned long flags;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	/*
 	 * Before we call engine->cancel_requests(), we should have exclusive
@@ -3259,8 +3251,8 @@ static void execlists_reset_finish(struct intel_engine_cs *engine)
 	if (__tasklet_enable(&execlists->tasklet))
 		/* And kick in case we missed a new request submission. */
 		tasklet_hi_schedule(&execlists->tasklet);
-	GEM_TRACE("%s: depth->%d\n", engine->name,
-		  atomic_read(&execlists->tasklet.count));
+	ENGINE_TRACE(engine, "depth->%d\n",
+		     atomic_read(&execlists->tasklet.count));
 }
 
 static int gen8_emit_bb_start(struct i915_request *rq,
@@ -4309,10 +4301,9 @@ static intel_engine_mask_t virtual_submission_mask(struct virtual_engine *ve)
 		mask = ve->siblings[0]->mask;
 	}
 
-	GEM_TRACE("%s: rq=%llx:%lld, mask=%x, prio=%d\n",
-		  ve->base.name,
-		  rq->fence.context, rq->fence.seqno,
-		  mask, ve->base.execlists.queue_priority_hint);
+	ENGINE_TRACE(&ve->base, "rq=%llx:%lld, mask=%x, prio=%d\n",
+		     rq->fence.context, rq->fence.seqno,
+		     mask, ve->base.execlists.queue_priority_hint);
 
 	return mask;
 }
@@ -4403,10 +4394,9 @@ static void virtual_submit_request(struct i915_request *rq)
 	struct i915_request *old;
 	unsigned long flags;
 
-	GEM_TRACE("%s: rq=%llx:%lld\n",
-		  ve->base.name,
-		  rq->fence.context,
-		  rq->fence.seqno);
+	ENGINE_TRACE(&ve->base, "rq=%llx:%lld\n",
+		     rq->fence.context,
+		     rq->fence.seqno);
 
 	GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 8408cb84e52c..f5b2e7c7e6c8 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -1089,7 +1089,7 @@ int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
 	bool uses_guc = intel_engine_in_guc_submission_mode(engine);
 	int ret;
 
-	GEM_TRACE("%s flags=%lx\n", engine->name, gt->reset.flags);
+	ENGINE_TRACE(engine, "flags=%lx\n", gt->reset.flags);
 	GEM_BUG_ON(!test_bit(I915_RESET_ENGINE + engine->id, &gt->reset.flags));
 
 	if (!intel_engine_pm_get_if_awake(engine))
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 5c22ca6f998a..32334476cd77 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -632,8 +632,8 @@ static int xcs_resume(struct intel_engine_cs *engine)
 	struct intel_ring *ring = engine->legacy.ring;
 	int ret = 0;
 
-	GEM_TRACE("%s: ring:{HEAD:%04x, TAIL:%04x}\n",
-		  engine->name, ring->head, ring->tail);
+	ENGINE_TRACE(engine, "ring:{HEAD:%04x, TAIL:%04x}\n",
+		     ring->head, ring->tail);
 
 	intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
 
@@ -746,10 +746,10 @@ static void reset_prepare(struct intel_engine_cs *engine)
 	 *
 	 * FIXME: Wa for more modern gens needs to be validated
 	 */
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	if (intel_engine_stop_cs(engine))
-		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
+		ENGINE_TRACE(engine, "timed out on STOP_RING\n");
 
 	intel_uncore_write_fw(uncore,
 			      RING_HEAD(base),
@@ -765,8 +765,7 @@ static void reset_prepare(struct intel_engine_cs *engine)
 
 	/* Check acts as a post */
 	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
-		GEM_TRACE("%s: ring head [%x] not parked\n",
-			  engine->name,
+		ENGINE_TRACE(engine, "ring head [%x] not parked\n",
 			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 172220e83079..af04ed6e48d9 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -375,7 +375,7 @@ static void guc_reset_prepare(struct intel_engine_cs *engine)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	/*
 	 * Prevent request submission to the hardware until we have
@@ -434,7 +434,7 @@ static void guc_cancel_requests(struct intel_engine_cs *engine)
 	struct rb_node *rb;
 	unsigned long flags;
 
-	GEM_TRACE("%s\n", engine->name);
+	ENGINE_TRACE(engine, "\n");
 
 	/*
 	 * Before we call engine->cancel_requests(), we should have exclusive
@@ -495,8 +495,8 @@ static void guc_reset_finish(struct intel_engine_cs *engine)
 		/* And kick in case we missed a new request submission. */
 		tasklet_hi_schedule(&execlists->tasklet);
 
-	GEM_TRACE("%s: depth->%d\n", engine->name,
-		  atomic_read(&execlists->tasklet.count));
+	ENGINE_TRACE(engine, "depth->%d\n",
+		     atomic_read(&execlists->tasklet.count));
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 51bb8a0812a1..c6d59d263550 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -223,10 +223,7 @@ bool i915_request_retire(struct i915_request *rq)
 	if (!i915_request_completed(rq))
 		return false;
 
-	GEM_TRACE("%s fence %llx:%lld, current %d\n",
-		  rq->engine->name,
-		  rq->fence.context, rq->fence.seqno,
-		  hwsp_seqno(rq));
+	RQ_TRACE(rq, "\n");
 
 	GEM_BUG_ON(!i915_sw_fence_signaled(&rq->submit));
 	trace_i915_request_retire(rq);
@@ -287,10 +284,7 @@ void i915_request_retire_upto(struct i915_request *rq)
 	struct intel_timeline * const tl = i915_request_timeline(rq);
 	struct i915_request *tmp;
 
-	GEM_TRACE("%s fence %llx:%lld, current %d\n",
-		  rq->engine->name,
-		  rq->fence.context, rq->fence.seqno,
-		  hwsp_seqno(rq));
+	RQ_TRACE(rq, "\n");
 
 	GEM_BUG_ON(!i915_request_completed(rq));
 
@@ -351,10 +345,7 @@ bool __i915_request_submit(struct i915_request *request)
 	struct intel_engine_cs *engine = request->engine;
 	bool result = false;
 
-	GEM_TRACE("%s fence %llx:%lld, current %d\n",
-		  engine->name,
-		  request->fence.context, request->fence.seqno,
-		  hwsp_seqno(request));
+	RQ_TRACE(request, "\n");
 
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->active.lock);
@@ -443,10 +434,7 @@ void __i915_request_unsubmit(struct i915_request *request)
 {
 	struct intel_engine_cs *engine = request->engine;
 
-	GEM_TRACE("%s fence %llx:%lld, current %d\n",
-		  engine->name,
-		  request->fence.context, request->fence.seqno,
-		  hwsp_seqno(request));
+	RQ_TRACE(request, "\n");
 
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->active.lock);
@@ -1261,8 +1249,7 @@ struct i915_request *__i915_request_commit(struct i915_request *rq)
 	struct intel_ring *ring = rq->ring;
 	u32 *cs;
 
-	GEM_TRACE("%s fence %llx:%lld\n",
-		  engine->name, rq->fence.context, rq->fence.seqno);
+	RQ_TRACE(rq, "\n");
 
 	/*
 	 * To ensure that this call will not fail, space for its emissions
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 96991d64759c..11e546f172a6 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -49,6 +49,14 @@ struct i915_capture_list {
 	struct i915_vma *vma;
 };
 
+#define RQ_TRACE(rq__, fmt, ...) do {					\
+	typecheck(struct i915_request, *(rq__));			\
+	ENGINE_TRACE((rq__)->engine,					\
+		     "fence %llx:%lld, current %d" fmt,			\
+		     (rq__)->fence.context, (rq__)->fence.seqno,	\
+		     hwsp_seqno((rq__)),##__VA_ARGS__);			\
+} while(0)
+
 enum {
 	/*
 	 * I915_FENCE_FLAG_ACTIVE - this request is currently submitted to HW.
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing
  2019-12-13 15:51 ` [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing Venkata Sandeep Dhanalakota
@ 2019-12-13 15:58   ` Chris Wilson
  0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2019-12-13 15:58 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx

Quoting Venkata Sandeep Dhanalakota (2019-12-13 15:51:52)
> New macros ENGINE_TRACE(), CE_TRACE(), RQ_TRACE() and
> GT_TRACE() are introduce to tag device name and engine
> name with contexts and requests tracing in i915.
> 
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

That is a nice improvement!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-13 15:51 [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Venkata Sandeep Dhanalakota
  2019-12-13 15:51 ` [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing Venkata Sandeep Dhanalakota
@ 2019-12-13 16:09 ` Chris Wilson
  2019-12-13 17:41   ` Lucas De Marchi
  2019-12-13 21:48 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] " Patchwork
  2 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2019-12-13 16:09 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx

Quoting Venkata Sandeep Dhanalakota (2019-12-13 15:51:51)
> We do not require to register the sysctl paths per instance,
> so making registration global.
> 
> v2: make sysctl path register and unregister function driver
>     specific (Tvrtko and Lucas).
> 
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_perf.c       | 18 ++++++++++++++----
>  drivers/gpu/drm/i915/i915_perf.h       |  2 ++
>  drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index bba6b50e6beb..4b33128070da 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -30,6 +30,7 @@
>  #include "display/intel_fbdev.h"
>  
>  #include "i915_drv.h"
> +#include "i915_perf.h"
>  #include "i915_globals.h"
>  #include "i915_selftest.h"
>  
> @@ -1051,7 +1052,12 @@ static int __init i915_init(void)
>                 return 0;
>         }
>  
> -       return pci_register_driver(&i915_pci_driver);
> +       err = pci_register_driver(&i915_pci_driver);
> +       if (err)
> +               return err;
> +
> +       i915_perf_sysctl_register();
> +       return 0;
>  }
>  
>  static void __exit i915_exit(void)
> @@ -1059,6 +1065,7 @@ static void __exit i915_exit(void)
>         if (!i915_pci_driver.driver.owner)
>                 return;
>  
> +       i915_perf_sysctl_unregister();
>         pci_unregister_driver(&i915_pci_driver);
>         i915_globals_exit();
>  }
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 8d2e37949f46..4abd7623ef2d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>         struct i915_vma *vma;
>  };
>  
> +static struct ctl_table_header *sysctl_header;
> +
>  static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>  
>  void i915_oa_config_release(struct kref *ref)
> @@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
>  };
>  
>  /**
> - * i915_perf_init - initialize i915-perf state on module load
> + * i915_perf_init - initialize i915-perf state on module bind
>   * @i915: i915 device instance
>   *
>   * Initializes i915-perf state without exposing anything to userspace.
> @@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
>  
>                 oa_sample_rate_hard_limit = 1000 *
>                         (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> -               perf->sysctl_header = register_sysctl_table(dev_root);
>  
>                 mutex_init(&perf->metrics_lock);
>                 idr_init(&perf->metrics_idr);
> @@ -4381,6 +4382,17 @@ static int destroy_config(int id, void *p, void *data)
>         return 0;
>  }
>  
> +void i915_perf_sysctl_register(void)
> +{
> +       sysctl_header = register_sysctl_table(dev_root);
> +}
> +
> +void i915_perf_sysctl_unregister(void)
> +{
> +       if (sysctl_header)
> +               unregister_sysctl_table(sysctl_header);

unregister_sysctl_table() is NULL-safe.

Other than that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-13 16:09 ` [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Chris Wilson
@ 2019-12-13 17:41   ` Lucas De Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Lucas De Marchi @ 2019-12-13 17:41 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Dec 13, 2019 at 04:09:20PM +0000, Chris Wilson wrote:
>Quoting Venkata Sandeep Dhanalakota (2019-12-13 15:51:51)
>> We do not require to register the sysctl paths per instance,
>> so making registration global.
>>
>> v2: make sysctl path register and unregister function driver
>>     specific (Tvrtko and Lucas).
>>
>> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
>>  drivers/gpu/drm/i915/i915_perf.c       | 18 ++++++++++++++----
>>  drivers/gpu/drm/i915/i915_perf.h       |  2 ++
>>  drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>>  4 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index bba6b50e6beb..4b33128070da 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -30,6 +30,7 @@
>>  #include "display/intel_fbdev.h"
>>
>>  #include "i915_drv.h"
>> +#include "i915_perf.h"
>>  #include "i915_globals.h"
>>  #include "i915_selftest.h"
>>
>> @@ -1051,7 +1052,12 @@ static int __init i915_init(void)
>>                 return 0;
>>         }
>>
>> -       return pci_register_driver(&i915_pci_driver);
>> +       err = pci_register_driver(&i915_pci_driver);
>> +       if (err)
>> +               return err;
>> +
>> +       i915_perf_sysctl_register();
>> +       return 0;
>>  }
>>
>>  static void __exit i915_exit(void)
>> @@ -1059,6 +1065,7 @@ static void __exit i915_exit(void)
>>         if (!i915_pci_driver.driver.owner)
>>                 return;
>>
>> +       i915_perf_sysctl_unregister();
>>         pci_unregister_driver(&i915_pci_driver);
>>         i915_globals_exit();
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 8d2e37949f46..4abd7623ef2d 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>>         struct i915_vma *vma;
>>  };
>>
>> +static struct ctl_table_header *sysctl_header;
>> +
>>  static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>>
>>  void i915_oa_config_release(struct kref *ref)
>> @@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
>>  };
>>
>>  /**
>> - * i915_perf_init - initialize i915-perf state on module load
>> + * i915_perf_init - initialize i915-perf state on module bind
>>   * @i915: i915 device instance
>>   *
>>   * Initializes i915-perf state without exposing anything to userspace.
>> @@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
>>
>>                 oa_sample_rate_hard_limit = 1000 *
>>                         (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
>> -               perf->sysctl_header = register_sysctl_table(dev_root);
>>
>>                 mutex_init(&perf->metrics_lock);
>>                 idr_init(&perf->metrics_idr);
>> @@ -4381,6 +4382,17 @@ static int destroy_config(int id, void *p, void *data)
>>         return 0;
>>  }
>>
>> +void i915_perf_sysctl_register(void)
>> +{
>> +       sysctl_header = register_sysctl_table(dev_root);
>> +}
>> +
>> +void i915_perf_sysctl_unregister(void)
>> +{
>> +       if (sysctl_header)
>> +               unregister_sysctl_table(sysctl_header);
>
>unregister_sysctl_table() is NULL-safe.

I had the same comment.

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>
>Other than that,
>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>-Chris
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] drm/i915/perf: Register sysctl path globally
  2019-12-13 15:51 [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Venkata Sandeep Dhanalakota
  2019-12-13 15:51 ` [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing Venkata Sandeep Dhanalakota
  2019-12-13 16:09 ` [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Chris Wilson
@ 2019-12-13 21:48 ` Patchwork
  2 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2019-12-13 21:48 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/perf: Register sysctl path globally
URL   : https://patchwork.freedesktop.org/series/70888/
State : failure

== Summary ==

Applying: drm/i915/perf: Register sysctl path globally
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_pci.c
M	drivers/gpu/drm/i915/i915_perf.c
M	drivers/gpu/drm/i915/i915_perf.h
M	drivers/gpu/drm/i915/i915_perf_types.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_perf.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_perf.c
Auto-merging drivers/gpu/drm/i915/i915_pci.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915/perf: Register sysctl path globally
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-16 10:27 ` Jani Nikula
@ 2019-12-16 11:23   ` Chris Wilson
  0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2019-12-16 11:23 UTC (permalink / raw)
  To: Jani Nikula, Venkata Sandeep Dhanalakota, intel-gfx

Quoting Jani Nikula (2019-12-16 10:27:59)
> On Fri, 13 Dec 2019, Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com> wrote:
> > We do not require to register the sysctl paths per instance,
> > so making registration global.
> >
> > v2: make sysctl path register and unregister function driver
> >     specific (Tvrtko and Lucas).
> >
> > v3: remove the NULL-check as unregister_sysctl_table is null
> >     safe. (Chris and Lucas)
> >
> > Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> 
> Chris, I understand that *some* version of this passed IGT, and that the
> patch series was re-sent rather too many times over a short period of
> time, but please only push versions that actually pass CI.

CI Bug Log - changes from CI_DRM_7556_full -> Patchwork_15738_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_15738_full absolutely need to be
  verified manually.

  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_15738_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.



Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_15738_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_ctx_persistence@bcs0-mixed-process:
    - shard-iclb:         [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7556/shard-iclb1/igt@gem_ctx_persistence@bcs0-mixed-process.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15738/shard-iclb3/igt@gem_ctx_persistence@bcs0-mixed-process.html


Which is just a known timing issue.

Is the version I was looking at.
-Chris
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-13 20:02 [Intel-gfx] [PATCH 1/2] " Venkata Sandeep Dhanalakota
@ 2019-12-16 10:27 ` Jani Nikula
  2019-12-16 11:23   ` Chris Wilson
  0 siblings, 1 reply; 20+ messages in thread
From: Jani Nikula @ 2019-12-16 10:27 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx; +Cc: chris.p.wilson

On Fri, 13 Dec 2019, Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com> wrote:
> We do not require to register the sysctl paths per instance,
> so making registration global.
>
> v2: make sysctl path register and unregister function driver
>     specific (Tvrtko and Lucas).
>
> v3: remove the NULL-check as unregister_sysctl_table is null
>     safe. (Chris and Lucas)
>
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>

Chris, I understand that *some* version of this passed IGT, and that the
patch series was re-sent rather too many times over a short period of
time, but please only push versions that actually pass CI.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_perf.c       | 17 +++++++++++++----
>  drivers/gpu/drm/i915/i915_perf.h       |  2 ++
>  drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>  4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 877560b1031e..9571611b4b16 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -30,6 +30,7 @@
>  #include "display/intel_fbdev.h"
>  
>  #include "i915_drv.h"
> +#include "i915_perf.h"
>  #include "i915_globals.h"
>  #include "i915_selftest.h"
>  
> @@ -1053,7 +1054,12 @@ static int __init i915_init(void)
>  		return 0;
>  	}
>  
> -	return pci_register_driver(&i915_pci_driver);
> +	err = pci_register_driver(&i915_pci_driver);
> +	if (err)
> +		return err;
> +
> +	i915_perf_sysctl_register();
> +	return 0;
>  }
>  
>  static void __exit i915_exit(void)
> @@ -1061,6 +1067,7 @@ static void __exit i915_exit(void)
>  	if (!i915_pci_driver.driver.owner)
>  		return;
>  
> +	i915_perf_sysctl_unregister();
>  	pci_unregister_driver(&i915_pci_driver);
>  	i915_globals_exit();
>  }
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 8d2e37949f46..3c163a9d69a9 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>  	struct i915_vma *vma;
>  };
>  
> +static struct ctl_table_header *sysctl_header;
> +
>  static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>  
>  void i915_oa_config_release(struct kref *ref)
> @@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
>  };
>  
>  /**
> - * i915_perf_init - initialize i915-perf state on module load
> + * i915_perf_init - initialize i915-perf state on module bind
>   * @i915: i915 device instance
>   *
>   * Initializes i915-perf state without exposing anything to userspace.
> @@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
>  
>  		oa_sample_rate_hard_limit = 1000 *
>  			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> -		perf->sysctl_header = register_sysctl_table(dev_root);
>  
>  		mutex_init(&perf->metrics_lock);
>  		idr_init(&perf->metrics_idr);
> @@ -4381,6 +4382,16 @@ static int destroy_config(int id, void *p, void *data)
>  	return 0;
>  }
>  
> +void i915_perf_sysctl_register(void)
> +{
> +	sysctl_header = register_sysctl_table(dev_root);
> +}
> +
> +void i915_perf_sysctl_unregister(void)
> +{
> +	unregister_sysctl_table(sysctl_header);
> +}
> +
>  /**
>   * i915_perf_fini - Counter part to i915_perf_init()
>   * @i915: i915 device instance
> @@ -4395,8 +4406,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
>  	idr_for_each(&perf->metrics_idr, destroy_config, perf);
>  	idr_destroy(&perf->metrics_idr);
>  
> -	unregister_sysctl_table(perf->sysctl_header);
> -
>  	memset(&perf->ops, 0, sizeof(perf->ops));
>  	perf->i915 = NULL;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
> index 4ceebce72060..882fdd0a7680 100644
> --- a/drivers/gpu/drm/i915/i915_perf.h
> +++ b/drivers/gpu/drm/i915/i915_perf.h
> @@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
>  void i915_perf_register(struct drm_i915_private *i915);
>  void i915_perf_unregister(struct drm_i915_private *i915);
>  int i915_perf_ioctl_version(void);
> +void i915_perf_sysctl_register(void);
> +void i915_perf_sysctl_unregister(void);
>  
>  int i915_perf_open_ioctl(struct drm_device *dev, void *data,
>  			 struct drm_file *file);
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index 74ddc20a0d37..45e581455f5d 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -380,7 +380,6 @@ struct i915_perf {
>  	struct drm_i915_private *i915;
>  
>  	struct kobject *metrics_kobj;
> -	struct ctl_table_header *sysctl_header;
>  
>  	/*
>  	 * Lock associated with adding/modifying/removing OA configs

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
@ 2019-12-13 20:02 Venkata Sandeep Dhanalakota
  2019-12-16 10:27 ` Jani Nikula
  0 siblings, 1 reply; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-13 20:02 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

We do not require to register the sysctl paths per instance,
so making registration global.

v2: make sysctl path register and unregister function driver
    specific (Tvrtko and Lucas).

v3: remove the NULL-check as unregister_sysctl_table is null
    safe. (Chris and Lucas)

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
 drivers/gpu/drm/i915/i915_perf.c       | 17 +++++++++++++----
 drivers/gpu/drm/i915/i915_perf.h       |  2 ++
 drivers/gpu/drm/i915/i915_perf_types.h |  1 -
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 877560b1031e..9571611b4b16 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -30,6 +30,7 @@
 #include "display/intel_fbdev.h"
 
 #include "i915_drv.h"
+#include "i915_perf.h"
 #include "i915_globals.h"
 #include "i915_selftest.h"
 
@@ -1053,7 +1054,12 @@ static int __init i915_init(void)
 		return 0;
 	}
 
-	return pci_register_driver(&i915_pci_driver);
+	err = pci_register_driver(&i915_pci_driver);
+	if (err)
+		return err;
+
+	i915_perf_sysctl_register();
+	return 0;
 }
 
 static void __exit i915_exit(void)
@@ -1061,6 +1067,7 @@ static void __exit i915_exit(void)
 	if (!i915_pci_driver.driver.owner)
 		return;
 
+	i915_perf_sysctl_unregister();
 	pci_unregister_driver(&i915_pci_driver);
 	i915_globals_exit();
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 8d2e37949f46..3c163a9d69a9 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
 	struct i915_vma *vma;
 };
 
+static struct ctl_table_header *sysctl_header;
+
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
 
 void i915_oa_config_release(struct kref *ref)
@@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
 };
 
 /**
- * i915_perf_init - initialize i915-perf state on module load
+ * i915_perf_init - initialize i915-perf state on module bind
  * @i915: i915 device instance
  *
  * Initializes i915-perf state without exposing anything to userspace.
@@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
 
 		oa_sample_rate_hard_limit = 1000 *
 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
-		perf->sysctl_header = register_sysctl_table(dev_root);
 
 		mutex_init(&perf->metrics_lock);
 		idr_init(&perf->metrics_idr);
@@ -4381,6 +4382,16 @@ static int destroy_config(int id, void *p, void *data)
 	return 0;
 }
 
+void i915_perf_sysctl_register(void)
+{
+	sysctl_header = register_sysctl_table(dev_root);
+}
+
+void i915_perf_sysctl_unregister(void)
+{
+	unregister_sysctl_table(sysctl_header);
+}
+
 /**
  * i915_perf_fini - Counter part to i915_perf_init()
  * @i915: i915 device instance
@@ -4395,8 +4406,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
 	idr_destroy(&perf->metrics_idr);
 
-	unregister_sysctl_table(perf->sysctl_header);
-
 	memset(&perf->ops, 0, sizeof(perf->ops));
 	perf->i915 = NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
index 4ceebce72060..882fdd0a7680 100644
--- a/drivers/gpu/drm/i915/i915_perf.h
+++ b/drivers/gpu/drm/i915/i915_perf.h
@@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
 void i915_perf_register(struct drm_i915_private *i915);
 void i915_perf_unregister(struct drm_i915_private *i915);
 int i915_perf_ioctl_version(void);
+void i915_perf_sysctl_register(void);
+void i915_perf_sysctl_unregister(void);
 
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
index 74ddc20a0d37..45e581455f5d 100644
--- a/drivers/gpu/drm/i915/i915_perf_types.h
+++ b/drivers/gpu/drm/i915/i915_perf_types.h
@@ -380,7 +380,6 @@ struct i915_perf {
 	struct drm_i915_private *i915;
 
 	struct kobject *metrics_kobj;
-	struct ctl_table_header *sysctl_header;
 
 	/*
 	 * Lock associated with adding/modifying/removing OA configs
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
@ 2019-12-13  9:12 Venkata Sandeep Dhanalakota
  0 siblings, 0 replies; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-13  9:12 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

We do not require to register the sysctl paths per instance,
so making registration global.

v2: make sysctl path register and unregister function driver
    specific (Tvrtko and Lucas).

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c        |  9 ++++++++-
 drivers/gpu/drm/i915/i915_perf.c       | 18 ++++++++++++++----
 drivers/gpu/drm/i915/i915_perf.h       |  2 ++
 drivers/gpu/drm/i915/i915_perf_types.h |  1 -
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index bba6b50e6beb..4b33128070da 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -30,6 +30,7 @@
 #include "display/intel_fbdev.h"
 
 #include "i915_drv.h"
+#include "i915_perf.h"
 #include "i915_globals.h"
 #include "i915_selftest.h"
 
@@ -1051,7 +1052,12 @@ static int __init i915_init(void)
 		return 0;
 	}
 
-	return pci_register_driver(&i915_pci_driver);
+	err = pci_register_driver(&i915_pci_driver);
+	if (err)
+		return err;
+
+	i915_perf_sysctl_register();
+	return 0;
 }
 
 static void __exit i915_exit(void)
@@ -1059,6 +1065,7 @@ static void __exit i915_exit(void)
 	if (!i915_pci_driver.driver.owner)
 		return;
 
+	i915_perf_sysctl_unregister();
 	pci_unregister_driver(&i915_pci_driver);
 	i915_globals_exit();
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 8d2e37949f46..4abd7623ef2d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
 	struct i915_vma *vma;
 };
 
+static struct ctl_table_header *sysctl_header;
+
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
 
 void i915_oa_config_release(struct kref *ref)
@@ -4228,7 +4230,7 @@ static struct ctl_table dev_root[] = {
 };
 
 /**
- * i915_perf_init - initialize i915-perf state on module load
+ * i915_perf_init - initialize i915-perf state on module bind
  * @i915: i915 device instance
  *
  * Initializes i915-perf state without exposing anything to userspace.
@@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
 
 		oa_sample_rate_hard_limit = 1000 *
 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
-		perf->sysctl_header = register_sysctl_table(dev_root);
 
 		mutex_init(&perf->metrics_lock);
 		idr_init(&perf->metrics_idr);
@@ -4381,6 +4382,17 @@ static int destroy_config(int id, void *p, void *data)
 	return 0;
 }
 
+void i915_perf_sysctl_register(void)
+{
+	sysctl_header = register_sysctl_table(dev_root);
+}
+
+void i915_perf_sysctl_unregister(void)
+{
+	if (sysctl_header)
+		unregister_sysctl_table(sysctl_header);
+}
+
 /**
  * i915_perf_fini - Counter part to i915_perf_init()
  * @i915: i915 device instance
@@ -4395,8 +4407,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
 	idr_destroy(&perf->metrics_idr);
 
-	unregister_sysctl_table(perf->sysctl_header);
-
 	memset(&perf->ops, 0, sizeof(perf->ops));
 	perf->i915 = NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
index 4ceebce72060..882fdd0a7680 100644
--- a/drivers/gpu/drm/i915/i915_perf.h
+++ b/drivers/gpu/drm/i915/i915_perf.h
@@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
 void i915_perf_register(struct drm_i915_private *i915);
 void i915_perf_unregister(struct drm_i915_private *i915);
 int i915_perf_ioctl_version(void);
+void i915_perf_sysctl_register(void);
+void i915_perf_sysctl_unregister(void);
 
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
index 74ddc20a0d37..45e581455f5d 100644
--- a/drivers/gpu/drm/i915/i915_perf_types.h
+++ b/drivers/gpu/drm/i915/i915_perf_types.h
@@ -380,7 +380,6 @@ struct i915_perf {
 	struct drm_i915_private *i915;
 
 	struct kobject *metrics_kobj;
-	struct ctl_table_header *sysctl_header;
 
 	/*
 	 * Lock associated with adding/modifying/removing OA configs
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-12 21:34 ` Lucas De Marchi
@ 2019-12-13  5:07   ` Venkata Sandeep Dhanalakota
  0 siblings, 0 replies; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-13  5:07 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx, chris.p.wilson

On 19/12/12 01:34, Lucas De Marchi wrote:
> On Wed, Dec 11, 2019 at 11:35:21PM -0800, Venkata Sandeep Dhanalakota wrote:
> > We do not require to register the sysctl paths per instance,
> > so making registration global.
> > 
> > v2: make sysctl path register and unregister function driver
> >    specific (Tvrtko and Lucas).
> > 
> > Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_pci.c        |  6 ++++++
> > drivers/gpu/drm/i915/i915_perf.c       | 19 ++++++++++++++++---
> > drivers/gpu/drm/i915/i915_perf.h       |  2 ++
> > drivers/gpu/drm/i915/i915_perf_types.h |  1 -
> > 4 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index bba6b50e6beb..c5a2bb5e87fe 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -30,6 +30,7 @@
> > #include "display/intel_fbdev.h"
> > 
> > #include "i915_drv.h"
> > +#include "i915_perf.h"
> > #include "i915_globals.h"
> > #include "i915_selftest.h"
> > 
> > @@ -1051,6 +1052,10 @@ static int __init i915_init(void)
> > 		return 0;
> > 	}
> > 
> > +	err = i915_perf_sysctl_register();
> > +	if (err)
> > +		return err;
> > +
> > 	return pci_register_driver(&i915_pci_driver);
> > }
> > 
> > @@ -1059,6 +1064,7 @@ static void __exit i915_exit(void)
> > 	if (!i915_pci_driver.driver.owner)
> > 		return;
> > 
> > +	i915_perf_sysctl_unregister();
> 
> honoring  the init order means to unregister this after
> pci_unregister_driver()
I think we should reverse the init order, because if we cannot
register pci driver successfully then we dont need to register
sysctl table.

> 
> > 	pci_unregister_driver(&i915_pci_driver);
> > 	i915_globals_exit();
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 8d2e37949f46..f039beed1771 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
> > 	struct i915_vma *vma;
> > };
> > 
> > +static struct ctl_table_header *sysctl_header;
> > +
> > static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
> > 
> > void i915_oa_config_release(struct kref *ref)
> > @@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
> > 
> > 		oa_sample_rate_hard_limit = 1000 *
> > 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> > -		perf->sysctl_header = register_sysctl_table(dev_root);
> 
> doc for this function also needs an update with
> s/module load/module bind/
sure.
> 
> > 
> > 		mutex_init(&perf->metrics_lock);
> > 		idr_init(&perf->metrics_idr);
> > @@ -4381,6 +4382,20 @@ static int destroy_config(int id, void *p, void *data)
> > 	return 0;
> > }
> > 
> > +int i915_perf_sysctl_register(void)
> > +{
> > +	sysctl_header = register_sysctl_table(dev_root);
> > +	if (!sysctl_header)
> > +		return -ENOMEM;
> 
> Not sure about this return code here. grepping other drivers, this seems
> to be common, but checking register_sysctl_table() it can actually fail
> for other reasons.
> 
> The previous behavior was to ignore it and not fail the entire thing...
> just living without this sysctl. I'd say to keep that behavior.
> 
Sure, we could ignore the return code for now.
> Lucas De Marchi
> 
> > +
> > +	return 0;
> > +}
> > +
> > +void i915_perf_sysctl_unregister(void)
> > +{
> > +	unregister_sysctl_table(sysctl_header);
> > +}
> > +
> > /**
> >  * i915_perf_fini - Counter part to i915_perf_init()
> >  * @i915: i915 device instance
> > @@ -4395,8 +4410,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
> > 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
> > 	idr_destroy(&perf->metrics_idr);
> > 
> > -	unregister_sysctl_table(perf->sysctl_header);
> > -
> > 	memset(&perf->ops, 0, sizeof(perf->ops));
> > 	perf->i915 = NULL;
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
> > index 4ceebce72060..1d1329e5af3a 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.h
> > +++ b/drivers/gpu/drm/i915/i915_perf.h
> > @@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
> > void i915_perf_register(struct drm_i915_private *i915);
> > void i915_perf_unregister(struct drm_i915_private *i915);
> > int i915_perf_ioctl_version(void);
> > +int i915_perf_sysctl_register(void);
> > +void i915_perf_sysctl_unregister(void);
> > 
> > int i915_perf_open_ioctl(struct drm_device *dev, void *data,
> > 			 struct drm_file *file);
> > diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> > index 74ddc20a0d37..45e581455f5d 100644
> > --- a/drivers/gpu/drm/i915/i915_perf_types.h
> > +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> > @@ -380,7 +380,6 @@ struct i915_perf {
> > 	struct drm_i915_private *i915;
> > 
> > 	struct kobject *metrics_kobj;
> > -	struct ctl_table_header *sysctl_header;
> > 
> > 	/*
> > 	 * Lock associated with adding/modifying/removing OA configs
> > -- 
> > 2.21.0.5.gaeb582a983
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-12  7:35 Venkata Sandeep Dhanalakota
@ 2019-12-12 21:34 ` Lucas De Marchi
  2019-12-13  5:07   ` Venkata Sandeep Dhanalakota
  0 siblings, 1 reply; 20+ messages in thread
From: Lucas De Marchi @ 2019-12-12 21:34 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota; +Cc: intel-gfx, chris.p.wilson

On Wed, Dec 11, 2019 at 11:35:21PM -0800, Venkata Sandeep Dhanalakota wrote:
>We do not require to register the sysctl paths per instance,
>so making registration global.
>
>v2: make sysctl path register and unregister function driver
>    specific (Tvrtko and Lucas).
>
>Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Jani Nikula <jani.nikula@intel.com>
>Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
>---
> drivers/gpu/drm/i915/i915_pci.c        |  6 ++++++
> drivers/gpu/drm/i915/i915_perf.c       | 19 ++++++++++++++++---
> drivers/gpu/drm/i915/i915_perf.h       |  2 ++
> drivers/gpu/drm/i915/i915_perf_types.h |  1 -
> 4 files changed, 24 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>index bba6b50e6beb..c5a2bb5e87fe 100644
>--- a/drivers/gpu/drm/i915/i915_pci.c
>+++ b/drivers/gpu/drm/i915/i915_pci.c
>@@ -30,6 +30,7 @@
> #include "display/intel_fbdev.h"
>
> #include "i915_drv.h"
>+#include "i915_perf.h"
> #include "i915_globals.h"
> #include "i915_selftest.h"
>
>@@ -1051,6 +1052,10 @@ static int __init i915_init(void)
> 		return 0;
> 	}
>
>+	err = i915_perf_sysctl_register();
>+	if (err)
>+		return err;
>+
> 	return pci_register_driver(&i915_pci_driver);
> }
>
>@@ -1059,6 +1064,7 @@ static void __exit i915_exit(void)
> 	if (!i915_pci_driver.driver.owner)
> 		return;
>
>+	i915_perf_sysctl_unregister();

honoring  the init order means to unregister this after
pci_unregister_driver()

> 	pci_unregister_driver(&i915_pci_driver);
> 	i915_globals_exit();
> }
>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>index 8d2e37949f46..f039beed1771 100644
>--- a/drivers/gpu/drm/i915/i915_perf.c
>+++ b/drivers/gpu/drm/i915/i915_perf.c
>@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
> 	struct i915_vma *vma;
> };
>
>+static struct ctl_table_header *sysctl_header;
>+
> static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>
> void i915_oa_config_release(struct kref *ref)
>@@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
>
> 		oa_sample_rate_hard_limit = 1000 *
> 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
>-		perf->sysctl_header = register_sysctl_table(dev_root);

doc for this function also needs an update with
s/module load/module bind/

>
> 		mutex_init(&perf->metrics_lock);
> 		idr_init(&perf->metrics_idr);
>@@ -4381,6 +4382,20 @@ static int destroy_config(int id, void *p, void *data)
> 	return 0;
> }
>
>+int i915_perf_sysctl_register(void)
>+{
>+	sysctl_header = register_sysctl_table(dev_root);
>+	if (!sysctl_header)
>+		return -ENOMEM;

Not sure about this return code here. grepping other drivers, this seems
to be common, but checking register_sysctl_table() it can actually fail
for other reasons.

The previous behavior was to ignore it and not fail the entire thing...
just living without this sysctl. I'd say to keep that behavior.

Lucas De Marchi

>+
>+	return 0;
>+}
>+
>+void i915_perf_sysctl_unregister(void)
>+{
>+	unregister_sysctl_table(sysctl_header);
>+}
>+
> /**
>  * i915_perf_fini - Counter part to i915_perf_init()
>  * @i915: i915 device instance
>@@ -4395,8 +4410,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
> 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
> 	idr_destroy(&perf->metrics_idr);
>
>-	unregister_sysctl_table(perf->sysctl_header);
>-
> 	memset(&perf->ops, 0, sizeof(perf->ops));
> 	perf->i915 = NULL;
> }
>diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
>index 4ceebce72060..1d1329e5af3a 100644
>--- a/drivers/gpu/drm/i915/i915_perf.h
>+++ b/drivers/gpu/drm/i915/i915_perf.h
>@@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
> void i915_perf_register(struct drm_i915_private *i915);
> void i915_perf_unregister(struct drm_i915_private *i915);
> int i915_perf_ioctl_version(void);
>+int i915_perf_sysctl_register(void);
>+void i915_perf_sysctl_unregister(void);
>
> int i915_perf_open_ioctl(struct drm_device *dev, void *data,
> 			 struct drm_file *file);
>diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
>index 74ddc20a0d37..45e581455f5d 100644
>--- a/drivers/gpu/drm/i915/i915_perf_types.h
>+++ b/drivers/gpu/drm/i915/i915_perf_types.h
>@@ -380,7 +380,6 @@ struct i915_perf {
> 	struct drm_i915_private *i915;
>
> 	struct kobject *metrics_kobj;
>-	struct ctl_table_header *sysctl_header;
>
> 	/*
> 	 * Lock associated with adding/modifying/removing OA configs
>-- 
>2.21.0.5.gaeb582a983
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
@ 2019-12-12  7:35 Venkata Sandeep Dhanalakota
  2019-12-12 21:34 ` Lucas De Marchi
  0 siblings, 1 reply; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-12  7:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

We do not require to register the sysctl paths per instance,
so making registration global.

v2: make sysctl path register and unregister function driver
    specific (Tvrtko and Lucas).

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c        |  6 ++++++
 drivers/gpu/drm/i915/i915_perf.c       | 19 ++++++++++++++++---
 drivers/gpu/drm/i915/i915_perf.h       |  2 ++
 drivers/gpu/drm/i915/i915_perf_types.h |  1 -
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index bba6b50e6beb..c5a2bb5e87fe 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -30,6 +30,7 @@
 #include "display/intel_fbdev.h"
 
 #include "i915_drv.h"
+#include "i915_perf.h"
 #include "i915_globals.h"
 #include "i915_selftest.h"
 
@@ -1051,6 +1052,10 @@ static int __init i915_init(void)
 		return 0;
 	}
 
+	err = i915_perf_sysctl_register();
+	if (err)
+		return err;
+
 	return pci_register_driver(&i915_pci_driver);
 }
 
@@ -1059,6 +1064,7 @@ static void __exit i915_exit(void)
 	if (!i915_pci_driver.driver.owner)
 		return;
 
+	i915_perf_sysctl_unregister();
 	pci_unregister_driver(&i915_pci_driver);
 	i915_globals_exit();
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 8d2e37949f46..f039beed1771 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
 	struct i915_vma *vma;
 };
 
+static struct ctl_table_header *sysctl_header;
+
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
 
 void i915_oa_config_release(struct kref *ref)
@@ -4345,7 +4347,6 @@ void i915_perf_init(struct drm_i915_private *i915)
 
 		oa_sample_rate_hard_limit = 1000 *
 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
-		perf->sysctl_header = register_sysctl_table(dev_root);
 
 		mutex_init(&perf->metrics_lock);
 		idr_init(&perf->metrics_idr);
@@ -4381,6 +4382,20 @@ static int destroy_config(int id, void *p, void *data)
 	return 0;
 }
 
+int i915_perf_sysctl_register(void)
+{
+	sysctl_header = register_sysctl_table(dev_root);
+	if (!sysctl_header)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void i915_perf_sysctl_unregister(void)
+{
+	unregister_sysctl_table(sysctl_header);
+}
+
 /**
  * i915_perf_fini - Counter part to i915_perf_init()
  * @i915: i915 device instance
@@ -4395,8 +4410,6 @@ void i915_perf_fini(struct drm_i915_private *i915)
 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
 	idr_destroy(&perf->metrics_idr);
 
-	unregister_sysctl_table(perf->sysctl_header);
-
 	memset(&perf->ops, 0, sizeof(perf->ops));
 	perf->i915 = NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.h b/drivers/gpu/drm/i915/i915_perf.h
index 4ceebce72060..1d1329e5af3a 100644
--- a/drivers/gpu/drm/i915/i915_perf.h
+++ b/drivers/gpu/drm/i915/i915_perf.h
@@ -23,6 +23,8 @@ void i915_perf_fini(struct drm_i915_private *i915);
 void i915_perf_register(struct drm_i915_private *i915);
 void i915_perf_unregister(struct drm_i915_private *i915);
 int i915_perf_ioctl_version(void);
+int i915_perf_sysctl_register(void);
+void i915_perf_sysctl_unregister(void);
 
 int i915_perf_open_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
index 74ddc20a0d37..45e581455f5d 100644
--- a/drivers/gpu/drm/i915/i915_perf_types.h
+++ b/drivers/gpu/drm/i915/i915_perf_types.h
@@ -380,7 +380,6 @@ struct i915_perf {
 	struct drm_i915_private *i915;
 
 	struct kobject *metrics_kobj;
-	struct ctl_table_header *sysctl_header;
 
 	/*
 	 * Lock associated with adding/modifying/removing OA configs
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 17:13     ` Venkata Sandeep Dhanalakota
  2019-12-11 17:25       ` Tvrtko Ursulin
@ 2019-12-11 22:14       ` Lucas De Marchi
  1 sibling, 0 replies; 20+ messages in thread
From: Lucas De Marchi @ 2019-12-11 22:14 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota; +Cc: intel-gfx, chris.p.wilson

On Wed, Dec 11, 2019 at 09:13:18AM -0800, Venkata Sandeep Dhanalakota wrote:
>On 19/12/11 04:39, Tvrtko Ursulin wrote:
>>
>> On 11/12/2019 16:31, Tvrtko Ursulin wrote:
>> > On 11/12/2019 16:07, Venkata Sandeep Dhanalakota wrote:
>> > > We do not require to register the sysctl paths per instance,
>> > > so making registration global.
>> > >
>> > > Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > > Cc: Jani Nikula <jani.nikula@intel.com>
>> > > Signed-off-by: Venkata Sandeep Dhanalakota
>> > > <venkata.s.dhanalakota@intel.com>
>> > > ---
>> > >   drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
>> > >   drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>> > >   2 files changed, 8 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_perf.c
>> > > b/drivers/gpu/drm/i915/i915_perf.c
>> > > index 8d2e37949f46..426d04214a5d 100644
>> > > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > > @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>> > >       struct i915_vma *vma;
>> > >   };
>> > > +static struct ctl_table_header *sysctl_header;
>> > > +
>> > >   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer
>> > > *hrtimer);
>> > >   void i915_oa_config_release(struct kref *ref)
>> > > @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
>> > >           oa_sample_rate_hard_limit = 1000 *
>> > >               (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
>> > > -        perf->sysctl_header = register_sysctl_table(dev_root);
>> > > +        if (!sysctl_header)
>> > > +            sysctl_header = register_sysctl_table(dev_root);
>> > >           mutex_init(&perf->metrics_lock);
>> > >           idr_init(&perf->metrics_idr);
>> > > @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
>> > >       idr_for_each(&perf->metrics_idr, destroy_config, perf);
>> > >       idr_destroy(&perf->metrics_idr);
>> > > -    unregister_sysctl_table(perf->sysctl_header);
>> > > +    if (sysctl_header) {
>> > > +        unregister_sysctl_table(sysctl_header);
>> > > +        sysctl_header = NULL;
>> > > +    }
>> >
>> > I am not sure if this could be racy with manual unbind from sysfs. Does
>> > PCI core serialize for us?
>>
>> Actually with two devices you also need to reference count it since you
>> don't want removal of the first device to remove the node but last.
>>
>Apparently this is not called during module exit, using krefs is
>way go to or have some helper which are called during module init/exit.
>
>void i915_perf_sysctl_register() {
>	sysctl_header = register_sysctl_table(dev_root);
>}
>
>void i915_perf_sysctl_unregister() {
>	unregister_sysctl_table(sysctl_header);
>}

yeah, since you are moving this to be global, I don't think it belongs
to the device, but rather to the module. So, IMO it makes more sense to
use this approach than kref/kunref on bind/unbind.

Lucas De Marchi

>
>
>> > Regards,
>> >
>> > Tvrtko
>> >
>> > >       memset(&perf->ops, 0, sizeof(perf->ops));
>> > >       perf->i915 = NULL;
>> > > diff --git a/drivers/gpu/drm/i915/i915_perf_types.h
>> > > b/drivers/gpu/drm/i915/i915_perf_types.h
>> > > index 74ddc20a0d37..45e581455f5d 100644
>> > > --- a/drivers/gpu/drm/i915/i915_perf_types.h
>> > > +++ b/drivers/gpu/drm/i915/i915_perf_types.h
>> > > @@ -380,7 +380,6 @@ struct i915_perf {
>> > >       struct drm_i915_private *i915;
>> > >       struct kobject *metrics_kobj;
>> > > -    struct ctl_table_header *sysctl_header;
>> > >       /*
>> > >        * Lock associated with adding/modifying/removing OA configs
>> > >
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 17:13     ` Venkata Sandeep Dhanalakota
@ 2019-12-11 17:25       ` Tvrtko Ursulin
  2019-12-11 22:14       ` Lucas De Marchi
  1 sibling, 0 replies; 20+ messages in thread
From: Tvrtko Ursulin @ 2019-12-11 17:25 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota; +Cc: intel-gfx, chris.p.wilson


On 11/12/2019 17:13, Venkata Sandeep Dhanalakota wrote:
> On 19/12/11 04:39, Tvrtko Ursulin wrote:
>>
>> On 11/12/2019 16:31, Tvrtko Ursulin wrote:
>>> On 11/12/2019 16:07, Venkata Sandeep Dhanalakota wrote:
>>>> We do not require to register the sysctl paths per instance,
>>>> so making registration global.
>>>>
>>>> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Signed-off-by: Venkata Sandeep Dhanalakota
>>>> <venkata.s.dhanalakota@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
>>>>    drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>>>>    2 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c
>>>> b/drivers/gpu/drm/i915/i915_perf.c
>>>> index 8d2e37949f46..426d04214a5d 100644
>>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>>> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>>>>        struct i915_vma *vma;
>>>>    };
>>>> +static struct ctl_table_header *sysctl_header;
>>>> +
>>>>    static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer
>>>> *hrtimer);
>>>>    void i915_oa_config_release(struct kref *ref)
>>>> @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
>>>>            oa_sample_rate_hard_limit = 1000 *
>>>>                (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
>>>> -        perf->sysctl_header = register_sysctl_table(dev_root);
>>>> +        if (!sysctl_header)
>>>> +            sysctl_header = register_sysctl_table(dev_root);
>>>>            mutex_init(&perf->metrics_lock);
>>>>            idr_init(&perf->metrics_idr);
>>>> @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
>>>>        idr_for_each(&perf->metrics_idr, destroy_config, perf);
>>>>        idr_destroy(&perf->metrics_idr);
>>>> -    unregister_sysctl_table(perf->sysctl_header);
>>>> +    if (sysctl_header) {
>>>> +        unregister_sysctl_table(sysctl_header);
>>>> +        sysctl_header = NULL;
>>>> +    }
>>>
>>> I am not sure if this could be racy with manual unbind from sysfs. Does
>>> PCI core serialize for us?
>>
>> Actually with two devices you also need to reference count it since you
>> don't want removal of the first device to remove the node but last.
>>
> Apparently this is not called during module exit, using krefs is
> way go to or have some helper which are called during module init/exit.

What is not called? i915_perf_fini? It better be! :)

Regards,

Tvrtko

> 
> void i915_perf_sysctl_register() {
> 	sysctl_header = register_sysctl_table(dev_root);
> }
> 
> void i915_perf_sysctl_unregister() {
> 	unregister_sysctl_table(sysctl_header);
> }
> 
> 
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>        memset(&perf->ops, 0, sizeof(perf->ops));
>>>>        perf->i915 = NULL;
>>>> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h
>>>> b/drivers/gpu/drm/i915/i915_perf_types.h
>>>> index 74ddc20a0d37..45e581455f5d 100644
>>>> --- a/drivers/gpu/drm/i915/i915_perf_types.h
>>>> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
>>>> @@ -380,7 +380,6 @@ struct i915_perf {
>>>>        struct drm_i915_private *i915;
>>>>        struct kobject *metrics_kobj;
>>>> -    struct ctl_table_header *sysctl_header;
>>>>        /*
>>>>         * Lock associated with adding/modifying/removing OA configs
>>>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 16:39   ` Tvrtko Ursulin
@ 2019-12-11 17:13     ` Venkata Sandeep Dhanalakota
  2019-12-11 17:25       ` Tvrtko Ursulin
  2019-12-11 22:14       ` Lucas De Marchi
  0 siblings, 2 replies; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-11 17:13 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, chris.p.wilson

On 19/12/11 04:39, Tvrtko Ursulin wrote:
> 
> On 11/12/2019 16:31, Tvrtko Ursulin wrote:
> > On 11/12/2019 16:07, Venkata Sandeep Dhanalakota wrote:
> > > We do not require to register the sysctl paths per instance,
> > > so making registration global.
> > > 
> > > Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Venkata Sandeep Dhanalakota
> > > <venkata.s.dhanalakota@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
> > >   drivers/gpu/drm/i915/i915_perf_types.h |  1 -
> > >   2 files changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_perf.c
> > > b/drivers/gpu/drm/i915/i915_perf.c
> > > index 8d2e37949f46..426d04214a5d 100644
> > > --- a/drivers/gpu/drm/i915/i915_perf.c
> > > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > > @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
> > >       struct i915_vma *vma;
> > >   };
> > > +static struct ctl_table_header *sysctl_header;
> > > +
> > >   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer
> > > *hrtimer);
> > >   void i915_oa_config_release(struct kref *ref)
> > > @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
> > >           oa_sample_rate_hard_limit = 1000 *
> > >               (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> > > -        perf->sysctl_header = register_sysctl_table(dev_root);
> > > +        if (!sysctl_header)
> > > +            sysctl_header = register_sysctl_table(dev_root);
> > >           mutex_init(&perf->metrics_lock);
> > >           idr_init(&perf->metrics_idr);
> > > @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
> > >       idr_for_each(&perf->metrics_idr, destroy_config, perf);
> > >       idr_destroy(&perf->metrics_idr);
> > > -    unregister_sysctl_table(perf->sysctl_header);
> > > +    if (sysctl_header) {
> > > +        unregister_sysctl_table(sysctl_header);
> > > +        sysctl_header = NULL;
> > > +    }
> > 
> > I am not sure if this could be racy with manual unbind from sysfs. Does
> > PCI core serialize for us?
> 
> Actually with two devices you also need to reference count it since you
> don't want removal of the first device to remove the node but last.
> 
Apparently this is not called during module exit, using krefs is
way go to or have some helper which are called during module init/exit.

void i915_perf_sysctl_register() {
	sysctl_header = register_sysctl_table(dev_root);
}

void i915_perf_sysctl_unregister() {
	unregister_sysctl_table(sysctl_header);
}


> > Regards,
> > 
> > Tvrtko
> > 
> > >       memset(&perf->ops, 0, sizeof(perf->ops));
> > >       perf->i915 = NULL;
> > > diff --git a/drivers/gpu/drm/i915/i915_perf_types.h
> > > b/drivers/gpu/drm/i915/i915_perf_types.h
> > > index 74ddc20a0d37..45e581455f5d 100644
> > > --- a/drivers/gpu/drm/i915/i915_perf_types.h
> > > +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> > > @@ -380,7 +380,6 @@ struct i915_perf {
> > >       struct drm_i915_private *i915;
> > >       struct kobject *metrics_kobj;
> > > -    struct ctl_table_header *sysctl_header;
> > >       /*
> > >        * Lock associated with adding/modifying/removing OA configs
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 16:31 ` Tvrtko Ursulin
@ 2019-12-11 16:39   ` Tvrtko Ursulin
  2019-12-11 17:13     ` Venkata Sandeep Dhanalakota
  0 siblings, 1 reply; 20+ messages in thread
From: Tvrtko Ursulin @ 2019-12-11 16:39 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx; +Cc: chris.p.wilson


On 11/12/2019 16:31, Tvrtko Ursulin wrote:
> On 11/12/2019 16:07, Venkata Sandeep Dhanalakota wrote:
>> We do not require to register the sysctl paths per instance,
>> so making registration global.
>>
>> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Venkata Sandeep Dhanalakota 
>> <venkata.s.dhanalakota@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
>>   drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>>   2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
>> b/drivers/gpu/drm/i915/i915_perf.c
>> index 8d2e37949f46..426d04214a5d 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>>       struct i915_vma *vma;
>>   };
>> +static struct ctl_table_header *sysctl_header;
>> +
>>   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer 
>> *hrtimer);
>>   void i915_oa_config_release(struct kref *ref)
>> @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
>>           oa_sample_rate_hard_limit = 1000 *
>>               (RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
>> -        perf->sysctl_header = register_sysctl_table(dev_root);
>> +        if (!sysctl_header)
>> +            sysctl_header = register_sysctl_table(dev_root);
>>           mutex_init(&perf->metrics_lock);
>>           idr_init(&perf->metrics_idr);
>> @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
>>       idr_for_each(&perf->metrics_idr, destroy_config, perf);
>>       idr_destroy(&perf->metrics_idr);
>> -    unregister_sysctl_table(perf->sysctl_header);
>> +    if (sysctl_header) {
>> +        unregister_sysctl_table(sysctl_header);
>> +        sysctl_header = NULL;
>> +    }
> 
> I am not sure if this could be racy with manual unbind from sysfs. Does 
> PCI core serialize for us?

Actually with two devices you also need to reference count it since you 
don't want removal of the first device to remove the node but last.

> Regards,
> 
> Tvrtko
> 
>>       memset(&perf->ops, 0, sizeof(perf->ops));
>>       perf->i915 = NULL;
>> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h 
>> b/drivers/gpu/drm/i915/i915_perf_types.h
>> index 74ddc20a0d37..45e581455f5d 100644
>> --- a/drivers/gpu/drm/i915/i915_perf_types.h
>> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
>> @@ -380,7 +380,6 @@ struct i915_perf {
>>       struct drm_i915_private *i915;
>>       struct kobject *metrics_kobj;
>> -    struct ctl_table_header *sysctl_header;
>>       /*
>>        * Lock associated with adding/modifying/removing OA configs
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 16:07 Venkata Sandeep Dhanalakota
  2019-12-11 16:13 ` Lionel Landwerlin
@ 2019-12-11 16:31 ` Tvrtko Ursulin
  2019-12-11 16:39   ` Tvrtko Ursulin
  1 sibling, 1 reply; 20+ messages in thread
From: Tvrtko Ursulin @ 2019-12-11 16:31 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx; +Cc: chris.p.wilson


On 11/12/2019 16:07, Venkata Sandeep Dhanalakota wrote:
> We do not require to register the sysctl paths per instance,
> so making registration global.
> 
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
>   drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 8d2e37949f46..426d04214a5d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>   	struct i915_vma *vma;
>   };
>   
> +static struct ctl_table_header *sysctl_header;
> +
>   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>   
>   void i915_oa_config_release(struct kref *ref)
> @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
>   
>   		oa_sample_rate_hard_limit = 1000 *
>   			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> -		perf->sysctl_header = register_sysctl_table(dev_root);
> +		if (!sysctl_header)
> +			sysctl_header = register_sysctl_table(dev_root);
>   
>   		mutex_init(&perf->metrics_lock);
>   		idr_init(&perf->metrics_idr);
> @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
>   	idr_for_each(&perf->metrics_idr, destroy_config, perf);
>   	idr_destroy(&perf->metrics_idr);
>   
> -	unregister_sysctl_table(perf->sysctl_header);
> +	if (sysctl_header) {
> +		unregister_sysctl_table(sysctl_header);
> +		sysctl_header = NULL;
> +	}

I am not sure if this could be racy with manual unbind from sysfs. Does 
PCI core serialize for us?

Regards,

Tvrtko

>   
>   	memset(&perf->ops, 0, sizeof(perf->ops));
>   	perf->i915 = NULL;
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index 74ddc20a0d37..45e581455f5d 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -380,7 +380,6 @@ struct i915_perf {
>   	struct drm_i915_private *i915;
>   
>   	struct kobject *metrics_kobj;
> -	struct ctl_table_header *sysctl_header;
>   
>   	/*
>   	 * Lock associated with adding/modifying/removing OA configs
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
  2019-12-11 16:07 Venkata Sandeep Dhanalakota
@ 2019-12-11 16:13 ` Lionel Landwerlin
  2019-12-11 16:31 ` Tvrtko Ursulin
  1 sibling, 0 replies; 20+ messages in thread
From: Lionel Landwerlin @ 2019-12-11 16:13 UTC (permalink / raw)
  To: Venkata Sandeep Dhanalakota, intel-gfx; +Cc: chris.p.wilson

On 11/12/2019 18:07, Venkata Sandeep Dhanalakota wrote:
> We do not require to register the sysctl paths per instance,
> so making registration global.
>
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>


Not sure what the pattern should be like for global settings like this one.

Anyway it's definitely required going forward :


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


Thanks!


> ---
>   drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
>   drivers/gpu/drm/i915/i915_perf_types.h |  1 -
>   2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 8d2e37949f46..426d04214a5d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -387,6 +387,8 @@ struct i915_oa_config_bo {
>   	struct i915_vma *vma;
>   };
>   
> +static struct ctl_table_header *sysctl_header;
> +
>   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>   
>   void i915_oa_config_release(struct kref *ref)
> @@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
>   
>   		oa_sample_rate_hard_limit = 1000 *
>   			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
> -		perf->sysctl_header = register_sysctl_table(dev_root);
> +		if (!sysctl_header)
> +			sysctl_header = register_sysctl_table(dev_root);
>   
>   		mutex_init(&perf->metrics_lock);
>   		idr_init(&perf->metrics_idr);
> @@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
>   	idr_for_each(&perf->metrics_idr, destroy_config, perf);
>   	idr_destroy(&perf->metrics_idr);
>   
> -	unregister_sysctl_table(perf->sysctl_header);
> +	if (sysctl_header) {
> +		unregister_sysctl_table(sysctl_header);
> +		sysctl_header = NULL;
> +	}
>   
>   	memset(&perf->ops, 0, sizeof(perf->ops));
>   	perf->i915 = NULL;
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index 74ddc20a0d37..45e581455f5d 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -380,7 +380,6 @@ struct i915_perf {
>   	struct drm_i915_private *i915;
>   
>   	struct kobject *metrics_kobj;
> -	struct ctl_table_header *sysctl_header;
>   
>   	/*
>   	 * Lock associated with adding/modifying/removing OA configs


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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally
@ 2019-12-11 16:07 Venkata Sandeep Dhanalakota
  2019-12-11 16:13 ` Lionel Landwerlin
  2019-12-11 16:31 ` Tvrtko Ursulin
  0 siblings, 2 replies; 20+ messages in thread
From: Venkata Sandeep Dhanalakota @ 2019-12-11 16:07 UTC (permalink / raw)
  To: intel-gfx; +Cc: chris.p.wilson

We do not require to register the sysctl paths per instance,
so making registration global.

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c       | 10 ++++++++--
 drivers/gpu/drm/i915/i915_perf_types.h |  1 -
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 8d2e37949f46..426d04214a5d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -387,6 +387,8 @@ struct i915_oa_config_bo {
 	struct i915_vma *vma;
 };
 
+static struct ctl_table_header *sysctl_header;
+
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
 
 void i915_oa_config_release(struct kref *ref)
@@ -4345,7 +4347,8 @@ void i915_perf_init(struct drm_i915_private *i915)
 
 		oa_sample_rate_hard_limit = 1000 *
 			(RUNTIME_INFO(i915)->cs_timestamp_frequency_khz / 2);
-		perf->sysctl_header = register_sysctl_table(dev_root);
+		if (!sysctl_header)
+			sysctl_header = register_sysctl_table(dev_root);
 
 		mutex_init(&perf->metrics_lock);
 		idr_init(&perf->metrics_idr);
@@ -4395,7 +4398,10 @@ void i915_perf_fini(struct drm_i915_private *i915)
 	idr_for_each(&perf->metrics_idr, destroy_config, perf);
 	idr_destroy(&perf->metrics_idr);
 
-	unregister_sysctl_table(perf->sysctl_header);
+	if (sysctl_header) {
+		unregister_sysctl_table(sysctl_header);
+		sysctl_header = NULL;
+	}
 
 	memset(&perf->ops, 0, sizeof(perf->ops));
 	perf->i915 = NULL;
diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
index 74ddc20a0d37..45e581455f5d 100644
--- a/drivers/gpu/drm/i915/i915_perf_types.h
+++ b/drivers/gpu/drm/i915/i915_perf_types.h
@@ -380,7 +380,6 @@ struct i915_perf {
 	struct drm_i915_private *i915;
 
 	struct kobject *metrics_kobj;
-	struct ctl_table_header *sysctl_header;
 
 	/*
 	 * Lock associated with adding/modifying/removing OA configs
-- 
2.21.0.5.gaeb582a983

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-12-16 11:23 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13 15:51 [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Venkata Sandeep Dhanalakota
2019-12-13 15:51 ` [Intel-gfx] [PATCH 2/2] drm/i915: Introduce new macros for tracing Venkata Sandeep Dhanalakota
2019-12-13 15:58   ` Chris Wilson
2019-12-13 16:09 ` [Intel-gfx] [PATCH 1/2] drm/i915/perf: Register sysctl path globally Chris Wilson
2019-12-13 17:41   ` Lucas De Marchi
2019-12-13 21:48 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/2] " Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2019-12-13 20:02 [Intel-gfx] [PATCH 1/2] " Venkata Sandeep Dhanalakota
2019-12-16 10:27 ` Jani Nikula
2019-12-16 11:23   ` Chris Wilson
2019-12-13  9:12 Venkata Sandeep Dhanalakota
2019-12-12  7:35 Venkata Sandeep Dhanalakota
2019-12-12 21:34 ` Lucas De Marchi
2019-12-13  5:07   ` Venkata Sandeep Dhanalakota
2019-12-11 16:07 Venkata Sandeep Dhanalakota
2019-12-11 16:13 ` Lionel Landwerlin
2019-12-11 16:31 ` Tvrtko Ursulin
2019-12-11 16:39   ` Tvrtko Ursulin
2019-12-11 17:13     ` Venkata Sandeep Dhanalakota
2019-12-11 17:25       ` Tvrtko Ursulin
2019-12-11 22:14       ` Lucas De Marchi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.