[PATCH v7 00/19] Support for sustained capturing of GuC firmware logs

[PATCH v7 00/19] 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 follows the half-full draining protocol where it expects that
while it is writing to 2nd half of the buffer, 1st half would get consumed
by Host and then get a flush completed acknowledgment 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 receiving 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.
Have added a debugfs interface '/sys/kernel/debug/dri/guc_log' for User to
collect the logs. Availed relay framework to implement this interface, where
Driver will have to just use a relay API to store snapshots of GuC log buffer
in a buffer managed by relay. The relay buffer can be operated in a mode,
equivalent to 'dmesg -c' where the old data, not yet collected by User, will
be overwritten if buffer becomes full or it can be operated in no-overwrite
mode where relay will stop accepting new data if all sub buffers are full.
Have used the latter mode to avoid the possibility of getting garbled data. 
Besides mmap method, through which User can directly access the relay
buffer contents, relay also supports the 'poll' method. Through the 'poll'
call on log file, User can come to know whenever a new snapshot of the log
buffer is taken by Driver, so can run in tandem with the Driver and thus
capture logs in a sustained/streaming manner, without any loss of data.

v2: Rebased to the latest drm-intel-nightly.

v3: Aligned with the modification of late debugfs registration, at the end of
    i915 Driver load. Did cleanup as per Tvrtko's review comments, added 3
    new patches to optimize the log-buffer flush interrupt handling, gather
    and report the logging related stats.

v4: Added 2 new patches to further optimize the log-buffer flush interrupt
    handling. Did cleanup as per Chris's review comments, fixed couple of
    issues related to clearing of Guc2Host message register. Switched to
    no-overwrite mode for the relay.

v5: Added a new patch to avail MOVNTDQA instruction based fast memcpy provided
    by a patch from Chris. Dropped the rt priority kthread patch, after
    evaluating all the optimizations with certain benchmarks like
    synmark_oglmultithread, synmark_oglbatch5 which generates flush interupts
    almost at every ms or less. Updated the older patches as per the review
    comments from Tvrtko and Chris W. Added a new patch to augment i915 error
    state with the GuC log buffer contents. Fixed the issue of User interrupt
    getting disabled for VEBOX ring, causing failure for certain IGTs.
    Also included 2 patches to support early logging for capturing boot
    time logs and use per CPU constructs on the relay side so as to address
    a WARNING issue with the call to relay_reserve(), without disabling
    preemption.

v6: Mainly did the rebasing, refactoring, cleanup as per the review comments
    and fixed error/warnings reported by checkpatch.

v7: Added a new patch to complete the pending log buffer flush work item in
    system suspend case. Cleaned up the irq handler & work item function
    by removing the check for GuC interrupts.

Akash Goel (13):
  drm/i915: New structure to contain GuC logging related fields
  drm/i915: Add low level set of routines for programming PM IER/IIR/IMR
    register set
  relay: Use per CPU constructs for the relay channel buffer pointers
  drm/i915: Add a relay backed debugfs interface for capturing GuC logs
  drm/i915: New lock to serialize the Host2GuC actions
  drm/i915: Add stats for GuC log buffer flush interrupts
  drm/i915: Optimization to reduce the sampling time of GuC log buffer
  drm/i915: Increase GuC log buffer size to reduce flush interrupts
  drm/i915: Augment i915 error state to include the dump of GuC log
    buffer
  drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer
  drm/i915: Use SSE4.1 movntdqa based memcpy for sampling GuC log buffer
  drm/i915: Early creation of relay channel for capturing boot time logs
  drm/i915: Sync against the GuC log buffer flush work item on system
    suspend

Sagar Arun Kamble (6):
  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
  drm/i915: Debugfs support for GuC logging control

 drivers/gpu/drm/i915/Kconfig               |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c        |  76 +++-
 drivers/gpu/drm/i915/i915_drv.c            |   6 +-
 drivers/gpu/drm/i915/i915_drv.h            |   5 +-
 drivers/gpu/drm/i915/i915_gpu_error.c      |  22 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 602 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.c            | 159 ++++++--
 drivers/gpu/drm/i915/i915_reg.h            |  11 +
 drivers/gpu/drm/i915/intel_drv.h           |   6 +
 drivers/gpu/drm/i915/intel_guc.h           |  32 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  82 +++-
 drivers/gpu/drm/i915/intel_guc_loader.c    |  10 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   4 +-
 include/linux/relay.h                      |  17 +-
 kernel/relay.c                             |  74 ++--
 15 files changed, 1016 insertions(+), 91 deletions(-)

-- 
1.9.2

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

[PATCH 01/19] 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 module param
i915.guc_log_level. User would be given a provision to enable firmware
logging at runtime, through a host2guc action, and not necessarily during
Driver load time. But the address of log buffer can be passed only in
init params, at firmware load time, so GuC has to be reset and firmware
needs to be reloaded to pass the log buffer address at runtime.
To avoid reset of GuC & reload of firmware, allocation of log buffer will
be done always but logging would be enabled initially on GuC side based on
the value of module parameter guc_log_level.

v2: Update commit message to describe the constraint with allocation of
    log buffer at runtime. (Tvrtko)

v3: Rebase.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index bb40792..fec5898 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -834,9 +834,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 324812d..b3751c4 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -187,11 +187,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;
 
+	params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+
 	if (i915.guc_log_level >= 0) {
-		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
 		params[GUC_CTL_DEBUG] =
 			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-	}
+	} else
+		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
 	if (guc->ads_vma) {
 		u32 ads = i915_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
-- 
1.9.2

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

[PATCH 02/19] 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.

v2:
- Make documentation of log buffer state structure more elaborate &
  rename LOGBUFFERFLUSH action to LOG_BUFFER_FLUSH for consistency.(Tvrtko)

v3: Add GuC log buffer layout diagram for more clarity.

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

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 944786d..47ef7e8 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -418,15 +418,87 @@ 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
+};
+
+/**
+ * DOC: GuC Log buffer Layout
+ *
+ * Page0  +-------------------------------+
+ *        |   ISR state header (32 bytes) |
+ *        |      DPC state header         |
+ *        |   Crash dump state header     |
+ * Page1  +-------------------------------+
+ *        |           ISR logs            |
+ * Page5  +-------------------------------+
+ *        |           DPC logs            |
+ * Page9  +-------------------------------+
+ *        |         Crash Dump logs       |
+ *        +-------------------------------+
+ *
+ * Below state structure is used for coordination of retrieval of GuC firmware
+ * logs. Separate state is maintained for each log buffer type.
+ * read_ptr points to the location where i915 read last in log buffer and
+ * is read only for GuC firmware. write_ptr is incremented by GuC with number
+ * of bytes written for each log entry and is read only for i915.
+ * When any type of log buffer becomes half full, GuC sends a flush interrupt.
+ * 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
+ * acknowledgment from Host, so that it does not end up doing any overwrite
+ * causing loss of logs. So when buffer gets half filled & i915 has requested
+ * for interrupt, GuC will set flush_to_file field, set the sampled_write_ptr
+ * to the value of write_ptr and raise the interrupt.
+ * On receiving the interrupt i915 should read the buffer, clear flush_to_file
+ * field and also update read_ptr with the value of sample_write_ptr, before
+ * sending an acknowledgment to GuC. marker & version fields are for internal
+ * usage of GuC and opaque to i915. buffer_full_cnt field is incremented every
+ * time GuC detects the log buffer overflow.
+ */
+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;
+
+union guc_log_control {
+	struct {
+		u32 logging_enabled:1;
+		u32 reserved1:3;
+		u32 verbosity:4;
+		u32 reserved2:24;
+	};
+	u32 value;
+} __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_LOG_BUFFER_FLUSH = 0x302,
 	HOST2GUC_ACTION_ENTER_S_STATE = 0x501,
 	HOST2GUC_ACTION_EXIT_S_STATE = 0x502,
 	HOST2GUC_ACTION_SLPC_REQUEST = 0x3003,
+	HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x0E000,
 	HOST2GUC_ACTION_LIMIT
 };
 
@@ -448,4 +520,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

[PATCH 03/19] drm/i915: New structure to contain GuC logging related fields

[akash.goel]

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

So far there were 2 fields related to GuC logs in 'intel_guc' structure.
For the support of capturing GuC logs & storing them in a local buffer,
multiple new fields would have to be added. This warrants a separate
structure to contain the fields related to GuC logging state.
Added a new structure 'intel_guc_log' and instance of it inside
'intel_guc' structure.

v2: Rebase.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 4 ++--
 drivers/gpu/drm/i915/i915_guc_submission.c | 8 ++++----
 drivers/gpu/drm/i915/intel_guc.h           | 8 ++++++--
 drivers/gpu/drm/i915/intel_guc_loader.c    | 2 +-
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b89478a..b2cfcf4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2578,10 +2578,10 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	struct drm_i915_gem_object *obj;
 	int i = 0, pg;
 
-	if (!dev_priv->guc.log_vma)
+	if (!dev_priv->guc.log.vma)
 		return 0;
 
-	obj = dev_priv->guc.log_vma->obj;
+	obj = dev_priv->guc.log.vma->obj;
 	for (pg = 0; pg < obj->base.size / PAGE_SIZE; pg++) {
 		u32 *log = kmap_atomic(i915_gem_object_get_page(obj, pg));
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index fec5898..34fb893 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -843,7 +843,7 @@ static void guc_create_log(struct intel_guc *guc)
 		GUC_LOG_ISR_PAGES + 1 +
 		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
 
-	vma = guc->log_vma;
+	vma = guc->log.vma;
 	if (!vma) {
 		vma = guc_allocate_vma(guc, size);
 		if (IS_ERR(vma)) {
@@ -852,7 +852,7 @@ static void guc_create_log(struct intel_guc *guc)
 			return;
 		}
 
-		guc->log_vma = vma;
+		guc->log.vma = vma;
 	}
 
 	/* each allocated unit is a page */
@@ -862,7 +862,7 @@ static void guc_create_log(struct intel_guc *guc)
 		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
 
 	offset = i915_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
-	guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 }
 
 static void init_guc_policies(struct guc_policies *policies)
@@ -1034,7 +1034,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	i915_vma_unpin_and_release(&guc->ads_vma);
-	i915_vma_unpin_and_release(&guc->log_vma);
+	i915_vma_unpin_and_release(&guc->log.vma);
 
 	if (guc->ctx_pool_vma)
 		ida_destroy(&guc->ctx_ids);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c973262..d32023c 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -121,10 +121,14 @@ struct intel_guc_fw {
 	uint32_t ucode_offset;
 };
 
+struct intel_guc_log {
+	uint32_t flags;
+	struct i915_vma *vma;
+};
+
 struct intel_guc {
 	struct intel_guc_fw guc_fw;
-	uint32_t log_flags;
-	struct i915_vma *log_vma;
+	struct intel_guc_log log;
 
 	struct i915_vma *ads_vma;
 	struct i915_vma *ctx_pool_vma;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b3751c4..6fd24d9 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -187,7 +187,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
 			GUC_CTL_VCS2_ENABLED;
 
-	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] =
-- 
1.9.2

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

[PATCH 04/19] drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set

[akash.goel]

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

So far PM IER/IIR/IMR registers were being used only for Turbo related
interrupts. But interrupts coming from GuC also use the same set.
As a precursor to supporting GuC interrupts, added new low level routines
so as to allow sharing the programming of PM IER/IIR/IMR registers between
Turbo & GuC.
Also similar to PM IMR, maintaining a bitmask for PM IER register, to allow
easy sharing of it between Turbo & GuC without involving a rmw operation.

v2:
- For appropriateness & avoid any ambiguity, rename old functions
  enable/disable pm_irq to mask/unmask pm_irq and rename new functions
  enable/disable pm_interrupts to enable/disable pm_irq. (Tvrtko)
- Use u32 in place of uint32_t. (Tvrtko)

v3:
- Rename the fields pm_irq_mask & pm_ier_mask and do some cleanup. (Chris)
- Rebase.

v4: Fix the inadvertent disabling of User interrupt for VECS ring causing
    failure for certain IGTs.

v5: Use dev_priv with HAS_VEBOX macro. (Tvrtko)

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  3 +-
 drivers/gpu/drm/i915/i915_irq.c         | 75 ++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c |  4 +-
 4 files changed, 57 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 35caa9b..a187ea7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1780,7 +1780,8 @@ struct drm_i915_private {
 		u32 de_irq_mask[I915_MAX_PIPES];
 	};
 	u32 gt_irq_mask;
-	u32 pm_irq_mask;
+	u32 pm_imr;
+	u32 pm_ier;
 	u32 pm_rps_events;
 	u32 pipestat_irq_mask[I915_MAX_PIPES];
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ebb83d5..83d708a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -303,18 +303,18 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	new_val = dev_priv->pm_irq_mask;
+	new_val = dev_priv->pm_imr;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
 
-	if (new_val != dev_priv->pm_irq_mask) {
-		dev_priv->pm_irq_mask = new_val;
-		I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_irq_mask);
+	if (new_val != dev_priv->pm_imr) {
+		dev_priv->pm_imr = new_val;
+		I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr);
 		POSTING_READ(gen6_pm_imr(dev_priv));
 	}
 }
 
-void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
@@ -322,28 +322,54 @@ void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 	snb_update_pm_irq(dev_priv, mask, mask);
 }
 
-static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
-				  uint32_t mask)
+static void __gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	snb_update_pm_irq(dev_priv, mask, 0);
 }
 
-void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask)
 {
 	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
 		return;
 
-	__gen6_disable_pm_irq(dev_priv, mask);
+	__gen6_mask_pm_irq(dev_priv, mask);
 }
 
-void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask)
 {
 	i915_reg_t reg = gen6_pm_iir(dev_priv);
 
-	spin_lock_irq(&dev_priv->irq_lock);
-	I915_WRITE(reg, dev_priv->pm_rps_events);
-	I915_WRITE(reg, dev_priv->pm_rps_events);
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	I915_WRITE(reg, reset_mask);
+	I915_WRITE(reg, reset_mask);
 	POSTING_READ(reg);
+}
+
+void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mask)
+{
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	dev_priv->pm_ier |= enable_mask;
+	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+	gen6_unmask_pm_irq(dev_priv, enable_mask);
+	/* unmask_pm_irq provides an implicit barrier (POSTING_READ) */
+}
+
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_mask)
+{
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	dev_priv->pm_ier &= ~disable_mask;
+	__gen6_mask_pm_irq(dev_priv, disable_mask);
+	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier);
+	/* though a barrier is missing here, but don't really need a one */
+}
+
+void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	gen6_reset_pm_iir(dev_priv, dev_priv->pm_rps_events);
 	dev_priv->rps.pm_iir = 0;
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
@@ -354,8 +380,6 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
 	WARN_ON_ONCE(dev_priv->rps.pm_iir);
 	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
 	dev_priv->rps.interrupts_enabled = true;
-	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) |
-				dev_priv->pm_rps_events);
 	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
@@ -373,9 +397,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 
 	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0));
 
-	__gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
-	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
-				~dev_priv->pm_rps_events);
+	gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 	synchronize_irq(dev_priv->drm.irq);
@@ -1078,7 +1100,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
 	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
-	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	gen6_unmask_pm_irq(dev_priv, dev_priv->pm_rps_events);
 	client_boost = dev_priv->rps.client_boost;
 	dev_priv->rps.client_boost = false;
 	spin_unlock_irq(&dev_priv->irq_lock);
@@ -1579,7 +1601,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
 	if (pm_iir & dev_priv->pm_rps_events) {
 		spin_lock(&dev_priv->irq_lock);
-		gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
+		gen6_mask_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
 		if (dev_priv->rps.interrupts_enabled) {
 			dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
 			schedule_work(&dev_priv->rps.work);
@@ -3568,11 +3590,13 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		 * RPS interrupts will get enabled/disabled on demand when RPS
 		 * itself is enabled/disabled.
 		 */
-		if (HAS_VEBOX(dev))
+		if (HAS_VEBOX(dev_priv)) {
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
+			dev_priv->pm_ier |= PM_VEBOX_USER_INTERRUPT;
+		}
 
-		dev_priv->pm_irq_mask = 0xffffffff;
-		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
+		dev_priv->pm_imr = 0xffffffff;
+		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_imr, pm_irqs);
 	}
 }
 
@@ -3692,14 +3716,15 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 	if (HAS_L3_DPF(dev_priv))
 		gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 
-	dev_priv->pm_irq_mask = 0xffffffff;
+	dev_priv->pm_ier = 0x0;
+	dev_priv->pm_imr = ~dev_priv->pm_ier;
 	GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
 	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
 	/*
 	 * RPS interrupts will get enabled/disabled on demand when RPS itself
 	 * is enabled/disabled.
 	 */
-	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, 0);
+	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
 	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1c700b0..9b99a84 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1095,6 +1095,9 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv);
 /* i915_irq.c */
 void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 mask);
+void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
+void gen6_unmask_pm_irq(struct drm_i915_private *dev_priv, u32 mask);
 void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index fa22bd8..590d047 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1652,7 +1652,7 @@ hsw_vebox_irq_enable(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	I915_WRITE_IMR(engine, ~engine->irq_enable_mask);
-	gen6_enable_pm_irq(dev_priv, engine->irq_enable_mask);
+	gen6_unmask_pm_irq(dev_priv, engine->irq_enable_mask);
 }
 
 static void
@@ -1661,7 +1661,7 @@ hsw_vebox_irq_disable(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	I915_WRITE_IMR(engine, ~0);
-	gen6_disable_pm_irq(dev_priv, engine->irq_enable_mask);
+	gen6_mask_pm_irq(dev_priv, engine->irq_enable_mask);
 }
 
 static void
-- 
1.9.2

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

[PATCH 05/19] 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 receive 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.

v2:
- Use common low level routines for PM IER/IIR programming (Chris)
- Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
- Replace disabling of wake ref asserts with rpm get/put (Chris)

v3:
- Update comments for more clarity. (Tvrtko)
- Remove the masking of GuC interrupt, which was kept masked till the
  start of bottom half, its not really needed as there is only a
  single instance of work item & wq is ordered. (Tvrtko)

v4:
- Rebase.
- Rename guc_events to pm_guc_events so as to be indicative of the
  register/control block it is associated with. (Chris)
- Add handling for back to back log buffer flush interrupts.

v5:
- Move the read & clearing of register, containing Guc2Host message
  bits, outside the irq spinlock. (Tvrtko)

v6:
- Move the log buffer flush interrupt related stuff to the following
  patch so as to do only generic bits in this patch. (Tvrtko)
- Rebase.

v7:
- Remove the interrupts_enabled check from gen9_guc_irq_handler, want to
  process that last interrupt also before disabling the interrupt, sync
  against the work queued by irq handler will be done by caller disabling
  the 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/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_guc_submission.c |  5 +++
 drivers/gpu/drm/i915/i915_irq.c            | 56 +++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h            | 11 ++++++
 drivers/gpu/drm/i915/intel_drv.h           |  3 ++
 drivers/gpu/drm/i915/intel_guc.h           |  3 ++
 drivers/gpu/drm/i915/intel_guc_loader.c    |  4 +++
 7 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a187ea7..08553b9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1783,6 +1783,7 @@ struct drm_i915_private {
 	u32 pm_imr;
 	u32 pm_ier;
 	u32 pm_rps_events;
+	u32 pm_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 34fb893..b062da6 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1055,6 +1055,8 @@ int intel_guc_suspend(struct drm_device *dev)
 	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
 		return 0;
 
+	gen9_disable_guc_interrupts(dev_priv);
+
 	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
@@ -1081,6 +1083,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)
+		gen9_enable_guc_interrupts(dev_priv);
+
 	ctx = dev_priv->kernel_context;
 
 	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 83d708a..fc1fe72 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 gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
 /* For display hotplug interrupt */
 static inline void
@@ -411,6 +412,38 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	gen6_reset_rps_interrupts(dev_priv);
 }
 
+void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (!dev_priv->guc.interrupts_enabled) {
+		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+				       dev_priv->pm_guc_events);
+		dev_priv->guc.interrupts_enabled = true;
+		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+	}
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->guc.interrupts_enabled = false;
+
+	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+	synchronize_irq(dev_priv->drm.irq);
+
+	gen9_reset_guc_interrupts(dev_priv);
+}
+
 /**
  * bdw_update_port_irq - update DE port interrupt
  * @dev_priv: driver private
@@ -1339,11 +1372,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->pm_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->pm_guc_events));
 			ret = IRQ_HANDLED;
 		} else
 			DRM_ERROR("The master control interrupt lied (PM)!\n");
@@ -1375,6 +1410,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->pm_guc_events)
+		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1621,6 +1659,13 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
+{
+	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+		/* TODO: Handle events for which GuC interrupted host */
+	}
+}
+
 static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
 				     enum pipe pipe)
 {
@@ -3722,7 +3767,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
 	/*
 	 * RPS interrupts will get enabled/disabled on demand when RPS itself
-	 * is enabled/disabled.
+	 * is enabled/disabled. Same wil be the case for GuC interrupts.
 	 */
 	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
 	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
@@ -4508,6 +4553,9 @@ 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);
 
+	if (HAS_GUC_SCHED(dev))
+		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
+
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
 		/* WaGsvRC0ResidencyMethod:vlv */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d4adf28..cab089a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6011,6 +6011,7 @@ enum {
 #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)
@@ -6022,6 +6023,16 @@ enum {
 #define GEN8_GT_IIR(which) _MMIO(0x44308 + (0x10 * (which)))
 #define GEN8_GT_IER(which) _MMIO(0x4430c + (0x10 * (which)))
 
+#define GEN9_GUC_TO_HOST_INT_EVENT	(1<<31)
+#define GEN9_GUC_EXEC_ERROR_EVENT	(1<<30)
+#define GEN9_GUC_DISPLAY_EVENT		(1<<29)
+#define GEN9_GUC_SEMA_SIGNAL_EVENT	(1<<28)
+#define GEN9_GUC_IOMMU_MSG_EVENT	(1<<27)
+#define GEN9_GUC_DB_RING_EVENT		(1<<26)
+#define GEN9_GUC_DMA_DONE_EVENT		(1<<25)
+#define GEN9_GUC_FATAL_ERROR_EVENT	(1<<24)
+#define GEN9_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 9b99a84..f7f16ac 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1120,6 +1120,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 gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv);
+void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv);
+void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv);
 
 /* 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 d32023c..1fc63fe 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -130,6 +130,9 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	struct intel_guc_log log;
 
+	/* GuC2Host interrupt related state */
+	bool interrupts_enabled;
+
 	struct i915_vma *ads_vma;
 	struct i915_vma *ctx_pool_vma;
 	struct ida ctx_ids;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 6fd24d9..ef35e99 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -464,6 +464,7 @@ int intel_guc_setup(struct drm_device *dev)
 	}
 
 	direct_interrupts_to_host(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
@@ -510,6 +511,9 @@ int intel_guc_setup(struct drm_device *dev)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
 		err = i915_guc_submission_enable(dev_priv);
 		if (err)
 			goto fail;
-- 
1.9.2

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

[PATCH 06/19] 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.

v2:
- Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
- Reduce the overall log buffer copying time by skipping the copy of
  crash buffer area for regular cases and copying only the state
  structure data in first page.

v3:
 - Create a vmalloc mapping of log buffer. (Chris)
 - Cover the flush acknowledgment under rpm get & put.(Chris)
 - Revert the change of skipping the copy of crash dump area, as
   not really needed, will be covered by subsequent patch.

v4:
 - Destroy the wq under the same condition in which it was created,
   pass dev_piv pointer instead of dev to newly added GuC function,
   add more comments & rename variable for clarity. (Tvrtko)

v5:
- Allocate & destroy the dedicated wq, for handling flush interrupt,
  from the setup/teardown routines of GuC logging. (Chris)
- Validate the log buffer size value retrieved from state structure
  and do some minor cleanup. (Tvrtko)
- Fix error/warnings reported by checkpatch. (Tvrtko)
- Rebase.

v6:
 - Remove the interrupts_enabled check from guc_capture_logs_work, need
   to process that last work item also, queued just before disabling the
   interrupt as log buffer flush interrupt handling is a bit different
   case where GuC is actually expecting an ACK from host, which should be
   provided to keep the logging going.
   Sync against the work will be done by caller disabling the interrupt.
 - Don't sample the log buffer size value from state structure, directly
   use the expected value to move the pointer & do the copy and that cannot
   go wrong (out of bounds) as Driver only allocated the log buffer and the
   relay buffers. Driver should refrain from interpreting the log packet,
   as much possible and let Userspace parser detect the anomaly. (Chris)

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 | 186 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c            |  28 ++++-
 drivers/gpu/drm/i915/intel_guc.h           |   4 +
 3 files changed, 217 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b062da6..ade51cb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
 
+static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
+{
+	u32 data[1];
+
+	data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
+
+	return host2guc_action(guc, data, 1);
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -828,6 +837,163 @@ err:
 	return NULL;
 }
 
+static void guc_move_to_next_buf(struct intel_guc *guc)
+{
+}
+
+static void *guc_get_write_buffer(struct intel_guc *guc)
+{
+	return NULL;
+}
+
+static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type type)
+{
+	if (type == GUC_ISR_LOG_BUFFER)
+		return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
+	else if (type == GUC_DPC_LOG_BUFFER)
+		return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
+	else
+		return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
+}
+
+static void guc_read_update_log_buffer(struct intel_guc *guc)
+{
+	struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
+	struct guc_log_buffer_state log_buffer_state_local;
+	void *src_data_ptr, *dst_data_ptr;
+	unsigned int buffer_size;
+	enum guc_log_buffer_type type;
+
+	if (WARN_ON(!guc->log.buf_addr))
+		return;
+
+	/* Get the pointer to shared GuC log buffer */
+	log_buffer_state = src_data_ptr = guc->log.buf_addr;
+
+	/* Get the pointer to local buffer to store the logs */
+	dst_data_ptr = log_buffer_snapshot_state = guc_get_write_buffer(guc);
+
+	/* Actual logs are present from the 2nd page */
+	src_data_ptr += PAGE_SIZE;
+	dst_data_ptr += PAGE_SIZE;
+
+	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
+		/* Make a copy of the state structure in GuC log buffer (which
+		 * is uncached mapped) on the stack to avoid reading from it
+		 * multiple times.
+		 */
+		memcpy(&log_buffer_state_local, log_buffer_state,
+		       sizeof(struct guc_log_buffer_state));
+		buffer_size = guc_get_log_buffer_size(type);
+
+		if (log_buffer_snapshot_state) {
+			/* First copy the state structure in snapshot buffer */
+			memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
+			       sizeof(struct guc_log_buffer_state));
+
+			/* 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_snapshot_state->write_ptr =
+				log_buffer_snapshot_state->sampled_write_ptr;
+
+			log_buffer_snapshot_state++;
+
+			/* Now copy the actual logs. */
+			memcpy(dst_data_ptr, src_data_ptr, buffer_size);
+
+			src_data_ptr += buffer_size;
+			dst_data_ptr += buffer_size;
+		}
+
+		/* FIXME: invalidate/flush for log buffer needed */
+
+		/* Update the read pointer in the shared log buffer */
+		log_buffer_state->read_ptr =
+			log_buffer_state_local.sampled_write_ptr;
+
+		/* Clear the 'flush to file' flag */
+		log_buffer_state->flush_to_file = 0;
+		log_buffer_state++;
+	}
+
+	if (log_buffer_snapshot_state)
+		guc_move_to_next_buf(guc);
+}
+
+static void guc_capture_logs_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, guc.log.flush_work);
+
+	i915_guc_capture_logs(dev_priv);
+}
+
+static void guc_log_cleanup(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	if (i915.guc_log_level < 0)
+		return;
+
+	/* First disable the flush interrupt */
+	gen9_disable_guc_interrupts(dev_priv);
+
+	if (guc->log.flush_wq)
+		destroy_workqueue(guc->log.flush_wq);
+
+	guc->log.flush_wq = NULL;
+
+	if (guc->log.buf_addr)
+		i915_gem_object_unpin_map(guc->log.vma->obj);
+
+	guc->log.buf_addr = NULL;
+}
+
+static int guc_create_log_extras(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	void *vaddr;
+	int ret;
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	/* Nothing to do */
+	if (i915.guc_log_level < 0)
+		return 0;
+
+	if (!guc->log.buf_addr) {
+		/* Create a vmalloc mapping of log buffer pages */
+		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WB);
+		if (IS_ERR(vaddr)) {
+			ret = PTR_ERR(vaddr);
+			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
+			return ret;
+		}
+
+		guc->log.buf_addr = vaddr;
+	}
+
+	if (!guc->log.flush_wq) {
+		INIT_WORK(&guc->log.flush_work, guc_capture_logs_work);
+
+		/* Need a dedicated wq to process log buffer flush interrupts
+		 * from GuC without much delay so as to avoid any loss of logs.
+		 */
+		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log", 0);
+		if (guc->log.flush_wq == NULL) {
+			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static void guc_create_log(struct intel_guc *guc)
 {
 	struct i915_vma *vma;
@@ -853,6 +1019,13 @@ static void guc_create_log(struct intel_guc *guc)
 		}
 
 		guc->log.vma = vma;
+
+		if (guc_create_log_extras(guc)) {
+			guc_log_cleanup(guc);
+			i915_vma_unpin_and_release(&guc->log.vma);
+			i915.guc_log_level = -1;
+			return;
+		}
 	}
 
 	/* each allocated unit is a page */
@@ -1034,6 +1207,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	i915_vma_unpin_and_release(&guc->ads_vma);
+	guc_log_cleanup(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 
 	if (guc->ctx_pool_vma)
@@ -1095,3 +1269,15 @@ int intel_guc_resume(struct drm_device *dev)
 
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
+
+void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
+{
+	guc_read_update_log_buffer(&dev_priv->guc);
+
+	/* Generally device is expected to be active only at this
+	 * time, so get/put should be really quick.
+	 */
+	intel_runtime_pm_get(dev_priv);
+	host2guc_logbuffer_flush_complete(&dev_priv->guc);
+	intel_runtime_pm_put(dev_priv);
+}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fc1fe72..19c0078 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1662,7 +1662,33 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 {
 	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
-		/* TODO: Handle events for which GuC interrupted host */
+		/* Sample the log buffer flush related bits & clear them out now
+		 * itself from the message identity register to minimize the
+		 * probability of losing a flush interrupt, when there are back
+		 * to back flush interrupts.
+		 * There can be a new flush interrupt, for different log buffer
+		 * type (like for ISR), whilst Host is handling one (for DPC).
+		 * Since same bit is used in message register for ISR & DPC, it
+		 * could happen that GuC sets the bit for 2nd interrupt but Host
+		 * clears out the bit on handling the 1st interrupt.
+		 */
+		u32 msg, flush;
+
+		msg = I915_READ(SOFT_SCRATCH(15));
+		flush = msg & (GUC2HOST_MSG_CRASH_DUMP_POSTED |
+			       GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+		if (flush) {
+			/* Clear the message bits that are handled */
+			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
+
+			/* Handle flush interrupt in bottom half */
+			queue_work(dev_priv->guc.log.flush_wq,
+				   &dev_priv->guc.log.flush_work);
+		} else {
+			/* Not clearing of unhandled event bits won't result in
+			 * re-triggering of the interrupt.
+			 */
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 1fc63fe..d053a18 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 {
 	uint32_t flags;
 	struct i915_vma *vma;
+	void *buf_addr;
+	struct workqueue_struct *flush_wq;
+	struct work_struct flush_work;
 };
 
 struct intel_guc {
@@ -167,5 +170,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
 int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
+void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
 
 #endif
-- 
1.9.2

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

[PATCH 07/19] relay: Use per CPU constructs for the relay channel buffer pointers

[akash.goel]

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

relay essentially needs to maintain the per CPU array of channel buffer
pointers but it manually creates that array.
Instead its better to avail the per CPU constructs, provided by the
kernel, to allocate & access the array of pointer to channel buffers.

v2: Include <linux/percpu.h> in relay.h so that it pulls in the percpu
    api explicitly. (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 include/linux/relay.h | 17 +++++++-----
 kernel/relay.c        | 74 +++++++++++++++++++++++++++++----------------------
 2 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index d7c8359..eb295e3 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -19,6 +19,7 @@
 #include <linux/fs.h>
 #include <linux/poll.h>
 #include <linux/kref.h>
+#include <linux/percpu.h>
 
 /*
  * Tracks changes to rchan/rchan_buf structs
@@ -63,7 +64,7 @@ struct rchan
 	struct kref kref;		/* channel refcount */
 	void *private_data;		/* for user-defined data */
 	size_t last_toobig;		/* tried to log event > subbuf size */
-	struct rchan_buf *buf[NR_CPUS]; /* per-cpu channel buffers */
+	struct rchan_buf ** __percpu buf; /* per-cpu channel buffers */
 	int is_global;			/* One global buffer ? */
 	struct list_head list;		/* for channel list */
 	struct dentry *parent;		/* parent dentry passed to open */
@@ -204,7 +205,7 @@ static inline void relay_write(struct rchan *chan,
 	struct rchan_buf *buf;
 
 	local_irq_save(flags);
-	buf = chan->buf[smp_processor_id()];
+	buf = *this_cpu_ptr(chan->buf);
 	if (unlikely(buf->offset + length > chan->subbuf_size))
 		length = relay_switch_subbuf(buf, length);
 	memcpy(buf->data + buf->offset, data, length);
@@ -230,12 +231,12 @@ static inline void __relay_write(struct rchan *chan,
 {
 	struct rchan_buf *buf;
 
-	buf = chan->buf[get_cpu()];
+	buf = *get_cpu_ptr(chan->buf);
 	if (unlikely(buf->offset + length > buf->chan->subbuf_size))
 		length = relay_switch_subbuf(buf, length);
 	memcpy(buf->data + buf->offset, data, length);
 	buf->offset += length;
-	put_cpu();
+	put_cpu_ptr(chan->buf);
 }
 
 /**
@@ -251,17 +252,19 @@ static inline void __relay_write(struct rchan *chan,
  */
 static inline void *relay_reserve(struct rchan *chan, size_t length)
 {
-	void *reserved;
-	struct rchan_buf *buf = chan->buf[smp_processor_id()];
+	void *reserved = NULL;
+	struct rchan_buf *buf = *get_cpu_ptr(chan->buf);
 
 	if (unlikely(buf->offset + length > buf->chan->subbuf_size)) {
 		length = relay_switch_subbuf(buf, length);
 		if (!length)
-			return NULL;
+			goto end;
 	}
 	reserved = buf->data + buf->offset;
 	buf->offset += length;
 
+end:
+	put_cpu_ptr(chan->buf);
 	return reserved;
 }
 
diff --git a/kernel/relay.c b/kernel/relay.c
index d797502..f55ab82 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -214,7 +214,7 @@ static void relay_destroy_buf(struct rchan_buf *buf)
 			__free_page(buf->page_array[i]);
 		relay_free_page_array(buf->page_array);
 	}
-	chan->buf[buf->cpu] = NULL;
+	*per_cpu_ptr(chan->buf, buf->cpu) = NULL;
 	kfree(buf->padding);
 	kfree(buf);
 	kref_put(&chan->kref, relay_destroy_channel);
@@ -382,20 +382,21 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
  */
 void relay_reset(struct rchan *chan)
 {
+	struct rchan_buf *buf;
 	unsigned int i;
 
 	if (!chan)
 		return;
 
-	if (chan->is_global && chan->buf[0]) {
-		__relay_reset(chan->buf[0], 0);
+	if (chan->is_global && (buf = *per_cpu_ptr(chan->buf, 0))) {
+		__relay_reset(buf, 0);
 		return;
 	}
 
 	mutex_lock(&relay_channels_mutex);
 	for_each_possible_cpu(i)
-		if (chan->buf[i])
-			__relay_reset(chan->buf[i], 0);
+		if ((buf = *per_cpu_ptr(chan->buf, i)))
+			__relay_reset(buf, 0);
 	mutex_unlock(&relay_channels_mutex);
 }
 EXPORT_SYMBOL_GPL(relay_reset);
@@ -440,7 +441,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 	struct dentry *dentry;
 
  	if (chan->is_global)
-		return chan->buf[0];
+		return *per_cpu_ptr(chan->buf, 0);
 
 	buf = relay_create_buf(chan);
 	if (!buf)
@@ -464,7 +465,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
  	__relay_reset(buf, 1);
 
  	if(chan->is_global) {
- 		chan->buf[0] = buf;
+		*per_cpu_ptr(chan->buf, 0) = buf;
  		buf->cpu = 0;
   	}
 
@@ -526,22 +527,24 @@ static int relay_hotcpu_callback(struct notifier_block *nb,
 {
 	unsigned int hotcpu = (unsigned long)hcpu;
 	struct rchan *chan;
+	struct rchan_buf *buf;
 
 	switch(action) {
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		mutex_lock(&relay_channels_mutex);
 		list_for_each_entry(chan, &relay_channels, list) {
-			if (chan->buf[hotcpu])
+			if ((buf = *per_cpu_ptr(chan->buf, hotcpu)))
 				continue;
-			chan->buf[hotcpu] = relay_open_buf(chan, hotcpu);
-			if(!chan->buf[hotcpu]) {
+			buf = relay_open_buf(chan, hotcpu);
+			if(!buf) {
 				printk(KERN_ERR
 					"relay_hotcpu_callback: cpu %d buffer "
 					"creation failed\n", hotcpu);
 				mutex_unlock(&relay_channels_mutex);
 				return notifier_from_errno(-ENOMEM);
 			}
+			*per_cpu_ptr(chan->buf, hotcpu) = buf;
 		}
 		mutex_unlock(&relay_channels_mutex);
 		break;
@@ -583,6 +586,7 @@ struct rchan *relay_open(const char *base_filename,
 {
 	unsigned int i;
 	struct rchan *chan;
+	struct rchan_buf *buf;
 
 	if (!(subbuf_size && n_subbufs))
 		return NULL;
@@ -593,6 +597,7 @@ struct rchan *relay_open(const char *base_filename,
 	if (!chan)
 		return NULL;
 
+	chan->buf = alloc_percpu(struct rchan_buf *);
 	chan->version = RELAYFS_CHANNEL_VERSION;
 	chan->n_subbufs = n_subbufs;
 	chan->subbuf_size = subbuf_size;
@@ -608,9 +613,10 @@ struct rchan *relay_open(const char *base_filename,
 
 	mutex_lock(&relay_channels_mutex);
 	for_each_online_cpu(i) {
-		chan->buf[i] = relay_open_buf(chan, i);
-		if (!chan->buf[i])
+		buf = relay_open_buf(chan, i);
+		if (!buf)
 			goto free_bufs;
+		*per_cpu_ptr(chan->buf, i) = buf;
 	}
 	list_add(&chan->list, &relay_channels);
 	mutex_unlock(&relay_channels_mutex);
@@ -619,8 +625,8 @@ struct rchan *relay_open(const char *base_filename,
 
 free_bufs:
 	for_each_possible_cpu(i) {
-		if (chan->buf[i])
-			relay_close_buf(chan->buf[i]);
+		if ((buf = *per_cpu_ptr(chan->buf, i)))
+			relay_close_buf(buf);
 	}
 
 	kref_put(&chan->kref, relay_destroy_channel);
@@ -666,6 +672,7 @@ int relay_late_setup_files(struct rchan *chan,
 	unsigned int i, curr_cpu;
 	unsigned long flags;
 	struct dentry *dentry;
+	struct rchan_buf *buf;
 	struct rchan_percpu_buf_dispatcher disp;
 
 	if (!chan || !base_filename)
@@ -684,10 +691,11 @@ int relay_late_setup_files(struct rchan *chan,
 
 	if (chan->is_global) {
 		err = -EINVAL;
-		if (!WARN_ON_ONCE(!chan->buf[0])) {
-			dentry = relay_create_buf_file(chan, chan->buf[0], 0);
+		buf = *per_cpu_ptr(chan->buf, 0);
+		if (!WARN_ON_ONCE(!buf)) {
+			dentry = relay_create_buf_file(chan, buf, 0);
 			if (dentry && !WARN_ON_ONCE(!chan->is_global)) {
-				relay_set_buf_dentry(chan->buf[0], dentry);
+				relay_set_buf_dentry(buf, dentry);
 				err = 0;
 			}
 		}
@@ -702,13 +710,14 @@ int relay_late_setup_files(struct rchan *chan,
 	 * on all currently online CPUs.
 	 */
 	for_each_online_cpu(i) {
-		if (unlikely(!chan->buf[i])) {
+		buf = *per_cpu_ptr(chan->buf, i);
+		if (unlikely(!buf)) {
 			WARN_ONCE(1, KERN_ERR "CPU has no buffer!\n");
 			err = -EINVAL;
 			break;
 		}
 
-		dentry = relay_create_buf_file(chan, chan->buf[i], i);
+		dentry = relay_create_buf_file(chan, buf, i);
 		if (unlikely(!dentry)) {
 			err = -EINVAL;
 			break;
@@ -716,10 +725,10 @@ int relay_late_setup_files(struct rchan *chan,
 
 		if (curr_cpu == i) {
 			local_irq_save(flags);
-			relay_set_buf_dentry(chan->buf[i], dentry);
+			relay_set_buf_dentry(buf, dentry);
 			local_irq_restore(flags);
 		} else {
-			disp.buf = chan->buf[i];
+			disp.buf = buf;
 			disp.dentry = dentry;
 			smp_mb();
 			/* relay_channels_mutex must be held, so wait. */
@@ -822,11 +831,10 @@ void relay_subbufs_consumed(struct rchan *chan,
 	if (!chan)
 		return;
 
-	if (cpu >= NR_CPUS || !chan->buf[cpu] ||
-					subbufs_consumed > chan->n_subbufs)
+	buf = *per_cpu_ptr(chan->buf, cpu);
+	if (cpu >= NR_CPUS || !buf || subbufs_consumed > chan->n_subbufs)
 		return;
 
-	buf = chan->buf[cpu];
 	if (subbufs_consumed > buf->subbufs_produced - buf->subbufs_consumed)
 		buf->subbufs_consumed = buf->subbufs_produced;
 	else
@@ -842,18 +850,19 @@ EXPORT_SYMBOL_GPL(relay_subbufs_consumed);
  */
 void relay_close(struct rchan *chan)
 {
+	struct rchan_buf *buf;
 	unsigned int i;
 
 	if (!chan)
 		return;
 
 	mutex_lock(&relay_channels_mutex);
-	if (chan->is_global && chan->buf[0])
-		relay_close_buf(chan->buf[0]);
+	if (chan->is_global && (buf = *per_cpu_ptr(chan->buf, 0)))
+		relay_close_buf(buf);
 	else
 		for_each_possible_cpu(i)
-			if (chan->buf[i])
-				relay_close_buf(chan->buf[i]);
+			if ((buf = *per_cpu_ptr(chan->buf, i)))
+				relay_close_buf(buf);
 
 	if (chan->last_toobig)
 		printk(KERN_WARNING "relay: one or more items not logged "
@@ -874,20 +883,21 @@ EXPORT_SYMBOL_GPL(relay_close);
  */
 void relay_flush(struct rchan *chan)
 {
+	struct rchan_buf *buf;
 	unsigned int i;
 
 	if (!chan)
 		return;
 
-	if (chan->is_global && chan->buf[0]) {
-		relay_switch_subbuf(chan->buf[0], 0);
+	if (chan->is_global && (buf = *per_cpu_ptr(chan->buf, 0))) {
+		relay_switch_subbuf(buf, 0);
 		return;
 	}
 
 	mutex_lock(&relay_channels_mutex);
 	for_each_possible_cpu(i)
-		if (chan->buf[i])
-			relay_switch_subbuf(chan->buf[i], 0);
+		if ((buf = *per_cpu_ptr(chan->buf, i)))
+			relay_switch_subbuf(buf, 0);
 	mutex_unlock(&relay_channels_mutex);
 }
 EXPORT_SYMBOL_GPL(relay_flush);
-- 
1.9.2

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

[PATCH 08/19] 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 firmware logs. Availed relay framework to implement
the interface, where Driver will have to just use a relay API to store
snapshots of the GuC log buffer in the buffer managed by relay.
The snapshot will be taken when GuC firmware sends a log buffer flush
interrupt and up to four snapshots could be stored in the relay buffer.
The relay buffer will be operated in a mode where it will overwrite the
data not yet collected by User.
Besides mmap method, through which User can directly access the relay
buffer contents, relay also supports the 'poll' method. Through the 'poll'
call on log file, User can come to know whenever a new snapshot of the
log buffer is taken by Driver, so can run in tandem with the Driver and
capture the logs in a sustained/streaming manner, without any loss of data.

v2: Defer the creation of relay channel & associated debugfs file, as
    debugfs setup is now done at the end of i915 Driver load. (Chris)

v3:
- Switch to no-overwrite mode for relay.
- Fix the relay sub buffer switching sequence.

v4:
- Update i915 Kconfig to select RELAY config. (TvrtKo)
- Log a message when there is no sub buffer available to capture
  the GuC log buffer. (Tvrtko)
- Increase the number of relay sub buffers to 8 from 4, to have
  sufficient buffering for boot time logs

v5:
- Fix the alignment, indentation issues and some minor cleanup. (Tvrtko)
- Update the comment to elaborate on why a relay channel has to be
  associated with the debugfs file. (Tvrtko)

v6:
- Move the write to 'is_global' after the NULL check on parent directory
  dentry pointer. (Tvrtko)

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/Kconfig               |   1 +
 drivers/gpu/drm/i915/i915_drv.c            |   2 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 212 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h           |   3 +
 4 files changed, 216 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 7769e46..fc900d2 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -11,6 +11,7 @@ config DRM_I915
 	select DRM_KMS_HELPER
 	select DRM_PANEL
 	select DRM_MIPI_DSI
+	select RELAY
 	# i915 depends on ACPI_VIDEO when ACPI is enabled
 	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
 	select BACKLIGHT_LCD_SUPPORT if ACPI
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 13ae340..cdee60b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1133,6 +1133,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	/* Reveal our presence to userspace */
 	if (drm_dev_register(dev, 0) == 0) {
 		i915_debugfs_register(dev_priv);
+		i915_guc_register(dev_priv);
 		i915_setup_sysfs(dev);
 	} else
 		DRM_ERROR("Failed to register driver for userspace access!\n");
@@ -1171,6 +1172,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	intel_opregion_unregister(dev_priv);
 
 	i915_teardown_sysfs(&dev_priv->drm);
+	i915_guc_unregister(dev_priv);
 	i915_debugfs_unregister(dev_priv);
 	drm_dev_unregister(&dev_priv->drm);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ade51cb..1713ce8 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,13 +839,159 @@ err:
 	return NULL;
 }
 
+/*
+ * Sub buffer switch callback. Called whenever relay has to switch to a new
+ * sub buffer, relay stays on the same sub buffer if 0 is returned.
+ */
+static int subbuf_start_callback(struct rchan_buf *buf,
+				 void *subbuf,
+				 void *prev_subbuf,
+				 size_t prev_padding)
+{
+	/* Use no-overwrite mode by default, where relay will stop accepting
+	 * new data if there are no empty sub buffers left.
+	 * There is no strict synchronization enforced by relay between Consumer
+	 * and Producer. In overwrite mode, there is a possibility of getting
+	 * inconsistent/garbled data, the producer could be writing on to the
+	 * same sub buffer from which Consumer is reading. This can't be avoided
+	 * unless Consumer is fast enough and can always run in tandem with
+	 * Producer.
+	 */
+	if (relay_buf_full(buf))
+		return 0;
+
+	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)
+{
+	struct dentry *buf_file;
+
+	if (!parent)
+		return NULL;
+
+	/* 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 collect the logs in order without any post-processing.
+	 */
+	*is_global = 1;
+
+	/* Not using the channel filename passed as an argument, since for each
+	 * channel relay appends the corresponding CPU number to the filename
+	 * passed in relay_open(). This should be fine as relay just needs a
+	 * dentry of the file associated with the channel buffer and that file's
+	 * name need not be same as the filename passed as an argument.
+	 */
+	buf_file = debugfs_create_file("guc_log", mode,
+				       parent, buf, &relay_file_operations);
+	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 int guc_create_log_relay_file(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct rchan *guc_log_relay_chan;
+	struct dentry *log_dir;
+	size_t n_subbufs, subbuf_size;
+
+	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
+	log_dir = dev_priv->drm.primary->debugfs_root;
+
+	/* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is
+	 * not mounted and so can't create the relay file.
+	 * The relay API seems to fit well with debugfs only, for availing relay
+	 * there are 3 requirements which can be met for debugfs file only in a
+	 * straightforward/clean manner :-
+	 * i)   Need the associated dentry pointer of the file, while opening the
+	 *      relay channel.
+	 * ii)  Should be able to use 'relay_file_operations' fops for the file.
+	 * iii) Set the 'i_private' field of file's inode to the pointer of
+	 *	relay channel buffer.
+	 */
+	if (!log_dir) {
+		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
+		return -ENODEV;
+	}
+
+	/* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log.vma->obj->base.size;
+
+	/* Store up to 8 snapshots, which is large enough to buffer sufficient
+	 * boot time logs and provides enough leeway to User, in terms of
+	 * latency, for consuming the logs from relay. Also doesn't take
+	 * up too much memory.
+	 */
+	n_subbufs = 8;
+
+	guc_log_relay_chan = relay_open("guc_log", log_dir, subbuf_size,
+					n_subbufs, &relay_callbacks, dev_priv);
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
+		return -ENOMEM;
+	}
+
+	/* FIXME: Cover the update under a lock ? */
+	guc->log.relay_chan = guc_log_relay_chan;
+	return 0;
+}
+
 static void guc_move_to_next_buf(struct intel_guc *guc)
 {
+	/* Make sure the updates made in the sub buffer are visible when
+	 * Consumer sees the following update to offset inside the sub buffer.
+	 */
+	smp_wmb();
+
+	/* All data has been written, so now move the offset of sub buffer. */
+	relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
+
+	/* Switch to the next sub buffer */
+	relay_flush(guc->log.relay_chan);
 }
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-	return NULL;
+	/* FIXME: Cover the check under a lock ? */
+	if (!guc->log.relay_chan)
+		return NULL;
+
+	/* Just get the base address of a new sub buffer and copy data into it
+	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
+	 * buffers are full. Could have used the relay_write() to indirectly
+	 * copy the data, but that would have been bit convoluted, as we need to
+	 * write to only certain locations inside a sub buffer which cannot be
+	 * done without using relay_reserve() along with relay_write(). So its
+	 * better to use relay_reserve() alone.
+	 */
+	return relay_reserve(guc->log.relay_chan, 0);
 }
 
 static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type type)
@@ -921,6 +1069,12 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 
 	if (log_buffer_snapshot_state)
 		guc_move_to_next_buf(guc);
+	else {
+		/* Used rate limited to avoid deluge of messages, logs might be
+		 * getting consumed by User at a slow rate.
+		 */
+		DRM_ERROR_RATELIMITED("no sub-buffer to capture log buffer\n");
+	}
 }
 
 static void guc_capture_logs_work(struct work_struct *work)
@@ -948,6 +1102,11 @@ static void guc_log_cleanup(struct intel_guc *guc)
 
 	guc->log.flush_wq = NULL;
 
+	if (guc->log.relay_chan)
+		guc_remove_log_relay_file(guc);
+
+	guc->log.relay_chan = NULL;
+
 	if (guc->log.buf_addr)
 		i915_gem_object_unpin_map(guc->log.vma->obj);
 
@@ -1038,6 +1197,36 @@ static void guc_create_log(struct intel_guc *guc)
 	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 }
 
+static int guc_log_late_setup(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	int ret;
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	if (i915.guc_log_level < 0)
+		return -EINVAL;
+
+	/* If log_level was set as -1 at boot time, then setup needed to
+	 * handle log buffer flush interrupts would not have been done yet,
+	 * so do that now.
+	 */
+	ret = guc_create_log_extras(guc);
+	if (ret)
+		goto err;
+
+	ret = guc_create_log_relay_file(guc);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	guc_log_cleanup(guc);
+	/* logging will remain off */
+	i915.guc_log_level = -1;
+	return ret;
+}
+
 static void init_guc_policies(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
@@ -1207,7 +1396,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 
 	i915_vma_unpin_and_release(&guc->ads_vma);
-	guc_log_cleanup(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 
 	if (guc->ctx_pool_vma)
@@ -1281,3 +1469,23 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
 	host2guc_logbuffer_flush_complete(&dev_priv->guc);
 	intel_runtime_pm_put(dev_priv);
 }
+
+void i915_guc_unregister(struct drm_i915_private *dev_priv)
+{
+	if (!i915.enable_guc_submission)
+		return;
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	guc_log_cleanup(&dev_priv->guc);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+}
+
+void i915_guc_register(struct drm_i915_private *dev_priv)
+{
+	if (!i915.enable_guc_submission)
+		return;
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	guc_log_late_setup(&dev_priv->guc);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d053a18..3299cce 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -127,6 +127,7 @@ struct intel_guc_log {
 	void *buf_addr;
 	struct workqueue_struct *flush_wq;
 	struct work_struct flush_work;
+	struct rchan *relay_chan;
 };
 
 struct intel_guc {
@@ -171,5 +172,7 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
+void i915_guc_register(struct drm_i915_private *dev_priv);
+void i915_guc_unregister(struct drm_i915_private *dev_priv);
 
 #endif
-- 
1.9.2

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

[PATCH 09/19] drm/i915: New lock to serialize the Host2GuC actions

[akash.goel]

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

With the addition of new Host2GuC actions related to GuC logging, there
is a need of a lock to serialize them, as they can execute concurrently
with each other and also with other existing actions.

v2: Use mutex in place of spinlock to serialize, as sleep can happen
    while waiting for the action's response from GuC. (Tvrtko)

v3: To conform to the general rules, acquire mutex before taking the
    forcewake. (Tvrtko)

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 1713ce8..99f4afa 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -87,6 +87,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
 	if (WARN_ON(len < 1 || len > 15))
 		return -EINVAL;
 
+	mutex_lock(&guc->action_lock);
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	dev_priv->guc.action_count += 1;
@@ -127,6 +128,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
 	dev_priv->guc.action_status = status;
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+	mutex_unlock(&guc->action_lock);
 
 	return ret;
 }
@@ -1344,6 +1346,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	guc->ctx_pool_vma = vma;
 	ida_init(&guc->ctx_ids);
+	mutex_init(&guc->action_lock);
 	guc_create_log(guc);
 	guc_create_ads(guc);
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 3299cce..1704495 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -155,6 +155,9 @@ struct intel_guc {
 
 	uint64_t submissions[I915_NUM_ENGINES];
 	uint32_t last_seqno[I915_NUM_ENGINES];
+
+	/* To serialize the Host2GuC actions */
+	struct mutex action_lock;
 };
 
 /* intel_guc_loader.c */
-- 
1.9.2

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

[PATCH 10/19] drm/i915: Add stats for GuC log buffer flush interrupts

[akash.goel]

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

GuC firmware sends an interrupt to flush the log buffer when it
becomes half full. GuC firmware also tracks how many times the
buffer overflowed.
It would be useful to maintain a statistics of how many flush
interrupts were received and for which type of log buffer,
along with the overflow count of each buffer type.
Augmented i915_log_info debugfs to report back these statistics.

v2:
- Update the logic to detect multiple overflows between the 2
  flush interrupts and also log a message for overflow (Tvrtko)
- Track the number of times there was no free sub buffer to capture
  the GuC log buffer. (Tvrtko)

v3:
- Fix the printf field width for overflow counter, set it to 10 as per the
  max value of u32, which takes 10 digits in decimal form. (Tvrtko)

Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c            |  2 ++
 drivers/gpu/drm/i915/intel_guc.h           |  7 +++++++
 4 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b2cfcf4..aaa51bc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2491,6 +2491,32 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static void i915_guc_log_info(struct seq_file *m,
+			      struct drm_i915_private *dev_priv)
+{
+	struct intel_guc *guc = &dev_priv->guc;
+
+	seq_puts(m, "\nGuC logging stats:\n");
+
+	seq_printf(m, "\tISR:   flush count %10u, overflow count %10u\n",
+		   guc->log.flush_count[GUC_ISR_LOG_BUFFER],
+		   guc->log.total_overflow_count[GUC_ISR_LOG_BUFFER]);
+
+	seq_printf(m, "\tDPC:   flush count %10u, overflow count %10u\n",
+		   guc->log.flush_count[GUC_DPC_LOG_BUFFER],
+		   guc->log.total_overflow_count[GUC_DPC_LOG_BUFFER]);
+
+	seq_printf(m, "\tCRASH: flush count %10u, overflow count %10u\n",
+		   guc->log.flush_count[GUC_CRASH_DUMP_LOG_BUFFER],
+		   guc->log.total_overflow_count[GUC_CRASH_DUMP_LOG_BUFFER]);
+
+	seq_printf(m, "\tTotal flush interrupt count: %u\n",
+		   guc->log.flush_interrupt_count);
+
+	seq_printf(m, "\tCapture miss count: %u\n",
+		   guc->log.capture_miss_count);
+}
+
 static void i915_guc_client_info(struct seq_file *m,
 				 struct drm_i915_private *dev_priv,
 				 struct i915_guc_client *client)
@@ -2565,6 +2591,8 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc.execbuf_client);
 	i915_guc_client_info(m, dev_priv, &client);
 
+	i915_guc_log_info(m, dev_priv);
+
 	/* Add more as required ... */
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 99f4afa..8b53a22 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1036,6 +1036,25 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		       sizeof(struct guc_log_buffer_state));
 		buffer_size = guc_get_log_buffer_size(type);
 
+		/* Bookkeeping stuff */
+		guc->log.flush_count[type] += log_buffer_state_local.flush_to_file;
+		if (log_buffer_state_local.buffer_full_cnt !=
+		    guc->log.prev_overflow_count[type]) {
+			guc->log.total_overflow_count[type] +=
+				(log_buffer_state_local.buffer_full_cnt -
+				 guc->log.prev_overflow_count[type]);
+
+			if (log_buffer_state_local.buffer_full_cnt <
+			    guc->log.prev_overflow_count[type]) {
+				/* buffer_full_cnt is a 4 bit counter */
+				guc->log.total_overflow_count[type] += 16;
+			}
+
+			guc->log.prev_overflow_count[type] =
+					log_buffer_state_local.buffer_full_cnt;
+			DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
+		}
+
 		if (log_buffer_snapshot_state) {
 			/* First copy the state structure in snapshot buffer */
 			memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
@@ -1076,6 +1095,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		 * getting consumed by User at a slow rate.
 		 */
 		DRM_ERROR_RATELIMITED("no sub-buffer to capture log buffer\n");
+		guc->log.capture_miss_count++;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 19c0078..8cbb911 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1684,6 +1684,8 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 			/* Handle flush interrupt in bottom half */
 			queue_work(dev_priv->guc.log.flush_wq,
 				   &dev_priv->guc.log.flush_work);
+
+			dev_priv->guc.log.flush_interrupt_count++;
 		} else {
 			/* Not clearing of unhandled event bits won't result in
 			 * re-triggering of the interrupt.
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 1704495..8598f38 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -128,6 +128,13 @@ struct intel_guc_log {
 	struct workqueue_struct *flush_wq;
 	struct work_struct flush_work;
 	struct rchan *relay_chan;
+
+	/* logging related stats */
+	u32 capture_miss_count;
+	u32 flush_interrupt_count;
+	u32 prev_overflow_count[GUC_MAX_LOG_BUFFER];
+	u32 total_overflow_count[GUC_MAX_LOG_BUFFER];
+	u32 flush_count[GUC_MAX_LOG_BUFFER];
 };
 
 struct intel_guc {
-- 
1.9.2

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

[PATCH 11/19] drm/i915: Optimization to reduce the sampling time of GuC log buffer

[akash.goel]

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

GuC firmware sends an interrupt to flush the log buffer when it becomes
half full, so Driver doesn't really need to sample the complete buffer
and can just copy only the newly written data by GuC into the local
buffer, i.e. as per the read & write pointer values.
Moreover the flush interrupt would generally come for one type of log
buffer, when it becomes half full, so at that time the other 2 types of
log buffer would comparatively have much lesser unread data in them.
In case of overflow reported by GuC, Driver do need to copy the entire
buffer as the whole buffer would contain the unread data.

v2: Rebase.

v3: Fix the blooper of doing the copy twice. (Tvrtko)

v4: Add curlies for 'else' case also, matching the 'if'. (Tvrtko)

Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 40 +++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8b53a22..e46828b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1011,8 +1011,9 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
 	struct guc_log_buffer_state log_buffer_state_local;
 	void *src_data_ptr, *dst_data_ptr;
-	unsigned int buffer_size;
+	unsigned int buffer_size, read_offset, write_offset, bytes_to_copy;
 	enum guc_log_buffer_type type;
+	bool new_overflow;
 
 	if (WARN_ON(!guc->log.buf_addr))
 		return;
@@ -1035,11 +1036,14 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		memcpy(&log_buffer_state_local, log_buffer_state,
 		       sizeof(struct guc_log_buffer_state));
 		buffer_size = guc_get_log_buffer_size(type);
+		read_offset = log_buffer_state_local.read_ptr;
+		write_offset = log_buffer_state_local.sampled_write_ptr;
 
 		/* Bookkeeping stuff */
 		guc->log.flush_count[type] += log_buffer_state_local.flush_to_file;
 		if (log_buffer_state_local.buffer_full_cnt !=
 		    guc->log.prev_overflow_count[type]) {
+			new_overflow = 1;
 			guc->log.total_overflow_count[type] +=
 				(log_buffer_state_local.buffer_full_cnt -
 				 guc->log.prev_overflow_count[type]);
@@ -1053,6 +1057,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 			guc->log.prev_overflow_count[type] =
 					log_buffer_state_local.buffer_full_cnt;
 			DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
+		} else {
+			new_overflow = 0;
 		}
 
 		if (log_buffer_snapshot_state) {
@@ -1065,13 +1071,36 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 			 * for consistency set the write pointer value to same
 			 * value of sampled_write_ptr in the snapshot buffer.
 			 */
-			log_buffer_snapshot_state->write_ptr =
-				log_buffer_snapshot_state->sampled_write_ptr;
+			log_buffer_snapshot_state->write_ptr = write_offset;
 
 			log_buffer_snapshot_state++;
 
 			/* Now copy the actual logs. */
-			memcpy(dst_data_ptr, src_data_ptr, buffer_size);
+			if (unlikely(new_overflow)) {
+				/* copy the whole buffer in case of overflow */
+				read_offset = 0;
+				write_offset = buffer_size;
+			} else if (unlikely((read_offset > buffer_size) ||
+					    (write_offset > buffer_size))) {
+				DRM_ERROR("invalid log buffer state\n");
+				/* copy whole buffer as offsets are unreliable */
+				read_offset = 0;
+				write_offset = buffer_size;
+			}
+
+			/* Just copy the newly written data */
+			if (read_offset <= write_offset) {
+				bytes_to_copy = write_offset - read_offset;
+				memcpy(dst_data_ptr + read_offset,
+				       src_data_ptr + read_offset, bytes_to_copy);
+			} else {
+				bytes_to_copy = buffer_size - read_offset;
+				memcpy(dst_data_ptr + read_offset,
+				       src_data_ptr + read_offset, bytes_to_copy);
+
+				bytes_to_copy = write_offset;
+				memcpy(dst_data_ptr, src_data_ptr, bytes_to_copy);
+			}
 
 			src_data_ptr += buffer_size;
 			dst_data_ptr += buffer_size;
@@ -1080,8 +1109,7 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 		/* FIXME: invalidate/flush for log buffer needed */
 
 		/* Update the read pointer in the shared log buffer */
-		log_buffer_state->read_ptr =
-			log_buffer_state_local.sampled_write_ptr;
+		log_buffer_state->read_ptr = write_offset;
 
 		/* Clear the 'flush to file' flag */
 		log_buffer_state->flush_to_file = 0;
-- 
1.9.2

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

[PATCH 12/19] drm/i915: Increase GuC log buffer size to reduce flush interrupts

[akash.goel]

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

In cases where GuC generate logs at a very high rate, correspondingly
the rate of flush interrupts is also very high.
So far total 8 pages were allocated for storing both ISR & DPC logs.
As per the half-full draining protocol followed by GuC, by doubling
the number of pages, the frequency of flush interrupts can be cut down
to almost half, which then helps in reducing the logging overhead.
So now allocating 8 pages apiece for ISR & DPC logs.
This also helps in reducing the output log file size, apart from
reducing the flush interrupt count. With the original settings,
44 KB was needed for one snapshot. With modified settings, 76 KB is
needed for a snapshot which will be equivalent to 2 snapshots of the
original setting. So 12KB saving, every 88 KB, over the original setting.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 47ef7e8..49dbe25 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -104,9 +104,9 @@
 #define   GUC_LOG_ALLOC_IN_MEGABYTE	(1 << 3)
 #define   GUC_LOG_CRASH_PAGES		1
 #define   GUC_LOG_CRASH_SHIFT		4
-#define   GUC_LOG_DPC_PAGES		3
+#define   GUC_LOG_DPC_PAGES		7
 #define   GUC_LOG_DPC_SHIFT		6
-#define   GUC_LOG_ISR_PAGES		3
+#define   GUC_LOG_ISR_PAGES		7
 #define   GUC_LOG_ISR_SHIFT		9
 #define   GUC_LOG_BUF_ADDR_SHIFT	12
 
@@ -436,9 +436,9 @@ enum guc_log_buffer_type {
  *        |   Crash dump state header     |
  * Page1  +-------------------------------+
  *        |           ISR logs            |
- * Page5  +-------------------------------+
- *        |           DPC logs            |
  * Page9  +-------------------------------+
+ *        |           DPC logs            |
+ * Page17 +-------------------------------+
  *        |         Crash Dump logs       |
  *        +-------------------------------+
  *
-- 
1.9.2

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

[PATCH 13/19] drm/i915: Augment i915 error state to include the dump of GuC log buffer

[akash.goel]

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

Added the dump of GuC log buffer to i915 error state, as the contents of
GuC log buffer would also be useful to determine that why the GPU reset
was triggered.

v2:
- For uniformity use existing helper function print_error_obj() to
  dump out contents of GuC log buffer, pretty printing is better left
  to userspace. (Chris)
- Skip the dumping of GuC log buffer when logging is disabled as it
  won't be of any use.
- Rebase.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_gpu_error.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 08553b9..de89596 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -734,6 +734,7 @@ struct drm_i915_error_state {
 	struct intel_overlay_error_state *overlay;
 	struct intel_display_error_state *display;
 	struct drm_i915_error_object *semaphore;
+	struct drm_i915_error_object *guc_log;
 
 	struct drm_i915_error_engine {
 		int engine_id;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 0c3f30c..94297aa 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -572,6 +572,13 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		}
 	}
 
+	obj = error->guc_log;
+	if (obj) {
+		err_printf(m, "GuC log buffer = 0x%08x\n",
+			   lower_32_bits(obj->gtt_offset));
+		print_error_obj(m, obj);
+	}
+
 	if (error->overlay)
 		intel_overlay_print_error_state(m, error->overlay);
 
@@ -651,6 +658,7 @@ static void i915_error_state_free(struct kref *error_ref)
 	}
 
 	i915_error_object_free(error->semaphore);
+	i915_error_object_free(error->guc_log);
 
 	for (i = 0; i < ARRAY_SIZE(error->active_bo); i++)
 		kfree(error->active_bo[i]);
@@ -1286,6 +1294,17 @@ static void i915_capture_pinned_buffers(struct drm_i915_private *dev_priv,
 	error->pinned_bo = bo;
 }
 
+static void i915_gem_capture_guc_log_buffer(struct drm_i915_private *dev_priv,
+					    struct drm_i915_error_state *error)
+{
+	/* Capturing log buf contents won't be useful if logging was disabled */
+	if (!dev_priv->guc.log.vma || (i915.guc_log_level < 0))
+		return;
+
+	error->guc_log = i915_error_object_create(dev_priv,
+						  dev_priv->guc.log.vma);
+}
+
 /* Capture all registers which don't fit into another category. */
 static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
 				   struct drm_i915_error_state *error)
@@ -1437,6 +1456,7 @@ void i915_capture_error_state(struct drm_i915_private *dev_priv,
 	i915_gem_record_rings(dev_priv, error);
 	i915_capture_active_buffers(dev_priv, error);
 	i915_capture_pinned_buffers(dev_priv, error);
+	i915_gem_capture_guc_log_buffer(dev_priv, error);
 
 	do_gettimeofday(&error->time);
 
-- 
1.9.2

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

[PATCH 14/19] drm/i915: Forcefully flush GuC log buffer on reset

[akash.goel]

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

Before capturing the GuC logs as a part of error state, there should be a
force log buffer flush action sent to GuC before proceeding with GPU reset
and re-initializing GUC. There could be some data in the log buffer which
is yet to be captured and those logs would be particularly useful to
understand that why the GPU reset was initiated.

v2:
- Avoid the wait via flush_work, to serialize against an ongoing log
  buffer flush, from the error state capture path. (Chris)
- Rebase.

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_gpu_error.c      |  2 ++
 drivers/gpu/drm/i915/i915_guc_submission.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 94297aa..b73c671 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1301,6 +1301,8 @@ static void i915_gem_capture_guc_log_buffer(struct drm_i915_private *dev_priv,
 	if (!dev_priv->guc.log.vma || (i915.guc_log_level < 0))
 		return;
 
+	i915_guc_flush_logs(dev_priv, false);
+
 	error->guc_log = i915_error_object_create(dev_priv,
 						  dev_priv->guc.log.vma);
 }
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e46828b..beeeb23 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -185,6 +185,16 @@ static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
 	return host2guc_action(guc, data, 1);
 }
 
+static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
+{
+	u32 data[2];
+
+	data[0] = HOST2GUC_ACTION_FORCE_LOG_BUFFER_FLUSH;
+	data[1] = 0;
+
+	return host2guc_action(guc, data, 2);
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -1521,6 +1531,26 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
 	intel_runtime_pm_put(dev_priv);
 }
 
+void i915_guc_flush_logs(struct drm_i915_private *dev_priv, bool can_wait)
+{
+	if (!i915.enable_guc_submission || (i915.guc_log_level < 0))
+		return;
+
+	/* First disable the interrupts, will be renabled afterwards */
+	gen9_disable_guc_interrupts(dev_priv);
+
+	/* Before initiating the forceful flush, wait for any pending/ongoing
+	 * flush to complete otherwise forceful flush may not happen, but wait
+	 * can't be done for some paths like error state capture in which case
+	 * take a chance & directly attempt the forceful flush.
+	 */
+	if (can_wait)
+		flush_work(&dev_priv->guc.log.flush_work);
+
+	/* Ask GuC to update the log buffer state */
+	host2guc_force_logbuffer_flush(&dev_priv->guc);
+}
+
 void i915_guc_unregister(struct drm_i915_private *dev_priv)
 {
 	if (!i915.enable_guc_submission)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 8598f38..d7eda42 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -182,6 +182,7 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
+void i915_guc_flush_logs(struct drm_i915_private *dev_priv, bool can_wait);
 void i915_guc_register(struct drm_i915_private *dev_priv);
 void i915_guc_unregister(struct drm_i915_private *dev_priv);
 
-- 
1.9.2

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

[PATCH 15/19] drm/i915: Debugfs support for GuC logging control

[akash.goel]

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

This patch provides debugfs interface i915_guc_output_control for
on the fly enabling/disabling of logging in GuC firmware and controlling
the verbosity level of logs.
The value written to the file, should have bit 0 set to enable logging and
bits 4-7 should contain the verbosity info.

v2: Add a forceful flush, to collect left over logs, on disabling logging.
    Useful for Validation.

v3: Besides minor cleanup, implement read method for the debugfs file and
    set the guc_log_level to -1 when logging is disabled. (Tvrtko)

v4: Minor cleanup & rebase. (Tvrtko)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 44 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_guc_submission.c | 62 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index aaa51bc..bfc22bf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2626,6 +2626,47 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_guc_log_control_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (!dev_priv->guc.log.vma)
+		return -EINVAL;
+
+	*val = i915.guc_log_level;
+
+	return 0;
+}
+
+static int i915_guc_log_control_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	if (!dev_priv->guc.log.vma) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	intel_runtime_pm_get(dev_priv);
+	ret = i915_guc_log_control(dev_priv, val);
+	intel_runtime_pm_put(dev_priv);
+
+end:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+			i915_guc_log_control_get, i915_guc_log_control_set,
+			"%lld\n");
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5436,7 +5477,8 @@ static const struct i915_debugfs_files {
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
 	{"i915_dp_test_data", &i915_displayport_test_data_fops},
 	{"i915_dp_test_type", &i915_displayport_test_type_fops},
-	{"i915_dp_test_active", &i915_displayport_test_active_fops}
+	{"i915_dp_test_active", &i915_displayport_test_active_fops},
+	{"i915_guc_log_control", &i915_guc_log_control_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index beeeb23..5a302fe 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -195,6 +195,16 @@ static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
 	return host2guc_action(guc, data, 2);
 }
 
+static int host2guc_logging_control(struct intel_guc *guc, u32 control_val)
+{
+	u32 data[2];
+
+	data[0] = HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING;
+	data[1] = control_val;
+
+	return host2guc_action(guc, data, 2);
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -1570,3 +1580,55 @@ void i915_guc_register(struct drm_i915_private *dev_priv)
 	guc_log_late_setup(&dev_priv->guc);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 }
+
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
+{
+	union guc_log_control log_param;
+	int ret;
+
+	log_param.value = control_val;
+
+	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
+	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
+		return -EINVAL;
+
+	/* This combination doesn't make sense & won't have any effect */
+	if (!log_param.logging_enabled && (i915.guc_log_level < 0))
+		return 0;
+
+	ret = host2guc_logging_control(&dev_priv->guc, log_param.value);
+	if (ret < 0) {
+		DRM_DEBUG_DRIVER("host2guc action failed %d\n", ret);
+		return ret;
+	}
+
+	i915.guc_log_level = log_param.verbosity;
+
+	/* If log_level was set as -1 at boot time, then the relay channel file
+	 * wouldn't have been created by now and interrupts also would not have
+	 * been enabled.
+	 */
+	if (!dev_priv->guc.log.relay_chan) {
+		ret = guc_log_late_setup(&dev_priv->guc);
+		if (!ret)
+			gen9_enable_guc_interrupts(dev_priv);
+	} else if (!log_param.logging_enabled) {
+		/* Once logging is disabled, GuC won't generate logs & send an
+		 * interrupt. But there could be some data in the log buffer
+		 * which is yet to be captured. So request GuC to update the log
+		 * buffer state and then collect the left over logs.
+		 */
+		i915_guc_flush_logs(dev_priv, true);
+
+		/* GuC would have updated log buffer by now, so capture it */
+		i915_guc_capture_logs(dev_priv);
+
+		/* As logging is disabled, update log level to reflect that */
+		i915.guc_log_level = -1;
+	} else {
+		/* In case interrupts were disabled, enable them now */
+		gen9_enable_guc_interrupts(dev_priv);
+	}
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d7eda42..b56fe24 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -185,5 +185,6 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
 void i915_guc_flush_logs(struct drm_i915_private *dev_priv, bool can_wait);
 void i915_guc_register(struct drm_i915_private *dev_priv);
 void i915_guc_unregister(struct drm_i915_private *dev_priv);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 
 #endif
-- 
1.9.2

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

[PATCH 16/19] drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer

[akash.goel]

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

Host needs to sample the GuC log buffer on every flush interrupt from GuC.
To ensure that we always get the up-to-date data from log buffer, its
better to access the buffer through an uncached CPU mapping. Also the way
buffer is accessed from GuC & Host side, manually doing cache flush may
not be effective always if cached CPU mapping is used.
Though there could be some performance implication with Uncached read, but
reliability of data will be ensured.

v2: Rebase.

v3: Rebase.

v4: Rebase.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 5a302fe..3e8af71 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1126,8 +1126,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 			dst_data_ptr += buffer_size;
 		}
 
-		/* FIXME: invalidate/flush for log buffer needed */
-
 		/* Update the read pointer in the shared log buffer */
 		log_buffer_state->read_ptr = write_offset;
 
@@ -1196,8 +1194,11 @@ static int guc_create_log_extras(struct intel_guc *guc)
 		return 0;
 
 	if (!guc->log.buf_addr) {
-		/* Create a vmalloc mapping of log buffer pages */
-		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WB);
+		/* Create a WC (Uncached for read) vmalloc mapping of log
+		 * buffer pages, so that we can directly get the data
+		 * (up-to-date) from memory.
+		 */
+		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
 		if (IS_ERR(vaddr)) {
 			ret = PTR_ERR(vaddr);
 			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
-- 
1.9.2

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

[PATCH 17/19] drm/i915: Use SSE4.1 movntdqa based memcpy for sampling GuC log buffer

[akash.goel]

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

In order to have fast reads from the GuC log buffer, used SSE4.1 movntdqa
based memcpy function i915_memcpy_from_wc.
GuC log buffer has a WC type vmalloc mapping and copying using movntqda
from WC type memory is almost as fast as reading from WB memory.
This will further reduce the log buffer sampling time, so is needed dearly
to deal with the flush interrupt storm when GuC is generating logs at a
very high rate.
Ideally SSE 4.1 should be present on all chipsets supporting GuC based
submisssions, but if not then logging will not be enabled.

v2: Rebase.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3e8af71..6b81fbd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1111,15 +1111,18 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 			/* Just copy the newly written data */
 			if (read_offset <= write_offset) {
 				bytes_to_copy = write_offset - read_offset;
-				memcpy(dst_data_ptr + read_offset,
-				       src_data_ptr + read_offset, bytes_to_copy);
+				i915_memcpy_from_wc(dst_data_ptr + read_offset,
+						    src_data_ptr + read_offset,
+						    bytes_to_copy);
 			} else {
 				bytes_to_copy = buffer_size - read_offset;
-				memcpy(dst_data_ptr + read_offset,
-				       src_data_ptr + read_offset, bytes_to_copy);
+				i915_memcpy_from_wc(dst_data_ptr + read_offset,
+						    src_data_ptr + read_offset,
+						    bytes_to_copy);
 
 				bytes_to_copy = write_offset;
-				memcpy(dst_data_ptr, src_data_ptr, bytes_to_copy);
+				i915_memcpy_from_wc(dst_data_ptr, src_data_ptr,
+						    bytes_to_copy);
 			}
 
 			src_data_ptr += buffer_size;
@@ -1241,6 +1244,16 @@ static void guc_create_log(struct intel_guc *guc)
 
 	vma = guc->log.vma;
 	if (!vma) {
+		/* We require SSE 4.1 for fast reads from the GuC log buffer and
+		 * it should be present on the chipsets supporting GuC based
+		 * submisssions.
+		 */
+		if (WARN_ON(!i915_memcpy_from_wc(NULL, NULL, 0))) {
+			/* logging will not be enabled */
+			i915.guc_log_level = -1;
+			return;
+		}
+
 		vma = guc_allocate_vma(guc, size);
 		if (IS_ERR(vma)) {
 			/* logging will be off */
-- 
1.9.2

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

[PATCH 18/19] drm/i915: Early creation of relay channel for capturing boot time logs

[akash.goel]

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

As per the current i915 Driver load sequence, debugfs registration is done
at the end and so the relay channel debugfs file is also created after that
but the GuC firmware is loaded much earlier in the sequence.
As a result Driver could miss capturing the boot-time logs of GuC firmware
if there are flush interrupts from the GuC side.
Relay has a provision to support early logging where initially only relay
channel can be created, to have buffers for storing logs, and later on
channel can be associated with a debugfs file at appropriate time.
Have availed that, which allows Driver to capture boot time logs also,
which can be collected once Userspace comes up.

v2:
- Remove the couple of FIXMEs, as now the relay channel will be created
  early before enabling the flush interrupts, so no possibility of relay
  channel pointer being modified & read at the same time from 2 different
  execution contexts.
- Rebase.

v3:
- Add a comment to justiy setting 'is_global' before the NULL check on the
  parent directory dentry pointer.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 73 ++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6b81fbd..ef0c116 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -896,15 +896,16 @@ static struct dentry *create_buf_file_callback(const char *filename,
 {
 	struct dentry *buf_file;
 
-	if (!parent)
-		return NULL;
-
 	/* 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 collect the logs in order without any post-processing.
+	 * Need to set 'is_global' even if parent is NULL for early logging.
 	 */
 	*is_global = 1;
 
+	if (!parent)
+		return NULL;
+
 	/* Not using the channel filename passed as an argument, since for each
 	 * channel relay appends the corresponding CPU number to the filename
 	 * passed in relay_open(). This should be fine as relay just needs a
@@ -937,13 +938,39 @@ static void guc_remove_log_relay_file(struct intel_guc *guc)
 	relay_close(guc->log.relay_chan);
 }
 
-static int guc_create_log_relay_file(struct intel_guc *guc)
+static int guc_create_relay_channel(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct rchan *guc_log_relay_chan;
-	struct dentry *log_dir;
 	size_t n_subbufs, subbuf_size;
 
+	/* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log.vma->obj->base.size;
+
+	/* Store up to 8 snapshots, which is large enough to buffer sufficient
+	 * boot time logs and provides enough leeway to User, in terms of
+	 * latency, for consuming the logs from relay. Also doesn't take
+	 * up too much memory.
+	 */
+	n_subbufs = 8;
+
+	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
+					n_subbufs, &relay_callbacks, dev_priv);
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
+		return -ENOMEM;
+	}
+
+	guc->log.relay_chan = guc_log_relay_chan;
+	return 0;
+}
+
+static int guc_create_log_relay_file(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct dentry *log_dir;
+	int ret;
+
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
 
@@ -963,25 +990,12 @@ static int guc_create_log_relay_file(struct intel_guc *guc)
 		return -ENODEV;
 	}
 
-	/* Keep the size of sub buffers same as shared log buffer */
-	subbuf_size = guc->log.vma->obj->base.size;
-
-	/* Store up to 8 snapshots, which is large enough to buffer sufficient
-	 * boot time logs and provides enough leeway to User, in terms of
-	 * latency, for consuming the logs from relay. Also doesn't take
-	 * up too much memory.
-	 */
-	n_subbufs = 8;
-
-	guc_log_relay_chan = relay_open("guc_log", log_dir, subbuf_size,
-					n_subbufs, &relay_callbacks, dev_priv);
-	if (!guc_log_relay_chan) {
-		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
-		return -ENOMEM;
+	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
+	if (ret) {
+		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
+		return ret;
 	}
 
-	/* FIXME: Cover the update under a lock ? */
-	guc->log.relay_chan = guc_log_relay_chan;
 	return 0;
 }
 
@@ -1001,7 +1015,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-	/* FIXME: Cover the check under a lock ? */
 	if (!guc->log.relay_chan)
 		return NULL;
 
@@ -1211,6 +1224,16 @@ static int guc_create_log_extras(struct intel_guc *guc)
 		guc->log.buf_addr = vaddr;
 	}
 
+	if (!guc->log.relay_chan) {
+		/* Create a relay channel, so that we have buffers for storing
+		 * the GuC firmware logs, the channel will be linked with a file
+		 * later on when debugfs is registered.
+		 */
+		ret = guc_create_relay_channel(guc);
+		if (ret)
+			return ret;
+	}
+
 	if (!guc->log.flush_wq) {
 		INIT_WORK(&guc->log.flush_work, guc_capture_logs_work);
 
@@ -1299,6 +1322,10 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	if (ret)
 		goto err;
 
+	/* Parent debugfs dir should be available by now, associate the already
+	 * opened relay channel with a debugfs file, which will then allow User
+	 * to pull the logs.
+	 */
 	ret = guc_create_log_relay_file(guc);
 	if (ret)
 		goto err;
-- 
1.9.2

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

[PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[akash.goel]

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

The GuC log buffer flush work item does a register access to send the ack
to GuC and this work item, if not synced before suspend, can potentially
get executed after the GFX device is suspended.
The work item function uses rpm_get/rpm_put calls around the Hw access,
this covers the runtime suspend case but for system suspend case (which can
be done asychronously/forcefully) sync would be required as kernel can
potentially schedule the work items even after some devices, including GFX,
have been put to suspend.
Also sync has to be done conditionally i.e. only for the system suspend
case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
suspend path.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
 drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
 drivers/gpu/drm/i915/intel_guc.h           | 2 +-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cdee60b..2ae0ad4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 		goto out;
 	}
 
-	intel_guc_suspend(dev);
+	intel_guc_suspend(dev, false);
 
 	intel_display_suspend(dev);
 
@@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
 	i915_gem_release_all_mmaps(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_suspend(dev);
+	intel_guc_suspend(dev, true);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ef0c116..1af8a8b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
  * intel_guc_suspend() - notify GuC entering suspend state
  * @dev:	drm device
  */
-int intel_guc_suspend(struct drm_device *dev)
+int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_guc *guc = &dev_priv->guc;
@@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
 		return 0;
 
 	gen9_disable_guc_interrupts(dev_priv);
+	/* Sync is needed only for the system suspend case, runtime suspend
+	 * case is covered due to rpm get/put calls used around Hw access in
+	 * the work item function.
+	 */
+	if (!rpm_suspend && (i915.guc_log_level >= 0))
+		flush_work(&dev_priv->guc.log.flush_work);
 
 	ctx = dev_priv->kernel_context;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b56fe24..1367314 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -172,7 +172,7 @@ extern void intel_guc_init(struct drm_device *dev);
 extern int intel_guc_setup(struct drm_device *dev);
 extern void intel_guc_fini(struct drm_device *dev);
 extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
-extern int intel_guc_suspend(struct drm_device *dev);
+extern int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend);
 extern int intel_guc_resume(struct drm_device *dev);
 
 /* i915_guc_submission.c */
-- 
1.9.2

_______________________________________________
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 (rev8)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test gem_exec_gttfill:
        Subgroup basic:
                skip       -> PASS       (fi-snb-i7-2600)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:219  dwarn:2   dfail:0   fail:1   skip:18 
ro-bdw-i7-5557U  total:240  pass:220  dwarn:2   dfail:0   fail:0   skip:18 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
fi-kbl-qkkr failed to connect after reboot
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1897/

a363b72 drm-intel-nightly: 2016y-08m-17d-05h-29m-02s UTC integration manifest
2d22ed9 drm/i915: Sync against the GuC log buffer flush work item on system suspend
8171314 drm/i915: Early creation of relay channel for capturing boot time logs
101f5714 drm/i915: Use SSE4.1 movntdqa based memcpy for sampling GuC log buffer
68b244c drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer
9ecee71 drm/i915: Debugfs support for GuC logging control
7016c58 drm/i915: Forcefully flush GuC log buffer on reset
2cc8d8e drm/i915: Augment i915 error state to include the dump of GuC log buffer
290455d drm/i915: Increase GuC log buffer size to reduce flush interrupts
e1e37e6 drm/i915: Optimization to reduce the sampling time of GuC log buffer
f5bc1bf drm/i915: Add stats for GuC log buffer flush interrupts
0253078 drm/i915: New lock to serialize the Host2GuC actions
55bcc15 drm/i915: Add a relay backed debugfs interface for capturing GuC logs
37c2f5c relay: Use per CPU constructs for the relay channel buffer pointers
65d75b2 drm/i915: Handle log buffer flush interrupt event from GuC
7a03b11 drm/i915: Support for GuC interrupts
fab69f8 drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
0f9eca5 drm/i915: New structure to contain GuC logging related fields
6a87cfe drm/i915: Add GuC ukernel logging related fields to fw interface file
3bdb775 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: [PATCH 05/19] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> There are certain types of interrupts which Host can receive 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.
>
> v2:
> - Use common low level routines for PM IER/IIR programming (Chris)
> - Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
> - Replace disabling of wake ref asserts with rpm get/put (Chris)
>
> v3:
> - Update comments for more clarity. (Tvrtko)
> - Remove the masking of GuC interrupt, which was kept masked till the
>    start of bottom half, its not really needed as there is only a
>    single instance of work item & wq is ordered. (Tvrtko)
>
> v4:
> - Rebase.
> - Rename guc_events to pm_guc_events so as to be indicative of the
>    register/control block it is associated with. (Chris)
> - Add handling for back to back log buffer flush interrupts.
>
> v5:
> - Move the read & clearing of register, containing Guc2Host message
>    bits, outside the irq spinlock. (Tvrtko)
>
> v6:
> - Move the log buffer flush interrupt related stuff to the following
>    patch so as to do only generic bits in this patch. (Tvrtko)
> - Rebase.
>
> v7:
> - Remove the interrupts_enabled check from gen9_guc_irq_handler, want to
>    process that last interrupt also before disabling the interrupt, sync
>    against the work queued by irq handler will be done by caller disabling
>    the 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/i915_drv.h            |  1 +
>   drivers/gpu/drm/i915/i915_guc_submission.c |  5 +++
>   drivers/gpu/drm/i915/i915_irq.c            | 56 +++++++++++++++++++++++++++---
>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++++
>   drivers/gpu/drm/i915/intel_drv.h           |  3 ++
>   drivers/gpu/drm/i915/intel_guc.h           |  3 ++
>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 +++
>   7 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a187ea7..08553b9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1783,6 +1783,7 @@ struct drm_i915_private {
>   	u32 pm_imr;
>   	u32 pm_ier;
>   	u32 pm_rps_events;
> +	u32 pm_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 34fb893..b062da6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1055,6 +1055,8 @@ int intel_guc_suspend(struct drm_device *dev)
>   	if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>   		return 0;
>
> +	gen9_disable_guc_interrupts(dev_priv);
> +
>   	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
> @@ -1081,6 +1083,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)
> +		gen9_enable_guc_interrupts(dev_priv);
> +
>   	ctx = dev_priv->kernel_context;
>
>   	data[0] = HOST2GUC_ACTION_EXIT_S_STATE;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 83d708a..fc1fe72 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 gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>
>   /* For display hotplug interrupt */
>   static inline void
> @@ -411,6 +412,38 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	gen6_reset_rps_interrupts(dev_priv);
>   }
>
> +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> +				       dev_priv->pm_guc_events);
> +		dev_priv->guc.interrupts_enabled = true;
> +		gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +	}
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->guc.interrupts_enabled = false;
> +
> +	gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +	synchronize_irq(dev_priv->drm.irq);
> +
> +	gen9_reset_guc_interrupts(dev_priv);
> +}
> +
>   /**
>    * bdw_update_port_irq - update DE port interrupt
>    * @dev_priv: driver private
> @@ -1339,11 +1372,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->pm_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->pm_guc_events));
>   			ret = IRQ_HANDLED;
>   		} else
>   			DRM_ERROR("The master control interrupt lied (PM)!\n");
> @@ -1375,6 +1410,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->pm_guc_events)
> +		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>   }
>
>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1621,6 +1659,13 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   	}
>   }
>
> +static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
> +{
> +	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> +		/* TODO: Handle events for which GuC interrupted host */
> +	}
> +}
> +
>   static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
>   				     enum pipe pipe)
>   {
> @@ -3722,7 +3767,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>   	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>   	/*
>   	 * RPS interrupts will get enabled/disabled on demand when RPS itself
> -	 * is enabled/disabled.
> +	 * is enabled/disabled. Same wil be the case for GuC interrupts.
>   	 */
>   	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_imr, dev_priv->pm_ier);
>   	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
> @@ -4508,6 +4553,9 @@ 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);
>
> +	if (HAS_GUC_SCHED(dev))
> +		dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
> +
>   	/* Let's track the enabled rps events */
>   	if (IS_VALLEYVIEW(dev_priv))
>   		/* WaGsvRC0ResidencyMethod:vlv */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d4adf28..cab089a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6011,6 +6011,7 @@ enum {
>   #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)
> @@ -6022,6 +6023,16 @@ enum {
>   #define GEN8_GT_IIR(which) _MMIO(0x44308 + (0x10 * (which)))
>   #define GEN8_GT_IER(which) _MMIO(0x4430c + (0x10 * (which)))
>
> +#define GEN9_GUC_TO_HOST_INT_EVENT	(1<<31)
> +#define GEN9_GUC_EXEC_ERROR_EVENT	(1<<30)
> +#define GEN9_GUC_DISPLAY_EVENT		(1<<29)
> +#define GEN9_GUC_SEMA_SIGNAL_EVENT	(1<<28)
> +#define GEN9_GUC_IOMMU_MSG_EVENT	(1<<27)
> +#define GEN9_GUC_DB_RING_EVENT		(1<<26)
> +#define GEN9_GUC_DMA_DONE_EVENT		(1<<25)
> +#define GEN9_GUC_FATAL_ERROR_EVENT	(1<<24)
> +#define GEN9_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 9b99a84..f7f16ac 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1120,6 +1120,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 gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> +void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> +void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv);
>
>   /* 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 d32023c..1fc63fe 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -130,6 +130,9 @@ struct intel_guc {
>   	struct intel_guc_fw guc_fw;
>   	struct intel_guc_log log;
>
> +	/* GuC2Host interrupt related state */
> +	bool interrupts_enabled;
> +
>   	struct i915_vma *ads_vma;
>   	struct i915_vma *ctx_pool_vma;
>   	struct ida ctx_ids;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 6fd24d9..ef35e99 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -464,6 +464,7 @@ int intel_guc_setup(struct drm_device *dev)
>   	}
>
>   	direct_interrupts_to_host(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
>
>   	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> @@ -510,6 +511,9 @@ int intel_guc_setup(struct drm_device *dev)
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
>   		err = i915_guc_submission_enable(dev_priv);
>   		if (err)
>   			goto fail;
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 06/19] drm/i915: Handle log buffer flush interrupt event from GuC

[Tvrtko Ursulin]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> 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.
>
> v2:
> - Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
> - Reduce the overall log buffer copying time by skipping the copy of
>    crash buffer area for regular cases and copying only the state
>    structure data in first page.
>
> v3:
>   - Create a vmalloc mapping of log buffer. (Chris)
>   - Cover the flush acknowledgment under rpm get & put.(Chris)
>   - Revert the change of skipping the copy of crash dump area, as
>     not really needed, will be covered by subsequent patch.
>
> v4:
>   - Destroy the wq under the same condition in which it was created,
>     pass dev_piv pointer instead of dev to newly added GuC function,
>     add more comments & rename variable for clarity. (Tvrtko)
>
> v5:
> - Allocate & destroy the dedicated wq, for handling flush interrupt,
>    from the setup/teardown routines of GuC logging. (Chris)
> - Validate the log buffer size value retrieved from state structure
>    and do some minor cleanup. (Tvrtko)
> - Fix error/warnings reported by checkpatch. (Tvrtko)
> - Rebase.
>
> v6:
>   - Remove the interrupts_enabled check from guc_capture_logs_work, need
>     to process that last work item also, queued just before disabling the
>     interrupt as log buffer flush interrupt handling is a bit different
>     case where GuC is actually expecting an ACK from host, which should be
>     provided to keep the logging going.
>     Sync against the work will be done by caller disabling the interrupt.
>   - Don't sample the log buffer size value from state structure, directly
>     use the expected value to move the pointer & do the copy and that cannot
>     go wrong (out of bounds) as Driver only allocated the log buffer and the
>     relay buffers. Driver should refrain from interpreting the log packet,
>     as much possible and let Userspace parser detect the anomaly. (Chris)
>
> 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 | 186 +++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_irq.c            |  28 ++++-
>   drivers/gpu/drm/i915/intel_guc.h           |   4 +
>   3 files changed, 217 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index b062da6..ade51cb 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
>   	return host2guc_action(guc, data, ARRAY_SIZE(data));
>   }
>
> +static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
> +{
> +	u32 data[1];
> +
> +	data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
> +
> +	return host2guc_action(guc, data, 1);
> +}
> +
>   /*
>    * Initialise, update, or clear doorbell data shared with the GuC
>    *
> @@ -828,6 +837,163 @@ err:
>   	return NULL;
>   }
>
> +static void guc_move_to_next_buf(struct intel_guc *guc)
> +{
> +}
> +
> +static void *guc_get_write_buffer(struct intel_guc *guc)
> +{
> +	return NULL;
> +}
> +
> +static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type type)
> +{
> +	if (type == GUC_ISR_LOG_BUFFER)
> +		return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
> +	else if (type == GUC_DPC_LOG_BUFFER)
> +		return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
> +	else
> +		return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
> +}

Could do it with a switch statement to get automatic reminder of size 
not being handled if some day a new log buffer type gets added. It would 
probably more in the style of the rest of the driver as well.

> +
> +static void guc_read_update_log_buffer(struct intel_guc *guc)
> +{
> +	struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
> +	struct guc_log_buffer_state log_buffer_state_local;
> +	void *src_data_ptr, *dst_data_ptr;
> +	unsigned int buffer_size;
> +	enum guc_log_buffer_type type;
> +
> +	if (WARN_ON(!guc->log.buf_addr))
> +		return;
> +
> +	/* Get the pointer to shared GuC log buffer */
> +	log_buffer_state = src_data_ptr = guc->log.buf_addr;
> +
> +	/* Get the pointer to local buffer to store the logs */
> +	dst_data_ptr = log_buffer_snapshot_state = guc_get_write_buffer(guc);
> +
> +	/* Actual logs are present from the 2nd page */
> +	src_data_ptr += PAGE_SIZE;
> +	dst_data_ptr += PAGE_SIZE;
> +
> +	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
> +		/* Make a copy of the state structure in GuC log buffer (which
> +		 * is uncached mapped) on the stack to avoid reading from it
> +		 * multiple times.
> +		 */
> +		memcpy(&log_buffer_state_local, log_buffer_state,
> +		       sizeof(struct guc_log_buffer_state));
> +		buffer_size = guc_get_log_buffer_size(type);
> +
> +		if (log_buffer_snapshot_state) {
> +			/* First copy the state structure in snapshot buffer */
> +			memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
> +			       sizeof(struct guc_log_buffer_state));

I've noticed now log_buffer_state_local is used only for the 
sample_write_ptr below and otherwise it causes two copies of the same data.

Since this branch is an interesting one, you could avoid one copy by 
updating the log_buffer_state->read_ptr from the 
log_buffer_snapshot_state in this branch, and add an else branch to 
update it directly from log_buffer_state when log_buffer_snapshot_state 
is not available.

Sounds like it would be worth eliminating double memcpy, what do you think>

> +
> +			/* 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_snapshot_state->write_ptr =
> +				log_buffer_snapshot_state->sampled_write_ptr;
> +
> +			log_buffer_snapshot_state++;
> +
> +			/* Now copy the actual logs. */
> +			memcpy(dst_data_ptr, src_data_ptr, buffer_size);
> +
> +			src_data_ptr += buffer_size;
> +			dst_data_ptr += buffer_size;
> +		}
> +
> +		/* FIXME: invalidate/flush for log buffer needed */
> +
> +		/* Update the read pointer in the shared log buffer */
> +		log_buffer_state->read_ptr =
> +			log_buffer_state_local.sampled_write_ptr;
> +
> +		/* Clear the 'flush to file' flag */
> +		log_buffer_state->flush_to_file = 0;
> +		log_buffer_state++;
> +	}
> +
> +	if (log_buffer_snapshot_state)
> +		guc_move_to_next_buf(guc);
> +}
> +
> +static void guc_capture_logs_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, guc.log.flush_work);
> +
> +	i915_guc_capture_logs(dev_priv);
> +}
> +
> +static void guc_log_cleanup(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> +	if (i915.guc_log_level < 0)
> +		return;
> +
> +	/* First disable the flush interrupt */
> +	gen9_disable_guc_interrupts(dev_priv);
> +
> +	if (guc->log.flush_wq)
> +		destroy_workqueue(guc->log.flush_wq);
> +
> +	guc->log.flush_wq = NULL;
> +
> +	if (guc->log.buf_addr)
> +		i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> +	guc->log.buf_addr = NULL;
> +}
> +
> +static int guc_create_log_extras(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	void *vaddr;
> +	int ret;
> +
> +	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> +	/* Nothing to do */
> +	if (i915.guc_log_level < 0)
> +		return 0;
> +
> +	if (!guc->log.buf_addr) {
> +		/* Create a vmalloc mapping of log buffer pages */
> +		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WB);
> +		if (IS_ERR(vaddr)) {
> +			ret = PTR_ERR(vaddr);
> +			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> +			return ret;
> +		}
> +
> +		guc->log.buf_addr = vaddr;
> +	}
> +
> +	if (!guc->log.flush_wq) {
> +		INIT_WORK(&guc->log.flush_work, guc_capture_logs_work);
> +
> +		/* Need a dedicated wq to process log buffer flush interrupts
> +		 * from GuC without much delay so as to avoid any loss of logs.
> +		 */
> +		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log", 0);
> +		if (guc->log.flush_wq == NULL) {
> +			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static void guc_create_log(struct intel_guc *guc)
>   {
>   	struct i915_vma *vma;
> @@ -853,6 +1019,13 @@ static void guc_create_log(struct intel_guc *guc)
>   		}
>
>   		guc->log.vma = vma;
> +
> +		if (guc_create_log_extras(guc)) {
> +			guc_log_cleanup(guc);
> +			i915_vma_unpin_and_release(&guc->log.vma);
> +			i915.guc_log_level = -1;
> +			return;
> +		}
>   	}
>
>   	/* each allocated unit is a page */
> @@ -1034,6 +1207,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>
>   	i915_vma_unpin_and_release(&guc->ads_vma);
> +	guc_log_cleanup(guc);
>   	i915_vma_unpin_and_release(&guc->log.vma);
>
>   	if (guc->ctx_pool_vma)
> @@ -1095,3 +1269,15 @@ int intel_guc_resume(struct drm_device *dev)
>
>   	return host2guc_action(guc, data, ARRAY_SIZE(data));
>   }
> +
> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
> +{
> +	guc_read_update_log_buffer(&dev_priv->guc);
> +
> +	/* Generally device is expected to be active only at this
> +	 * time, so get/put should be really quick.
> +	 */
> +	intel_runtime_pm_get(dev_priv);
> +	host2guc_logbuffer_flush_complete(&dev_priv->guc);
> +	intel_runtime_pm_put(dev_priv);
> +}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fc1fe72..19c0078 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1662,7 +1662,33 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>   static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
>   {
>   	if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> -		/* TODO: Handle events for which GuC interrupted host */
> +		/* Sample the log buffer flush related bits & clear them out now
> +		 * itself from the message identity register to minimize the
> +		 * probability of losing a flush interrupt, when there are back
> +		 * to back flush interrupts.
> +		 * There can be a new flush interrupt, for different log buffer
> +		 * type (like for ISR), whilst Host is handling one (for DPC).
> +		 * Since same bit is used in message register for ISR & DPC, it
> +		 * could happen that GuC sets the bit for 2nd interrupt but Host
> +		 * clears out the bit on handling the 1st interrupt.
> +		 */
> +		u32 msg, flush;
> +
> +		msg = I915_READ(SOFT_SCRATCH(15));
> +		flush = msg & (GUC2HOST_MSG_CRASH_DUMP_POSTED |
> +			       GUC2HOST_MSG_FLUSH_LOG_BUFFER);
> +		if (flush) {
> +			/* Clear the message bits that are handled */
> +			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
> +
> +			/* Handle flush interrupt in bottom half */
> +			queue_work(dev_priv->guc.log.flush_wq,
> +				   &dev_priv->guc.log.flush_work);
> +		} else {
> +			/* Not clearing of unhandled event bits won't result in
> +			 * re-triggering of the interrupt.
> +			 */
> +		}
>   	}
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 1fc63fe..d053a18 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 {
>   	uint32_t flags;
>   	struct i915_vma *vma;
> +	void *buf_addr;
> +	struct workqueue_struct *flush_wq;
> +	struct work_struct flush_work;
>   };
>
>   struct intel_guc {
> @@ -167,5 +170,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
>   int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
>
>   #endif
>

Otherwise looks OK to me.

Regards,

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

Re: [PATCH 08/19] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

[Tvrtko Ursulin]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> Added a new debugfs interface '/sys/kernel/debug/dri/guc_log' for the
> User to capture GuC firmware logs. Availed relay framework to implement
> the interface, where Driver will have to just use a relay API to store
> snapshots of the GuC log buffer in the buffer managed by relay.
> The snapshot will be taken when GuC firmware sends a log buffer flush
> interrupt and up to four snapshots could be stored in the relay buffer.
> The relay buffer will be operated in a mode where it will overwrite the
> data not yet collected by User.
> Besides mmap method, through which User can directly access the relay
> buffer contents, relay also supports the 'poll' method. Through the 'poll'
> call on log file, User can come to know whenever a new snapshot of the
> log buffer is taken by Driver, so can run in tandem with the Driver and
> capture the logs in a sustained/streaming manner, without any loss of data.
>
> v2: Defer the creation of relay channel & associated debugfs file, as
>      debugfs setup is now done at the end of i915 Driver load. (Chris)
>
> v3:
> - Switch to no-overwrite mode for relay.
> - Fix the relay sub buffer switching sequence.
>
> v4:
> - Update i915 Kconfig to select RELAY config. (TvrtKo)
> - Log a message when there is no sub buffer available to capture
>    the GuC log buffer. (Tvrtko)
> - Increase the number of relay sub buffers to 8 from 4, to have
>    sufficient buffering for boot time logs
>
> v5:
> - Fix the alignment, indentation issues and some minor cleanup. (Tvrtko)
> - Update the comment to elaborate on why a relay channel has to be
>    associated with the debugfs file. (Tvrtko)
>
> v6:
> - Move the write to 'is_global' after the NULL check on parent directory
>    dentry pointer. (Tvrtko)
>
> 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/Kconfig               |   1 +
>   drivers/gpu/drm/i915/i915_drv.c            |   2 +
>   drivers/gpu/drm/i915/i915_guc_submission.c | 212 ++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_guc.h           |   3 +
>   4 files changed, 216 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 7769e46..fc900d2 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -11,6 +11,7 @@ config DRM_I915
>   	select DRM_KMS_HELPER
>   	select DRM_PANEL
>   	select DRM_MIPI_DSI
> +	select RELAY
>   	# i915 depends on ACPI_VIDEO when ACPI is enabled
>   	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
>   	select BACKLIGHT_LCD_SUPPORT if ACPI
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 13ae340..cdee60b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1133,6 +1133,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   	/* Reveal our presence to userspace */
>   	if (drm_dev_register(dev, 0) == 0) {
>   		i915_debugfs_register(dev_priv);
> +		i915_guc_register(dev_priv);
>   		i915_setup_sysfs(dev);
>   	} else
>   		DRM_ERROR("Failed to register driver for userspace access!\n");
> @@ -1171,6 +1172,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>   	intel_opregion_unregister(dev_priv);
>
>   	i915_teardown_sysfs(&dev_priv->drm);
> +	i915_guc_unregister(dev_priv);
>   	i915_debugfs_unregister(dev_priv);
>   	drm_dev_unregister(&dev_priv->drm);
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index ade51cb..1713ce8 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,13 +839,159 @@ err:
>   	return NULL;
>   }
>
> +/*
> + * Sub buffer switch callback. Called whenever relay has to switch to a new
> + * sub buffer, relay stays on the same sub buffer if 0 is returned.
> + */
> +static int subbuf_start_callback(struct rchan_buf *buf,
> +				 void *subbuf,
> +				 void *prev_subbuf,
> +				 size_t prev_padding)
> +{
> +	/* Use no-overwrite mode by default, where relay will stop accepting
> +	 * new data if there are no empty sub buffers left.
> +	 * There is no strict synchronization enforced by relay between Consumer
> +	 * and Producer. In overwrite mode, there is a possibility of getting
> +	 * inconsistent/garbled data, the producer could be writing on to the
> +	 * same sub buffer from which Consumer is reading. This can't be avoided
> +	 * unless Consumer is fast enough and can always run in tandem with
> +	 * Producer.
> +	 */
> +	if (relay_buf_full(buf))
> +		return 0;
> +
> +	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)
> +{
> +	struct dentry *buf_file;
> +
> +	if (!parent)
> +		return NULL;
> +
> +	/* 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 collect the logs in order without any post-processing.
> +	 */
> +	*is_global = 1;
> +
> +	/* Not using the channel filename passed as an argument, since for each
> +	 * channel relay appends the corresponding CPU number to the filename
> +	 * passed in relay_open(). This should be fine as relay just needs a
> +	 * dentry of the file associated with the channel buffer and that file's
> +	 * name need not be same as the filename passed as an argument.
> +	 */
> +	buf_file = debugfs_create_file("guc_log", mode,
> +				       parent, buf, &relay_file_operations);
> +	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 int guc_create_log_relay_file(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct rchan *guc_log_relay_chan;
> +	struct dentry *log_dir;
> +	size_t n_subbufs, subbuf_size;
> +
> +	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
> +	log_dir = dev_priv->drm.primary->debugfs_root;
> +
> +	/* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is
> +	 * not mounted and so can't create the relay file.
> +	 * The relay API seems to fit well with debugfs only, for availing relay
> +	 * there are 3 requirements which can be met for debugfs file only in a
> +	 * straightforward/clean manner :-
> +	 * i)   Need the associated dentry pointer of the file, while opening the
> +	 *      relay channel.
> +	 * ii)  Should be able to use 'relay_file_operations' fops for the file.
> +	 * iii) Set the 'i_private' field of file's inode to the pointer of
> +	 *	relay channel buffer.
> +	 */
> +	if (!log_dir) {
> +		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Keep the size of sub buffers same as shared log buffer */
> +	subbuf_size = guc->log.vma->obj->base.size;
> +
> +	/* Store up to 8 snapshots, which is large enough to buffer sufficient
> +	 * boot time logs and provides enough leeway to User, in terms of
> +	 * latency, for consuming the logs from relay. Also doesn't take
> +	 * up too much memory.
> +	 */
> +	n_subbufs = 8;
> +
> +	guc_log_relay_chan = relay_open("guc_log", log_dir, subbuf_size,
> +					n_subbufs, &relay_callbacks, dev_priv);
> +	if (!guc_log_relay_chan) {
> +		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* FIXME: Cover the update under a lock ? */
> +	guc->log.relay_chan = guc_log_relay_chan;
> +	return 0;
> +}
> +
>   static void guc_move_to_next_buf(struct intel_guc *guc)
>   {
> +	/* Make sure the updates made in the sub buffer are visible when
> +	 * Consumer sees the following update to offset inside the sub buffer.
> +	 */
> +	smp_wmb();
> +
> +	/* All data has been written, so now move the offset of sub buffer. */
> +	relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
> +
> +	/* Switch to the next sub buffer */
> +	relay_flush(guc->log.relay_chan);
>   }
>
>   static void *guc_get_write_buffer(struct intel_guc *guc)
>   {
> -	return NULL;
> +	/* FIXME: Cover the check under a lock ? */
> +	if (!guc->log.relay_chan)
> +		return NULL;
> +
> +	/* Just get the base address of a new sub buffer and copy data into it
> +	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
> +	 * buffers are full. Could have used the relay_write() to indirectly
> +	 * copy the data, but that would have been bit convoluted, as we need to
> +	 * write to only certain locations inside a sub buffer which cannot be
> +	 * done without using relay_reserve() along with relay_write(). So its
> +	 * better to use relay_reserve() alone.
> +	 */
> +	return relay_reserve(guc->log.relay_chan, 0);
>   }
>
>   static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type type)
> @@ -921,6 +1069,12 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>
>   	if (log_buffer_snapshot_state)
>   		guc_move_to_next_buf(guc);
> +	else {
> +		/* Used rate limited to avoid deluge of messages, logs might be
> +		 * getting consumed by User at a slow rate.
> +		 */
> +		DRM_ERROR_RATELIMITED("no sub-buffer to capture log buffer\n");
> +	}
>   }
>
>   static void guc_capture_logs_work(struct work_struct *work)
> @@ -948,6 +1102,11 @@ static void guc_log_cleanup(struct intel_guc *guc)
>
>   	guc->log.flush_wq = NULL;
>
> +	if (guc->log.relay_chan)
> +		guc_remove_log_relay_file(guc);
> +
> +	guc->log.relay_chan = NULL;
> +
>   	if (guc->log.buf_addr)
>   		i915_gem_object_unpin_map(guc->log.vma->obj);
>
> @@ -1038,6 +1197,36 @@ static void guc_create_log(struct intel_guc *guc)
>   	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
>   }
>
> +static int guc_log_late_setup(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	int ret;
> +
> +	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> +	if (i915.guc_log_level < 0)
> +		return -EINVAL;
> +
> +	/* If log_level was set as -1 at boot time, then setup needed to
> +	 * handle log buffer flush interrupts would not have been done yet,
> +	 * so do that now.
> +	 */
> +	ret = guc_create_log_extras(guc);
> +	if (ret)
> +		goto err;
> +
> +	ret = guc_create_log_relay_file(guc);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	guc_log_cleanup(guc);
> +	/* logging will remain off */
> +	i915.guc_log_level = -1;
> +	return ret;
> +}
> +
>   static void init_guc_policies(struct guc_policies *policies)
>   {
>   	struct guc_policy *policy;
> @@ -1207,7 +1396,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>
>   	i915_vma_unpin_and_release(&guc->ads_vma);
> -	guc_log_cleanup(guc);
>   	i915_vma_unpin_and_release(&guc->log.vma);
>
>   	if (guc->ctx_pool_vma)
> @@ -1281,3 +1469,23 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
>   	host2guc_logbuffer_flush_complete(&dev_priv->guc);
>   	intel_runtime_pm_put(dev_priv);
>   }
> +
> +void i915_guc_unregister(struct drm_i915_private *dev_priv)
> +{
> +	if (!i915.enable_guc_submission)
> +		return;
> +
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +	guc_log_cleanup(&dev_priv->guc);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +}
> +
> +void i915_guc_register(struct drm_i915_private *dev_priv)
> +{
> +	if (!i915.enable_guc_submission)
> +		return;
> +
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +	guc_log_late_setup(&dev_priv->guc);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index d053a18..3299cce 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -127,6 +127,7 @@ struct intel_guc_log {
>   	void *buf_addr;
>   	struct workqueue_struct *flush_wq;
>   	struct work_struct flush_work;
> +	struct rchan *relay_chan;
>   };
>
>   struct intel_guc {
> @@ -171,5 +172,7 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>   void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
> +void i915_guc_register(struct drm_i915_private *dev_priv);
> +void i915_guc_unregister(struct drm_i915_private *dev_priv);
>
>   #endif
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


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

Re: [PATCH 14/19] drm/i915: Forcefully flush GuC log buffer on reset

[Tvrtko Ursulin]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> Before capturing the GuC logs as a part of error state, there should be a
> force log buffer flush action sent to GuC before proceeding with GPU reset
> and re-initializing GUC. There could be some data in the log buffer which
> is yet to be captured and those logs would be particularly useful to
> understand that why the GPU reset was initiated.
>
> v2:
> - Avoid the wait via flush_work, to serialize against an ongoing log
>    buffer flush, from the error state capture path. (Chris)
> - Rebase.
>
> 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_gpu_error.c      |  2 ++
>   drivers/gpu/drm/i915/i915_guc_submission.c | 30 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   3 files changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 94297aa..b73c671 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1301,6 +1301,8 @@ static void i915_gem_capture_guc_log_buffer(struct drm_i915_private *dev_priv,
>   	if (!dev_priv->guc.log.vma || (i915.guc_log_level < 0))
>   		return;
>
> +	i915_guc_flush_logs(dev_priv, false);
> +
>   	error->guc_log = i915_error_object_create(dev_priv,
>   						  dev_priv->guc.log.vma);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index e46828b..beeeb23 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -185,6 +185,16 @@ static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
>   	return host2guc_action(guc, data, 1);
>   }
>
> +static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
> +{
> +	u32 data[2];
> +
> +	data[0] = HOST2GUC_ACTION_FORCE_LOG_BUFFER_FLUSH;
> +	data[1] = 0;
> +
> +	return host2guc_action(guc, data, 2);
> +}
> +
>   /*
>    * Initialise, update, or clear doorbell data shared with the GuC
>    *
> @@ -1521,6 +1531,26 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
>   	intel_runtime_pm_put(dev_priv);
>   }
>
> +void i915_guc_flush_logs(struct drm_i915_private *dev_priv, bool can_wait)
> +{
> +	if (!i915.enable_guc_submission || (i915.guc_log_level < 0))
> +		return;
> +
> +	/* First disable the interrupts, will be renabled afterwards */
> +	gen9_disable_guc_interrupts(dev_priv);
> +
> +	/* Before initiating the forceful flush, wait for any pending/ongoing
> +	 * flush to complete otherwise forceful flush may not happen, but wait
> +	 * can't be done for some paths like error state capture in which case
> +	 * take a chance & directly attempt the forceful flush.
> +	 */
> +	if (can_wait)
> +		flush_work(&dev_priv->guc.log.flush_work);
> +
> +	/* Ask GuC to update the log buffer state */
> +	host2guc_force_logbuffer_flush(&dev_priv->guc);
> +}
> +
>   void i915_guc_unregister(struct drm_i915_private *dev_priv)
>   {
>   	if (!i915.enable_guc_submission)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 8598f38..d7eda42 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -182,6 +182,7 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>   void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
> +void i915_guc_flush_logs(struct drm_i915_private *dev_priv, bool can_wait);
>   void i915_guc_register(struct drm_i915_private *dev_priv);
>   void i915_guc_unregister(struct drm_i915_private *dev_priv);
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH 06/19] drm/i915: Handle log buffer flush interrupt event from GuC

[Goel, Akash]

On 8/17/2016 4:37 PM, Tvrtko Ursulin wrote:
>
> On 17/08/16 11:14, akash.goel@intel.com wrote:
>> 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.
>>
>> v2:
>> - Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
>> - Reduce the overall log buffer copying time by skipping the copy of
>>    crash buffer area for regular cases and copying only the state
>>    structure data in first page.
>>
>> v3:
>>   - Create a vmalloc mapping of log buffer. (Chris)
>>   - Cover the flush acknowledgment under rpm get & put.(Chris)
>>   - Revert the change of skipping the copy of crash dump area, as
>>     not really needed, will be covered by subsequent patch.
>>
>> v4:
>>   - Destroy the wq under the same condition in which it was created,
>>     pass dev_piv pointer instead of dev to newly added GuC function,
>>     add more comments & rename variable for clarity. (Tvrtko)
>>
>> v5:
>> - Allocate & destroy the dedicated wq, for handling flush interrupt,
>>    from the setup/teardown routines of GuC logging. (Chris)
>> - Validate the log buffer size value retrieved from state structure
>>    and do some minor cleanup. (Tvrtko)
>> - Fix error/warnings reported by checkpatch. (Tvrtko)
>> - Rebase.
>>
>> v6:
>>   - Remove the interrupts_enabled check from guc_capture_logs_work, need
>>     to process that last work item also, queued just before disabling the
>>     interrupt as log buffer flush interrupt handling is a bit different
>>     case where GuC is actually expecting an ACK from host, which
>> should be
>>     provided to keep the logging going.
>>     Sync against the work will be done by caller disabling the interrupt.
>>   - Don't sample the log buffer size value from state structure, directly
>>     use the expected value to move the pointer & do the copy and that
>> cannot
>>     go wrong (out of bounds) as Driver only allocated the log buffer
>> and the
>>     relay buffers. Driver should refrain from interpreting the log
>> packet,
>>     as much possible and let Userspace parser detect the anomaly. (Chris)
>>
>> 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 | 186
>> +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_irq.c            |  28 ++++-
>>   drivers/gpu/drm/i915/intel_guc.h           |   4 +
>>   3 files changed, 217 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index b062da6..ade51cb 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct
>> intel_guc *guc,
>>       return host2guc_action(guc, data, ARRAY_SIZE(data));
>>   }
>>
>> +static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
>> +{
>> +    u32 data[1];
>> +
>> +    data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
>> +
>> +    return host2guc_action(guc, data, 1);
>> +}
>> +
>>   /*
>>    * Initialise, update, or clear doorbell data shared with the GuC
>>    *
>> @@ -828,6 +837,163 @@ err:
>>       return NULL;
>>   }
>>
>> +static void guc_move_to_next_buf(struct intel_guc *guc)
>> +{
>> +}
>> +
>> +static void *guc_get_write_buffer(struct intel_guc *guc)
>> +{
>> +    return NULL;
>> +}
>> +
>> +static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type
>> type)
>> +{
>> +    if (type == GUC_ISR_LOG_BUFFER)
>> +        return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
>> +    else if (type == GUC_DPC_LOG_BUFFER)
>> +        return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
>> +    else
>> +        return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
>> +}
>
> Could do it with a switch statement to get automatic reminder of size
> not being handled if some day a new log buffer type gets added. It would
> probably more in the style of the rest of the driver as well.
>
Fine will use switch statement here.

Should I use BUG_ON for the default/unhandled case ?
	case GUC_ISR_LOG_BUFFER
	
	case GUC_DPC_LOG_BUFFER

	case GUC_LOG_CRASH_PAGES

	default
		BUG_ON(1)

>> +
>> +static void guc_read_update_log_buffer(struct intel_guc *guc)
>> +{
>> +    struct guc_log_buffer_state *log_buffer_state,
>> *log_buffer_snapshot_state;
>> +    struct guc_log_buffer_state log_buffer_state_local;
>> +    void *src_data_ptr, *dst_data_ptr;
>> +    unsigned int buffer_size;
>> +    enum guc_log_buffer_type type;
>> +
>> +    if (WARN_ON(!guc->log.buf_addr))
>> +        return;
>> +
>> +    /* Get the pointer to shared GuC log buffer */
>> +    log_buffer_state = src_data_ptr = guc->log.buf_addr;
>> +
>> +    /* Get the pointer to local buffer to store the logs */
>> +    dst_data_ptr = log_buffer_snapshot_state =
>> guc_get_write_buffer(guc);
>> +
>> +    /* Actual logs are present from the 2nd page */
>> +    src_data_ptr += PAGE_SIZE;
>> +    dst_data_ptr += PAGE_SIZE;
>> +
>> +    for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
>> +        /* Make a copy of the state structure in GuC log buffer (which
>> +         * is uncached mapped) on the stack to avoid reading from it
>> +         * multiple times.
>> +         */
>> +        memcpy(&log_buffer_state_local, log_buffer_state,
>> +               sizeof(struct guc_log_buffer_state));
>> +        buffer_size = guc_get_log_buffer_size(type);
>> +
>> +        if (log_buffer_snapshot_state) {
>> +            /* First copy the state structure in snapshot buffer */
>> +            memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
>> +                   sizeof(struct guc_log_buffer_state));
>
> I've noticed now log_buffer_state_local is used only for the
> sample_write_ptr below and otherwise it causes two copies of the same data.
>
Actually log_buffer_state_local gets used more in the subsequent 
patches, where we do the bookkeeping and optimize the copying.

[PATCH 10/19] drm/i915: Add stats for GuC log buffer flush interrupts
[PATCH 11/19] drm/i915: Optimization to reduce the sampling time of GuC 
log buffer

> Since this branch is an interesting one, you could avoid one copy by
> updating the log_buffer_state->read_ptr from the
> log_buffer_snapshot_state in this branch, and add an else branch to
> update it directly from log_buffer_state when log_buffer_snapshot_state
> is not available.
>
> Sounds like it would be worth eliminating double memcpy, what do you think>
>
Won't the 2nd memcpy (from the copy on stack to the relay buffer) be 
really fast ?
The copy on stack (16 bytes) will most likely be in the CPU cache and 
the same area is used for all 3 buffer types.

Best regards
Akash

>> +
>> +            /* 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_snapshot_state->write_ptr =
>> +                log_buffer_snapshot_state->sampled_write_ptr;
>> +
>> +            log_buffer_snapshot_state++;
>> +
>> +            /* Now copy the actual logs. */
>> +            memcpy(dst_data_ptr, src_data_ptr, buffer_size);
>> +
>> +            src_data_ptr += buffer_size;
>> +            dst_data_ptr += buffer_size;
>> +        }
>> +
>> +        /* FIXME: invalidate/flush for log buffer needed */
>> +
>> +        /* Update the read pointer in the shared log buffer */
>> +        log_buffer_state->read_ptr =
>> +            log_buffer_state_local.sampled_write_ptr;
>> +
>> +        /* Clear the 'flush to file' flag */
>> +        log_buffer_state->flush_to_file = 0;
>> +        log_buffer_state++;
>> +    }
>> +
>> +    if (log_buffer_snapshot_state)
>> +        guc_move_to_next_buf(guc);
>> +}
>> +
>> +static void guc_capture_logs_work(struct work_struct *work)
>> +{
>> +    struct drm_i915_private *dev_priv =
>> +        container_of(work, struct drm_i915_private, guc.log.flush_work);
>> +
>> +    i915_guc_capture_logs(dev_priv);
>> +}
>> +
>> +static void guc_log_cleanup(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> +    lockdep_assert_held(&dev_priv->drm.struct_mutex);
>> +
>> +    if (i915.guc_log_level < 0)
>> +        return;
>> +
>> +    /* First disable the flush interrupt */
>> +    gen9_disable_guc_interrupts(dev_priv);
>> +
>> +    if (guc->log.flush_wq)
>> +        destroy_workqueue(guc->log.flush_wq);
>> +
>> +    guc->log.flush_wq = NULL;
>> +
>> +    if (guc->log.buf_addr)
>> +        i915_gem_object_unpin_map(guc->log.vma->obj);
>> +
>> +    guc->log.buf_addr = NULL;
>> +}
>> +
>> +static int guc_create_log_extras(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    void *vaddr;
>> +    int ret;
>> +
>> +    lockdep_assert_held(&dev_priv->drm.struct_mutex);
>> +
>> +    /* Nothing to do */
>> +    if (i915.guc_log_level < 0)
>> +        return 0;
>> +
>> +    if (!guc->log.buf_addr) {
>> +        /* Create a vmalloc mapping of log buffer pages */
>> +        vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WB);
>> +        if (IS_ERR(vaddr)) {
>> +            ret = PTR_ERR(vaddr);
>> +            DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
>> +            return ret;
>> +        }
>> +
>> +        guc->log.buf_addr = vaddr;
>> +    }
>> +
>> +    if (!guc->log.flush_wq) {
>> +        INIT_WORK(&guc->log.flush_work, guc_capture_logs_work);
>> +
>> +        /* Need a dedicated wq to process log buffer flush interrupts
>> +         * from GuC without much delay so as to avoid any loss of logs.
>> +         */
>> +        guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log", 0);
>> +        if (guc->log.flush_wq == NULL) {
>> +            DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void guc_create_log(struct intel_guc *guc)
>>   {
>>       struct i915_vma *vma;
>> @@ -853,6 +1019,13 @@ static void guc_create_log(struct intel_guc *guc)
>>           }
>>
>>           guc->log.vma = vma;
>> +
>> +        if (guc_create_log_extras(guc)) {
>> +            guc_log_cleanup(guc);
>> +            i915_vma_unpin_and_release(&guc->log.vma);
>> +            i915.guc_log_level = -1;
>> +            return;
>> +        }
>>       }
>>
>>       /* each allocated unit is a page */
>> @@ -1034,6 +1207,7 @@ void i915_guc_submission_fini(struct
>> drm_i915_private *dev_priv)
>>       struct intel_guc *guc = &dev_priv->guc;
>>
>>       i915_vma_unpin_and_release(&guc->ads_vma);
>> +    guc_log_cleanup(guc);
>>       i915_vma_unpin_and_release(&guc->log.vma);
>>
>>       if (guc->ctx_pool_vma)
>> @@ -1095,3 +1269,15 @@ int intel_guc_resume(struct drm_device *dev)
>>
>>       return host2guc_action(guc, data, ARRAY_SIZE(data));
>>   }
>> +
>> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
>> +{
>> +    guc_read_update_log_buffer(&dev_priv->guc);
>> +
>> +    /* Generally device is expected to be active only at this
>> +     * time, so get/put should be really quick.
>> +     */
>> +    intel_runtime_pm_get(dev_priv);
>> +    host2guc_logbuffer_flush_complete(&dev_priv->guc);
>> +    intel_runtime_pm_put(dev_priv);
>> +}
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index fc1fe72..19c0078 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1662,7 +1662,33 @@ static void gen6_rps_irq_handler(struct
>> drm_i915_private *dev_priv, u32 pm_iir)
>>   static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
>> u32 gt_iir)
>>   {
>>       if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
>> -        /* TODO: Handle events for which GuC interrupted host */
>> +        /* Sample the log buffer flush related bits & clear them out now
>> +         * itself from the message identity register to minimize the
>> +         * probability of losing a flush interrupt, when there are back
>> +         * to back flush interrupts.
>> +         * There can be a new flush interrupt, for different log buffer
>> +         * type (like for ISR), whilst Host is handling one (for DPC).
>> +         * Since same bit is used in message register for ISR & DPC, it
>> +         * could happen that GuC sets the bit for 2nd interrupt but Host
>> +         * clears out the bit on handling the 1st interrupt.
>> +         */
>> +        u32 msg, flush;
>> +
>> +        msg = I915_READ(SOFT_SCRATCH(15));
>> +        flush = msg & (GUC2HOST_MSG_CRASH_DUMP_POSTED |
>> +                   GUC2HOST_MSG_FLUSH_LOG_BUFFER);
>> +        if (flush) {
>> +            /* Clear the message bits that are handled */
>> +            I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
>> +
>> +            /* Handle flush interrupt in bottom half */
>> +            queue_work(dev_priv->guc.log.flush_wq,
>> +                   &dev_priv->guc.log.flush_work);
>> +        } else {
>> +            /* Not clearing of unhandled event bits won't result in
>> +             * re-triggering of the interrupt.
>> +             */
>> +        }
>>       }
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 1fc63fe..d053a18 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 {
>>       uint32_t flags;
>>       struct i915_vma *vma;
>> +    void *buf_addr;
>> +    struct workqueue_struct *flush_wq;
>> +    struct work_struct flush_work;
>>   };
>>
>>   struct intel_guc {
>> @@ -167,5 +170,6 @@ int i915_guc_submission_enable(struct
>> drm_i915_private *dev_priv);
>>   int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
>>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
>>
>>   #endif
>>
>
> Otherwise looks OK to me.
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Tvrtko Ursulin]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> The GuC log buffer flush work item does a register access to send the ack
> to GuC and this work item, if not synced before suspend, can potentially
> get executed after the GFX device is suspended.
> The work item function uses rpm_get/rpm_put calls around the Hw access,
> this covers the runtime suspend case but for system suspend case (which can
> be done asychronously/forcefully) sync would be required as kernel can
> potentially schedule the work items even after some devices, including GFX,
> have been put to suspend.
> Also sync has to be done conditionally i.e. only for the system suspend
> case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
> suspend path.
>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
>   drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
>   drivers/gpu/drm/i915/intel_guc.h           | 2 +-
>   3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index cdee60b..2ae0ad4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>   		goto out;
>   	}
>
> -	intel_guc_suspend(dev);
> +	intel_guc_suspend(dev, false);
>
>   	intel_display_suspend(dev);
>
> @@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
>   	i915_gem_release_all_mmaps(dev_priv);
>   	mutex_unlock(&dev->struct_mutex);
>
> -	intel_guc_suspend(dev);
> +	intel_guc_suspend(dev, true);
>
>   	intel_runtime_pm_disable_interrupts(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index ef0c116..1af8a8b 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>    * intel_guc_suspend() - notify GuC entering suspend state
>    * @dev:	drm device
>    */
> -int intel_guc_suspend(struct drm_device *dev)
> +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(dev);
>   	struct intel_guc *guc = &dev_priv->guc;
> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
>   		return 0;
>
>   	gen9_disable_guc_interrupts(dev_priv);
> +	/* Sync is needed only for the system suspend case, runtime suspend
> +	 * case is covered due to rpm get/put calls used around Hw access in
> +	 * the work item function.
> +	 */
> +	if (!rpm_suspend && (i915.guc_log_level >= 0))
> +		flush_work(&dev_priv->guc.log.flush_work);
>
>   	ctx = dev_priv->kernel_context;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b56fe24..1367314 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -172,7 +172,7 @@ extern void intel_guc_init(struct drm_device *dev);
>   extern int intel_guc_setup(struct drm_device *dev);
>   extern void intel_guc_fini(struct drm_device *dev);
>   extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> -extern int intel_guc_suspend(struct drm_device *dev);
> +extern int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend);
>   extern int intel_guc_resume(struct drm_device *dev);
>
>   /* i915_guc_submission.c */
>

Sounds believable but would prefer is someone with better knowledge of 
suspend/resume paths would gave it a look as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 06/19] drm/i915: Handle log buffer flush interrupt event from GuC

[Tvrtko Ursulin]

On 17/08/16 12:24, Goel, Akash wrote:
> On 8/17/2016 4:37 PM, Tvrtko Ursulin wrote:
>>
>> On 17/08/16 11:14, akash.goel@intel.com wrote:
>>> 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.
>>>
>>> v2:
>>> - Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
>>> - Reduce the overall log buffer copying time by skipping the copy of
>>>    crash buffer area for regular cases and copying only the state
>>>    structure data in first page.
>>>
>>> v3:
>>>   - Create a vmalloc mapping of log buffer. (Chris)
>>>   - Cover the flush acknowledgment under rpm get & put.(Chris)
>>>   - Revert the change of skipping the copy of crash dump area, as
>>>     not really needed, will be covered by subsequent patch.
>>>
>>> v4:
>>>   - Destroy the wq under the same condition in which it was created,
>>>     pass dev_piv pointer instead of dev to newly added GuC function,
>>>     add more comments & rename variable for clarity. (Tvrtko)
>>>
>>> v5:
>>> - Allocate & destroy the dedicated wq, for handling flush interrupt,
>>>    from the setup/teardown routines of GuC logging. (Chris)
>>> - Validate the log buffer size value retrieved from state structure
>>>    and do some minor cleanup. (Tvrtko)
>>> - Fix error/warnings reported by checkpatch. (Tvrtko)
>>> - Rebase.
>>>
>>> v6:
>>>   - Remove the interrupts_enabled check from guc_capture_logs_work, need
>>>     to process that last work item also, queued just before disabling
>>> the
>>>     interrupt as log buffer flush interrupt handling is a bit different
>>>     case where GuC is actually expecting an ACK from host, which
>>> should be
>>>     provided to keep the logging going.
>>>     Sync against the work will be done by caller disabling the
>>> interrupt.
>>>   - Don't sample the log buffer size value from state structure,
>>> directly
>>>     use the expected value to move the pointer & do the copy and that
>>> cannot
>>>     go wrong (out of bounds) as Driver only allocated the log buffer
>>> and the
>>>     relay buffers. Driver should refrain from interpreting the log
>>> packet,
>>>     as much possible and let Userspace parser detect the anomaly.
>>> (Chris)
>>>
>>> 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 | 186
>>> +++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/i915_irq.c            |  28 ++++-
>>>   drivers/gpu/drm/i915/intel_guc.h           |   4 +
>>>   3 files changed, 217 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index b062da6..ade51cb 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct
>>> intel_guc *guc,
>>>       return host2guc_action(guc, data, ARRAY_SIZE(data));
>>>   }
>>>
>>> +static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
>>> +{
>>> +    u32 data[1];
>>> +
>>> +    data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
>>> +
>>> +    return host2guc_action(guc, data, 1);
>>> +}
>>> +
>>>   /*
>>>    * Initialise, update, or clear doorbell data shared with the GuC
>>>    *
>>> @@ -828,6 +837,163 @@ err:
>>>       return NULL;
>>>   }
>>>
>>> +static void guc_move_to_next_buf(struct intel_guc *guc)
>>> +{
>>> +}
>>> +
>>> +static void *guc_get_write_buffer(struct intel_guc *guc)
>>> +{
>>> +    return NULL;
>>> +}
>>> +
>>> +static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type
>>> type)
>>> +{
>>> +    if (type == GUC_ISR_LOG_BUFFER)
>>> +        return (GUC_LOG_ISR_PAGES + 1) * PAGE_SIZE;
>>> +    else if (type == GUC_DPC_LOG_BUFFER)
>>> +        return (GUC_LOG_DPC_PAGES + 1) * PAGE_SIZE;
>>> +    else
>>> +        return (GUC_LOG_CRASH_PAGES + 1) * PAGE_SIZE;
>>> +}
>>
>> Could do it with a switch statement to get automatic reminder of size
>> not being handled if some day a new log buffer type gets added. It would
>> probably more in the style of the rest of the driver as well.
>>
> Fine will use switch statement here.
>
> Should I use BUG_ON for the default/unhandled case ?
>      case GUC_ISR_LOG_BUFFER
>
>      case GUC_DPC_LOG_BUFFER
>
>      case GUC_LOG_CRASH_PAGES
>
>      default
>          BUG_ON(1)

You could use MISSING_CASE, but I think compiler should complain as 
well. Suppose MISSING_CASE is safer.

>>> +
>>> +static void guc_read_update_log_buffer(struct intel_guc *guc)
>>> +{
>>> +    struct guc_log_buffer_state *log_buffer_state,
>>> *log_buffer_snapshot_state;
>>> +    struct guc_log_buffer_state log_buffer_state_local;
>>> +    void *src_data_ptr, *dst_data_ptr;
>>> +    unsigned int buffer_size;
>>> +    enum guc_log_buffer_type type;
>>> +
>>> +    if (WARN_ON(!guc->log.buf_addr))
>>> +        return;
>>> +
>>> +    /* Get the pointer to shared GuC log buffer */
>>> +    log_buffer_state = src_data_ptr = guc->log.buf_addr;
>>> +
>>> +    /* Get the pointer to local buffer to store the logs */
>>> +    dst_data_ptr = log_buffer_snapshot_state =
>>> guc_get_write_buffer(guc);
>>> +
>>> +    /* Actual logs are present from the 2nd page */
>>> +    src_data_ptr += PAGE_SIZE;
>>> +    dst_data_ptr += PAGE_SIZE;
>>> +
>>> +    for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER;
>>> type++) {
>>> +        /* Make a copy of the state structure in GuC log buffer (which
>>> +         * is uncached mapped) on the stack to avoid reading from it
>>> +         * multiple times.
>>> +         */
>>> +        memcpy(&log_buffer_state_local, log_buffer_state,
>>> +               sizeof(struct guc_log_buffer_state));
>>> +        buffer_size = guc_get_log_buffer_size(type);
>>> +
>>> +        if (log_buffer_snapshot_state) {
>>> +            /* First copy the state structure in snapshot buffer */
>>> +            memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
>>> +                   sizeof(struct guc_log_buffer_state));
>>
>> I've noticed now log_buffer_state_local is used only for the
>> sample_write_ptr below and otherwise it causes two copies of the same
>> data.
>>
> Actually log_buffer_state_local gets used more in the subsequent
> patches, where we do the bookkeeping and optimize the copying.
>
> [PATCH 10/19] drm/i915: Add stats for GuC log buffer flush interrupts
> [PATCH 11/19] drm/i915: Optimization to reduce the sampling time of GuC
> log buffer
>
>> Since this branch is an interesting one, you could avoid one copy by
>> updating the log_buffer_state->read_ptr from the
>> log_buffer_snapshot_state in this branch, and add an else branch to
>> update it directly from log_buffer_state when log_buffer_snapshot_state
>> is not available.
>>
>> Sounds like it would be worth eliminating double memcpy, what do you
>> think>
>>
> Won't the 2nd memcpy (from the copy on stack to the relay buffer) be
> really fast ?
> The copy on stack (16 bytes) will most likely be in the CPU cache and
> the same area is used for all 3 buffer types.

Yes I realized later you use it more in later patches.

I don't think it is slow but was just wondering if it could be made 
tidier by getting rid of one copy.

Would have to apply the series to see how the final loop looks like, but 
would something like the below be possible:

	if (!log_buffer_snapshot_state) {
		.. read_ptr update from wc ..
		continue;
	}

	memcpy from wc to final buffer

	.. the rest of processing you do reading from the copy ..

?

Regards,

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Chris Wilson]

On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/08/16 11:14, akash.goel@intel.com wrote:
> >From: Akash Goel <akash.goel@intel.com>
> >
> >The GuC log buffer flush work item does a register access to send the ack
> >to GuC and this work item, if not synced before suspend, can potentially
> >get executed after the GFX device is suspended.
> >The work item function uses rpm_get/rpm_put calls around the Hw access,
> >this covers the runtime suspend case but for system suspend case (which can
> >be done asychronously/forcefully) sync would be required as kernel can
> >potentially schedule the work items even after some devices, including GFX,
> >have been put to suspend.
> >Also sync has to be done conditionally i.e. only for the system suspend
> >case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
> >suspend path.
> >
> >Cc: Imre Deak <imre.deak@intel.com>
> >Signed-off-by: Akash Goel <akash.goel@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
> >  drivers/gpu/drm/i915/intel_guc.h           | 2 +-
> >  3 files changed, 10 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index cdee60b..2ae0ad4 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  		goto out;
> >  	}
> >
> >-	intel_guc_suspend(dev);
> >+	intel_guc_suspend(dev, false);
> >
> >  	intel_display_suspend(dev);
> >
> >@@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
> >  	i915_gem_release_all_mmaps(dev_priv);
> >  	mutex_unlock(&dev->struct_mutex);
> >
> >-	intel_guc_suspend(dev);
> >+	intel_guc_suspend(dev, true);
> >
> >  	intel_runtime_pm_disable_interrupts(dev_priv);
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index ef0c116..1af8a8b 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> >   * intel_guc_suspend() - notify GuC entering suspend state
> >   * @dev:	drm device
> >   */
> >-int intel_guc_suspend(struct drm_device *dev)
> >+int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_guc *guc = &dev_priv->guc;
> >@@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
> >  		return 0;
> >
> >  	gen9_disable_guc_interrupts(dev_priv);
> >+	/* Sync is needed only for the system suspend case, runtime suspend
> >+	 * case is covered due to rpm get/put calls used around Hw access in
> >+	 * the work item function.
> >+	 */
> >+	if (!rpm_suspend && (i915.guc_log_level >= 0))
> >+		flush_work(&dev_priv->guc.log.flush_work);

In which case (rpm suspend) the flush_work is idle and this a noop. That
you have to pass around such state suggests that you are papering over a
bug?
-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: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/17/2016 5:11 PM, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
>>
>> On 17/08/16 11:14, akash.goel@intel.com wrote:
>>> From: Akash Goel <akash.goel@intel.com>
>>>
>>> The GuC log buffer flush work item does a register access to send the ack
>>> to GuC and this work item, if not synced before suspend, can potentially
>>> get executed after the GFX device is suspended.
>>> The work item function uses rpm_get/rpm_put calls around the Hw access,
>>> this covers the runtime suspend case but for system suspend case (which can
>>> be done asychronously/forcefully) sync would be required as kernel can
>>> potentially schedule the work items even after some devices, including GFX,
>>> have been put to suspend.
>>> Also sync has to be done conditionally i.e. only for the system suspend
>>> case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
>>> suspend path.
>>>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
>>>  drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
>>>  drivers/gpu/drm/i915/intel_guc.h           | 2 +-
>>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index cdee60b..2ae0ad4 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>  		goto out;
>>>  	}
>>>
>>> -	intel_guc_suspend(dev);
>>> +	intel_guc_suspend(dev, false);
>>>
>>>  	intel_display_suspend(dev);
>>>
>>> @@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
>>>  	i915_gem_release_all_mmaps(dev_priv);
>>>  	mutex_unlock(&dev->struct_mutex);
>>>
>>> -	intel_guc_suspend(dev);
>>> +	intel_guc_suspend(dev, true);
>>>
>>>  	intel_runtime_pm_disable_interrupts(dev_priv);
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index ef0c116..1af8a8b 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>>>   * intel_guc_suspend() - notify GuC entering suspend state
>>>   * @dev:	drm device
>>>   */
>>> -int intel_guc_suspend(struct drm_device *dev)
>>> +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
>>>  {
>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>  	struct intel_guc *guc = &dev_priv->guc;
>>> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
>>>  		return 0;
>>>
>>>  	gen9_disable_guc_interrupts(dev_priv);
>>> +	/* Sync is needed only for the system suspend case, runtime suspend
>>> +	 * case is covered due to rpm get/put calls used around Hw access in
>>> +	 * the work item function.
>>> +	 */
>>> +	if (!rpm_suspend && (i915.guc_log_level >= 0))
>>> +		flush_work(&dev_priv->guc.log.flush_work);
>
> In which case (rpm suspend) the flush_work is idle and this a noop. That
> you have to pass around such state suggests that you are papering over a
> bug?
In case of rpm suspend the flush_work may not be a NOOP.
Can use the flush_work for runtime suspend also but in spite of that 
can't prevent the 'RPM wakelock' asserts, as the work item can get
executed after the rpm ref count drops to zero and before runtime
suspend kicks in (after autosuspend delay).

For that you had earlier suggested to use rpm get/put in the work item 
function, around the register access, but with that had to remove the 
flush_work from the suspend hook, otherwise a deadlock can happen.
So doing the flush_work conditionally for system suspend case, as rpm 
get/put won't cause the resume of device in that case.

Actually I had discussed about this with Imre and as per his inputs 
prepared this patch.

Best regards
Akash





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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Imre Deak]

On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
> 
> On 8/17/2016 5:11 PM, Chris Wilson wrote:
> > On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 17/08/16 11:14, akash.goel@intel.com wrote:
> > > > From: Akash Goel <akash.goel@intel.com>
> > > > 
> > > > The GuC log buffer flush work item does a register access to send the ack
> > > > to GuC and this work item, if not synced before suspend, can potentially
> > > > get executed after the GFX device is suspended.
> > > > The work item function uses rpm_get/rpm_put calls around the Hw access,
> > > > this covers the runtime suspend case but for system suspend case (which can
> > > > be done asychronously/forcefully) sync would be required as kernel can
> > > > potentially schedule the work items even after some devices, including GFX,
> > > > have been put to suspend.
> > > > Also sync has to be done conditionally i.e. only for the system suspend
> > > > case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
> > > > suspend path.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
> > > >  drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
> > > >  drivers/gpu/drm/i915/intel_guc.h           | 2 +-
> > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index cdee60b..2ae0ad4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> > > >  		goto out;
> > > >  	}
> > > > 
> > > > -	intel_guc_suspend(dev);
> > > > +	intel_guc_suspend(dev, false);
> > > > 
> > > >  	intel_display_suspend(dev);
> > > > 
> > > > @@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
> > > >  	i915_gem_release_all_mmaps(dev_priv);
> > > >  	mutex_unlock(&dev->struct_mutex);
> > > > 
> > > > -	intel_guc_suspend(dev);
> > > > +	intel_guc_suspend(dev, true);
> > > > 
> > > >  	intel_runtime_pm_disable_interrupts(dev_priv);
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index ef0c116..1af8a8b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> > > >   * intel_guc_suspend() - notify GuC entering suspend state
> > > >   * @dev:	drm device
> > > >   */
> > > > -int intel_guc_suspend(struct drm_device *dev)
> > > > +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	struct intel_guc *guc = &dev_priv->guc;
> > > > @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
> > > >  		return 0;
> > > > 
> > > >  	gen9_disable_guc_interrupts(dev_priv);
> > > > +	/* Sync is needed only for the system suspend case, runtime suspend
> > > > +	 * case is covered due to rpm get/put calls used around Hw access in
> > > > +	 * the work item function.
> > > > +	 */
> > > > +	if (!rpm_suspend && (i915.guc_log_level >= 0))
> > > > +		flush_work(&dev_priv->guc.log.flush_work);
> > 
> > In which case (rpm suspend) the flush_work is idle and this a noop. That
> > you have to pass around such state suggests that you are papering over a
> > bug?
> In case of rpm suspend the flush_work may not be a NOOP.
> Can use the flush_work for runtime suspend also but in spite of that 
> can't prevent the 'RPM wakelock' asserts, as the work item can get
> executed after the rpm ref count drops to zero and before runtime
> suspend kicks in (after autosuspend delay).
> 
> For that you had earlier suggested to use rpm get/put in the work item 
> function, around the register access, but with that had to remove the 
> flush_work from the suspend hook, otherwise a deadlock can happen.
> So doing the flush_work conditionally for system suspend case, as rpm 
> get/put won't cause the resume of device in that case.
> 
> Actually I had discussed about this with Imre and as per his inputs 
> prepared this patch.

There would be this alternative:

in gen9_guc_irq_handler():
   WARN_ON(!intel_runtime_pm_get_if_in_use());
   if (!queue_work(log.flush_work))
       intel_runtime_pm_put();

and dropping the reference at the end of the work item. This would make
the flush_work() a nop in case of runtime_suspend().

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

Re: [PATCH 06/19] drm/i915: Handle log buffer flush interrupt event from GuC

[Tvrtko Ursulin]

On 17/08/16 12:35, Tvrtko Ursulin wrote:
> On 17/08/16 12:24, Goel, Akash wrote:

[snip]

>> Won't the 2nd memcpy (from the copy on stack to the relay buffer) be
>> really fast ?
>> The copy on stack (16 bytes) will most likely be in the CPU cache and
>> the same area is used for all 3 buffer types.
> 
> Yes I realized later you use it more in later patches.
> 
> I don't think it is slow but was just wondering if it could be made 
> tidier by getting rid of one copy.
> 
> Would have to apply the series to see how the final loop looks like, but 
> would something like the below be possible:
> 
>      if (!log_buffer_snapshot_state) {
>          .. read_ptr update from wc ..
>          continue;
>      }
> 
>      memcpy from wc to final buffer
> 
>      .. the rest of processing you do reading from the copy ..
> 
> ?

Not even compile tested and possibly incorrectly refactored,
but what do you think of:

static bool
guc_log_check_overflow(struct intel_guc *guc,
		       enum guc_log_buffer_type type,
		       struct guc_log_buffer *snapshot)
{
	unsigned int full_cnt, prev_full_cnt;
	bool overflow = false;

	guc->log.flush_count[type]++;

	full_cnt = snapshot.buffer_full_cnt;
	prev_full_cnt = guc->log.prev_overflow_count[type];

	if (full_cnt != prev_full_cnt) {
		overflow = true;

		guc->log.prev_overflow_count[type] = full_cnt;
		guc->log.total_overflow_count[type] += full_cnt - prev_full_cnt;

		if (full_cnt < prev_full_cnt) {
			/* buffer_full_cnt is a 4 bit counter */
			guc->log.total_overflow_count[type] += 16;
		}
	}

	return overflow;
}

static void guc_read_update_log_buffer(struct intel_guc *guc)
{
	struct guc_log_buffer *log_buffer, *snapshot;
	void *src_data, *dst_data;
	enum guc_log_buffer_type type;

	if (WARN_ON(!guc->log.buf_addr))
		return;

	/* Get the pointer to shared GuC log buffer */
	log_buffer = src_data = guc->log.buf_addr;

	/* Get the pointer to local buffer to store the logs */
	dst_data = snapshot = guc_get_write_buffer(guc);

	/* Actual logs are present from the 2nd page */
	src_data += PAGE_SIZE;
	dst_data += PAGE_SIZE;

	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER;
	     type++, log_buffer++) {
		unsigned int buffer_size, bytes_to_copy;
		unsigned int read_offset, write_offset;
		bool new_overflow;

		/* Clear the 'flush to file' flag */
		log_buffer->flush_to_file = 0;

		if (unlikely(!snapshot)) {
			/* Update the read pointer in the shared log buffer */
			log_buffer->read_ptr = log_buffer.sampled_write_ptr;
			continue;
		}

		memcpy(snapshot, &log_buffer, sizeof(*snapshot));

		buffer_size = guc_get_log_buffer_size(type);
		read_offset = snapshot.read_ptr;
		write_offset = snapshot.sampled_write_ptr;

		/* 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.
		 */
		snapshot->write_ptr = write_offset;

		new_overflow = guc_log_check_overflow(guc, type, snapshot);
		if (unlikely(new_overflow)) {
			DRM_ERROR_RATELIMITED("GuC log buffer overflow\n");
			/* copy the whole buffer in case of overflow */
			read_offset = 0;
			write_offset = buffer_size;
		} else if (unlikely((read_offset > buffer_size) ||
		           (write_offset > buffer_size))) {
			DRM_ERROR("invalid log buffer state\n");
			/* copy whole buffer as offsets are unreliable */
			read_offset = 0;
			write_offset = buffer_size;
		}

		/* Just copy the newly written data */
		if (read_offset > write_offset) {
			bytes_to_copy = buffer_size - read_offset;

			i915_memcpy_from_wc(dst_data, src_data, write_offset);
		} else {
			bytes_to_copy = write_offset - read_offset;
		}
		i915_memcpy_from_wc(dst_data + read_offset,
				    src_data + read_offset, bytes_to_copy);

		src_data += buffer_size;
		dst_data += buffer_size;

		/* Update the read pointer in the shared log buffer */
		log_buffer->read_ptr = snapshot.sampled_write_ptr;

		snapshot++;
	}

	if (snapshot) {
		guc_move_to_next_buf(guc);
	} else {
		/* Used rate limited to avoid deluge of messages, logs might be
		 * getting consumed by User at a slow rate.
		 */
		DRM_ERROR_RATELIMITED("no sub-buffer to capture log buffer\n");
		guc->log.capture_miss_count++;
	}
}

Removes the double memcpy and shortens some variable names in order
to make the whole thing more readable.

Regards,

Tvrtko

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/17/2016 6:41 PM, Imre Deak wrote:
> On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
>>
>> On 8/17/2016 5:11 PM, Chris Wilson wrote:
>>> On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 17/08/16 11:14, akash.goel@intel.com wrote:
>>>>> From: Akash Goel <akash.goel@intel.com>
>>>>>
>>>>> The GuC log buffer flush work item does a register access to send the ack
>>>>> to GuC and this work item, if not synced before suspend, can potentially
>>>>> get executed after the GFX device is suspended.
>>>>> The work item function uses rpm_get/rpm_put calls around the Hw access,
>>>>> this covers the runtime suspend case but for system suspend case (which can
>>>>> be done asychronously/forcefully) sync would be required as kernel can
>>>>> potentially schedule the work items even after some devices, including GFX,
>>>>> have been put to suspend.
>>>>> Also sync has to be done conditionally i.e. only for the system suspend
>>>>> case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
>>>>> suspend path.
>>>>>
>>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/i915_drv.c            | 4 ++--
>>>>>  drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
>>>>>  drivers/gpu/drm/i915/intel_guc.h           | 2 +-
>>>>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>>> index cdee60b..2ae0ad4 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>>> @@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>>>  		goto out;
>>>>>  	}
>>>>>
>>>>> -	intel_guc_suspend(dev);
>>>>> +	intel_guc_suspend(dev, false);
>>>>>
>>>>>  	intel_display_suspend(dev);
>>>>>
>>>>> @@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
>>>>>  	i915_gem_release_all_mmaps(dev_priv);
>>>>>  	mutex_unlock(&dev->struct_mutex);
>>>>>
>>>>> -	intel_guc_suspend(dev);
>>>>> +	intel_guc_suspend(dev, true);
>>>>>
>>>>>  	intel_runtime_pm_disable_interrupts(dev_priv);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> index ef0c116..1af8a8b 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> @@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>>>>>   * intel_guc_suspend() - notify GuC entering suspend state
>>>>>   * @dev:	drm device
>>>>>   */
>>>>> -int intel_guc_suspend(struct drm_device *dev)
>>>>> +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
>>>>>  {
>>>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>>  	struct intel_guc *guc = &dev_priv->guc;
>>>>> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
>>>>>  		return 0;
>>>>>
>>>>>  	gen9_disable_guc_interrupts(dev_priv);
>>>>> +	/* Sync is needed only for the system suspend case, runtime suspend
>>>>> +	 * case is covered due to rpm get/put calls used around Hw access in
>>>>> +	 * the work item function.
>>>>> +	 */
>>>>> +	if (!rpm_suspend && (i915.guc_log_level >= 0))
>>>>> +		flush_work(&dev_priv->guc.log.flush_work);
>>>
>>> In which case (rpm suspend) the flush_work is idle and this a noop. That
>>> you have to pass around such state suggests that you are papering over a
>>> bug?
>> In case of rpm suspend the flush_work may not be a NOOP.
>> Can use the flush_work for runtime suspend also but in spite of that
>> can't prevent the 'RPM wakelock' asserts, as the work item can get
>> executed after the rpm ref count drops to zero and before runtime
>> suspend kicks in (after autosuspend delay).
>>
>> For that you had earlier suggested to use rpm get/put in the work item
>> function, around the register access, but with that had to remove the
>> flush_work from the suspend hook, otherwise a deadlock can happen.
>> So doing the flush_work conditionally for system suspend case, as rpm
>> get/put won't cause the resume of device in that case.
>>
>> Actually I had discussed about this with Imre and as per his inputs
>> prepared this patch.
>
> There would be this alternative:
>
Thanks much for suggesting the alternate approach.

Just to confirm whether I understood everything correctly,

> in gen9_guc_irq_handler():
>    WARN_ON(!intel_runtime_pm_get_if_in_use());
Used WARN, as we don't expect the device to be suspended at this 
juncture, so intel_runtime_pm_get_if_in_use() should return true.

>    if (!queue_work(log.flush_work))
If queue_work returns 0, then work item is already pending, so it won't
be queued hence can release the rpm ref count now only.
>        intel_runtime_pm_put();

>
> and dropping the reference at the end of the work item.
This will be just like the __intel_autoenable_gt_powersave

> This would make the flush_work() a nop in case of runtime_suspend().
So can call the flush_work unconditionally.

Hope I understood it correctly.

Best regards
Akash

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/17/2016 9:07 PM, Goel, Akash wrote:
>
>
> On 8/17/2016 6:41 PM, Imre Deak wrote:
>> On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
>>>
>>> On 8/17/2016 5:11 PM, Chris Wilson wrote:
>>>> On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
>>>>>

>>>>>> +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
>>>>>>  {
>>>>>>      struct drm_i915_private *dev_priv = to_i915(dev);
>>>>>>      struct intel_guc *guc = &dev_priv->guc;
>>>>>> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
>>>>>>          return 0;
>>>>>>
>>>>>>      gen9_disable_guc_interrupts(dev_priv);
>>>>>> +    /* Sync is needed only for the system suspend case, runtime
>>>>>> suspend
>>>>>> +     * case is covered due to rpm get/put calls used around Hw
>>>>>> access in
>>>>>> +     * the work item function.
>>>>>> +     */
>>>>>> +    if (!rpm_suspend && (i915.guc_log_level >= 0))
>>>>>> +        flush_work(&dev_priv->guc.log.flush_work);
>>>>
>>>> In which case (rpm suspend) the flush_work is idle and this a noop.
>>>> That
>>>> you have to pass around such state suggests that you are papering
>>>> over a
>>>> bug?
>>> In case of rpm suspend the flush_work may not be a NOOP.
>>> Can use the flush_work for runtime suspend also but in spite of that
>>> can't prevent the 'RPM wakelock' asserts, as the work item can get
>>> executed after the rpm ref count drops to zero and before runtime
>>> suspend kicks in (after autosuspend delay).
>>>
>>> For that you had earlier suggested to use rpm get/put in the work item
>>> function, around the register access, but with that had to remove the
>>> flush_work from the suspend hook, otherwise a deadlock can happen.
>>> So doing the flush_work conditionally for system suspend case, as rpm
>>> get/put won't cause the resume of device in that case.
>>>
>>> Actually I had discussed about this with Imre and as per his inputs
>>> prepared this patch.
>>
>> There would be this alternative:
>>
> Thanks much for suggesting the alternate approach.
>
> Just to confirm whether I understood everything correctly,
>
>> in gen9_guc_irq_handler():
>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
> Used WARN, as we don't expect the device to be suspended at this
> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>
>>    if (!queue_work(log.flush_work))
> If queue_work returns 0, then work item is already pending, so it won't
> be queued hence can release the rpm ref count now only.
>>        intel_runtime_pm_put();
>
>>
>> and dropping the reference at the end of the work item.
> This will be just like the __intel_autoenable_gt_powersave
>
>> This would make the flush_work() a nop in case of runtime_suspend().
> So can call the flush_work unconditionally.
>
> Hope I understood it correctly.
>
Hi Imre,

You had suggested to use the below code from irq handler, suspecting 
that intel_runtime_pm_get_if_in_use() can return false, if interrupt 
gets handled just after device goes out of use.

	if (intel_runtime_pm_get_if_in_use()) {
		if (!queue_work(log.flush_work))
			intel_runtime_pm_put();
	}

Do you mean to say that interrupt can come when rpm suspend has already 
started but before the interrupt is disabled from the suspend hook ?
Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
will return false.
1)	Autosuspend delay elapses (device is marked as suspending)
2)		intel_runtime_suspend
3)			intel_guc_suspend
4)				gen9_disable_guc_interrupts(dev_priv);

If the above hypothesis is correct, then it implies that interrupt has 
to come after autosuspend delay has elapsed for the above scenario to arise.

I think it would be unlikely for the interrupt to come so late because 
device would have gone idle just before the autosuspend period started 
and so no GuC submissions would have been done after that.
So the probability of missing a work item could be very less and we
can bear that.

Best regards
Akash

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Imre Deak]

On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
> 
> On 8/17/2016 9:07 PM, Goel, Akash wrote:
> > 
> > 
> > On 8/17/2016 6:41 PM, Imre Deak wrote:
> > > On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
> > > > 
> > > > On 8/17/2016 5:11 PM, Chris Wilson wrote:
> > > > > On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin
> > > > > wrote:
> > > > > > 
> 
> > > > > > > +int intel_guc_suspend(struct drm_device *dev, bool
> > > > > > > rpm_suspend)
> > > > > > >  {
> > > > > > >      struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > >      struct intel_guc *guc = &dev_priv->guc;
> > > > > > > @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct
> > > > > > > drm_device *dev)
> > > > > > >          return 0;
> > > > > > > 
> > > > > > >      gen9_disable_guc_interrupts(dev_priv);
> > > > > > > +    /* Sync is needed only for the system suspend case,
> > > > > > > runtime
> > > > > > > suspend
> > > > > > > +     * case is covered due to rpm get/put calls used
> > > > > > > around Hw
> > > > > > > access in
> > > > > > > +     * the work item function.
> > > > > > > +     */
> > > > > > > +    if (!rpm_suspend && (i915.guc_log_level >= 0))
> > > > > > > +        flush_work(&dev_priv->guc.log.flush_work);
> > > > > 
> > > > > In which case (rpm suspend) the flush_work is idle and this a
> > > > > noop.
> > > > > That
> > > > > you have to pass around such state suggests that you are
> > > > > papering
> > > > > over a
> > > > > bug?
> > > > In case of rpm suspend the flush_work may not be a NOOP.
> > > > Can use the flush_work for runtime suspend also but in spite of
> > > > that
> > > > can't prevent the 'RPM wakelock' asserts, as the work item can
> > > > get
> > > > executed after the rpm ref count drops to zero and before
> > > > runtime
> > > > suspend kicks in (after autosuspend delay).
> > > > 
> > > > For that you had earlier suggested to use rpm get/put in the
> > > > work item
> > > > function, around the register access, but with that had to
> > > > remove the
> > > > flush_work from the suspend hook, otherwise a deadlock can
> > > > happen.
> > > > So doing the flush_work conditionally for system suspend case,
> > > > as rpm
> > > > get/put won't cause the resume of device in that case.
> > > > 
> > > > Actually I had discussed about this with Imre and as per his
> > > > inputs
> > > > prepared this patch.
> > > 
> > > There would be this alternative:
> > > 
> > Thanks much for suggesting the alternate approach.
> > 
> > Just to confirm whether I understood everything correctly,
> > 
> > > in gen9_guc_irq_handler():
> > >    WARN_ON(!intel_runtime_pm_get_if_in_use());
> > Used WARN, as we don't expect the device to be suspended at this
> > juncture, so intel_runtime_pm_get_if_in_use() should return true.
> > 
> > >    if (!queue_work(log.flush_work))
> > If queue_work returns 0, then work item is already pending, so it
> > won't
> > be queued hence can release the rpm ref count now only.
> > >        intel_runtime_pm_put();
> > 
> > > 
> > > and dropping the reference at the end of the work item.
> > This will be just like the __intel_autoenable_gt_powersave
> > 
> > > This would make the flush_work() a nop in case of
> > > runtime_suspend().
> > So can call the flush_work unconditionally.
> > 
> > Hope I understood it correctly.

Yes, the above is correct except for my mistake in
handling intel_runtime_pm_get_if_in_use() returning false as discussed
below.

> > 
> Hi Imre,
> 
> You had suggested to use the below code from irq handler, suspecting 
> that intel_runtime_pm_get_if_in_use() can return false, if interrupt 
> gets handled just after device goes out of use.
> 
> 	if (intel_runtime_pm_get_if_in_use()) {
> 		if (!queue_work(log.flush_work))
> 			intel_runtime_pm_put();
> 	}
> 
> Do you mean to say that interrupt can come when rpm suspend has
> already 
> started but before the interrupt is disabled from the suspend hook ?
> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
> will return false.
> 1)	Autosuspend delay elapses (device is marked as suspending)
> 2)		intel_runtime_suspend
> 3)			intel_guc_suspend
> 4)				gen9_disable_guc_interrupts(dev_pri
> v);

No, it can return false anytime the last RPM reference is dropped, that
is even before the autosuspend delay elapses. But that still makes the
likelihood for a missed work item scheduling small, because 1) we want
to reduce the autosuspend delay considerably from the current 10 sec
and 2) because what you say below about the GPU actually idling before
the RPM refcount going to 0.

> If the above hypothesis is correct, then it implies that interrupt
> has 
> to come after autosuspend delay has elapsed for the above scenario to
> arise.
> 
> I think it would be unlikely for the interrupt to come so late
> because 
> device would have gone idle just before the autosuspend period
> started 
> and so no GuC submissions would have been done after that.

Right.

> So the probability of missing a work item could be very less and we
> can bear that.

I haven't looked into what is the consequence of missing a work item,
you know this better. In any case - since it is still a possibility -
if it's a problem you could still make sure in intel_guc_suspend() that
any pending work is completed by calling guc_read_update_log_buffer(),
host2guc_logbuffer_flush_complete() if necessary after disabling
interrupts in intel_guc_suspend().

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/18/2016 4:25 PM, Imre Deak wrote:
> On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
>>
>> On 8/17/2016 9:07 PM, Goel, Akash wrote:
>>>
>>>
>>> On 8/17/2016 6:41 PM, Imre Deak wrote:
>>>> On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
>>>>>
>>>>> On 8/17/2016 5:11 PM, Chris Wilson wrote:
>>>>>> On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin
>>>>>> wrote:
>>>>>>>
>>
>>>>>>>> +int intel_guc_suspend(struct drm_device *dev, bool
>>>>>>>> rpm_suspend)
>>>>>>>>  {
>>>>>>>>      struct drm_i915_private *dev_priv = to_i915(dev);
>>>>>>>>      struct intel_guc *guc = &dev_priv->guc;
>>>>>>>> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct
>>>>>>>> drm_device *dev)
>>>>>>>>          return 0;
>>>>>>>>
>>>>>>>>      gen9_disable_guc_interrupts(dev_priv);
>>>>>>>> +    /* Sync is needed only for the system suspend case,
>>>>>>>> runtime
>>>>>>>> suspend
>>>>>>>> +     * case is covered due to rpm get/put calls used
>>>>>>>> around Hw
>>>>>>>> access in
>>>>>>>> +     * the work item function.
>>>>>>>> +     */
>>>>>>>> +    if (!rpm_suspend && (i915.guc_log_level >= 0))
>>>>>>>> +        flush_work(&dev_priv->guc.log.flush_work);
>>>>>>
>>>>>> In which case (rpm suspend) the flush_work is idle and this a
>>>>>> noop.
>>>>>> That
>>>>>> you have to pass around such state suggests that you are
>>>>>> papering
>>>>>> over a
>>>>>> bug?
>>>>> In case of rpm suspend the flush_work may not be a NOOP.
>>>>> Can use the flush_work for runtime suspend also but in spite of
>>>>> that
>>>>> can't prevent the 'RPM wakelock' asserts, as the work item can
>>>>> get
>>>>> executed after the rpm ref count drops to zero and before
>>>>> runtime
>>>>> suspend kicks in (after autosuspend delay).
>>>>>
>>>>> For that you had earlier suggested to use rpm get/put in the
>>>>> work item
>>>>> function, around the register access, but with that had to
>>>>> remove the
>>>>> flush_work from the suspend hook, otherwise a deadlock can
>>>>> happen.
>>>>> So doing the flush_work conditionally for system suspend case,
>>>>> as rpm
>>>>> get/put won't cause the resume of device in that case.
>>>>>
>>>>> Actually I had discussed about this with Imre and as per his
>>>>> inputs
>>>>> prepared this patch.
>>>>
>>>> There would be this alternative:
>>>>
>>> Thanks much for suggesting the alternate approach.
>>>
>>> Just to confirm whether I understood everything correctly,
>>>
>>>> in gen9_guc_irq_handler():
>>>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
>>> Used WARN, as we don't expect the device to be suspended at this
>>> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>>>
>>>>    if (!queue_work(log.flush_work))
>>> If queue_work returns 0, then work item is already pending, so it
>>> won't
>>> be queued hence can release the rpm ref count now only.
>>>>        intel_runtime_pm_put();
>>>
>>>>
>>>> and dropping the reference at the end of the work item.
>>> This will be just like the __intel_autoenable_gt_powersave
>>>
>>>> This would make the flush_work() a nop in case of
>>>> runtime_suspend().
>>> So can call the flush_work unconditionally.
>>>
>>> Hope I understood it correctly.
>
> Yes, the above is correct except for my mistake in
> handling intel_runtime_pm_get_if_in_use() returning false as discussed
> below.
>
>>>
>> Hi Imre,
>>
>> You had suggested to use the below code from irq handler, suspecting
>> that intel_runtime_pm_get_if_in_use() can return false, if interrupt
>> gets handled just after device goes out of use.
>>
>> 	if (intel_runtime_pm_get_if_in_use()) {
>> 		if (!queue_work(log.flush_work))
>> 			intel_runtime_pm_put();
>> 	}
>>
>> Do you mean to say that interrupt can come when rpm suspend has
>> already
>> started but before the interrupt is disabled from the suspend hook ?
>> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
>> will return false.
>> 1)	Autosuspend delay elapses (device is marked as suspending)
>> 2)		intel_runtime_suspend
>> 3)			intel_guc_suspend
>> 4)				gen9_disable_guc_interrupts(dev_pri
>> v);
>
> No, it can return false anytime the last RPM reference is dropped, that
> is even before the autosuspend delay elapses.

Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref 
count has dropped to 0, even if device is still in runtime active state 
(as autosuspend delay has not elapsed).

 > But that still makes the
> likelihood for a missed work item scheduling small, because 1) we want
> to reduce the autosuspend delay considerably from the current 10 sec
> and 2) because what you say below about the GPU actually idling before
> the RPM refcount going to 0.
>
>> If the above hypothesis is correct, then it implies that interrupt
>> has
>> to come after autosuspend delay has elapsed for the above scenario to
>> arise.
>>
>> I think it would be unlikely for the interrupt to come so late
>> because
>> device would have gone idle just before the autosuspend period
>> started
>> and so no GuC submissions would have been done after that.
>
> Right.
>
>> So the probability of missing a work item could be very less and we
>> can bear that.
>
> I haven't looked into what is the consequence of missing a work item,
> you know this better. In any case - since it is still a possibility -
> if it's a problem you could still make sure in intel_guc_suspend() that
> any pending work is completed by calling guc_read_update_log_buffer(),
> host2guc_logbuffer_flush_complete() if necessary after disabling
> interrupts in intel_guc_suspend().
Actually ideally guc_read_update_log_buffer() and 
host2guc_logbuffer_flush_complete() should be called only if the work
item was actually missed. So will have to detect the missing of work item.

Isn't the original implementation i.e. conditional flushing of work item 
for the system suspend case, a simpler & cleaner solution ?

Best regards
Akash

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Imre Deak]

On to, 2016-08-18 at 16:54 +0530, Goel, Akash wrote:
> 
> On 8/18/2016 4:25 PM, Imre Deak wrote:
> > On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
> > > 
> > > On 8/17/2016 9:07 PM, Goel, Akash wrote:
> > > > 
> > > > 
> > > > On 8/17/2016 6:41 PM, Imre Deak wrote:
> > > > > On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
> > > > > > 
> > > > > > On 8/17/2016 5:11 PM, Chris Wilson wrote:
> > > > > > > On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin
> > > > > > > wrote:
> > > > > > > > 
> > > 
> > > > > > > > > +int intel_guc_suspend(struct drm_device *dev, bool
> > > > > > > > > rpm_suspend)
> > > > > > > > >  {
> > > > > > > > >      struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > >      struct intel_guc *guc = &dev_priv->guc;
> > > > > > > > > @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct
> > > > > > > > > drm_device *dev)
> > > > > > > > >          return 0;
> > > > > > > > > 
> > > > > > > > >      gen9_disable_guc_interrupts(dev_priv);
> > > > > > > > > +    /* Sync is needed only for the system suspend case,
> > > > > > > > > runtime
> > > > > > > > > suspend
> > > > > > > > > +     * case is covered due to rpm get/put calls used
> > > > > > > > > around Hw
> > > > > > > > > access in
> > > > > > > > > +     * the work item function.
> > > > > > > > > +     */
> > > > > > > > > +    if (!rpm_suspend && (i915.guc_log_level >= 0))
> > > > > > > > > +        flush_work(&dev_priv->guc.log.flush_work);
> > > > > > > 
> > > > > > > In which case (rpm suspend) the flush_work is idle and this a
> > > > > > > noop.
> > > > > > > That
> > > > > > > you have to pass around such state suggests that you are
> > > > > > > papering
> > > > > > > over a
> > > > > > > bug?
> > > > > > In case of rpm suspend the flush_work may not be a NOOP.
> > > > > > Can use the flush_work for runtime suspend also but in spite of
> > > > > > that
> > > > > > can't prevent the 'RPM wakelock' asserts, as the work item can
> > > > > > get
> > > > > > executed after the rpm ref count drops to zero and before
> > > > > > runtime
> > > > > > suspend kicks in (after autosuspend delay).
> > > > > > 
> > > > > > For that you had earlier suggested to use rpm get/put in the
> > > > > > work item
> > > > > > function, around the register access, but with that had to
> > > > > > remove the
> > > > > > flush_work from the suspend hook, otherwise a deadlock can
> > > > > > happen.
> > > > > > So doing the flush_work conditionally for system suspend case,
> > > > > > as rpm
> > > > > > get/put won't cause the resume of device in that case.
> > > > > > 
> > > > > > Actually I had discussed about this with Imre and as per his
> > > > > > inputs
> > > > > > prepared this patch.
> > > > > 
> > > > > There would be this alternative:
> > > > > 
> > > > Thanks much for suggesting the alternate approach.
> > > > 
> > > > Just to confirm whether I understood everything correctly,
> > > > 
> > > > > in gen9_guc_irq_handler():
> > > > >    WARN_ON(!intel_runtime_pm_get_if_in_use());
> > > > Used WARN, as we don't expect the device to be suspended at this
> > > > juncture, so intel_runtime_pm_get_if_in_use() should return true.
> > > > 
> > > > >    if (!queue_work(log.flush_work))
> > > > If queue_work returns 0, then work item is already pending, so it
> > > > won't
> > > > be queued hence can release the rpm ref count now only.
> > > > >        intel_runtime_pm_put();
> > > > 
> > > > > 
> > > > > and dropping the reference at the end of the work item.
> > > > This will be just like the __intel_autoenable_gt_powersave
> > > > 
> > > > > This would make the flush_work() a nop in case of
> > > > > runtime_suspend().
> > > > So can call the flush_work unconditionally.
> > > > 
> > > > Hope I understood it correctly.
> > 
> > Yes, the above is correct except for my mistake in
> > handling intel_runtime_pm_get_if_in_use() returning false as discussed
> > below.
> > 
> > > > 
> > > Hi Imre,
> > > 
> > > You had suggested to use the below code from irq handler, suspecting
> > > that intel_runtime_pm_get_if_in_use() can return false, if interrupt
> > > gets handled just after device goes out of use.
> > > 
> > > 	if (intel_runtime_pm_get_if_in_use()) {
> > > 		if (!queue_work(log.flush_work))
> > > 			intel_runtime_pm_put();
> > > 	}
> > > 
> > > Do you mean to say that interrupt can come when rpm suspend has
> > > already
> > > started but before the interrupt is disabled from the suspend hook ?
> > > Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
> > > will return false.
> > > 1)	Autosuspend delay elapses (device is marked as suspending)
> > > 2)		intel_runtime_suspend
> > > 3)			intel_guc_suspend
> > > 4)				gen9_disable_guc_interrupts(dev_pri
> > > v);
> > 
> > No, it can return false anytime the last RPM reference is dropped, that
> > is even before the autosuspend delay elapses.
> 
> Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref 
> count has dropped to 0, even if device is still in runtime active state 
> (as autosuspend delay has not elapsed).
> 
>  > But that still makes the
> > likelihood for a missed work item scheduling small, because 1) we want
> > to reduce the autosuspend delay considerably from the current 10 sec
> > and 2) because what you say below about the GPU actually idling before
> > the RPM refcount going to 0.
> > 
> > > If the above hypothesis is correct, then it implies that interrupt
> > > has
> > > to come after autosuspend delay has elapsed for the above scenario to
> > > arise.
> > > 
> > > I think it would be unlikely for the interrupt to come so late
> > > because
> > > device would have gone idle just before the autosuspend period
> > > started
> > > and so no GuC submissions would have been done after that.
> > 
> > Right.
> > 
> > > So the probability of missing a work item could be very less and we
> > > can bear that.
> > 
> > I haven't looked into what is the consequence of missing a work item,
> > you know this better. In any case - since it is still a possibility -
> > if it's a problem you could still make sure in intel_guc_suspend() that
> > any pending work is completed by calling guc_read_update_log_buffer(),
> > host2guc_logbuffer_flush_complete() if necessary after disabling
> > interrupts in intel_guc_suspend().
> Actually ideally guc_read_update_log_buffer() and 
> host2guc_logbuffer_flush_complete() should be called only if the work
> item was actually missed. So will have to detect the missing of work item.

Ok. But note that missing an interrupt when runtime suspending is not
unimaginable in any case, since interrupts can get disabled (and
cleared) before they would get serviced.

> Isn't the original implementation i.e. conditional flushing of work item 
> for the system suspend case, a simpler & cleaner solution ?

Yes, perhaps, especially with the missed work item detection. How about
making the log.flush_wq freezable? Then we could forgo the flush in
both runtime and system suspend.

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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/18/2016 6:29 PM, Imre Deak wrote:
> On to, 2016-08-18 at 16:54 +0530, Goel, Akash wrote:
>>
>> On 8/18/2016 4:25 PM, Imre Deak wrote:
>>> On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
>>>>
>>>> On 8/17/2016 9:07 PM, Goel, Akash wrote:
>>>>>
>>>>>
>>>>> On 8/17/2016 6:41 PM, Imre Deak wrote:
>>>>>> On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
>>>>>>>
>>>>>>> On 8/17/2016 5:11 PM, Chris Wilson wrote:
>>>>>>>> On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin
>>>>>>>> wrote:
>>>>>>>>>
>>>>
>>>>>>>>>> +int intel_guc_suspend(struct drm_device *dev, bool
>>>>>>>>>> rpm_suspend)
>>>>>>>>>>  {
>>>>>>>>>>      struct drm_i915_private *dev_priv = to_i915(dev);
>>>>>>>>>>      struct intel_guc *guc = &dev_priv->guc;
>>>>>>>>>> @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct
>>>>>>>>>> drm_device *dev)
>>>>>>>>>>          return 0;
>>>>>>>>>>
>>>>>>>>>>      gen9_disable_guc_interrupts(dev_priv);
>>>>>>>>>> +    /* Sync is needed only for the system suspend case,
>>>>>>>>>> runtime
>>>>>>>>>> suspend
>>>>>>>>>> +     * case is covered due to rpm get/put calls used
>>>>>>>>>> around Hw
>>>>>>>>>> access in
>>>>>>>>>> +     * the work item function.
>>>>>>>>>> +     */
>>>>>>>>>> +    if (!rpm_suspend && (i915.guc_log_level >= 0))
>>>>>>>>>> +        flush_work(&dev_priv->guc.log.flush_work);
>>>>>>>>
>>>>>>>> In which case (rpm suspend) the flush_work is idle and this a
>>>>>>>> noop.
>>>>>>>> That
>>>>>>>> you have to pass around such state suggests that you are
>>>>>>>> papering
>>>>>>>> over a
>>>>>>>> bug?
>>>>>>> In case of rpm suspend the flush_work may not be a NOOP.
>>>>>>> Can use the flush_work for runtime suspend also but in spite of
>>>>>>> that
>>>>>>> can't prevent the 'RPM wakelock' asserts, as the work item can
>>>>>>> get
>>>>>>> executed after the rpm ref count drops to zero and before
>>>>>>> runtime
>>>>>>> suspend kicks in (after autosuspend delay).
>>>>>>>
>>>>>>> For that you had earlier suggested to use rpm get/put in the
>>>>>>> work item
>>>>>>> function, around the register access, but with that had to
>>>>>>> remove the
>>>>>>> flush_work from the suspend hook, otherwise a deadlock can
>>>>>>> happen.
>>>>>>> So doing the flush_work conditionally for system suspend case,
>>>>>>> as rpm
>>>>>>> get/put won't cause the resume of device in that case.
>>>>>>>
>>>>>>> Actually I had discussed about this with Imre and as per his
>>>>>>> inputs
>>>>>>> prepared this patch.
>>>>>>
>>>>>> There would be this alternative:
>>>>>>
>>>>> Thanks much for suggesting the alternate approach.
>>>>>
>>>>> Just to confirm whether I understood everything correctly,
>>>>>
>>>>>> in gen9_guc_irq_handler():
>>>>>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
>>>>> Used WARN, as we don't expect the device to be suspended at this
>>>>> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>>>>>
>>>>>>    if (!queue_work(log.flush_work))
>>>>> If queue_work returns 0, then work item is already pending, so it
>>>>> won't
>>>>> be queued hence can release the rpm ref count now only.
>>>>>>        intel_runtime_pm_put();
>>>>>
>>>>>>
>>>>>> and dropping the reference at the end of the work item.
>>>>> This will be just like the __intel_autoenable_gt_powersave
>>>>>
>>>>>> This would make the flush_work() a nop in case of
>>>>>> runtime_suspend().
>>>>> So can call the flush_work unconditionally.
>>>>>
>>>>> Hope I understood it correctly.
>>>
>>> Yes, the above is correct except for my mistake in
>>> handling intel_runtime_pm_get_if_in_use() returning false as discussed
>>> below.
>>>
>>>>>
>>>> Hi Imre,
>>>>
>>>> You had suggested to use the below code from irq handler, suspecting
>>>> that intel_runtime_pm_get_if_in_use() can return false, if interrupt
>>>> gets handled just after device goes out of use.
>>>>
>>>> 	if (intel_runtime_pm_get_if_in_use()) {
>>>> 		if (!queue_work(log.flush_work))
>>>> 			intel_runtime_pm_put();
>>>> 	}
>>>>
>>>> Do you mean to say that interrupt can come when rpm suspend has
>>>> already
>>>> started but before the interrupt is disabled from the suspend hook ?
>>>> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
>>>> will return false.
>>>> 1)	Autosuspend delay elapses (device is marked as suspending)
>>>> 2)		intel_runtime_suspend
>>>> 3)			intel_guc_suspend
>>>> 4)				gen9_disable_guc_interrupts(dev_pri
>>>> v);
>>>
>>> No, it can return false anytime the last RPM reference is dropped, that
>>> is even before the autosuspend delay elapses.
>>
>> Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref
>> count has dropped to 0, even if device is still in runtime active state
>> (as autosuspend delay has not elapsed).
>>
>>  > But that still makes the
>>> likelihood for a missed work item scheduling small, because 1) we want
>>> to reduce the autosuspend delay considerably from the current 10 sec
>>> and 2) because what you say below about the GPU actually idling before
>>> the RPM refcount going to 0.
>>>
>>>> If the above hypothesis is correct, then it implies that interrupt
>>>> has
>>>> to come after autosuspend delay has elapsed for the above scenario to
>>>> arise.
>>>>
>>>> I think it would be unlikely for the interrupt to come so late
>>>> because
>>>> device would have gone idle just before the autosuspend period
>>>> started
>>>> and so no GuC submissions would have been done after that.
>>>
>>> Right.
>>>
>>>> So the probability of missing a work item could be very less and we
>>>> can bear that.
>>>
>>> I haven't looked into what is the consequence of missing a work item,
>>> you know this better. In any case - since it is still a possibility -
>>> if it's a problem you could still make sure in intel_guc_suspend() that
>>> any pending work is completed by calling guc_read_update_log_buffer(),
>>> host2guc_logbuffer_flush_complete() if necessary after disabling
>>> interrupts in intel_guc_suspend().
>> Actually ideally guc_read_update_log_buffer() and
>> host2guc_logbuffer_flush_complete() should be called only if the work
>> item was actually missed. So will have to detect the missing of work item.
>
> Ok. But note that missing an interrupt when runtime suspending is not
> unimaginable in any case, since interrupts can get disabled (and
> cleared) before they would get serviced.
>
Fine, missing of interrupt will always be a possibility.

>> Isn't the original implementation i.e. conditional flushing of work item
>> for the system suspend case, a simpler & cleaner solution ?
>
> Yes, perhaps, especially with the missed work item detection. How about
> making the log.flush_wq freezable? Then we could forgo the flush in
> both runtime and system suspend.
>
Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
But after looking at code, this is what I understood :-
1. freezable Workqueues will be frozen before the system suspend
    callbacks are invoked for the devices.
2. Any work item queued after the WQ is marked frozen will be scheduled
    later, on resume.
3. But if a work item was already present in the freezable Workqueue,
    before it was frozen and it did not complete, then system suspend
    itself will be aborted.
4. So if the log.flush_wq is marked as freezable, then flush of
    work item will not be required for the system suspend case.
    And runtime suspend case is already covered with rpm get/put
    around register access in work item function.

It seems there are 2 config options CONFIG_SUSPEND_FREEZER and
CONFIG_FREEZER which have to be enabled for all the above to happen.
If these config options will always be enabled then probably marking
log.flush_wq would work.

Please kindly confirm whether I understood correctly or not, accordingly 
will proceed further.

Best regards
Akash



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

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Imre Deak]

On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
> [...]
> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
> But after looking at code, this is what I understood :-
> 1. freezable Workqueues will be frozen before the system suspend
>     callbacks are invoked for the devices.

Yes.

> 2. Any work item queued after the WQ is marked frozen will be scheduled
>     later, on resume.

Yes.

> 3. But if a work item was already present in the freezable Workqueue,
>     before it was frozen and it did not complete, then system suspend
>     itself will be aborted.

System suspend will be aborted only if any kernel thread didn't
complete within a reasonable amount of time (freeze_timeout_msecs, 20
sec by default). Otherwise already queued items will be properly
waited upon and suspend will proceed.

> 4. So if the log.flush_wq is marked as freezable, then flush of
>     work item will not be required for the system suspend case.
>     And runtime suspend case is already covered with rpm get/put
>     around register access in work item function.

Yes.

> 
> It seems there are 2 config options CONFIG_SUSPEND_FREEZER

This is set whenever system suspend is enabled.

> and
> CONFIG_FREEZER 

This is set except for one platform (powerpc), where I assume freezing
of the tasks is achieved in a different way. In any case it doesn't
matter for us.

--Imre

> which have to be enabled for all the above to happen.
> If these config options will always be enabled then probably marking
> log.flush_wq would work.
> 
> Please kindly confirm whether I understood correctly or not, accordingly 
> will proceed further.
> 
> Best regards
> Akash
> 
> 
> 
> > --Imre
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/18/2016 7:48 PM, Imre Deak wrote:
> On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
>> [...]
>> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
>> But after looking at code, this is what I understood :-
>> 1. freezable Workqueues will be frozen before the system suspend
>>     callbacks are invoked for the devices.
>
> Yes.
>
>> 2. Any work item queued after the WQ is marked frozen will be scheduled
>>     later, on resume.
>
> Yes.
>
>> 3. But if a work item was already present in the freezable Workqueue,
>>     before it was frozen and it did not complete, then system suspend
>>     itself will be aborted.
>
> System suspend will be aborted only if any kernel thread didn't
> complete within a reasonable amount of time (freeze_timeout_msecs, 20
> sec by default). Otherwise already queued items will be properly
> waited upon and suspend will proceed.
Sorry for getting this wrong.
What I understood is that even if there are pending work items on
freezable WQ after freeze_timeout_msecs, then also system suspend would 
be performed.
Sorry couldn't find an explicit/synchronous wait in kernel for the 
pending work items for freezable WQs, but it doesn't matter.

>
>> 4. So if the log.flush_wq is marked as freezable, then flush of
>>     work item will not be required for the system suspend case.
>>     And runtime suspend case is already covered with rpm get/put
>>     around register access in work item function.
>
> Yes.
>
>>
>> It seems there are 2 config options CONFIG_SUSPEND_FREEZER
>
> This is set whenever system suspend is enabled.
>
>> and
>> CONFIG_FREEZER
>
> This is set except for one platform (powerpc), where I assume freezing
> of the tasks is achieved in a different way. In any case it doesn't
> matter for us.
>
Many thanks for providing all this info.

Will then mark the log.flush_wq as freezable.

Best regards
Akash
> --Imre
>
>> which have to be enabled for all the above to happen.
>> If these config options will always be enabled then probably marking
>> log.flush_wq would work.
>>
>> Please kindly confirm whether I understood correctly or not, accordingly
>> will proceed further.
>>
>> Best regards
>> Akash
>>
>>
>>
>>> --Imre
>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Imre Deak]

On to, 2016-08-18 at 20:05 +0530, Goel, Akash wrote:
> 
> On 8/18/2016 7:48 PM, Imre Deak wrote:
> > On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
> > > [...]
> > > Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
> > > But after looking at code, this is what I understood :-
> > > 1. freezable Workqueues will be frozen before the system suspend
> > >     callbacks are invoked for the devices.
> > 
> > Yes.
> > 
> > > 2. Any work item queued after the WQ is marked frozen will be scheduled
> > >     later, on resume.
> > 
> > Yes.
> > 
> > > 3. But if a work item was already present in the freezable Workqueue,
> > >     before it was frozen and it did not complete, then system suspend
> > >     itself will be aborted.
> > 
> > System suspend will be aborted only if any kernel thread didn't
> > complete within a reasonable amount of time (freeze_timeout_msecs, 20
> > sec by default). Otherwise already queued items will be properly
> > waited upon and suspend will proceed.
> Sorry for getting this wrong.
> What I understood is that even if there are pending work items on
> freezable WQ after freeze_timeout_msecs, then also system suspend would 
> be performed.

In case of timeout suspend_prepare()->suspend_freeze_processes()
->freeze_kernel_threads()->try_to_freeze_tasks() will return -EBUSY and
suspend will fail.

> Sorry couldn't find an explicit/synchronous wait in kernel for the 
> pending work items for freezable WQs, but it doesn't matter.

The above try_to_freeze_tasks() will wait until
freeze_workqueues_busy() indicates that there are no work items active
on any freezable queues.

--Imre

> 
> > 
> > > 4. So if the log.flush_wq is marked as freezable, then flush of
> > >     work item will not be required for the system suspend case.
> > >     And runtime suspend case is already covered with rpm get/put
> > >     around register access in work item function.
> > 
> > Yes.
> > 
> > > 
> > > It seems there are 2 config options CONFIG_SUSPEND_FREEZER
> > 
> > This is set whenever system suspend is enabled.
> > 
> > > and
> > > CONFIG_FREEZER
> > 
> > This is set except for one platform (powerpc), where I assume freezing
> > of the tasks is achieved in a different way. In any case it doesn't
> > matter for us.
> > 
> Many thanks for providing all this info.
> 
> Will then mark the log.flush_wq as freezable.
> 
> Best regards
> Akash
> > --Imre
> > 
> > > which have to be enabled for all the above to happen.
> > > If these config options will always be enabled then probably marking
> > > log.flush_wq would work.
> > > 
> > > Please kindly confirm whether I understood correctly or not, accordingly
> > > will proceed further.
> > > 
> > > Best regards
> > > Akash
> > > 
> > > 
> > > 
> > > > --Imre
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Goel, Akash]

On 8/18/2016 8:25 PM, Imre Deak wrote:
> On to, 2016-08-18 at 20:05 +0530, Goel, Akash wrote:
>>
>> On 8/18/2016 7:48 PM, Imre Deak wrote:
>>> On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
>>>> [...]
>>>> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
>>>> But after looking at code, this is what I understood :-
>>>> 1. freezable Workqueues will be frozen before the system suspend
>>>>     callbacks are invoked for the devices.
>>>
>>> Yes.
>>>
>>>> 2. Any work item queued after the WQ is marked frozen will be scheduled
>>>>     later, on resume.
>>>
>>> Yes.
>>>
>>>> 3. But if a work item was already present in the freezable Workqueue,
>>>>     before it was frozen and it did not complete, then system suspend
>>>>     itself will be aborted.
>>>
>>> System suspend will be aborted only if any kernel thread didn't
>>> complete within a reasonable amount of time (freeze_timeout_msecs, 20
>>> sec by default). Otherwise already queued items will be properly
>>> waited upon and suspend will proceed.
>> Sorry for getting this wrong.
>> What I understood is that even if there are pending work items on
>> freezable WQ after freeze_timeout_msecs, then also system suspend would
>> be performed.
>
> In case of timeout suspend_prepare()->suspend_freeze_processes()
> ->freeze_kernel_threads()->try_to_freeze_tasks() will return -EBUSY and
> suspend will fail.
>
So sorry, there was a typo in my last mail, instead of writing 'system 
suspend would be aborted', I wrote 'system suspend would be performed'.

>> Sorry couldn't find an explicit/synchronous wait in kernel for the
>> pending work items for freezable WQs, but it doesn't matter.
>
> The above try_to_freeze_tasks() will wait until
> freeze_workqueues_busy() indicates that there are no work items active
> on any freezable queues.
>
Thanks much for clarifying. I will go through that function again.

Best regards
Akash
> --Imre
>
>>
>>>
>>>> 4. So if the log.flush_wq is marked as freezable, then flush of
>>>>     work item will not be required for the system suspend case.
>>>>     And runtime suspend case is already covered with rpm get/put
>>>>     around register access in work item function.
>>>
>>> Yes.
>>>
>>>>
>>>> It seems there are 2 config options CONFIG_SUSPEND_FREEZER
>>>
>>> This is set whenever system suspend is enabled.
>>>
>>>> and
>>>> CONFIG_FREEZER
>>>
>>> This is set except for one platform (powerpc), where I assume freezing
>>> of the tasks is achieved in a different way. In any case it doesn't
>>> matter for us.
>>>
>> Many thanks for providing all this info.
>>
>> Will then mark the log.flush_wq as freezable.
>>
>> Best regards
>> Akash
>>> --Imre
>>>
>>>> which have to be enabled for all the above to happen.
>>>> If these config options will always be enabled then probably marking
>>>> log.flush_wq would work.
>>>>
>>>> Please kindly confirm whether I understood correctly or not, accordingly
>>>> will proceed further.
>>>>
>>>> Best regards
>>>> Akash
>>>>
>>>>
>>>>
>>>>> --Imre
>>>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx