[RFC 00/12] Support for sustained capturing of GuC firmware logs

[RFC 00/12] Support for sustained capturing of GuC firmware logs

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

GuC firmware log its debug messages into a Host-GuC shared memory buffer
and when the buffer is half full it sends a Flush interrupt to Host.
GuC firmware expects that while it is writing to 2nd half of the buffer,
first half would get consumed by Host and then get a flush completed
acknowledgement from Host, so that it does not end up doing any overwrite
causing loss of logs.
So far Flush interrupt wasn't enabled on Host side & User could capture the
contents/snapshot of log buffer through 'i915_guc_log_dump' debugfs iface.
But this couldn't meet couple of key requirements, especially of Validation,
first is to ensure capturing of all boot time logs even with high verbosity
level and second is to enable capturing of logs in a sustained manner like
for the entire duration of a workload.
Now Driver will enable Flush interrupt and on receving it, would copy the
contents of log buffer into its local buffer. The size of local buffer would
be big enough to contain multiple snapshots of the log buffer giving ample
time to User to pull boot time messages. Also reads from User space will be
blocked if there isn't any data left in the local buffer until another
Flush interrupt is received and new logs are copied into the local buffer,
enabling capturing of logs in a streaming manner.

Have added 3 interfaces for capturing logs.
Patch 5 & 6 avails relay framework to store logs and export that to User,
in which Driver just needs to use a relay API to copy logs into the relay
buffer and behavior is equivalent to 'dmesg -c'. But there are certain
requirements with relay which in most likelihood can be met only for a
debugfs file. But since debugfs may not be always available in production
kernels, added support for other interfaces as a fall back, where Driver
only will have to do allocation/mapping of local buffer pages, handle file
read calls, read/write pointer management, overflow handling etc.
One is a sysfs interface '/sys/class/drm/card0/guc_log', which is generally
the preferred location. This mimics '/proc/kmsg' behavior, so meets the
streaming requirement but at a time only single client can pull the logs
in a consistent manner.
Other is a character device file interface '/dev/dri/guc_log', this mimics
'/dev/kmsg' behavior, so can meet the streaming requirement and also allow
multiple clients to consume logs at the same time.
Based on the comments and considering the relative merits, can decide which
interface to actually use.

Akash Goel (7):
  drm/i915: Add a relay backed debugfs interface for capturing GuC logs
  drm/i915: Store GuC ukernel logs in the relay buffer
  drm/i915: Allocate local buffer to store GuC ukernel logs
  drm/i915: Store GuC ukernel logs in the local buffer
  drm/i915: Add a char device file interface to capture GuC ukernel logs
  drm/i915: Support to capture GuC logs by multiple clients via device
    file iface
  drm/i915: Add sysfs interface to capture the GuC ukernel logs

Sagar Arun Kamble (5):
  drm/i915: Decouple GuC log setup from verbosity parameter
  drm/i915: Add GuC ukernel logging related fields to fw interface file
  drm/i915: Support for GuC interrupts
  drm/i915: Handle log buffer flush interrupt event from GuC
  drm/i915: Forcefully flush GuC log buffer on reset

 drivers/gpu/drm/i915/i915_drv.h            |   1 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 483 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.c            | 120 ++++++-
 drivers/gpu/drm/i915/i915_reg.h            |  11 +
 drivers/gpu/drm/i915/i915_sysfs.c          |  40 +++
 drivers/gpu/drm/i915/intel_drv.h           |   3 +
 drivers/gpu/drm/i915/intel_guc.h           |  30 ++
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  42 +++
 drivers/gpu/drm/i915/intel_guc_loader.c    |   9 +-
 9 files changed, 730 insertions(+), 9 deletions(-)

-- 
1.9.2

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

[RFC 01/12] drm/i915: Decouple GuC log setup from verbosity parameter

[akash.goel]

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

GuC Log buffer allocation was tied up with verbosity level kernel parameter
i915.guc_log_level. User could be given a provision to enable logging at
runtime and not necessarily during load time only. This patch will perform
allocation of shared log buffer always but will initially enable logging on
GuC side through init params based on i915.guc_log_level.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
 drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ac72451..da7c242 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -776,9 +776,6 @@ static void guc_create_log(struct intel_guc *guc)
 	unsigned long offset;
 	uint32_t size, flags;
 
-	if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
-		return;
-
 	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
 		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 29273e5..fdaeed8 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -162,11 +162,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
 			GUC_CTL_VCS2_ENABLED;
 
-	if (i915.guc_log_level >= 0) {
-		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+	params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+
+	if (i915.guc_log_level >= 0)
 		params[GUC_CTL_DEBUG] =
 			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-	}
+	else
+		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
 	if (guc->ads_obj) {
 		u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
-- 
1.9.2

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

[RFC 02/12] drm/i915: Add GuC ukernel logging related fields to fw interface file

[akash.goel]

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

The first page of the GuC log buffer contains state info or meta data
which is required to parse the logs contained in the subsequent pages.
The structure representing the state info is added to interface file
as Driver would need to handle log buffer flush interrupts from GuC.
Added an enum for the different message/event types that can be send
by the GuC ukernel to Host.
Also added 2 new Host to GuC action types to inform GuC when Host has
flushed the log buffer and forcefuly cause the GuC to send a new
log buffer flush interrupt.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h | 42 +++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 944786d..b85b23f 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -418,12 +418,48 @@ struct guc_ads {
 	u32 reserved2[4];
 } __packed;
 
+/* GuC logging structures */
+
+enum guc_log_buffer_type {
+	GUC_ISR_LOG_BUFFER,
+	GUC_DPC_LOG_BUFFER,
+	GUC_CRASH_DUMP_LOG_BUFFER,
+	GUC_MAX_LOG_BUFFER
+};
+
+/*
+ * Below state is used for coordination of retrival of GuC logs
+ * by i915. read_ptr is where i915 read last. GuC keeps incrementing
+ * write_ptr on each log entry. When buffer gets half filled and i915
+ * has requested interrupt, GuC will set flush_to_file field and raise the
+ * interrupt. i915 should read the buffer and clear flush_to_file field.
+ * i915 should also update the read_ptr.
+*/
+struct guc_log_buffer_state {
+	u32 marker[2];
+	u32 read_ptr;
+	u32 write_ptr;
+	u32 size;
+	u32 sampled_write_ptr;
+	union {
+		struct {
+			u32 flush_to_file:1;
+			u32 buffer_full_cnt:4;
+			u32 reserved:27;
+		};
+		u32 flags;
+	};
+	u32 version;
+} __packed;
+
 /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
 enum host2guc_action {
 	HOST2GUC_ACTION_DEFAULT = 0x0,
 	HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6,
 	HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10,
 	HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20,
+	HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE = 0x30,
+	HOST2GUC_ACTION_FORCE_LOGBUFFERFLUSH = 0x302,
 	HOST2GUC_ACTION_ENTER_S_STATE = 0x501,
 	HOST2GUC_ACTION_EXIT_S_STATE = 0x502,
 	HOST2GUC_ACTION_SLPC_REQUEST = 0x3003,
@@ -448,4 +484,10 @@ enum guc2host_status {
 	GUC2HOST_STATUS_GENERIC_FAIL = GUC2HOST_STATUS(0x0000F000)
 };
 
+/* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
+enum guc2host_message {
+	GUC2HOST_MSG_CRASH_DUMP_POSTED = (1 << 1),
+	GUC2HOST_MSG_FLUSH_LOG_BUFFER = (1 << 3)
+};
+
 #endif
-- 
1.9.2

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

[RFC 03/12] drm/i915: Support for GuC interrupts

[akash.goel]

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

There are certain types of interrupts which Host can recieve from GuC.
GuC ukernel sends an interrupt to Host for certain events, like for
example retrieve/consume the logs generated by ukernel.
This patch adds support to receive interrupts from GuC but currently
enables & partially handles only the interrupt sent by GuC ukernel.
Future patches will add support for handling other interrupt types.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |   1 +
 drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
 drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
 drivers/gpu/drm/i915/intel_drv.h           |   3 +
 drivers/gpu/drm/i915/intel_guc.h           |   5 ++
 drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
 7 files changed, 120 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4c8e34..7aae033 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1790,6 +1790,7 @@ struct drm_i915_private {
 	u32 gt_irq_mask;
 	u32 pm_irq_mask;
 	u32 pm_rps_events;
+	u32 guc_events;
 	u32 pipestat_irq_mask[I915_MAX_PIPES];
 
 	struct i915_hotplug hotplug;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da7c242..c2f3a67 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev)
 	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
+	gen8_disable_guc_interrupts(dev);
+
 	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index caaf1e2..b4294a8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
 } while (0)
 
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
+static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
 /* For display hotplug interrupt */
 static inline void
@@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	synchronize_irq(dev_priv->dev->irq);
 }
 
+void gen8_reset_guc_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	i915_reg_t reg = gen6_pm_iir(dev_priv);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	I915_WRITE(reg, dev_priv->guc_events);
+	I915_WRITE(reg, dev_priv->guc_events);
+	POSTING_READ(reg);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen8_enable_guc_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (!dev_priv->guc.interrupts_enabled) {
+		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
+					dev_priv->guc_events);
+		dev_priv->guc.interrupts_enabled = true;
+		I915_WRITE(gen6_pm_ier(dev_priv),
+		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
+		gen6_enable_pm_irq(dev_priv, dev_priv->guc_events);
+	}
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen8_disable_guc_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->guc.interrupts_enabled = false;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	cancel_work_sync(&dev_priv->guc.events_work);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	__gen6_disable_pm_irq(dev_priv, dev_priv->guc_events);
+	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
+				~dev_priv->guc_events);
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	synchronize_irq(dev->irq);
+}
+
 /**
  * bdw_update_port_irq - update DE port interrupt
  * @dev_priv: driver private
@@ -1174,6 +1224,27 @@ out:
 	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
 }
 
+static void gen8_guc2host_events_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, guc.events_work);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	/* Speed up work cancelation during disabling guc interrupts. */
+	if (!dev_priv->guc.interrupts_enabled) {
+		spin_unlock_irq(&dev_priv->irq_lock);
+		return;
+	}
+
+	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
+
+	gen6_enable_pm_irq(dev_priv, GEN8_GUC_TO_HOST_INT_EVENT);
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	/* TODO: Handle the events for which GuC interrupted host */
+
+	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
+}
 
 /**
  * ivybridge_parity_work - Workqueue called when a parity error interrupt
@@ -1349,11 +1420,13 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
 			DRM_ERROR("The master control interrupt lied (GT3)!\n");
 	}
 
-	if (master_ctl & GEN8_GT_PM_IRQ) {
+	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
 		gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
-		if (gt_iir[2] & dev_priv->pm_rps_events) {
+		if (gt_iir[2] & (dev_priv->pm_rps_events |
+				 dev_priv->guc_events)) {
 			I915_WRITE_FW(GEN8_GT_IIR(2),
-				      gt_iir[2] & dev_priv->pm_rps_events);
+				      gt_iir[2] & (dev_priv->pm_rps_events |
+						   dev_priv->guc_events));
 			ret = IRQ_HANDLED;
 		} else
 			DRM_ERROR("The master control interrupt lied (PM)!\n");
@@ -1385,6 +1458,9 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
 
 	if (gt_iir[2] & dev_priv->pm_rps_events)
 		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
+
+	if (gt_iir[2] & dev_priv->guc_events)
+		gen8_guc_irq_handler(dev_priv, gt_iir[2]);
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1631,6 +1707,20 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
+{
+	if (gt_iir & GEN8_GUC_TO_HOST_INT_EVENT) {
+		spin_lock(&dev_priv->irq_lock);
+		if (dev_priv->guc.interrupts_enabled) {
+			/* Process all the GuC to Host events in bottom half */
+			gen6_disable_pm_irq(dev_priv,
+				GEN8_GUC_TO_HOST_INT_EVENT);
+			queue_work(dev_priv->wq, &dev_priv->guc.events_work);
+		}
+		spin_unlock(&dev_priv->irq_lock);
+	}
+}
+
 static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
 				     enum pipe pipe)
 {
@@ -4570,6 +4660,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
 	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
+	INIT_WORK(&dev_priv->guc.events_work, gen8_guc2host_events_work);
+
+	if (HAS_GUC_UCODE(dev))
+		dev_priv->guc_events = GEN8_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e307725..54e1477 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5922,6 +5922,7 @@ enum skl_disp_power_wells {
 #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
 #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+(pipe)))
 #define  GEN8_GT_VECS_IRQ		(1<<6)
+#define  GEN8_GT_GUC_IRQ		(1<<5)
 #define  GEN8_GT_PM_IRQ			(1<<4)
 #define  GEN8_GT_VCS2_IRQ		(1<<3)
 #define  GEN8_GT_VCS1_IRQ		(1<<2)
@@ -5933,6 +5934,16 @@ enum skl_disp_power_wells {
 #define GEN8_GT_IIR(which) _MMIO(0x44308 + (0x10 * (which)))
 #define GEN8_GT_IER(which) _MMIO(0x4430c + (0x10 * (which)))
 
+#define GEN8_GUC_TO_HOST_INT_EVENT	(1<<31)
+#define GEN8_GUC_EXEC_ERROR_EVENT	(1<<30)
+#define GEN8_GUC_DISPLAY_EVENT		(1<<29)
+#define GEN8_GUC_SEMA_SIGNAL_EVENT	(1<<28)
+#define GEN8_GUC_IOMMU_MSG_EVENT	(1<<27)
+#define GEN8_GUC_DB_RING_EVENT		(1<<26)
+#define GEN8_GUC_DMA_DONE_EVENT		(1<<25)
+#define GEN8_GUC_FATAL_ERROR_EVENT	(1<<24)
+#define GEN8_GUC_NOTIFICATION_EVENT	(1<<23)
+
 #define GEN8_RCS_IRQ_SHIFT 0
 #define GEN8_BCS_IRQ_SHIFT 16
 #define GEN8_VCS1_IRQ_SHIFT 0
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9b5f663..f6dd3370 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1075,6 +1075,9 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
 				     unsigned int pipe_mask);
 void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
 				     unsigned int pipe_mask);
+void gen8_reset_guc_interrupts(struct drm_device *dev);
+void gen8_enable_guc_interrupts(struct drm_device *dev);
+void gen8_disable_guc_interrupts(struct drm_device *dev);
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 41601c7..e20792d 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -125,6 +125,11 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
+	/*
+	 * work, interrupts_enabled are protected by dev_priv->irq_lock
+	 */
+	struct work_struct events_work;
+	bool interrupts_enabled;
 
 	struct drm_i915_gem_object *ads_obj;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index fdaeed8..9c2eb79 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -433,6 +433,7 @@ int intel_guc_setup(struct drm_device *dev)
 	}
 
 	direct_interrupts_to_host(dev_priv);
+	gen8_reset_guc_interrupts(dev);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
-- 
1.9.2

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

[RFC 04/12] drm/i915: Handle log buffer flush interrupt event from GuC

[akash.goel]

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

GuC ukernel sends an interrupt to Host to flush the log buffer
and expects Host to correspondingly update the read pointer
information in the state structure, once it has consumed the
log buffer contents by copying them to a file or buffer.
Even if Host couldn't copy the contents, it can still update the
read pointer so that logging state is not disturbed on GuC side.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 81 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c            | 19 ++++++-
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index c2f3a67..f7f29f6 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -769,6 +769,74 @@ err:
 	return NULL;
 }
 
+static void guc_move_to_next_buf(struct intel_guc *guc)
+{
+	return;
+}
+
+static void* guc_get_write_buffer(struct intel_guc *guc)
+{
+	void *base_addr = NULL;
+
+	return base_addr;
+}
+
+static void guc_read_update_log_buffer(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	struct guc_log_buffer_state *log_buffer_state;
+	struct guc_log_buffer_state *log_buffer_copy_state;
+	void *page, *data_ptr;
+	int i;
+
+	if (!guc->log_obj)
+		return;
+
+	log_buffer_state = page =
+		kmap_atomic(i915_gem_object_get_page(guc->log_obj, 0));
+
+	/* Get the pointer to local buffer to store the logs */
+	data_ptr = log_buffer_copy_state = guc_get_write_buffer(guc);
+	if (log_buffer_copy_state)
+		memcpy(log_buffer_copy_state, log_buffer_state, PAGE_SIZE);
+
+	for (i = 0; i < GUC_MAX_LOG_BUFFER; i++) {
+		/* Update the read pointer in the shared log buffer */
+		log_buffer_state->read_ptr =
+			log_buffer_state->sampled_write_ptr;
+
+		/* Clear the 'flush to file' flag */
+		log_buffer_state->flush_to_file = 0;
+		log_buffer_state++;
+
+		if (!log_buffer_copy_state)
+			continue;
+
+		/* The write pointer could have been updated by the GuC firmware,
+		 * after sending the flush interrupt to Host, for consistency
+		 * set the write pointer value to same value of sampled_write_ptr
+		 * in the snapshot buffer.
+		 */
+		log_buffer_copy_state->write_ptr =
+			log_buffer_copy_state->sampled_write_ptr;
+
+		log_buffer_copy_state++;
+	}
+
+	kunmap_atomic(page);
+
+	/* Now copy the actual logs */
+	for (i=1; (i < guc->log_obj->base.size / PAGE_SIZE) && data_ptr; i++) {
+		data_ptr += PAGE_SIZE;
+		page = kmap_atomic(i915_gem_object_get_page(guc->log_obj, i));
+		memcpy(data_ptr, page, PAGE_SIZE);
+		kunmap_atomic(page);
+	}
+
+	guc_move_to_next_buf(guc);
+}
+
 static void guc_create_log(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1027,3 +1095,16 @@ int intel_guc_resume(struct drm_device *dev)
 
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
+
+void i915_guc_capture_logs(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	u32 data[1];
+
+	guc_read_update_log_buffer(dev);
+
+	data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
+	if (host2guc_action(guc, data, 1))
+		DRM_ERROR("Failed\n");
+}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b4294a8..6cea65b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1228,6 +1228,7 @@ static void gen8_guc2host_events_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, struct drm_i915_private, guc.events_work);
+	u32 msg;
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	/* Speed up work cancelation during disabling guc interrupts. */
@@ -1241,7 +1242,23 @@ static void gen8_guc2host_events_work(struct work_struct *work)
 	gen6_enable_pm_irq(dev_priv, GEN8_GUC_TO_HOST_INT_EVENT);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	/* TODO: Handle the events for which GuC interrupted host */
+	/* Determine why GuC interrupted host and process */
+	msg = I915_READ(SOFT_SCRATCH(15));
+	if (msg & (GUC2HOST_MSG_CRASH_DUMP_POSTED |
+		   GUC2HOST_MSG_FLUSH_LOG_BUFFER)) {
+		i915_guc_capture_logs(dev_priv->dev);
+
+		/* Clear GuC to Host msg bits that are handled */
+		if (msg & GUC2HOST_MSG_FLUSH_LOG_BUFFER)
+			I915_WRITE(SOFT_SCRATCH(15),
+				I915_READ(SOFT_SCRATCH(15)) &
+				~GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+
+		if (msg & GUC2HOST_MSG_CRASH_DUMP_POSTED)
+			I915_WRITE(SOFT_SCRATCH(15),
+				I915_READ(SOFT_SCRATCH(15)) &
+				~GUC2HOST_MSG_CRASH_DUMP_POSTED);
+	}
 
 	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index e20792d..92d70d8 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -167,5 +167,6 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_device *dev);
 void i915_guc_submission_fini(struct drm_device *dev);
+void i915_guc_capture_logs(struct drm_device *dev);
 
 #endif
-- 
1.9.2

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

[RFC 05/12] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new debugfs interface '/sys/kernel/debug/dri/guc_log' for the
User to capture GuC ukernel logs. Availed relay framework to implement
the interface, so Driver will just use a relay API to copy the contents
of GuC log buffer into relay's buffer.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 125 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |   1 +
 2 files changed, 126 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f7f29f6..379e2843 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -23,6 +23,8 @@
  */
 #include <linux/firmware.h>
 #include <linux/circ_buf.h>
+#include <linux/debugfs.h>
+#include <linux/relay.h>
 #include "i915_drv.h"
 #include "intel_guc.h"
 
@@ -837,6 +839,124 @@ static void guc_read_update_log_buffer(struct drm_device *dev)
 	guc_move_to_next_buf(guc);
 }
 
+#ifdef CONFIG_DEBUG_FS
+/*
+ * Sub buffer switch callback. If this callback is not implemented
+ * relay will operate in non-overwrite mode so will stop accepting
+ * new data if there are no empty sub buffers left.
+ */
+static int subbuf_start_callback(struct rchan_buf *buf,
+				 void *subbuf,
+				 void *prev_subbuf,
+				 size_t prev_padding)
+{
+	/* Always switch to next sub buffer as we don't mind overwriting of
+	 * old data/logs.
+	 */
+	return 1;
+}
+
+/*
+ * file_create() callback. Creates relay file in debugfs.
+ */
+static struct dentry *create_buf_file_callback(const char *filename,
+					       struct dentry *parent,
+					       umode_t mode,
+					       struct rchan_buf *buf,
+					       int *is_global)
+{
+	/* The relay API seems to fit well with debugfs only.
+	 * There are 3 requirements to associate a file with relay channel,
+	 * a) Need the associated dentry pointer of the file while opening
+	 *    the relay channel.
+	 * b) Should use 'relay_file_operations' fops for the file.
+	 * c) The private field of file's inode should be set to the pointer
+	 *    of relay channel buffer.
+	 * All the above 3 requirements can be met for a debugfs file in a
+	 * straightforward manner, but not for a file created inside the
+	 * sysfs or for a file created as a character device file.
+	 */
+	struct dentry *buf_file = debugfs_create_file(filename, mode,
+			parent, buf, &relay_file_operations);
+
+	/* This to enable the use of a single buffer for the relay channel and
+	 * correspondingly have a single file exposed to User, through which
+	 * it can pull the logs in order without any post-processing.
+	 */
+	*is_global = 1;
+
+	return buf_file;
+}
+
+/*
+ * file_remove() default callback. Removes relay file in debugfs.
+ */
+static int remove_buf_file_callback(struct dentry *dentry)
+{
+	debugfs_remove(dentry);
+	return 0;
+}
+
+/* relay channel callbacks */
+static struct rchan_callbacks relay_callbacks = {
+	.subbuf_start = subbuf_start_callback,
+	.create_buf_file = create_buf_file_callback,
+	.remove_buf_file = remove_buf_file_callback,
+};
+
+static void guc_remove_log_relay_file(struct intel_guc *guc)
+{
+	relay_close(guc->log_relay_chan);
+}
+
+static void guc_create_log_relay_file(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_device *dev = dev_priv->dev;
+	struct dentry *log_dir;
+	struct rchan *guc_log_relay_chan;
+	size_t n_subbufs, subbuf_size;
+
+	/* For now create the log file in /sys/kernel/debug/dri dir. */
+	log_dir = dev->primary->debugfs_root->d_parent;
+
+	/* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log_obj->base.size;
+	/* TODO: Decide based on the User's input */
+	n_subbufs = 4;
+
+	guc_log_relay_chan = relay_open("guc_log", log_dir,
+			subbuf_size, n_subbufs, &relay_callbacks, dev);
+
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for guc logs\n");
+		return;
+	}
+
+	guc->log_relay_chan = guc_log_relay_chan;
+
+	/* Enable interrupt to start capturing of the logs inside the
+	 * relay channel's buffer.
+	 */
+	if (i915.guc_log_level >= 0)
+		gen8_enable_guc_interrupts(dev);
+}
+#endif
+
+static void guc_logging_fini(struct intel_guc *guc)
+{
+#ifdef CONFIG_DEBUG_FS
+        guc_remove_log_relay_file(guc);
+#endif
+}
+
+static void guc_logging_init(struct intel_guc *guc)
+{
+#ifdef CONFIG_DEBUG_FS
+	guc_create_log_relay_file(guc);
+#endif
+}
+
 static void guc_create_log(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -873,6 +993,7 @@ static void guc_create_log(struct intel_guc *guc)
 
 	offset = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; /* in pages */
 	guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+	guc_logging_init(guc);
 }
 
 static void init_guc_policies(struct guc_policies *policies)
@@ -1037,6 +1158,7 @@ void i915_guc_submission_fini(struct drm_device *dev)
 
 	gem_release_guc_obj(dev_priv->guc.log_obj);
 	guc->log_obj = NULL;
+	guc_logging_fini(guc);
 
 	if (guc->ctx_pool_obj)
 		ida_destroy(&guc->ctx_ids);
@@ -1086,6 +1208,9 @@ int intel_guc_resume(struct drm_device *dev)
 	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
+	if (i915.guc_log_level >= 0 && guc->log_relay_chan)
+		gen8_enable_guc_interrupts(dev);
+
 	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 92d70d8..bf61925 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -125,6 +125,7 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
+	struct rchan *log_relay_chan;
 	/*
 	 * work, interrupts_enabled are protected by dev_priv->irq_lock
 	 */
-- 
1.9.2

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

[RFC 06/12] drm/i915: Store GuC ukernel logs in the relay buffer

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Using the buffer managed by relay to store the snapshots of GuC log
buffer. The snapshot will be taken when GuC ukernel sends a log buffer
flush interrupt.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 379e2843..d6d6ead 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -773,14 +773,30 @@ err:
 
 static void guc_move_to_next_buf(struct intel_guc *guc)
 {
+#ifdef CONFIG_DEBUG_FS
+	/* nothing to do here, all managed by relay */
 	return;
+#endif
 }
 
 static void* guc_get_write_buffer(struct intel_guc *guc)
 {
 	void *base_addr = NULL;
 
+#ifdef CONFIG_DEBUG_FS
+	if (!guc->log_relay_chan)
+		return NULL;
+
+	/* Get the pointer to relay sub buffer and copy data into it ourselves.
+	 * Could have used the relay_write() but we anyways need the pointer
+	 * to update the state data in first page so to be consistent directly
+	 * write to all pages of sub buffer.
+	 */
+	base_addr =
+		relay_reserve(guc->log_relay_chan, guc->log_obj->base.size);
+
 	return base_addr;
+#endif
 }
 
 static void guc_read_update_log_buffer(struct drm_device *dev)
-- 
1.9.2

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

[RFC 07/12] drm/i915: Allocate local buffer to store GuC ukernel logs

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

If relay framework can't be used for all production kernels,
due to debugfs limitation, then need to have our own local buffer
to store the logs and manage it.
This patch allocates a buffer to store the GuC ukernel logs.
The contents of the GuC log buffer will be copied into local buffer
on every flush interrupt from GuC. The local buffer is allocated a
GEM object and is accessed through the vmalloc mapping.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 40 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h           |  6 +++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d6d6ead..26b95e7 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -963,6 +963,12 @@ static void guc_logging_fini(struct intel_guc *guc)
 {
 #ifdef CONFIG_DEBUG_FS
         guc_remove_log_relay_file(guc);
+#else
+	if (guc->log.buf_obj) {
+		i915_gem_object_unpin_map(guc->log.buf_obj);
+		drm_gem_object_unreference(&guc->log.buf_obj->base);
+		guc->log.buf_obj = NULL;
+	}
 #endif
 }
 
@@ -970,6 +976,38 @@ static void guc_logging_init(struct intel_guc *guc)
 {
 #ifdef CONFIG_DEBUG_FS
 	guc_create_log_relay_file(guc);
+#else
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_i915_gem_object *obj;
+	size_t n_subbufs, subbuf_size;
+	void *vaddr;
+
+	spin_lock_init(&guc->log.buf_lock);
+
+	subbuf_size = guc->log_obj->base.size;
+	/* TODO: Decide based on the User's input */
+	n_subbufs = GUC_SCRATCH_LOG_ENTRIES_NR;
+
+	obj = i915_gem_object_create(dev_priv->dev, subbuf_size * n_subbufs);
+	if (IS_ERR(obj))
+		return;
+
+	/* For now permanently pin the pages & create a persistent vmalloc
+	 * mapping of them.
+	 * TODO: Pin/unpin on the fly (to participate in vmap shrinking also),
+	 * once dependency on struct_mutex is obviated.
+	 */
+	vaddr = i915_gem_object_pin_map(obj);
+	if (IS_ERR(vaddr)) {
+		drm_gem_object_unreference(&obj->base);
+		return;
+	}
+
+	guc->log.buf_obj = obj;
+
+	/* Enable the flush interrupt, we have a buffer to store GuC logs */
+	if (i915.guc_log_level >= 0)
+		gen8_enable_guc_interrupts(dev_priv->dev);
 #endif
 }
 
@@ -1224,7 +1262,7 @@ int intel_guc_resume(struct drm_device *dev)
 	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915.guc_log_level >= 0 && guc->log_relay_chan)
+	if (i915.guc_log_level >= 0 && (guc->log_relay_chan || guc->log.buf_obj))
 		gen8_enable_guc_interrupts(dev);
 
 	ctx = dev_priv->kernel_context;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index bf61925..9e6ce2d 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -121,11 +121,17 @@ struct intel_guc_fw {
 	uint32_t ucode_offset;
 };
 
+struct intel_guc_log {
+	struct drm_i915_gem_object *buf_obj;
+	spinlock_t buf_lock;
+};
+
 struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
 	struct rchan *log_relay_chan;
+	struct intel_guc_log log;
 	/*
 	 * work, interrupts_enabled are protected by dev_priv->irq_lock
 	 */
-- 
1.9.2

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

[RFC 08/12] drm/i915: Store GuC ukernel logs in the local buffer

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch copies the content of GuC log buffer into the local buffer.
The data is written into local buffer in a circular manner, so 2 pointers
are maintained to track the data inside the buffer. And after copying the
logs, readers waiting for the data are woken up.
The data from the local buffer is supposed to be consumed by Userspace.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 26b95e7..149826a 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -776,6 +776,11 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 #ifdef CONFIG_DEBUG_FS
 	/* nothing to do here, all managed by relay */
 	return;
+#else
+	/* New log buffer produced, wake up the readers */
+	guc->log.next_seq++;
+	spin_unlock(&guc->log.buf_lock);
+	wake_up_interruptible_all(&guc->log.wq);
 #endif
 }
 
@@ -796,6 +801,27 @@ static void* guc_get_write_buffer(struct intel_guc *guc)
 		relay_reserve(guc->log_relay_chan, guc->log_obj->base.size);
 
 	return base_addr;
+#else
+	size_t n_subbufs, subbuf_size;
+	int next_index;
+
+	base_addr = guc->log.buf_obj->mapping;
+	if (!base_addr)
+		return NULL;
+
+	subbuf_size = guc->log_obj->base.size;
+	n_subbufs = guc->log.buf_obj->base.size / subbuf_size;
+
+	spin_lock(&guc->log.buf_lock);
+	next_index = guc->log.next_seq & (n_subbufs - 1);
+	/*
+	 * If the logs are not being consumed fast enough, do not wait
+	 * and overwrite the oldest data.
+	 */
+	if ((guc->log.next_seq - guc->log.first_seq) >= n_subbufs)
+		guc->log.first_seq++;
+
+	return (base_addr + next_index * subbuf_size);
 #endif
 }
 
@@ -983,6 +1009,7 @@ static void guc_logging_init(struct intel_guc *guc)
 	void *vaddr;
 
 	spin_lock_init(&guc->log.buf_lock);
+	init_waitqueue_head(&guc->log.wq);
 
 	subbuf_size = guc->log_obj->base.size;
 	/* TODO: Decide based on the User's input */
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 9e6ce2d..bdc3e63 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -124,6 +124,9 @@ struct intel_guc_fw {
 struct intel_guc_log {
 	struct drm_i915_gem_object *buf_obj;
 	spinlock_t buf_lock;
+	wait_queue_head_t wq;
+	uint64_t first_seq;
+	uint64_t next_seq;
 };
 
 struct intel_guc {
-- 
1.9.2

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

[RFC 09/12] drm/i915: Add a char device file interface to capture GuC ukernel logs

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch provides a new character device file interface '/dev/dri/guc_log'
for the User to capture the GuC ukernel logs.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 51 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 149826a..8a79b6d 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -25,6 +25,7 @@
 #include <linux/circ_buf.h>
 #include <linux/debugfs.h>
 #include <linux/relay.h>
+#include <linux/miscdevice.h>
 #include "i915_drv.h"
 #include "intel_guc.h"
 
@@ -983,6 +984,54 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
 	if (i915.guc_log_level >= 0)
 		gen8_enable_guc_interrupts(dev);
 }
+#else
+static ssize_t guc_log_read(struct file *file, char __user *buf,
+			   size_t len, loff_t *pos)
+{
+	return 0;
+}
+
+static int guc_log_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static int guc_log_open(struct inode *inode, struct file *file)
+{
+	struct miscdevice *miscdev = file->private_data;
+	struct intel_guc_log *guc_log =
+		container_of(miscdev, struct intel_guc_log, misc_dev);
+	struct intel_guc *guc =
+		container_of(guc_log, struct intel_guc, log);
+
+	return 0;
+}
+
+static const struct file_operations guc_log_fops = {
+	.owner          = THIS_MODULE,
+	.open           = guc_log_open,
+	.release        = guc_log_release,
+	.read		= guc_log_read,
+	.llseek		= no_llseek,
+};
+
+static void guc_remove_log_device_file(struct intel_guc *guc)
+{
+	misc_deregister(&guc->log.misc_dev);
+}
+
+static void guc_create_log_device_file(struct intel_guc *guc)
+{
+	guc->log.misc_dev.minor = MISC_DYNAMIC_MINOR;
+	guc->log.misc_dev.name = "guc_log";
+	guc->log.misc_dev.nodename = "dri/guc_log";
+	guc->log.misc_dev.fops = &guc_log_fops;
+
+	if (misc_register(&guc->log.misc_dev)) {
+		DRM_ERROR("failed to register misc device for guc logs!\n");
+		return;
+	}
+}
 #endif
 
 static void guc_logging_fini(struct intel_guc *guc)
@@ -991,6 +1040,7 @@ static void guc_logging_fini(struct intel_guc *guc)
         guc_remove_log_relay_file(guc);
 #else
 	if (guc->log.buf_obj) {
+		guc_remove_log_device_file(guc);
 		i915_gem_object_unpin_map(guc->log.buf_obj);
 		drm_gem_object_unreference(&guc->log.buf_obj->base);
 		guc->log.buf_obj = NULL;
@@ -1031,6 +1081,7 @@ static void guc_logging_init(struct intel_guc *guc)
 	}
 
 	guc->log.buf_obj = obj;
+	guc_create_log_device_file(guc);
 
 	/* Enable the flush interrupt, we have a buffer to store GuC logs */
 	if (i915.guc_log_level >= 0)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index bdc3e63..13810d0 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -127,6 +127,8 @@ struct intel_guc_log {
 	wait_queue_head_t wq;
 	uint64_t first_seq;
 	uint64_t next_seq;
+	/* For Userspace clients to pull logs via /dev/dri/guc_log */
+	struct miscdevice misc_dev;
 };
 
 struct intel_guc {
-- 
1.9.2

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

[RFC 10/12] drm/i915: Support to capture GuC logs by multiple clients via device file iface

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch adds support for multiple clients to capture GuC logs via
'/dev/dri/guc_log' interface at the same time.
The implementation is done on the lines of '/dev/kmsg', so provides a
streaming behavior, on issuing the 'cat' command User will enter into a
read loop and will have to do 'CTRL+C' type action to come out of the loop.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 124 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h           |   9 +++
 2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8a79b6d..f20e352 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -988,11 +988,40 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
 static ssize_t guc_log_read(struct file *file, char __user *buf,
 			   size_t len, loff_t *pos)
 {
-	return 0;
+	struct intel_guc_log_client *log_client = file->private_data;
+	struct intel_guc *guc;
+	ssize_t ret;
+
+	if (!log_client)
+		return -EBADF;
+
+	guc = log_client->guc;
+
+	/* Can't provide more than scratch buf size */
+	len = min(PAGE_SIZE, len);
+
+	spin_lock_irq(&guc->log.buf_lock);
+	ret = i915_guc_read_logs(log_client, len, file->f_flags);
+	spin_unlock_irq(&guc->log.buf_lock);
+
+	if (ret < 0)
+		return ret;
+
+	if (copy_to_user(buf, log_client->scratch_buf, ret))
+		return -EFAULT;
+
+	return ret;
 }
 
 static int guc_log_release(struct inode *inode, struct file *file)
 {
+	struct intel_guc_log_client *log_client = file->private_data;
+
+	if (!log_client)
+		return 0;
+
+	kfree(log_client->scratch_buf);
+	kfree(log_client);
 	return 0;
 }
 
@@ -1003,6 +1032,26 @@ static int guc_log_open(struct inode *inode, struct file *file)
 		container_of(miscdev, struct intel_guc_log, misc_dev);
 	struct intel_guc *guc =
 		container_of(guc_log, struct intel_guc, log);
+	struct intel_guc_log_client *log_client;
+
+	log_client = kzalloc(sizeof(struct intel_guc_log_client), GFP_KERNEL);
+	if (!log_client)
+		return -ENOMEM;
+
+	/* TODO, decide apt size for scratch_buf (and allocate as a GEM obj) */
+	log_client->scratch_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!log_client->scratch_buf) {
+		kfree(log_client);
+		return -ENOMEM;
+	}
+
+	log_client->guc = guc;
+
+	spin_lock(&guc->log.buf_lock);
+	log_client->read_seq = guc->log.first_seq;
+	spin_unlock(&guc->log.buf_lock);
+
+	file->private_data = log_client;
 
 	return 0;
 }
@@ -1365,3 +1414,76 @@ void i915_guc_capture_logs(struct drm_device *dev)
 	if (host2guc_action(guc, data, 1))
 		DRM_ERROR("Failed\n");
 }
+
+ssize_t i915_guc_read_logs(struct intel_guc_log_client *log_client,
+			   size_t count, uint32_t f_flags)
+{
+	struct intel_guc *guc = log_client->guc;
+	char *buf = log_client->scratch_buf;
+	void *log_buf_base, *log_buf_end, *log_buf_read, *log_buf_write;
+	uint32_t read_index, next_index;
+	size_t n_subbufs, subbuf_size;
+	ssize_t ret_count = 0;
+	int ret = 0;
+
+	assert_spin_locked(&guc->log.buf_lock);
+
+	subbuf_size = guc->log_obj->base.size;
+	n_subbufs = guc->log.buf_obj->base.size / subbuf_size;
+
+	/* Wait if there is no data to read */
+	while (log_client->read_seq == guc->log.next_seq) {
+		if (f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible_lock_irq(guc->log.wq,
+				log_client->read_seq != guc->log.next_seq,
+				guc->log.buf_lock);
+		if (ret)
+			return ret;
+	}
+
+	/* Check if our last seen data is gone */
+	if (log_client->read_seq < guc->log.first_seq) {
+		log_client->read_seq = guc->log.first_seq;
+		log_client->read_offset = 0;
+	}
+
+	read_index = log_client->read_seq & (n_subbufs - 1);
+	next_index = guc->log.next_seq & (n_subbufs - 1);
+
+	log_buf_base = guc->log.buf_obj->mapping;
+	log_buf_end = log_buf_base + guc->log.buf_obj->base.size;
+	log_buf_write = log_buf_base + next_index * subbuf_size;
+	log_buf_read = log_buf_base + read_index * subbuf_size +
+				log_client->read_offset;
+
+	if (log_buf_read < log_buf_write) {
+		ret_count =
+			min_t(size_t, count, (log_buf_write - log_buf_read));
+		memcpy(buf, log_buf_read, ret_count);
+	} else {
+		ssize_t ret_count1, ret_count2 = 0;
+		ret_count1 =
+			min_t(size_t, count, (log_buf_end - log_buf_read));
+		memcpy(buf, log_buf_read, ret_count1);
+		if (count > ret_count1) {
+			count -= ret_count1;
+			buf += ret_count1;
+			ret_count2 =
+			  min_t(size_t, count, (log_buf_write - log_buf_base));
+			memcpy(buf, log_buf_base, ret_count2);
+		}
+		ret_count = ret_count1 + ret_count2;
+	}
+
+	log_client->read_offset += ret_count;
+	if (log_client->read_offset >= subbuf_size) {
+		log_client->read_seq += log_client->read_offset / subbuf_size;
+		log_client->read_offset %= subbuf_size;
+	}
+
+	WARN_ON(log_client->read_seq > guc->log.next_seq);
+
+	return ret_count;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 13810d0..985fb4b 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -121,6 +121,13 @@ struct intel_guc_fw {
 	uint32_t ucode_offset;
 };
 
+struct intel_guc_log_client {
+	uint64_t read_seq;
+	uint32_t read_offset;
+	char *scratch_buf;
+	struct intel_guc *guc;
+};
+
 struct intel_guc_log {
 	struct drm_i915_gem_object *buf_obj;
 	spinlock_t buf_lock;
@@ -180,5 +187,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_device *dev);
 void i915_guc_submission_fini(struct drm_device *dev);
 void i915_guc_capture_logs(struct drm_device *dev);
+ssize_t i915_guc_read_logs(struct intel_guc_log_client *log_client,
+			   size_t count, uint32_t f_flags);
 
 #endif
-- 
1.9.2

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

[RFC 11/12] drm/i915: Add sysfs interface to capture the GuC ukernel logs

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch adds support to pull GuC logs stored in the local buffer
through a sysfs interface '/sys/class/drm/card0/guc_log'.
The implementation is on the lines of '/proc/kmsg' and sysfs will be
one of the client just like other clients pulling the logs via
'/dev/dri/guc_log' interface.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h  |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 02507bf..77d43cd 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -590,6 +590,40 @@ static struct bin_attribute error_state_attr = {
 	.write = error_state_write,
 };
 
+static ssize_t guc_log_data_read(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *attr, char *buf,
+				loff_t off, size_t count)
+{
+	struct device *kdev = kobj_to_dev(kobj);
+	struct drm_minor *minor = dev_to_drm_minor(kdev);
+	struct drm_device *dev = minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc_log_client *log_client = &guc->log.sysfs_client;
+	int ret;
+
+	if (!guc->log_obj || !guc->log.buf_obj)
+		return -ENODEV;
+
+	spin_lock_irq(&guc->log.buf_lock);
+
+	log_client->scratch_buf = buf;
+	log_client->guc = guc;
+	ret = i915_guc_read_logs(log_client, count, filp->f_flags);
+	log_client->scratch_buf = NULL;
+
+	spin_unlock_irq(&guc->log.buf_lock);
+
+	return ret;
+}
+
+static struct bin_attribute guc_log_attr = {
+	.attr.name = "guc_log",
+	.attr.mode = S_IRUSR,
+	.size = 0,
+	.read = guc_log_data_read,
+};
+
 void i915_setup_sysfs(struct drm_device *dev)
 {
 	int ret;
@@ -639,10 +673,16 @@ void i915_setup_sysfs(struct drm_device *dev)
 				    &error_state_attr);
 	if (ret)
 		DRM_ERROR("error_state sysfs setup failed\n");
+
+	ret = sysfs_create_bin_file(&dev->primary->kdev->kobj,
+				    &guc_log_attr);
+	if (ret)
+		DRM_ERROR("guc_log sysfs setup failed\n");
 }
 
 void i915_teardown_sysfs(struct drm_device *dev)
 {
+	sysfs_remove_bin_file(&dev->primary->kdev->kobj, &guc_log_attr);
 	sysfs_remove_bin_file(&dev->primary->kdev->kobj, &error_state_attr);
 	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
 		sysfs_remove_files(&dev->primary->kdev->kobj, vlv_attrs);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 985fb4b..5bb44b5 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -136,6 +136,8 @@ struct intel_guc_log {
 	uint64_t next_seq;
 	/* For Userspace clients to pull logs via /dev/dri/guc_log */
 	struct miscdevice misc_dev;
+	/* For getting logs via /sys/class/drm/card0/guc_log iface */
+	struct intel_guc_log_client sysfs_client;
 };
 
 struct intel_guc {
-- 
1.9.2

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

[RFC 12/12] drm/i915: Forcefully flush GuC log buffer on reset

[akash.goel]

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

If GuC logs are being captured, there should be a force log buffer flush
action sent to GuC before proceeding with GPU reset and re-initializing
GUC. Those logs would be useful to understand why the GPU reset was
initiated.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c            |  3 +++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f20e352..72506f0 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1402,6 +1402,24 @@ int intel_guc_resume(struct drm_device *dev)
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
 
+int i915_guc_log_buffer_force_flush(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	u32 data[2];
+	int ret = 0;
+
+	data[0] = HOST2GUC_ACTION_FORCE_LOGBUFFERFLUSH;
+	data[1] = 0;
+
+	if (host2guc_action(guc, data, 2)) {
+		ret = -EINVAL;
+		DRM_ERROR("Failed\n");
+	}
+
+	return ret;
+}
+
 void i915_guc_capture_logs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6cea65b..a78db29 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2812,6 +2812,9 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
 	vscnprintf(error_msg, sizeof(error_msg), fmt, args);
 	va_end(args);
 
+	flush_work(&dev_priv->guc.events_work);
+	i915_guc_log_buffer_force_flush(dev_priv->dev);
+
 	i915_capture_error_state(dev_priv, engine_mask, error_msg);
 	i915_report_and_clear_eir(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 5bb44b5..e3c4e08 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -188,6 +188,7 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_device *dev);
 void i915_guc_submission_fini(struct drm_device *dev);
+int i915_guc_log_buffer_force_flush(struct drm_device *dev);
 void i915_guc_capture_logs(struct drm_device *dev);
 ssize_t i915_guc_read_logs(struct intel_guc_log_client *log_client,
 			   size_t count, uint32_t f_flags);
-- 
1.9.2

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

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Chris Wilson]

On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> There are certain types of interrupts which Host can recieve from GuC.
> GuC ukernel sends an interrupt to Host for certain events, like for
> example retrieve/consume the logs generated by ukernel.
> This patch adds support to receive interrupts from GuC but currently
> enables & partially handles only the interrupt sent by GuC ukernel.
> Future patches will add support for handling other interrupt types.

gen8 doesn't have a GuC. How can these functions be applicable to gen8?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 09/12] drm/i915: Add a char device file interface to capture GuC ukernel logs

[Chris Wilson]

On Sat, May 28, 2016 at 01:13:00AM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch provides a new character device file interface '/dev/dri/guc_log'
> for the User to capture the GuC ukernel logs.

Do not make the device conditional on !CONFIG_DEBUGFS.
With a stable interface like the device, you do not need a debugfs
entry.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Chris Wilson]

On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> There are certain types of interrupts which Host can recieve from GuC.
> GuC ukernel sends an interrupt to Host for certain events, like for
> example retrieve/consume the logs generated by ukernel.
> This patch adds support to receive interrupts from GuC but currently
> enables & partially handles only the interrupt sent by GuC ukernel.
> Future patches will add support for handling other interrupt types.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |   1 +
>  drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
>  drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
>  drivers/gpu/drm/i915/intel_drv.h           |   3 +
>  drivers/gpu/drm/i915/intel_guc.h           |   5 ++
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
>  7 files changed, 120 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4c8e34..7aae033 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1790,6 +1790,7 @@ struct drm_i915_private {
>  	u32 gt_irq_mask;
>  	u32 pm_irq_mask;
>  	u32 pm_rps_events;
> +	u32 guc_events;
>  	u32 pipestat_irq_mask[I915_MAX_PIPES];
>  
>  	struct i915_hotplug hotplug;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index da7c242..c2f3a67 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev)
>  	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>  		return 0;
>  
> +	gen8_disable_guc_interrupts(dev);
> +
>  	ctx = dev_priv->kernel_context;
>  
>  	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index caaf1e2..b4294a8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>  } while (0)
>  
>  static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>  
>  /* For display hotplug interrupt */
>  static inline void
> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>  	synchronize_irq(dev_priv->dev->irq);
>  }
>  
> +void gen8_reset_guc_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	i915_reg_t reg = gen6_pm_iir(dev_priv);

From the looks of this we have multiple shadows for the same register.
That's very bad.

Now the platforms might be mutually exclusive, but it is still a mistake
that will catch us out.

> +	spin_lock_irq(&dev_priv->irq_lock);
> +	I915_WRITE(reg, dev_priv->guc_events);
> +	I915_WRITE(reg, dev_priv->guc_events);

What? Not even the tiniest of comments to explain?

> +	POSTING_READ(reg);

Again. Not even the tiniest of comments to explain?

> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen8_enable_guc_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
> +					dev_priv->guc_events);
> +		dev_priv->guc.interrupts_enabled = true;
> +		I915_WRITE(gen6_pm_ier(dev_priv),
> +		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);

ier should be known, rmw on the reg should not be required.

> +		gen6_enable_pm_irq(dev_priv, dev_priv->guc_events);
> +	}
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen8_disable_guc_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->guc.interrupts_enabled = false;
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	cancel_work_sync(&dev_priv->guc.events_work);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	__gen6_disable_pm_irq(dev_priv, dev_priv->guc_events);
> +	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
> +				~dev_priv->guc_events);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	synchronize_irq(dev->irq);
> +}
> +
>  /**
>   * bdw_update_port_irq - update DE port interrupt
>   * @dev_priv: driver private
> @@ -1174,6 +1224,27 @@ out:
>  	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>  }
>  
> +static void gen8_guc2host_events_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, guc.events_work);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	/* Speed up work cancelation during disabling guc interrupts. */
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
> +
> +	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);

This just shouts that the code is broken.


>  void intel_crt_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 41601c7..e20792d 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -125,6 +125,11 @@ struct intel_guc {
>  	struct intel_guc_fw guc_fw;
>  	uint32_t log_flags;
>  	struct drm_i915_gem_object *log_obj;
> +	/*
> +	 * work, interrupts_enabled are protected by dev_priv->irq_lock
> +	 */
> +	struct work_struct events_work;

The work gets added here, yet bugs are fixed for the worker in later
patches. Squash in the bug fixes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Support for sustained capturing of GuC firmware logs

[Patchwork]

== Series Details ==

Series: Support for sustained capturing of GuC firmware logs
URL   : https://patchwork.freedesktop.org/series/7910/
State : failure

== Summary ==

Series 7910v1 Support for sustained capturing of GuC firmware logs
http://patchwork.freedesktop.org/api/1.0/series/7910/revisions/1/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)

fi-bdw-i7-5557u  total:209  pass:192  dwarn:5   dfail:0   fail:0   skip:12 
fi-hsw-i7-4770k  total:102  pass:85   dwarn:1   dfail:0   fail:0   skip:15 
fi-hsw-i7-4770r  total:209  pass:179  dwarn:5   dfail:0   fail:0   skip:25 
fi-skl-i7-6700k  total:209  pass:180  dwarn:4   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:166  dwarn:4   dfail:0   fail:0   skip:39 
ro-bdw-i7-5557U  total:209  pass:188  dwarn:6   dfail:0   fail:0   skip:15 
ro-bdw-i7-5600u  total:209  pass:176  dwarn:5   dfail:0   fail:0   skip:28 
ro-byt-n2820     total:209  pass:165  dwarn:4   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:209  pass:181  dwarn:5   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:209  pass:181  dwarn:5   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
ro-ilk1-i5-650   total:204  pass:142  dwarn:4   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:209  pass:172  dwarn:5   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:209  pass:176  dwarn:5   dfail:0   fail:0   skip:28 
ro-snb-i7-2620M  total:209  pass:166  dwarn:4   dfail:0   fail:1   skip:38 
fi-bsw-n3050 failed to connect after reboot
fi-byt-n2820 failed to connect after reboot
ro-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1044/

5c0712f drm-intel-nightly: 2016y-05m-27d-12h-32m-45s UTC integration manifest
f00d84c drm/i915: Forcefully flush GuC log buffer on reset
956106a drm/i915: Add sysfs interface to capture the GuC ukernel logs
c7d48f8 drm/i915: Support to capture GuC logs by multiple clients via device file iface
c5e7b4f drm/i915: Add a char device file interface to capture GuC ukernel logs
e6f28b2 drm/i915: Store GuC ukernel logs in the local buffer
f9e5b8f drm/i915: Allocate local buffer to store GuC ukernel logs
5d5304c drm/i915: Store GuC ukernel logs in the relay buffer
79528fb drm/i915: Add a relay backed debugfs interface for capturing GuC logs
a98f933 drm/i915: Handle log buffer flush interrupt event from GuC
1426118 drm/i915: Support for GuC interrupts
b5c8993 drm/i915: Add GuC ukernel logging related fields to fw interface file
da0b2a1 drm/i915: Decouple GuC log setup from verbosity parameter

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

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 5/28/2016 1:13 AM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>
>> There are certain types of interrupts which Host can recieve from GuC.
>> GuC ukernel sends an interrupt to Host for certain events, like for
>> example retrieve/consume the logs generated by ukernel.
>> This patch adds support to receive interrupts from GuC but currently
>> enables & partially handles only the interrupt sent by GuC ukernel.
>> Future patches will add support for handling other interrupt types.
>
> gen8 doesn't have a GuC. How can these functions be applicable to gen8?

Sorry I missed that in the Driver GuC is used only from Gen9 onwards.
But Gen8 (both BDW & CHV) does have a GuC.
I will rename the functions.

Best regards
Akash

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

Re: [RFC 09/12] drm/i915: Add a char device file interface to capture GuC ukernel logs

[Goel, Akash]

On 5/28/2016 1:18 AM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 01:13:00AM +0530, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> This patch provides a new character device file interface '/dev/dri/guc_log'
>> for the User to capture the GuC ukernel logs.
>
> Do not make the device conditional on !CONFIG_DEBUGFS.
> With a stable interface like the device, you do not need a debugfs
> entry.

Sorry, actually I was unsure, don't know which interface would be most 
appropriate here.
Thought if CONFIG_DEBUG_FS is defined, then relay backed debugfs file 
might be the most preferred interface, in which case device file 
interface may not be required.

If device file interface is most suitable, then may not have a relay 
backed debugfs interface.

Please suggest.

Best regards
Akash
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 5/28/2016 1:26 AM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>
>> There are certain types of interrupts which Host can recieve from GuC.
>> GuC ukernel sends an interrupt to Host for certain events, like for
>> example retrieve/consume the logs generated by ukernel.
>> This patch adds support to receive interrupts from GuC but currently
>> enables & partially handles only the interrupt sent by GuC ukernel.
>> Future patches will add support for handling other interrupt types.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h            |   1 +
>>  drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
>>  drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
>>  drivers/gpu/drm/i915/intel_drv.h           |   3 +
>>  drivers/gpu/drm/i915/intel_guc.h           |   5 ++
>>  drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
>>  7 files changed, 120 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e4c8e34..7aae033 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1790,6 +1790,7 @@ struct drm_i915_private {
>>  	u32 gt_irq_mask;
>>  	u32 pm_irq_mask;
>>  	u32 pm_rps_events;
>> +	u32 guc_events;
>>  	u32 pipestat_irq_mask[I915_MAX_PIPES];
>>
>>  	struct i915_hotplug hotplug;
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index da7c242..c2f3a67 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev)
>>  	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>>  		return 0;
>>
>> +	gen8_disable_guc_interrupts(dev);
>> +
>>  	ctx = dev_priv->kernel_context;
>>
>>  	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index caaf1e2..b4294a8 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>>  } while (0)
>>
>>  static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>> +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>>
>>  /* For display hotplug interrupt */
>>  static inline void
>> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>>  	synchronize_irq(dev_priv->dev->irq);
>>  }
>>
>> +void gen8_reset_guc_interrupts(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	i915_reg_t reg = gen6_pm_iir(dev_priv);
>
> From the looks of this we have multiple shadows for the same register.
> That's very bad.
>
> Now the platforms might be mutually exclusive, but it is still a mistake
> that will catch us out.
>
Will check how it is in newer platforms.

>> +	spin_lock_irq(&dev_priv->irq_lock);
>> +	I915_WRITE(reg, dev_priv->guc_events);
>> +	I915_WRITE(reg, dev_priv->guc_events);
>
> What? Not even the tiniest of comments to explain?
>
Sorry actually just copied these steps as is from the 
gen6_reset_rps_interrupts(), considering that the same set of registers 
(IIR, IER, IMR) are involved here.
So the double clearing of IIR followed by posting read could be needed 
here also.

>> +	POSTING_READ(reg);
>
> Again. Not even the tiniest of comments to explain?
>
>> +	spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>> +void gen8_enable_guc_interrupts(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	spin_lock_irq(&dev_priv->irq_lock);
>> +	if (!dev_priv->guc.interrupts_enabled) {
>> +		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
>> +					dev_priv->guc_events);
>> +		dev_priv->guc.interrupts_enabled = true;
>> +		I915_WRITE(gen6_pm_ier(dev_priv),
>> +		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
>
> ier should be known, rmw on the reg should not be required.
>
Sorry same as above, copy paste from gen6_enable_rps_interrupts().
Without rmw, would this be fine ?

	if (dev_priv->rps.interrupts_enabled)
		I915_WRITE(gen6_pm_ier(dev_priv),
			dev_priv->pm_rps_events | dev_priv->guc_events);
	else
		I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);


>> +		gen6_enable_pm_irq(dev_priv, dev_priv->guc_events);
>> +	}
>> +	spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>> +void gen8_disable_guc_interrupts(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	spin_lock_irq(&dev_priv->irq_lock);
>> +	dev_priv->guc.interrupts_enabled = false;
>> +	spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> +	cancel_work_sync(&dev_priv->guc.events_work);
>> +
>> +	spin_lock_irq(&dev_priv->irq_lock);
>> +
>> +	__gen6_disable_pm_irq(dev_priv, dev_priv->guc_events);
>> +	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
>> +				~dev_priv->guc_events);
>> +
>> +	spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> +	synchronize_irq(dev->irq);
>> +}
>> +
>>  /**
>>   * bdw_update_port_irq - update DE port interrupt
>>   * @dev_priv: driver private
>> @@ -1174,6 +1224,27 @@ out:
>>  	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>>  }
>>
>> +static void gen8_guc2host_events_work(struct work_struct *work)
>> +{
>> +	struct drm_i915_private *dev_priv =
>> +		container_of(work, struct drm_i915_private, guc.events_work);
>> +
>> +	spin_lock_irq(&dev_priv->irq_lock);
>> +	/* Speed up work cancelation during disabling guc interrupts. */
>> +	if (!dev_priv->guc.interrupts_enabled) {
>> +		spin_unlock_irq(&dev_priv->irq_lock);
>> +		return;
>> +	}
>> +
>> +	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>
> This just shouts that the code is broken.
>
You mean to say that ideally the wakeref_count (& power.usage_count) 
should already be non zero here.

>
>>  void intel_crt_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
>> index 41601c7..e20792d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -125,6 +125,11 @@ struct intel_guc {
>>  	struct intel_guc_fw guc_fw;
>>  	uint32_t log_flags;
>>  	struct drm_i915_gem_object *log_obj;
>> +	/*
>> +	 * work, interrupts_enabled are protected by dev_priv->irq_lock
>> +	 */
>> +	struct work_struct events_work;
>
> The work gets added here, yet bugs are fixed for the worker in later
> patches. Squash in the bug fixes.

Sorry didn't get this. Are you alluding to cancellation of this 
work_item Or flushing of work item from the error handling path ?

Cancellation is being done as a part of disabling interrupt and the call 
to disable interrupt is there in intel_guc_suspend(), part of this patch 
only.

Best regards
Akash



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

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Chris Wilson]

On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote:
> 
> 
> On 5/28/2016 1:26 AM, Chris Wilson wrote:
> >On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
> >>From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >>
> >>There are certain types of interrupts which Host can recieve from GuC.
> >>GuC ukernel sends an interrupt to Host for certain events, like for
> >>example retrieve/consume the logs generated by ukernel.
> >>This patch adds support to receive interrupts from GuC but currently
> >>enables & partially handles only the interrupt sent by GuC ukernel.
> >>Future patches will add support for handling other interrupt types.
> >>
> >>Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>---
> >> drivers/gpu/drm/i915/i915_drv.h            |   1 +
> >> drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
> >> drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
> >> drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
> >> drivers/gpu/drm/i915/intel_drv.h           |   3 +
> >> drivers/gpu/drm/i915/intel_guc.h           |   5 ++
> >> drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
> >> 7 files changed, 120 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index e4c8e34..7aae033 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -1790,6 +1790,7 @@ struct drm_i915_private {
> >> 	u32 gt_irq_mask;
> >> 	u32 pm_irq_mask;
> >> 	u32 pm_rps_events;
> >>+	u32 guc_events;
> >> 	u32 pipestat_irq_mask[I915_MAX_PIPES];
> >>
> >> 	struct i915_hotplug hotplug;
> >>diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>index da7c242..c2f3a67 100644
> >>--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >>+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >>@@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev)
> >> 	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
> >> 		return 0;
> >>
> >>+	gen8_disable_guc_interrupts(dev);
> >>+
> >> 	ctx = dev_priv->kernel_context;
> >>
> >> 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
> >>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>index caaf1e2..b4294a8 100644
> >>--- a/drivers/gpu/drm/i915/i915_irq.c
> >>+++ b/drivers/gpu/drm/i915/i915_irq.c
> >>@@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
> >> } while (0)
> >>
> >> static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> >>+static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> >>
> >> /* For display hotplug interrupt */
> >> static inline void
> >>@@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
> >> 	synchronize_irq(dev_priv->dev->irq);
> >> }
> >>
> >>+void gen8_reset_guc_interrupts(struct drm_device *dev)
> >>+{
> >>+	struct drm_i915_private *dev_priv = dev->dev_private;
> >>+	i915_reg_t reg = gen6_pm_iir(dev_priv);
> >
> >From the looks of this we have multiple shadows for the same register.
> >That's very bad.
> >
> >Now the platforms might be mutually exclusive, but it is still a mistake
> >that will catch us out.
> >
> Will check how it is in newer platforms.
> 
> >>+	spin_lock_irq(&dev_priv->irq_lock);
> >>+	I915_WRITE(reg, dev_priv->guc_events);
> >>+	I915_WRITE(reg, dev_priv->guc_events);
> >
> >What? Not even the tiniest of comments to explain?
> >
> Sorry actually just copied these steps as is from the
> gen6_reset_rps_interrupts(), considering that the same set of
> registers (IIR, IER, IMR) are involved here.
> So the double clearing of IIR followed by posting read could be
> needed here also.

Move it all to i915_irq.c and export routines to manipulate pm_iir such
that multiple users do not conflict.
 
> >>+	POSTING_READ(reg);
> >
> >Again. Not even the tiniest of comments to explain?
> >
> >>+	spin_unlock_irq(&dev_priv->irq_lock);
> >>+}
> >>+
> >>+void gen8_enable_guc_interrupts(struct drm_device *dev)
> >>+{
> >>+	struct drm_i915_private *dev_priv = dev->dev_private;
> >>+
> >>+	spin_lock_irq(&dev_priv->irq_lock);
> >>+	if (!dev_priv->guc.interrupts_enabled) {
> >>+		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
> >>+					dev_priv->guc_events);
> >>+		dev_priv->guc.interrupts_enabled = true;
> >>+		I915_WRITE(gen6_pm_ier(dev_priv),
> >>+		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
> >
> >ier should be known, rmw on the reg should not be required.
> >
> Sorry same as above, copy paste from gen6_enable_rps_interrupts().
> Without rmw, would this be fine ?
> 
> 	if (dev_priv->rps.interrupts_enabled)
> 		I915_WRITE(gen6_pm_ier(dev_priv),
> 			dev_priv->pm_rps_events | dev_priv->guc_events);
> 	else
> 		I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);

Still has the presumption of owning a register that is ostensibly used
by others.

> >>+static void gen8_guc2host_events_work(struct work_struct *work)
> >>+{
> >>+	struct drm_i915_private *dev_priv =
> >>+		container_of(work, struct drm_i915_private, guc.events_work);
> >>+
> >>+	spin_lock_irq(&dev_priv->irq_lock);
> >>+	/* Speed up work cancelation during disabling guc interrupts. */
> >>+	if (!dev_priv->guc.interrupts_enabled) {
> >>+		spin_unlock_irq(&dev_priv->irq_lock);
> >>+		return;
> >>+	}
> >>+
> >>+	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
> >
> >This just shouts that the code is broken.
> >
> You mean to say that ideally the wakeref_count (& power.usage_count)
> should already be non zero here.

Yes. If it is not under your control, then you have a bug in your code.
Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug
(and hacks in place whilst we wait for patch review).

> >> void intel_crt_init(struct drm_device *dev);
> >>diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> >>index 41601c7..e20792d 100644
> >>--- a/drivers/gpu/drm/i915/intel_guc.h
> >>+++ b/drivers/gpu/drm/i915/intel_guc.h
> >>@@ -125,6 +125,11 @@ struct intel_guc {
> >> 	struct intel_guc_fw guc_fw;
> >> 	uint32_t log_flags;
> >> 	struct drm_i915_gem_object *log_obj;
> >>+	/*
> >>+	 * work, interrupts_enabled are protected by dev_priv->irq_lock
> >>+	 */
> >>+	struct work_struct events_work;
> >
> >The work gets added here, yet bugs are fixed for the worker in later
> >patches. Squash in the bug fixes.
> 
> Sorry didn't get this. Are you alluding to cancellation of this
> work_item Or flushing of work item from the error handling path ?
> 
> Cancellation is being done as a part of disabling interrupt and the
> call to disable interrupt is there in intel_guc_suspend(), part of
> this patch only.

You add a flush later, which seems unrelated to that patch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 5/28/2016 5:43 PM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote:
>>
>>
>> On 5/28/2016 1:26 AM, Chris Wilson wrote:
>>> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
>>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>
>>>> There are certain types of interrupts which Host can recieve from GuC.
>>>> GuC ukernel sends an interrupt to Host for certain events, like for
>>>> example retrieve/consume the logs generated by ukernel.
>>>> This patch adds support to receive interrupts from GuC but currently
>>>> enables & partially handles only the interrupt sent by GuC ukernel.
>>>> Future patches will add support for handling other interrupt types.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h            |   1 +
>>>> drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
>>>> drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
>>>> drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
>>>> drivers/gpu/drm/i915/intel_drv.h           |   3 +
>>>> drivers/gpu/drm/i915/intel_guc.h           |   5 ++
>>>> drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
>>>> 7 files changed, 120 insertions(+), 3 deletions(-)
>>>>>>>>
>>>> static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>>>> +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>>>>
>>>> /* For display hotplug interrupt */
>>>> static inline void
>>>> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>>>> 	synchronize_irq(dev_priv->dev->irq);
>>>> }
>>>>
>>>> +void gen8_reset_guc_interrupts(struct drm_device *dev)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +	i915_reg_t reg = gen6_pm_iir(dev_priv);
>>>
>> >From the looks of this we have multiple shadows for the same register.
>>> That's very bad.
>>>
>>> Now the platforms might be mutually exclusive, but it is still a mistake
>>> that will catch us out.
>>>
>> Will check how it is in newer platforms.
>>
>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	I915_WRITE(reg, dev_priv->guc_events);
>>>> +	I915_WRITE(reg, dev_priv->guc_events);
>>>
>>> What? Not even the tiniest of comments to explain?
>>>
>> Sorry actually just copied these steps as is from the
>> gen6_reset_rps_interrupts(), considering that the same set of
>> registers (IIR, IER, IMR) are involved here.
>> So the double clearing of IIR followed by posting read could be
>> needed here also.
>
> Move it all to i915_irq.c and export routines to manipulate pm_iir such
> that multiple users do not conflict.
>
Sorry but all interrupt related stuff for rps & GuC is already inside 
i915_irq.c file.
Also the IER, IMR, IIR registers are being updated in a non conflicting 
manner, no overlap between the PM bits & GuC events bits.

You mean to say need to have single set of routines only for interrupt
reset/enable/disable operations for rps & GuC.

>>>> +	POSTING_READ(reg);
>>>
>>> Again. Not even the tiniest of comments to explain?
>>>
>>>> +	spin_unlock_irq(&dev_priv->irq_lock);
>>>> +}
>>>> +
>>>> +void gen8_enable_guc_interrupts(struct drm_device *dev)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +
>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	if (!dev_priv->guc.interrupts_enabled) {
>>>> +		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
>>>> +					dev_priv->guc_events);
>>>> +		dev_priv->guc.interrupts_enabled = true;
>>>> +		I915_WRITE(gen6_pm_ier(dev_priv),
>>>> +		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
>>>
>>> ier should be known, rmw on the reg should not be required.
>>>
>> Sorry same as above, copy paste from gen6_enable_rps_interrupts().
>> Without rmw, would this be fine ?
>>
>> 	if (dev_priv->rps.interrupts_enabled)
>> 		I915_WRITE(gen6_pm_ier(dev_priv),
>> 			dev_priv->pm_rps_events | dev_priv->guc_events);
>> 	else
>> 		I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);
>
> Still has the presumption of owning a register that is ostensibly used
> by others.
>
Since pm_ier is a shared register and being used by others also, rmw
seem to be more suited here. Otherwise need to be aware of who all is
sharing it so as to update it without disturbing the bits owned by
others.

>>>> +static void gen8_guc2host_events_work(struct work_struct *work)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv =
>>>> +		container_of(work, struct drm_i915_private, guc.events_work);
>>>> +
>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>> +	/* Speed up work cancelation during disabling guc interrupts. */
>>>> +	if (!dev_priv->guc.interrupts_enabled) {
>>>> +		spin_unlock_irq(&dev_priv->irq_lock);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>>>
>>> This just shouts that the code is broken.
>>>
>> You mean to say that ideally the wakeref_count (& power.usage_count)
>> should already be non zero here.
>
> Yes. If it is not under your control, then you have a bug in your code.
> Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug
> (and hacks in place whilst we wait for patch review).
>

This work item can also execute in a window where wakeref_count (& 
power.usage_count) have become zero but runtime suspend has not yet 
kicked in (due to auto-suspend delay), so "RPM wakelock ref not held
during HW access" warning would come.

>>>> void intel_crt_init(struct drm_device *dev);
>>>> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
>>>> index 41601c7..e20792d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>>> @@ -125,6 +125,11 @@ struct intel_guc {
>>>> 	struct intel_guc_fw guc_fw;
>>>> 	uint32_t log_flags;
>>>> 	struct drm_i915_gem_object *log_obj;
>>>> +	/*
>>>> +	 * work, interrupts_enabled are protected by dev_priv->irq_lock
>>>> +	 */
>>>> +	struct work_struct events_work;
>>>
>>> The work gets added here, yet bugs are fixed for the worker in later
>>> patches. Squash in the bug fixes.
>>
>> Sorry didn't get this. Are you alluding to cancellation of this
>> work_item Or flushing of work item from the error handling path ?
>>
>> Cancellation is being done as a part of disabling interrupt and the
>> call to disable interrupt is there in intel_guc_suspend(), part of
>> this patch only.
>
> You add a flush later, which seems unrelated to that patch.

The flush of work item is there in the last patch, inside the error 
handling path for forcefully getting a new flush interrupt from GuC
and so that is not tightly coupled with this patch.

Best regards
Akash

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

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Chris Wilson]

On Sat, May 28, 2016 at 07:15:52PM +0530, Goel, Akash wrote:
> 
> 
> On 5/28/2016 5:43 PM, Chris Wilson wrote:
> >On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote:
> >>
> >>
> >>On 5/28/2016 1:26 AM, Chris Wilson wrote:
> >>>On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
> >>>>From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >>>>
> >>>>There are certain types of interrupts which Host can recieve from GuC.
> >>>>GuC ukernel sends an interrupt to Host for certain events, like for
> >>>>example retrieve/consume the logs generated by ukernel.
> >>>>This patch adds support to receive interrupts from GuC but currently
> >>>>enables & partially handles only the interrupt sent by GuC ukernel.
> >>>>Future patches will add support for handling other interrupt types.
> >>>>
> >>>>Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >>>>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>>>---
> >>>>drivers/gpu/drm/i915/i915_drv.h            |   1 +
> >>>>drivers/gpu/drm/i915/i915_guc_submission.c |   2 +
> >>>>drivers/gpu/drm/i915/i915_irq.c            | 100 ++++++++++++++++++++++++++++-
> >>>>drivers/gpu/drm/i915/i915_reg.h            |  11 ++++
> >>>>drivers/gpu/drm/i915/intel_drv.h           |   3 +
> >>>>drivers/gpu/drm/i915/intel_guc.h           |   5 ++
> >>>>drivers/gpu/drm/i915/intel_guc_loader.c    |   1 +
> >>>>7 files changed, 120 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> >>>>+static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
> >>>>
> >>>>/* For display hotplug interrupt */
> >>>>static inline void
> >>>>@@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
> >>>>	synchronize_irq(dev_priv->dev->irq);
> >>>>}
> >>>>
> >>>>+void gen8_reset_guc_interrupts(struct drm_device *dev)
> >>>>+{
> >>>>+	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>+	i915_reg_t reg = gen6_pm_iir(dev_priv);
> >>>
> >>>From the looks of this we have multiple shadows for the same register.
> >>>That's very bad.
> >>>
> >>>Now the platforms might be mutually exclusive, but it is still a mistake
> >>>that will catch us out.
> >>>
> >>Will check how it is in newer platforms.
> >>
> >>>>+	spin_lock_irq(&dev_priv->irq_lock);
> >>>>+	I915_WRITE(reg, dev_priv->guc_events);
> >>>>+	I915_WRITE(reg, dev_priv->guc_events);
> >>>
> >>>What? Not even the tiniest of comments to explain?
> >>>
> >>Sorry actually just copied these steps as is from the
> >>gen6_reset_rps_interrupts(), considering that the same set of
> >>registers (IIR, IER, IMR) are involved here.
> >>So the double clearing of IIR followed by posting read could be
> >>needed here also.
> >
> >Move it all to i915_irq.c and export routines to manipulate pm_iir such
> >that multiple users do not conflict.
> >
> Sorry but all interrupt related stuff for rps & GuC is already
> inside i915_irq.c file.

Didn't notice, because this code didn't match my expectations for an
interface exported from i915_irq.c

> Also the IER, IMR, IIR registers are being updated in a non
> conflicting manner, no overlap between the PM bits & GuC events
> bits.

They share a register, that mandates arbitration.
 
> You mean to say need to have single set of routines only for interrupt
> reset/enable/disable operations for rps & GuC.

Yes.
 
> >>>>+	POSTING_READ(reg);
> >>>
> >>>Again. Not even the tiniest of comments to explain?
> >>>
> >>>>+	spin_unlock_irq(&dev_priv->irq_lock);
> >>>>+}
> >>>>+
> >>>>+void gen8_enable_guc_interrupts(struct drm_device *dev)
> >>>>+{
> >>>>+	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>+
> >>>>+	spin_lock_irq(&dev_priv->irq_lock);
> >>>>+	if (!dev_priv->guc.interrupts_enabled) {
> >>>>+		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
> >>>>+					dev_priv->guc_events);
> >>>>+		dev_priv->guc.interrupts_enabled = true;
> >>>>+		I915_WRITE(gen6_pm_ier(dev_priv),
> >>>>+		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
> >>>
> >>>ier should be known, rmw on the reg should not be required.
> >>>
> >>Sorry same as above, copy paste from gen6_enable_rps_interrupts().
> >>Without rmw, would this be fine ?
> >>
> >>	if (dev_priv->rps.interrupts_enabled)
> >>		I915_WRITE(gen6_pm_ier(dev_priv),
> >>			dev_priv->pm_rps_events | dev_priv->guc_events);
> >>	else
> >>		I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);
> >
> >Still has the presumption of owning a register that is ostensibly used
> >by others.
> >
> Since pm_ier is a shared register and being used by others also, rmw
> seem to be more suited here. Otherwise need to be aware of who all is
> sharing it so as to update it without disturbing the bits owned by
> others.

Exactly, see above. The best interfaces from i915_irq.c do not use rmw
on the register values.
 
> >>>>+static void gen8_guc2host_events_work(struct work_struct *work)
> >>>>+{
> >>>>+	struct drm_i915_private *dev_priv =
> >>>>+		container_of(work, struct drm_i915_private, guc.events_work);
> >>>>+
> >>>>+	spin_lock_irq(&dev_priv->irq_lock);
> >>>>+	/* Speed up work cancelation during disabling guc interrupts. */
> >>>>+	if (!dev_priv->guc.interrupts_enabled) {
> >>>>+		spin_unlock_irq(&dev_priv->irq_lock);
> >>>>+		return;
> >>>>+	}
> >>>>+
> >>>>+	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
> >>>
> >>>This just shouts that the code is broken.
> >>>
> >>You mean to say that ideally the wakeref_count (& power.usage_count)
> >>should already be non zero here.
> >
> >Yes. If it is not under your control, then you have a bug in your code.
> >Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug
> >(and hacks in place whilst we wait for patch review).
> >
> 
> This work item can also execute in a window where wakeref_count (&
> power.usage_count) have become zero but runtime suspend has not yet
> kicked in (due to auto-suspend delay), so "RPM wakelock ref not held
> during HW access" warning would come.

i.e. your code is buggy, as DISABLE_RPM_WAKEREF_ASSERTS implied.

> >>>>void intel_crt_init(struct drm_device *dev);
> >>>>diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> >>>>index 41601c7..e20792d 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_guc.h
> >>>>+++ b/drivers/gpu/drm/i915/intel_guc.h
> >>>>@@ -125,6 +125,11 @@ struct intel_guc {
> >>>>	struct intel_guc_fw guc_fw;
> >>>>	uint32_t log_flags;
> >>>>	struct drm_i915_gem_object *log_obj;
> >>>>+	/*
> >>>>+	 * work, interrupts_enabled are protected by dev_priv->irq_lock
> >>>>+	 */
> >>>>+	struct work_struct events_work;
> >>>
> >>>The work gets added here, yet bugs are fixed for the worker in later
> >>>patches. Squash in the bug fixes.
> >>
> >>Sorry didn't get this. Are you alluding to cancellation of this
> >>work_item Or flushing of work item from the error handling path ?
> >>
> >>Cancellation is being done as a part of disabling interrupt and the
> >>call to disable interrupt is there in intel_guc_suspend(), part of
> >>this patch only.
> >
> >You add a flush later, which seems unrelated to that patch.
> 
> The flush of work item is there in the last patch, inside the error
> handling path for forcefully getting a new flush interrupt from GuC
> and so that is not tightly coupled with this patch.

Reseting the work would seem be part of the log infrastructure.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 03/12] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 5/28/2016 8:05 PM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 07:15:52PM +0530, Goel, Akash wrote:
>>
>>
>> On 5/28/2016 5:43 PM, Chris Wilson wrote:
>>> On Sat, May 28, 2016 at 02:52:16PM +0530, Goel, Akash wrote:
>>>>
>>>>
>>>> On 5/28/2016 1:26 AM, Chris Wilson wrote:
>>>>> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel@intel.com wrote:
>>>>>> +void gen8_reset_guc_interrupts(struct drm_device *dev)
>>>>>> +{
>>>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>> +	i915_reg_t reg = gen6_pm_iir(dev_priv);
>>>>>
>>>> >From the looks of this we have multiple shadows for the same register.
>>>>> That's very bad.
>>>>>
>>>>> Now the platforms might be mutually exclusive, but it is still a mistake
>>>>> that will catch us out.
>>>>>
>>>> Will check how it is in newer platforms.
>>>>
>>>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>>>> +	I915_WRITE(reg, dev_priv->guc_events);
>>>>>> +	I915_WRITE(reg, dev_priv->guc_events);
>>>>>
>>>>> What? Not even the tiniest of comments to explain?
>>>>>
>>>> Sorry actually just copied these steps as is from the
>>>> gen6_reset_rps_interrupts(), considering that the same set of
>>>> registers (IIR, IER, IMR) are involved here.
>>>> So the double clearing of IIR followed by posting read could be
>>>> needed here also.
>>>
>>> Move it all to i915_irq.c and export routines to manipulate pm_iir such
>>> that multiple users do not conflict.
>>>
>> Sorry but all interrupt related stuff for rps & GuC is already
>> inside i915_irq.c file.
>
> Didn't notice, because this code didn't match my expectations for an
> interface exported from i915_irq.c
>
>> Also the IER, IMR, IIR registers are being updated in a non
>> conflicting manner, no overlap between the PM bits & GuC events
>> bits.
>
> They share a register, that mandates arbitration.
>

I think the arbitration (& serialization) is already being provided by 
irq_lock.

>> You mean to say need to have single set of routines only for interrupt
>> reset/enable/disable operations for rps & GuC.
>
> Yes.
>

Fine will make them to use a single set of low level routines.

>>>>>> +	POSTING_READ(reg);
>>>>>
>>>>> Again. Not even the tiniest of comments to explain?
>>>>>
>>>>>> +	spin_unlock_irq(&dev_priv->irq_lock);
>>>>>> +}
>>>>>> +
>>>>>> +void gen8_enable_guc_interrupts(struct drm_device *dev)
>>>>>> +{
>>>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>> +
>>>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>>>> +	if (!dev_priv->guc.interrupts_enabled) {
>>>>>> +		WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
>>>>>> +					dev_priv->guc_events);
>>>>>> +		dev_priv->guc.interrupts_enabled = true;
>>>>>> +		I915_WRITE(gen6_pm_ier(dev_priv),
>>>>>> +		      I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
>>>>>
>>>>> ier should be known, rmw on the reg should not be required.
>>>>>
>>>> Sorry same as above, copy paste from gen6_enable_rps_interrupts().
>>>> Without rmw, would this be fine ?
>>>>
>>>> 	if (dev_priv->rps.interrupts_enabled)
>>>> 		I915_WRITE(gen6_pm_ier(dev_priv),
>>>> 			dev_priv->pm_rps_events | dev_priv->guc_events);
>>>> 	else
>>>> 		I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);
>>>
>>> Still has the presumption of owning a register that is ostensibly used
>>> by others.
>>>
>> Since pm_ier is a shared register and being used by others also, rmw
>> seem to be more suited here. Otherwise need to be aware of who all is
>> sharing it so as to update it without disturbing the bits owned by
>> others.
>
> Exactly, see above. The best interfaces from i915_irq.c do not use rmw
> on the register values.

Fine will try to do away with use rmw operation for pm_ier by
maintaining a bit mask of enabled interrupts (just like pm_irq_mask).

>
>>>>>> +static void gen8_guc2host_events_work(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct drm_i915_private *dev_priv =
>>>>>> +		container_of(work, struct drm_i915_private, guc.events_work);
>>>>>> +
>>>>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>>>> +	/* Speed up work cancelation during disabling guc interrupts. */
>>>>>> +	if (!dev_priv->guc.interrupts_enabled) {
>>>>>> +		spin_unlock_irq(&dev_priv->irq_lock);
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>>>>>
>>>>> This just shouts that the code is broken.
>>>>>
>>>> You mean to say that ideally the wakeref_count (& power.usage_count)
>>>> should already be non zero here.
>>>
>>> Yes. If it is not under your control, then you have a bug in your code.
>>> Existing DISABLE_RPM_WAKEREF_ASSERTS tell us where we know we have a bug
>>> (and hacks in place whilst we wait for patch review).
>>>
>>
>> This work item can also execute in a window where wakeref_count (&
>> power.usage_count) have become zero but runtime suspend has not yet
>> kicked in (due to auto-suspend delay), so "RPM wakelock ref not held
>> during HW access" warning would come.
>
> i.e. your code is buggy, as DISABLE_RPM_WAKEREF_ASSERTS implied.
>

But isn't this applicable to rps work item also ?.
If there is a way found to circumvent this, then same can be applied to 
GuC work item also. DISABLE_RPM_WAKEREF_ASSERTS is a stopgap solution.

>>>>>> void intel_crt_init(struct drm_device *dev);
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
>>>>>> index 41601c7..e20792d 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>>>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>>>>> @@ -125,6 +125,11 @@ struct intel_guc {
>>>>>> 	struct intel_guc_fw guc_fw;
>>>>>> 	uint32_t log_flags;
>>>>>> 	struct drm_i915_gem_object *log_obj;
>>>>>> +	/*
>>>>>> +	 * work, interrupts_enabled are protected by dev_priv->irq_lock
>>>>>> +	 */
>>>>>> +	struct work_struct events_work;
>>>>>
>>>>> The work gets added here, yet bugs are fixed for the worker in later
>>>>> patches. Squash in the bug fixes.
>>>>
>>>> Sorry didn't get this. Are you alluding to cancellation of this
>>>> work_item Or flushing of work item from the error handling path ?
>>>>
>>>> Cancellation is being done as a part of disabling interrupt and the
>>>> call to disable interrupt is there in intel_guc_suspend(), part of
>>>> this patch only.
>>>
>>> You add a flush later, which seems unrelated to that patch.
>>
>> The flush of work item is there in the last patch, inside the error
>> handling path for forcefully getting a new flush interrupt from GuC
>> and so that is not tightly coupled with this patch.
>
> Reseting the work would seem be part of the log infrastructure.

Fine will add the work item reset part in this patch only.

Best regards
Akash
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Daniel Vetter]

On Sat, May 28, 2016 at 01:12:51AM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> GuC firmware log its debug messages into a Host-GuC shared memory buffer
> and when the buffer is half full it sends a Flush interrupt to Host.
> GuC firmware expects that while it is writing to 2nd half of the buffer,
> first half would get consumed by Host and then get a flush completed
> acknowledgement from Host, so that it does not end up doing any overwrite
> causing loss of logs.
> So far Flush interrupt wasn't enabled on Host side & User could capture the
> contents/snapshot of log buffer through 'i915_guc_log_dump' debugfs iface.
> But this couldn't meet couple of key requirements, especially of Validation,
> first is to ensure capturing of all boot time logs even with high verbosity
> level and second is to enable capturing of logs in a sustained manner like
> for the entire duration of a workload.
> Now Driver will enable Flush interrupt and on receving it, would copy the
> contents of log buffer into its local buffer. The size of local buffer would
> be big enough to contain multiple snapshots of the log buffer giving ample
> time to User to pull boot time messages. Also reads from User space will be
> blocked if there isn't any data left in the local buffer until another
> Flush interrupt is received and new logs are copied into the local buffer,
> enabling capturing of logs in a streaming manner.
> 
> Have added 3 interfaces for capturing logs.
> Patch 5 & 6 avails relay framework to store logs and export that to User,
> in which Driver just needs to use a relay API to copy logs into the relay
> buffer and behavior is equivalent to 'dmesg -c'. But there are certain
> requirements with relay which in most likelihood can be met only for a
> debugfs file. But since debugfs may not be always available in production
> kernels, added support for other interfaces as a fall back, where Driver
> only will have to do allocation/mapping of local buffer pages, handle file
> read calls, read/write pointer management, overflow handling etc.
> One is a sysfs interface '/sys/class/drm/card0/guc_log', which is generally
> the preferred location. This mimics '/proc/kmsg' behavior, so meets the
> streaming requirement but at a time only single client can pull the logs
> in a consistent manner.
> Other is a character device file interface '/dev/dri/guc_log', this mimics
> '/dev/kmsg' behavior, so can meet the streaming requirement and also allow
> multiple clients to consume logs at the same time.
> Based on the comments and considering the relative merits, can decide which
> interface to actually use.

I still kinda like relayfs, except that it's not available in non-debug
builds. But so are plenty of other really interesting files we have hidden
in there.

sysfs isn't the solution, I already have a black eye from the sysfs
maintainer for our error state.

char dev feels like it's bad at discoverability, no one would guess you
can just cat it. Also, the relationship between driver instance and
chardev logging is a bit messy.

No idea really where to put stuff. One option might be to have an official
debug directory (like we have power already) in sysfs as the canonical
place where drivers can dump stuff. We're not the only ones with too much
data to get to userspace for debugging driver/hw issues, e.g. wireless
firmware has pretty similar solutions.

Adding Johannes, he's worked on something like this for the intel wireless
drivers iirc.

Cheers, Daniel

> 
> Akash Goel (7):
>   drm/i915: Add a relay backed debugfs interface for capturing GuC logs
>   drm/i915: Store GuC ukernel logs in the relay buffer
>   drm/i915: Allocate local buffer to store GuC ukernel logs
>   drm/i915: Store GuC ukernel logs in the local buffer
>   drm/i915: Add a char device file interface to capture GuC ukernel logs
>   drm/i915: Support to capture GuC logs by multiple clients via device
>     file iface
>   drm/i915: Add sysfs interface to capture the GuC ukernel logs
> 
> Sagar Arun Kamble (5):
>   drm/i915: Decouple GuC log setup from verbosity parameter
>   drm/i915: Add GuC ukernel logging related fields to fw interface file
>   drm/i915: Support for GuC interrupts
>   drm/i915: Handle log buffer flush interrupt event from GuC
>   drm/i915: Forcefully flush GuC log buffer on reset
> 
>  drivers/gpu/drm/i915/i915_drv.h            |   1 +
>  drivers/gpu/drm/i915/i915_guc_submission.c | 483 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_irq.c            | 120 ++++++-
>  drivers/gpu/drm/i915/i915_reg.h            |  11 +
>  drivers/gpu/drm/i915/i915_sysfs.c          |  40 +++
>  drivers/gpu/drm/i915/intel_drv.h           |   3 +
>  drivers/gpu/drm/i915/intel_guc.h           |  30 ++
>  drivers/gpu/drm/i915/intel_guc_fwif.h      |  42 +++
>  drivers/gpu/drm/i915/intel_guc_loader.c    |   9 +-
>  9 files changed, 730 insertions(+), 9 deletions(-)
> 
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Johannes Berg]

On Thu, 2016-06-02 at 10:16 +0000, Daniel Vetter wrote:

> I still kinda like relayfs, except that it's not available in non-
> debug builds. But so are plenty of other really interesting files we
> have hidden in there.
> 
> sysfs isn't the solution, I already have a black eye from the sysfs
> maintainer for our error state.

Heh. I tend to agree though.

> No idea really where to put stuff. One option might be to have an
> official debug directory (like we have power already) in sysfs as the
> canonical place where drivers can dump stuff. We're not the only ones
> with too much data to get to userspace for debugging driver/hw
> issues, e.g. wireless firmware has pretty similar solutions.

We have two things in wireless:

 1) the devcoredump stuff, but that's a one-time event when something 
    bad happens and dumps a big blob into userspace, doesn't seem
    relevant here

 2) continuous logging, which uses a debugfs file (though it could be
    relayfs as well, doesn't really make a difference)

There could be something said for using tracing, but that's only
independent of debugfs since the tracefs introduction in kernel 4.1.

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

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Daniel Vetter]

On Thu, Jun 02, 2016 at 12:21:49PM +0200, Johannes Berg wrote:
> On Thu, 2016-06-02 at 10:16 +0000, Daniel Vetter wrote:
> 
> > I still kinda like relayfs, except that it's not available in non-
> > debug builds. But so are plenty of other really interesting files we
> > have hidden in there.
> > 
> > sysfs isn't the solution, I already have a black eye from the sysfs
> > maintainer for our error state.
> 
> Heh. I tend to agree though.
> 
> > No idea really where to put stuff. One option might be to have an
> > official debug directory (like we have power already) in sysfs as the
> > canonical place where drivers can dump stuff. We're not the only ones
> > with too much data to get to userspace for debugging driver/hw
> > issues, e.g. wireless firmware has pretty similar solutions.
> 
> We have two things in wireless:
> 
>  1) the devcoredump stuff, but that's a one-time event when something 
>     bad happens and dumps a big blob into userspace, doesn't seem
>     relevant here
> 
>  2) continuous logging, which uses a debugfs file (though it could be
>     relayfs as well, doesn't really make a difference)

relayfs apparently moved in with debugfs. And a requirement (or at least
strong wishlist item) is that we can get at the data on production systems
(which really shouldn't mount debugfs). Seems like there's no place to
dump debug information outside of debugfs :(

> There could be something said for using tracing, but that's only
> independent of debugfs since the tracefs introduction in kernel 4.1.

We tried looking into tracing stuff for our performance counters, and at
least there the mismatch for dumping large-scale stuff was too much. But
tracefs looks like just the tracing debugfs directory cut out into a
separate filesystem, exactly to avoid that dreaded debugfs-is-insecure
issues.

I'd say we should smash it into debugfs, and if these troubles persist
then maybe we need to clean up the mess in there a bit and expose it as
drm_debugfs or whatever. Probably a topic for kernel summit even. At least
I feel like there's not enough consensus to add ABI at this point.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Goel, Akash]

On 6/3/2016 12:45 PM, Daniel Vetter wrote:
> On Thu, Jun 02, 2016 at 12:21:49PM +0200, Johannes Berg wrote:
>> On Thu, 2016-06-02 at 10:16 +0000, Daniel Vetter wrote:
>>
>>> I still kinda like relayfs, except that it's not available in non-
>>> debug builds. But so are plenty of other really interesting files we
>>> have hidden in there.
>>>
>>> sysfs isn't the solution, I already have a black eye from the sysfs
>>> maintainer for our error state.
>>
>> Heh. I tend to agree though.
>>
>>> No idea really where to put stuff. One option might be to have an
>>> official debug directory (like we have power already) in sysfs as the
>>> canonical place where drivers can dump stuff. We're not the only ones
>>> with too much data to get to userspace for debugging driver/hw
>>> issues, e.g. wireless firmware has pretty similar solutions.
>>
>> We have two things in wireless:
>>
>>  1) the devcoredump stuff, but that's a one-time event when something
>>     bad happens and dumps a big blob into userspace, doesn't seem
>>     relevant here
>>
>>  2) continuous logging, which uses a debugfs file (though it could be
>>     relayfs as well, doesn't really make a difference)
>
> relayfs apparently moved in with debugfs. And a requirement (or at least
> strong wishlist item) is that we can get at the data on production systems
> (which really shouldn't mount debugfs). Seems like there's no place to
> dump debug information outside of debugfs :(
>
>> There could be something said for using tracing, but that's only
>> independent of debugfs since the tracefs introduction in kernel 4.1.
>
> We tried looking into tracing stuff for our performance counters, and at
> least there the mismatch for dumping large-scale stuff was too much. But
> tracefs looks like just the tracing debugfs directory cut out into a
> separate filesystem, exactly to avoid that dreaded debugfs-is-insecure
> issues.
>
> I'd say we should smash it into debugfs, and if these troubles persist
> then maybe we need to clean up the mess in there a bit and expose it as
> drm_debugfs or whatever. Probably a topic for kernel summit even. At least
> I feel like there's not enough consensus to add ABI at this point.
Hi Daniel,

Thanks much for your inputs.

So, on interim basis, can we have a relay backed debugfs interface only
i.e. /sys/kernel/debug/dri/guc_log.

And once the support for drm_debugfs is added, its just a matter of 
changing the file location, i.e. move it inside the drm_debugfs.

Best regards
Akash

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

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Chris Wilson]

On Fri, Jun 03, 2016 at 03:44:09PM +0530, Goel, Akash wrote:
> 
> 
> On 6/3/2016 12:45 PM, Daniel Vetter wrote:
> >On Thu, Jun 02, 2016 at 12:21:49PM +0200, Johannes Berg wrote:
> >>On Thu, 2016-06-02 at 10:16 +0000, Daniel Vetter wrote:
> >>
> >>>I still kinda like relayfs, except that it's not available in non-
> >>>debug builds. But so are plenty of other really interesting files we
> >>>have hidden in there.
> >>>
> >>>sysfs isn't the solution, I already have a black eye from the sysfs
> >>>maintainer for our error state.
> >>
> >>Heh. I tend to agree though.
> >>
> >>>No idea really where to put stuff. One option might be to have an
> >>>official debug directory (like we have power already) in sysfs as the
> >>>canonical place where drivers can dump stuff. We're not the only ones
> >>>with too much data to get to userspace for debugging driver/hw
> >>>issues, e.g. wireless firmware has pretty similar solutions.
> >>
> >>We have two things in wireless:
> >>
> >> 1) the devcoredump stuff, but that's a one-time event when something
> >>    bad happens and dumps a big blob into userspace, doesn't seem
> >>    relevant here
> >>
> >> 2) continuous logging, which uses a debugfs file (though it could be
> >>    relayfs as well, doesn't really make a difference)
> >
> >relayfs apparently moved in with debugfs. And a requirement (or at least
> >strong wishlist item) is that we can get at the data on production systems
> >(which really shouldn't mount debugfs). Seems like there's no place to
> >dump debug information outside of debugfs :(
> >
> >>There could be something said for using tracing, but that's only
> >>independent of debugfs since the tracefs introduction in kernel 4.1.
> >
> >We tried looking into tracing stuff for our performance counters, and at
> >least there the mismatch for dumping large-scale stuff was too much. But
> >tracefs looks like just the tracing debugfs directory cut out into a
> >separate filesystem, exactly to avoid that dreaded debugfs-is-insecure
> >issues.
> >
> >I'd say we should smash it into debugfs, and if these troubles persist
> >then maybe we need to clean up the mess in there a bit and expose it as
> >drm_debugfs or whatever. Probably a topic for kernel summit even. At least
> >I feel like there's not enough consensus to add ABI at this point.
> Hi Daniel,
> 
> Thanks much for your inputs.
> 
> So, on interim basis, can we have a relay backed debugfs interface only
> i.e. /sys/kernel/debug/dri/guc_log.
> 
> And once the support for drm_debugfs is added, its just a matter of
> changing the file location, i.e. move it inside the drm_debugfs.

Only, it will be called drm_kernfs :) Don't want to confuse people that
it won't be a stable ABI. Maybe drm_stablefs!

To start the ball rolling, canonical mountpoint:

	/sys/kernel/gpu # like /sys/kerenl/mm
	/sys/kernel/drm
	/sys/kernel/dri # like /dev/dri

?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 00/12] Support for sustained capturing of GuC firmware logs

[Daniel Vetter]

On Fri, Jun 03, 2016 at 11:20:19AM +0100, Chris Wilson wrote:
> On Fri, Jun 03, 2016 at 03:44:09PM +0530, Goel, Akash wrote:
> > 
> > 
> > On 6/3/2016 12:45 PM, Daniel Vetter wrote:
> > >On Thu, Jun 02, 2016 at 12:21:49PM +0200, Johannes Berg wrote:
> > >>On Thu, 2016-06-02 at 10:16 +0000, Daniel Vetter wrote:
> > >>
> > >>>I still kinda like relayfs, except that it's not available in non-
> > >>>debug builds. But so are plenty of other really interesting files we
> > >>>have hidden in there.
> > >>>
> > >>>sysfs isn't the solution, I already have a black eye from the sysfs
> > >>>maintainer for our error state.
> > >>
> > >>Heh. I tend to agree though.
> > >>
> > >>>No idea really where to put stuff. One option might be to have an
> > >>>official debug directory (like we have power already) in sysfs as the
> > >>>canonical place where drivers can dump stuff. We're not the only ones
> > >>>with too much data to get to userspace for debugging driver/hw
> > >>>issues, e.g. wireless firmware has pretty similar solutions.
> > >>
> > >>We have two things in wireless:
> > >>
> > >> 1) the devcoredump stuff, but that's a one-time event when something
> > >>    bad happens and dumps a big blob into userspace, doesn't seem
> > >>    relevant here
> > >>
> > >> 2) continuous logging, which uses a debugfs file (though it could be
> > >>    relayfs as well, doesn't really make a difference)
> > >
> > >relayfs apparently moved in with debugfs. And a requirement (or at least
> > >strong wishlist item) is that we can get at the data on production systems
> > >(which really shouldn't mount debugfs). Seems like there's no place to
> > >dump debug information outside of debugfs :(
> > >
> > >>There could be something said for using tracing, but that's only
> > >>independent of debugfs since the tracefs introduction in kernel 4.1.
> > >
> > >We tried looking into tracing stuff for our performance counters, and at
> > >least there the mismatch for dumping large-scale stuff was too much. But
> > >tracefs looks like just the tracing debugfs directory cut out into a
> > >separate filesystem, exactly to avoid that dreaded debugfs-is-insecure
> > >issues.
> > >
> > >I'd say we should smash it into debugfs, and if these troubles persist
> > >then maybe we need to clean up the mess in there a bit and expose it as
> > >drm_debugfs or whatever. Probably a topic for kernel summit even. At least
> > >I feel like there's not enough consensus to add ABI at this point.
> > Hi Daniel,
> > 
> > Thanks much for your inputs.
> > 
> > So, on interim basis, can we have a relay backed debugfs interface only
> > i.e. /sys/kernel/debug/dri/guc_log.
> > 
> > And once the support for drm_debugfs is added, its just a matter of
> > changing the file location, i.e. move it inside the drm_debugfs.
> 
> Only, it will be called drm_kernfs :) Don't want to confuse people that
> it won't be a stable ABI. Maybe drm_stablefs!
> 
> To start the ball rolling, canonical mountpoint:
> 
> 	/sys/kernel/gpu # like /sys/kerenl/mm
> 	/sys/kernel/drm
> 	/sys/kernel/dri # like /dev/dri
> 
> ?

/me casts vote for gpu. Maybe someone will get bored and move the
vga_switcheroo stuff out of debugfs too ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx