[PATCH v2 00/11] Support for sustained capturing of GuC firmware logs

[PATCH v2 00/11] Support for sustained capturing of GuC firmware logs

[akash.goel]

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

GuC firmware log its debug messages into a Host-GuC shared memory buffer
and when the buffer is half full it sends a Flush interrupt to Host.
GuC firmware expects that while it is writing to 2nd half of the buffer,
first half would get consumed by Host and then get a flush completed
acknowledgement from Host, so that it does not end up doing any overwrite
causing loss of logs.
So far Flush interrupt wasn't enabled on Host side & User could capture the
contents/snapshot of log buffer through 'i915_guc_log_dump' debugfs iface.
But this couldn't meet couple of key requirements, especially of Validation,
first is to ensure capturing of all boot time logs even with high verbosity
level and second is to enable capturing of logs in a sustained manner like
for the entire duration of a workload.
Now Driver will enable Flush interrupt and on receving it, would copy the
contents of log buffer into its local buffer. The size of local buffer would
be big enough to contain multiple snapshots of the log buffer giving ample
time to User to pull boot time messages.
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 will be operated in a mode
where the old data, not yet collected by User, will be overwritten if buffer
becomes full. So the behavior exhibited will be equivalent to 'dmesg -c'.
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.

Akash Goel (4):
  drm/i915: Add low level set of routines for programming PM IER/IIR/IMR
    register set
  drm/i915: Add a relay backed debugfs interface for capturing GuC logs
  drm/i915: New module param to control the size of buffer used for
    storing GuC firmware logs
  drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer

Chris Wilson (1):
  drm/i915: Support to create write combined type vmaps

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/i915_debugfs.c        |  35 +++-
 drivers/gpu/drm/i915/i915_drv.h            |   6 +-
 drivers/gpu/drm/i915/i915_gem.c            |  57 ++++--
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     |   2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 304 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.c            | 169 ++++++++++++++--
 drivers/gpu/drm/i915/i915_params.c         |   5 +
 drivers/gpu/drm/i915/i915_params.h         |   1 +
 drivers/gpu/drm/i915/i915_reg.h            |  11 ++
 drivers/gpu/drm/i915/intel_drv.h           |   9 +
 drivers/gpu/drm/i915/intel_guc.h           |  10 +
 drivers/gpu/drm/i915/intel_guc_fwif.h      |  53 +++++
 drivers/gpu/drm/i915/intel_guc_loader.c    |  12 +-
 drivers/gpu/drm/i915/intel_lrc.c           |   8 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   2 +-
 15 files changed, 643 insertions(+), 41 deletions(-)

-- 
1.9.2

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

[PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

[akash.goel]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 355b647..28a810f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -826,9 +826,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 8fe96a2..db3c897 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -173,11 +173,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
 	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
 			GUC_CTL_VCS2_ENABLED;
 
-	if (i915.guc_log_level >= 0) {
-		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+	params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
+
+	if (i915.guc_log_level >= 0)
 		params[GUC_CTL_DEBUG] =
 			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
-	}
+	else
+		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
 	if (guc->ads_obj) {
 		u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
-- 
1.9.2

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

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

[akash.goel]

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 944786d..eb10324 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -418,15 +418,62 @@ struct guc_ads {
 	u32 reserved2[4];
 } __packed;
 
+/* GuC logging structures */
+
+enum guc_log_buffer_type {
+	GUC_ISR_LOG_BUFFER,
+	GUC_DPC_LOG_BUFFER,
+	GUC_CRASH_DUMP_LOG_BUFFER,
+	GUC_MAX_LOG_BUFFER
+};
+
+/*
+ * Below state is used for coordination of retrival of GuC logs
+ * by i915. read_ptr is where i915 read last. GuC keeps incrementing
+ * write_ptr on each log entry. When buffer gets half filled and i915
+ * has requested interrupt, GuC will set flush_to_file field and raise the
+ * interrupt. i915 should read the buffer and clear flush_to_file field.
+ * i915 should also update the read_ptr.
+*/
+struct guc_log_buffer_state {
+	u32 marker[2];
+	u32 read_ptr;
+	u32 write_ptr;
+	u32 size;
+	u32 sampled_write_ptr;
+	union {
+		struct {
+			u32 flush_to_file:1;
+			u32 buffer_full_cnt:4;
+			u32 reserved:27;
+		};
+		u32 flags;
+	};
+	u32 version;
+} __packed;
+
+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_LOGBUFFERFLUSH = 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 +495,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/11] 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.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/i915_irq.c  | 55 ++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h |  6 +++++
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ef4919..85a7103 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1806,6 +1806,7 @@ struct drm_i915_private {
 	};
 	u32 gt_irq_mask;
 	u32 pm_irq_mask;
+	u32 pm_ier_mask;
 	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 4378a65..7316ab4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 	__gen6_disable_pm_irq(dev_priv, mask);
 }
 
-void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
+			       uint32_t 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_interrupts(struct drm_i915_private *dev_priv,
+			       uint32_t enable_mask)
+{
+	uint32_t new_val;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	new_val = dev_priv->pm_ier_mask;
+	new_val |= enable_mask;
+
+	dev_priv->pm_ier_mask = new_val;
+	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
+	gen6_enable_pm_irq(dev_priv, enable_mask);
+}
+
+void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
+				uint32_t disable_mask)
+{
+	uint32_t new_val;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	new_val = dev_priv->pm_ier_mask;
+	new_val &= ~disable_mask;
+
+	dev_priv->pm_ier_mask = new_val;
+	__gen6_disable_pm_irq(dev_priv, disable_mask);
+	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
+}
+
+void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	gen6_reset_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
 	dev_priv->rps.pm_iir = 0;
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
@@ -355,9 +393,7 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
 	WARN_ON(dev_priv->rps.pm_iir);
 	WARN_ON(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);
+	gen6_enable_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
@@ -379,9 +415,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_interrupts(dev_priv, dev_priv->pm_rps_events);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
@@ -3770,6 +3804,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 		gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 
 	dev_priv->pm_irq_mask = 0xffffffff;
+	dev_priv->pm_ier_mask = 0x0;
 	GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
 	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
 	/*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8d..2a013fc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1060,6 +1060,12 @@ 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);
 void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv);
 void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv);
+void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
+			      uint32_t reset_mask);
+void gen6_enable_pm_interrupts(struct drm_i915_private *dev_priv,
+			       uint32_t enable_mask);
+void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
+				uint32_t disable_mask);
 u32 gen6_sanitize_rps_pm_mask(struct drm_i915_private *dev_priv, u32 mask);
 void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_enable_interrupts(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 04/11] drm/i915: Support for GuC interrupts

[akash.goel]

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

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

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)

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            | 95 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
 drivers/gpu/drm/i915/intel_drv.h           |  3 +
 drivers/gpu/drm/i915/intel_guc.h           |  5 ++
 drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
 7 files changed, 120 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 85a7103..20c701c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
 	u32 pm_irq_mask;
 	u32 pm_ier_mask;
 	u32 pm_rps_events;
+	u32 guc_events;
 	u32 pipestat_irq_mask[I915_MAX_PIPES];
 
 	struct i915_hotplug hotplug;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 28a810f..8105ddd 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1038,6 +1038,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;
@@ -1064,6 +1066,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 7316ab4..3043e45 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
@@ -422,6 +423,42 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	synchronize_irq(dev_priv->dev->irq);
 }
 
+void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
+{
+	spin_lock_irq(&dev_priv->irq_lock);
+	gen6_reset_pm_interrupts(dev_priv, dev_priv->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(I915_READ(gen6_pm_iir(dev_priv)) &
+					dev_priv->guc_events);
+		dev_priv->guc.interrupts_enabled = true;
+		gen6_enable_pm_interrupts(dev_priv, dev_priv->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;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	cancel_work_sync(&dev_priv->guc.events_work);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	gen6_disable_pm_interrupts(dev_priv, dev_priv->guc_events);
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	synchronize_irq(dev_priv->dev->irq);
+}
+
 /**
  * bdw_update_port_irq - update DE port interrupt
  * @dev_priv: driver private
@@ -1196,6 +1233,33 @@ out:
 	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
 }
 
+static void gen9_guc2host_events_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, guc.events_work);
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	/* Speed up work cancelation during disabling guc interrupts. */
+	if (!dev_priv->guc.interrupts_enabled) {
+		spin_unlock_irq(&dev_priv->irq_lock);
+		return;
+	}
+
+	/* Though this work item gets synced during rpm suspend, but still need
+	 * a rpm get/put to avoid the warning, as it could get executed in a
+	 * window, where rpm ref count has dropped to zero but rpm suspend has
+	 * not kicked in. Generally device is expected to be active only at this
+	 * time so get/put should be really quick.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
+	gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT);
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	/* TODO: Handle the events for which GuC interrupted host */
+
+	intel_runtime_pm_put(dev_priv);
+}
 
 /**
  * ivybridge_parity_work - Workqueue called when a parity error interrupt
@@ -1371,11 +1435,13 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
 			DRM_ERROR("The master control interrupt lied (GT3)!\n");
 	}
 
-	if (master_ctl & GEN8_GT_PM_IRQ) {
+	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
 		gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
-		if (gt_iir[2] & dev_priv->pm_rps_events) {
+		if (gt_iir[2] & (dev_priv->pm_rps_events |
+				 dev_priv->guc_events)) {
 			I915_WRITE_FW(GEN8_GT_IIR(2),
-				      gt_iir[2] & dev_priv->pm_rps_events);
+				      gt_iir[2] & (dev_priv->pm_rps_events |
+						   dev_priv->guc_events));
 			ret = IRQ_HANDLED;
 		} else
 			DRM_ERROR("The master control interrupt lied (PM)!\n");
@@ -1407,6 +1473,9 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
 
 	if (gt_iir[2] & dev_priv->pm_rps_events)
 		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
+
+	if (gt_iir[2] & dev_priv->guc_events)
+		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1653,6 +1722,20 @@ 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) {
+		spin_lock(&dev_priv->irq_lock);
+		if (dev_priv->guc.interrupts_enabled) {
+			/* Process all the GuC to Host events in bottom half */
+			gen6_disable_pm_irq(dev_priv,
+				GEN9_GUC_TO_HOST_INT_EVENT);
+			queue_work(dev_priv->wq, &dev_priv->guc.events_work);
+		}
+		spin_unlock(&dev_priv->irq_lock);
+	}
+}
+
 static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
 				     enum pipe pipe)
 {
@@ -3809,7 +3892,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_irq_mask, 0);
 	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
@@ -4594,6 +4677,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
 	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
+	INIT_WORK(&dev_priv->guc.events_work, gen9_guc2host_events_work);
+
+	if (HAS_GUC_UCODE(dev))
+		dev_priv->guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
 
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c6bfbf8..4441918 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5963,6 +5963,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)
@@ -5974,6 +5975,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 2a013fc..6966ffe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1083,6 +1083,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 3e3e743..ae787e2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -126,6 +126,11 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
+	/*
+	 * work, interrupts_enabled are protected by dev_priv->irq_lock
+	 */
+	struct work_struct events_work;
+	bool interrupts_enabled;
 
 	struct drm_i915_gem_object *ads_obj;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index db3c897..fcf36a2 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -448,6 +448,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;
 
@@ -494,6 +495,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 05/11] drm/i915: Handle log buffer flush interrupt event from GuC

[akash.goel]

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

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

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index b95a510..45f3396 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"
 
@@ -821,7 +823,15 @@ err:
 
 static void* guc_get_write_buffer(struct intel_guc *guc)
 {
-	return NULL;
+	if (!guc->log_relay_chan)
+		return NULL;
+
+	/* Get the pointer to relay sub buffer and copy data into it ourselves.
+	 * Could have used the relay_write() to indirectly copy the data, but
+	 * that would have been bit convoluted, as we also need to update the
+	 * first page containing state data.
+	 */
+	return relay_reserve(guc->log_relay_chan, guc->log_obj->base.size);
 }
 
 static void guc_read_update_log_buffer(struct drm_device *dev)
@@ -878,6 +888,125 @@ static void guc_read_update_log_buffer(struct drm_device *dev)
 	}
 }
 
+/*
+ * Sub buffer switch callback. If this callback is not implemented
+ * relay will operate in non-overwrite mode so will stop accepting
+ * new data if there are no empty sub buffers left.
+ */
+static int subbuf_start_callback(struct rchan_buf *buf,
+				 void *subbuf,
+				 void *prev_subbuf,
+				 size_t prev_padding)
+{
+	/* Always switch to next sub buffer as we don't mind overwriting of
+	 * old data/logs.
+	 */
+	return 1;
+}
+
+/*
+ * file_create() callback. Creates relay file in debugfs.
+ */
+static struct dentry *create_buf_file_callback(const char *filename,
+					       struct dentry *parent,
+					       umode_t mode,
+					       struct rchan_buf *buf,
+					       int *is_global)
+{
+	/*
+	 * 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.
+	 */
+	struct dentry *buf_file = debugfs_create_file("guc_log", mode,
+			parent, buf, &relay_file_operations);
+
+	/* This to enable the use of a single buffer for the relay channel and
+	 * correspondingly have a single file exposed to User, through which
+	 * it can pull the logs in order without any post-processing.
+	 */
+	*is_global = 1;
+
+	return buf_file;
+}
+
+/*
+ * file_remove() default callback. Removes relay file in debugfs.
+ */
+static int remove_buf_file_callback(struct dentry *dentry)
+{
+	debugfs_remove(dentry);
+	return 0;
+}
+
+/* relay channel callbacks */
+static struct rchan_callbacks relay_callbacks = {
+	.subbuf_start = subbuf_start_callback,
+	.create_buf_file = create_buf_file_callback,
+	.remove_buf_file = remove_buf_file_callback,
+};
+
+static void guc_remove_log_relay_file(struct intel_guc *guc)
+{
+	relay_close(guc->log_relay_chan);
+}
+
+static void guc_create_log_relay_file(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_device *dev = dev_priv->dev;
+	struct dentry *log_dir;
+	struct rchan *guc_log_relay_chan;
+	size_t n_subbufs, subbuf_size;
+
+	if (guc->log_relay_chan)
+		return;
+
+	/* 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.
+	 */
+	if (!dev->primary->debugfs_root) {
+		/* logging will remain off */
+		i915.guc_log_level = -1;
+		return;
+	}
+
+	/* For now create the log file in /sys/kernel/debug/dri dir. */
+	log_dir = dev->primary->debugfs_root->d_parent;
+
+	/* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log_obj->base.size;
+	/* TODO: Decide based on the User's input */
+	n_subbufs = 4;
+
+	guc_log_relay_chan = relay_open("guc_log", log_dir,
+			subbuf_size, n_subbufs, &relay_callbacks, dev);
+
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for guc logs\n");
+		/* keep logging off as couldn't create the relay channel in
+		 * in which the logs can be stored.
+		 */
+		i915.guc_log_level = -1;
+		return;
+	}
+
+	guc->log_relay_chan = guc_log_relay_chan;
+}
+
+static void guc_logging_fini(struct intel_guc *guc)
+{
+        guc_remove_log_relay_file(guc);
+}
+
+static void guc_logging_init(struct intel_guc *guc)
+{
+	guc_create_log_relay_file(guc);
+}
+
 static void guc_create_log(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -914,6 +1043,7 @@ static void guc_create_log(struct intel_guc *guc)
 
 	offset = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; /* in pages */
 	guc->log_flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+	guc_logging_init(guc);
 }
 
 static void init_guc_policies(struct guc_policies *policies)
@@ -1074,6 +1204,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	gem_release_guc_obj(dev_priv->guc.ads_obj);
 	guc->ads_obj = NULL;
 
+	guc_logging_fini(guc);
 	gem_release_guc_obj(dev_priv->guc.log_obj);
 	guc->log_obj = NULL;
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b20e167..c675856 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -126,6 +126,7 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
+	struct rchan *log_relay_chan;
 	/*
 	 * work, interrupts_enabled are protected by dev_priv->irq_lock
 	 */
-- 
1.9.2

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

[PATCH 07/11] drm/i915: Forcefully flush GuC log buffer on reset

[akash.goel]

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 45f3396..a32346a 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -168,6 +168,16 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
 
+static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
+{
+	u32 data[2];
+
+	data[0] = HOST2GUC_ACTION_FORCE_LOGBUFFERFLUSH;
+	data[1] = 0;
+
+	return host2guc_action(guc, data, 2);
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -1281,3 +1291,18 @@ void i915_guc_capture_logs(struct drm_device *dev)
 	if (host2guc_action(guc, data, 1))
 		DRM_ERROR("Failed\n");
 }
+
+void i915_guc_capture_logs_on_reset(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+
+	/* First disable the interrupts, will be renabled after reset */
+	gen9_disable_guc_interrupts(dev_priv);
+
+	/* Ask GuC to update the log buffer state */
+	host2guc_force_logbuffer_flush(guc);
+
+	/* GuC would have updated the log buffer by now, so capture it */
+	i915_guc_capture_logs(dev);
+}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bda9093..14d680a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2689,6 +2689,8 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv)
 		 */
 		intel_runtime_pm_get(dev_priv);
 
+		i915_guc_capture_logs_on_reset(dev_priv->dev);
+
 		intel_prepare_reset(dev_priv);
 
 		/*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c675856..1379df4 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -170,5 +170,6 @@ int i915_guc_submit(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_device *dev);
+void i915_guc_capture_logs_on_reset(struct drm_device *dev);
 
 #endif
-- 
1.9.2

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

[PATCH 08/11] 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.

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

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_debugfs.c        | 35 +++++++++++++++++++-
 drivers/gpu/drm/i915/i915_guc_submission.c | 52 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc.h           |  1 +
 3 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f664884..3dd29af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2629,6 +2629,38 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int
+i915_guc_log_control_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	struct intel_guc *guc = &dev_priv->guc;
+	int ret;
+
+	if (!HAS_GUC_UCODE(dev))
+		return -EINVAL;
+
+	if (!i915.enable_guc_submission)
+		return -EINVAL;
+
+	if (guc_fw->guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
+		return -EINVAL;
+
+	if (!guc->log_obj || !guc->log_relay_chan)
+		return -EINVAL;
+
+	intel_runtime_pm_get(dev_priv);
+	ret = i915_guc_log_control(dev, val);
+	intel_runtime_pm_put(dev_priv);
+
+	return ret;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
+			NULL, i915_guc_log_control_set,
+			"0x%08llx\n");
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5491,7 +5523,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 a32346a..fd26a9e 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -168,6 +168,16 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
 	return host2guc_action(guc, data, ARRAY_SIZE(data));
 }
 
+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);
+}
+
 static int host2guc_force_logbuffer_flush(struct intel_guc *guc)
 {
 	u32 data[2];
@@ -1306,3 +1316,45 @@ void i915_guc_capture_logs_on_reset(struct drm_device *dev)
 	/* GuC would have updated the log buffer by now, so capture it */
 	i915_guc_capture_logs(dev);
 }
+
+int i915_guc_log_control(struct drm_device *dev, uint64_t control_val)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	union guc_log_control log_param;
+
+	log_param.logging_enabled = control_val & 0x1;
+	log_param.verbosity = (control_val >> 4) & 0xF;
+
+	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
+	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
+		return -EINVAL;
+
+	if (!host2guc_logging_control(guc, log_param.value)) {
+		/* If log_level was set as -1 at boot time, then interrupt would
+		 * not have been enabled. Can keep the interrupt on even when
+		 * logging is being disabled at runtime, as GuC itself won't
+		 * generate an interrupt in that case.
+		 */
+		if (i915.guc_log_level < 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		/* 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 send the flush interrupt so that Host can
+		 * collect the left over logs also.
+		 */
+		if (!log_param.logging_enabled) {
+			flush_work(&guc->events_work);
+			host2guc_force_logbuffer_flush(guc);
+		}
+
+		i915.guc_log_level = log_param.verbosity;
+	} else {
+		DRM_ERROR("Failed\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 1379df4..01a1bb5 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -171,5 +171,6 @@ 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_device *dev);
 void i915_guc_capture_logs_on_reset(struct drm_device *dev);
+int i915_guc_log_control(struct drm_device *dev, uint64_t control_val);
 
 #endif
-- 
1.9.2

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

[PATCH 09/11] drm/i915: New module param to control the size of buffer used for storing GuC firmware logs

[akash.goel]

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

On recieving the log buffer flush interrupt from GuC firmware, Driver
stores the snapshot of the log buffer in a local buffer, from which
Userspace can pull the logs. By default Driver store, up to, 4 snapshots
of the log buffer in a local buffer (managed by relay).
Added a new module (read only) param, 'guc_log_size', through which User
can specify the number of snapshots of log buffer to be stored in local
buffer. This can be used to ensure capturing of all boot time logs even
with high verbosity level.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index fd26a9e..8c0fd83 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -999,8 +999,7 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
 
 	/* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = guc->log_obj->base.size;
-	/* TODO: Decide based on the User's input */
-	n_subbufs = 4;
+	n_subbufs = i915.guc_log_size;
 
 	guc_log_relay_chan = relay_open("guc_log", log_dir,
 			subbuf_size, n_subbufs, &relay_callbacks, dev);
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8b13bfa..14ce0c4 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -57,6 +57,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_guc_loading = -1,
 	.enable_guc_submission = -1,
 	.guc_log_level = -1,
+	.guc_log_size = 4,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
@@ -214,6 +215,10 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
 
+module_param_named(guc_log_size, i915.guc_log_size, int, 0400);
+MODULE_PARM_DESC(guc_log_size,
+	"Number of sub buffers to store GuC firmware logs (default: 4)");
+
 module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
 MODULE_PARM_DESC(enable_dp_mst,
 	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 0ad020b..89fa832 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -48,6 +48,7 @@ struct i915_params {
 	int enable_guc_loading;
 	int enable_guc_submission;
 	int guc_log_level;
+	int guc_log_size;
 	int use_mmio_flip;
 	int mmio_debug;
 	int edp_vswing;
-- 
1.9.2

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

[PATCH 10/11] drm/i915: Support to create write combined type vmaps

[akash.goel]

From: Chris Wilson <chris@chris-wilson.co.uk>

vmaps has a provision for controlling the page protection bits, with which
we can use to control the mapping type, e.g. WB, WC, UC or even WT.
To allow the caller to choose their mapping type, we add a parameter to
i915_gem_object_pin_map - but we still only allow one vmap to be cached
per object. If the object is currently not pinned, then we recreate the
previous vmap with the new access type, but if it was pinned we report an
error. This effectively limits the access via i915_gem_object_pin_map to a
single mapping type for the lifetime of the object. Not usually a problem,
but something to be aware of when setting up the object's vmap.

We will want to vary the access type to enable WC mappings of ringbuffer
and context objects on !llc platforms, as well as other objects where we
need coherent access to the GPU's pages without going through the GTT

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++-
 drivers/gpu/drm/i915/i915_gem.c         | 57 +++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c        |  8 ++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
 5 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20c701c..3ef1ee5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3197,6 +3197,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 /**
  * i915_gem_object_pin_map - return a contiguous mapping of the entire object
  * @obj - the object to map into kernel address space
+ * &use_wc - whether the mapping should be using WC or WB pgprot_t
  *
  * Calls i915_gem_object_pin_pages() to prevent reaping of the object's
  * pages and then returns a contiguous mapping of the backing storage into
@@ -3208,7 +3209,8 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
  * Returns the pointer through which to access the mapped object, or an
  * ERR_PTR() on error.
  */
-void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
+void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
+					 bool use_wc);
 
 /**
  * i915_gem_object_unpin_map - releases an earlier mapping
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 51191b87..1f65832 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2469,10 +2469,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	list_del(&obj->global_list);
 
 	if (obj->mapping) {
-		if (is_vmalloc_addr(obj->mapping))
-			vunmap(obj->mapping);
+		void *ptr = (void *)((uintptr_t)obj->mapping & ~1);
+		if (is_vmalloc_addr(ptr))
+			vunmap(ptr);
 		else
-			kunmap(kmap_to_page(obj->mapping));
+			kunmap(kmap_to_page(ptr));
 		obj->mapping = NULL;
 	}
 
@@ -2645,7 +2646,8 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 }
 
 /* The 'mapping' part of i915_gem_object_pin_map() below */
-static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
+static void *i915_gem_object_map(const struct drm_i915_gem_object *obj,
+				bool use_wc)
 {
 	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
 	struct sg_table *sgt = obj->pages;
@@ -2657,7 +2659,7 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 	void *addr;
 
 	/* A single page can always be kmapped */
-	if (n_pages == 1)
+	if (n_pages == 1 && !use_wc)
 		return kmap(sg_page(sgt->sgl));
 
 	if (n_pages > ARRAY_SIZE(stack_pages)) {
@@ -2673,7 +2675,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 	/* Check that we have the expected number of pages */
 	GEM_BUG_ON(i != n_pages);
 
-	addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
+	addr = vmap(pages, n_pages, VM_NO_GUARD,
+		    use_wc ? pgprot_writecombine(PAGE_KERNEL_IO) : PAGE_KERNEL);
 
 	if (pages != stack_pages)
 		drm_free_large(pages);
@@ -2682,27 +2685,55 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 }
 
 /* get, pin, and map the pages of the object into kernel space */
-void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
+void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
 {
+	void *ptr;
+	bool has_wc;
+	bool pinned;
 	int ret;
 
 	lockdep_assert_held(&obj->base.dev->struct_mutex);
+	GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
 
 	ret = i915_gem_object_get_pages(obj);
 	if (ret)
 		return ERR_PTR(ret);
 
+	GEM_BUG_ON(obj->pages == NULL);
 	i915_gem_object_pin_pages(obj);
 
-	if (!obj->mapping) {
-		obj->mapping = i915_gem_object_map(obj);
-		if (!obj->mapping) {
-			i915_gem_object_unpin_pages(obj);
-			return ERR_PTR(-ENOMEM);
+	pinned = (obj->pages_pin_count > 1);
+	ptr = (void *)((uintptr_t)obj->mapping & ~1);
+	has_wc = (uintptr_t)obj->mapping & 1;
+
+	if (ptr && has_wc != use_wc) {
+		if (pinned) {
+			ret = -EBUSY;
+			goto err;
+		}
+
+		if (is_vmalloc_addr(ptr))
+			vunmap(ptr);
+		else
+			kunmap(kmap_to_page(ptr));
+		ptr = obj->mapping = NULL;
+	}
+
+	if (!ptr) {
+		ptr = i915_gem_object_map(obj, use_wc);
+		if (!ptr) {
+			ret = -ENOMEM;
+			goto err;
 		}
+
+		obj->mapping = (void *)((uintptr_t)ptr | use_wc);
 	}
 
-	return obj->mapping;
+	return ptr;
+
+err:
+	i915_gem_object_unpin_pages(obj);
+	return ERR_PTR(ret);
 }
 
 void i915_vma_move_to_active(struct i915_vma *vma,
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 80bbe43..edcadce 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -115,7 +115,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 	if (ret)
 		return ERR_PTR(ret);
 
-	addr = i915_gem_object_pin_map(obj);
+	addr = i915_gem_object_pin_map(obj, false);
 	mutex_unlock(&dev->struct_mutex);
 
 	return addr;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 62b0dc6..b6397a7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -971,7 +971,7 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 	if (ret)
 		goto err;
 
-	vaddr = i915_gem_object_pin_map(ce->state);
+	vaddr = i915_gem_object_pin_map(ce->state, false);
 	if (IS_ERR(vaddr)) {
 		ret = PTR_ERR(vaddr);
 		goto unpin_ctx_obj;
@@ -2007,7 +2007,7 @@ lrc_setup_hws(struct intel_engine_cs *engine,
 	/* The HWSP is part of the default context object in LRC mode. */
 	engine->status_page.gfx_addr = i915_gem_obj_ggtt_offset(dctx_obj) +
 				       LRC_PPHWSP_PN * PAGE_SIZE;
-	hws = i915_gem_object_pin_map(dctx_obj);
+	hws = i915_gem_object_pin_map(dctx_obj, false);
 	if (IS_ERR(hws))
 		return PTR_ERR(hws);
 	engine->status_page.page_addr = hws + LRC_PPHWSP_PN * PAGE_SIZE;
@@ -2334,7 +2334,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 		return ret;
 	}
 
-	vaddr = i915_gem_object_pin_map(ctx_obj);
+	vaddr = i915_gem_object_pin_map(ctx_obj, false);
 	if (IS_ERR(vaddr)) {
 		ret = PTR_ERR(vaddr);
 		DRM_DEBUG_DRIVER("Could not map object pages! (%d)\n", ret);
@@ -2568,7 +2568,7 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv,
 		if (!ctx_obj)
 			continue;
 
-		vaddr = i915_gem_object_pin_map(ctx_obj);
+		vaddr = i915_gem_object_pin_map(ctx_obj, false);
 		if (WARN_ON(IS_ERR(vaddr)))
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 04a2d14..0b8ff69 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2212,7 +2212,7 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_i915_private *dev_priv,
 		if (ret)
 			goto err_unpin;
 
-		addr = i915_gem_object_pin_map(obj);
+		addr = i915_gem_object_pin_map(obj, false);
 		if (IS_ERR(addr)) {
 			ret = PTR_ERR(addr);
 			goto err_unpin;
-- 
1.9.2

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

[PATCH 11/11] 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.
Since the data to be read is less than 50 KB, there would not be any
performance implications.

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

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8c0fd83..71c626d 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -866,8 +866,7 @@ static void guc_read_update_log_buffer(struct drm_device *dev)
 	if (!guc->log_obj)
 		return;
 
-	log_buffer_state = src_ptr =
-		kmap_atomic(i915_gem_object_get_page(guc->log_obj, 0));
+	log_buffer_state = src_ptr = guc->log_buf_addr;
 
 	/* Get the pointer to local buffer to store the logs */
 	dst_ptr = log_buffer_copy_state = guc_get_write_buffer(guc);
@@ -897,14 +896,11 @@ static void guc_read_update_log_buffer(struct drm_device *dev)
 		log_buffer_copy_state++;
 	}
 
-	kunmap_atomic(src_ptr);
-
 	/* Now copy the actual logs */
 	for (i=1; (i < guc->log_obj->base.size / PAGE_SIZE) && dst_ptr; i++) {
 		dst_ptr += PAGE_SIZE;
-		src_ptr = kmap_atomic(i915_gem_object_get_page(guc->log_obj, i));
+		src_ptr += PAGE_SIZE;
 		memcpy(dst_ptr, src_ptr, PAGE_SIZE);
-		kunmap_atomic(src_ptr);
 	}
 }
 
@@ -1019,10 +1015,31 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
 static void guc_logging_fini(struct intel_guc *guc)
 {
         guc_remove_log_relay_file(guc);
+
+	if (guc->log_obj)
+		i915_gem_object_unpin_map(guc->log_obj);
+	guc->log_buf_addr = NULL;
 }
 
 static void guc_logging_init(struct intel_guc *guc)
 {
+	void *vaddr;
+
+	/* Create a WC (Uncached for read) mapping so that we can
+	 * directly get the data (up-to-date) from system memory.
+	 */
+	vaddr = i915_gem_object_pin_map(guc->log_obj, true);
+	if (IS_ERR(vaddr)) {
+		DRM_DEBUG_DRIVER("Couldn't map log buffer pages(%ld)\n",
+			PTR_ERR(vaddr));
+		i915.guc_log_level = -1;
+		gem_release_guc_obj(guc->log_obj);
+		guc->log_obj = NULL;
+		return;
+	}
+
+	guc->log_buf_addr = vaddr;
+
 	guc_create_log_relay_file(guc);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 01a1bb5..cf082fa 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -126,6 +126,7 @@ struct intel_guc {
 	struct intel_guc_fw guc_fw;
 	uint32_t log_flags;
 	struct drm_i915_gem_object *log_obj;
+	void *log_buf_addr;
 	struct rchan *log_relay_chan;
 	/*
 	 * work, interrupts_enabled are protected by dev_priv->irq_lock
-- 
1.9.2

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

Re: [PATCH 09/11] drm/i915: New module param to control the size of buffer used for storing GuC firmware logs

[Jani Nikula]

On Mon, 27 Jun 2016, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
>
> On recieving the log buffer flush interrupt from GuC firmware, Driver
> stores the snapshot of the log buffer in a local buffer, from which
> Userspace can pull the logs. By default Driver store, up to, 4 snapshots
> of the log buffer in a local buffer (managed by relay).
> Added a new module (read only) param, 'guc_log_size', through which User
> can specify the number of snapshots of log buffer to be stored in local
> buffer. This can be used to ensure capturing of all boot time logs even
> with high verbosity level.
>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>  drivers/gpu/drm/i915/i915_params.c         | 5 +++++
>  drivers/gpu/drm/i915/i915_params.h         | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index fd26a9e..8c0fd83 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -999,8 +999,7 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
>  
>  	/* Keep the size of sub buffers same as shared log buffer */
>  	subbuf_size = guc->log_obj->base.size;
> -	/* TODO: Decide based on the User's input */
> -	n_subbufs = 4;
> +	n_subbufs = i915.guc_log_size;
>  
>  	guc_log_relay_chan = relay_open("guc_log", log_dir,
>  			subbuf_size, n_subbufs, &relay_callbacks, dev);
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8b13bfa..14ce0c4 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -57,6 +57,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_guc_loading = -1,
>  	.enable_guc_submission = -1,
>  	.guc_log_level = -1,
> +	.guc_log_size = 4,
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
> @@ -214,6 +215,10 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>  MODULE_PARM_DESC(guc_log_level,
>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>  
> +module_param_named(guc_log_size, i915.guc_log_size, int, 0400);
> +MODULE_PARM_DESC(guc_log_size,
> +	"Number of sub buffers to store GuC firmware logs (default: 4)");
> +

I guess my battle against adding all sorts of module parameters all the
time is a futile and lost one. :(

Please at least make it clear what the unit of the size is. It's not
obvious to me, and I shouldn't have to look at the source for that.

BR,
Jani.


>  module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
>  MODULE_PARM_DESC(enable_dp_mst,
>  	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 0ad020b..89fa832 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -48,6 +48,7 @@ struct i915_params {
>  	int enable_guc_loading;
>  	int enable_guc_submission;
>  	int guc_log_level;
> +	int guc_log_size;
>  	int use_mmio_flip;
>  	int mmio_debug;
>  	int edp_vswing;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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 (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (fi-snb-i7-2600)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (fi-snb-i7-2600)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
Test gem_sync:
        Subgroup basic-each:
                pass       -> DMESG-FAIL (fi-hsw-i7-4770k)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (fi-snb-i7-2600)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (ro-ilk1-i5-650)
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (fi-snb-i7-2600)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-byt-n2820)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (ro-hsw-i3-4010u)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
        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)

fi-hsw-i7-4770k  total:229  pass:188  dwarn:5   dfail:1   fail:2   skip:33 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:170  dwarn:4   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:197  dwarn:7   dfail:1   fail:2   skip:22 
ro-bdw-i7-5600u  total:229  pass:185  dwarn:5   dfail:1   fail:0   skip:38 
ro-byt-n2820     total:229  pass:174  dwarn:4   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:190  dwarn:5   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:190  dwarn:5   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:151  dwarn:4   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:151  dwarn:4   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:181  dwarn:5   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:185  dwarn:5   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:201  dwarn:6   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:175  dwarn:4   dfail:1   fail:1   skip:48 
fi-kbl-qkkr failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1312/

892ee30 drm-intel-nightly: 2016y-06m-27d-13h-05m-35s UTC integration manifest
7ed97a3 drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer
9a3eb74 drm/i915: Support to create write combined type vmaps
ab703bb drm/i915: New module param to control the size of buffer used for storing GuC firmware logs
b9f3c3f drm/i915: Debugfs support for GuC logging control
739c548 drm/i915: Forcefully flush GuC log buffer on reset
620db9d drm/i915: Add a relay backed debugfs interface for capturing GuC logs
636a1cc drm/i915: Handle log buffer flush interrupt event from GuC
429e878 drm/i915: Support for GuC interrupts
d5d50a3 drm/i915: Add low level set of routines for programming PM IER/IIR/IMR register set
2a7831b drm/i915: Add GuC ukernel logging related fields to fw interface file
00f1b53 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 06/11] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160627]
[cannot apply to v4.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/akash-goel-intel-com/Support-for-sustained-capturing-of-GuC-firmware-logs/20160627-200950
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-b0-06271757 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "relay_file_operations" [drivers/gpu/drm/i915/i915.ko] undefined!
>> ERROR: "relay_switch_subbuf" [drivers/gpu/drm/i915/i915.ko] undefined!
>> ERROR: "relay_close" [drivers/gpu/drm/i915/i915.ko] undefined!
>> ERROR: "relay_open" [drivers/gpu/drm/i915/i915.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27797 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 09/11] drm/i915: New module param to control the size of buffer used for storing GuC firmware logs

[Goel, Akash]

On 6/27/2016 7:01 PM, Jani Nikula wrote:
> On Mon, 27 Jun 2016, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> On recieving the log buffer flush interrupt from GuC firmware, Driver
>> stores the snapshot of the log buffer in a local buffer, from which
>> Userspace can pull the logs. By default Driver store, up to, 4 snapshots
>> of the log buffer in a local buffer (managed by relay).
>> Added a new module (read only) param, 'guc_log_size', through which User
>> can specify the number of snapshots of log buffer to be stored in local
>> buffer. This can be used to ensure capturing of all boot time logs even
>> with high verbosity level.
>>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>>  drivers/gpu/drm/i915/i915_params.c         | 5 +++++
>>  drivers/gpu/drm/i915/i915_params.h         | 1 +
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index fd26a9e..8c0fd83 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -999,8 +999,7 @@ static void guc_create_log_relay_file(struct intel_guc *guc)
>>
>>  	/* Keep the size of sub buffers same as shared log buffer */
>>  	subbuf_size = guc->log_obj->base.size;
>> -	/* TODO: Decide based on the User's input */
>> -	n_subbufs = 4;
>> +	n_subbufs = i915.guc_log_size;
>>
>>  	guc_log_relay_chan = relay_open("guc_log", log_dir,
>>  			subbuf_size, n_subbufs, &relay_callbacks, dev);
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 8b13bfa..14ce0c4 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -57,6 +57,7 @@ struct i915_params i915 __read_mostly = {
>>  	.enable_guc_loading = -1,
>>  	.enable_guc_submission = -1,
>>  	.guc_log_level = -1,
>> +	.guc_log_size = 4,
>>  	.enable_dp_mst = true,
>>  	.inject_load_failure = 0,
>>  	.enable_dpcd_backlight = false,
>> @@ -214,6 +215,10 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>>  MODULE_PARM_DESC(guc_log_level,
>>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>>
>> +module_param_named(guc_log_size, i915.guc_log_size, int, 0400);
>> +MODULE_PARM_DESC(guc_log_size,
>> +	"Number of sub buffers to store GuC firmware logs (default: 4)");
>> +
>
> I guess my battle against adding all sorts of module parameters all the
> time is a futile and lost one. :(
>
> Please at least make it clear what the unit of the size is. It's not
> obvious to me, and I shouldn't have to look at the source for that.
>

Sorry for not choosing a suitable name in first place.
I agree the name should be indicative of the unit.
As you would have seen, the parameter provides number of snapshots of 
the Log buffer which can be stored on Driver side.
The size of one snapshot or Log buffer is not so important here and can
change in future.

Please suggest an appropriate name ('guc_log_buffer_nr' ?)

Best regards
Akash
> BR,
> Jani.
>
>
>>  module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600);
>>  MODULE_PARM_DESC(enable_dp_mst,
>>  	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>> index 0ad020b..89fa832 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -48,6 +48,7 @@ struct i915_params {
>>  	int enable_guc_loading;
>>  	int enable_guc_submission;
>>  	int guc_log_level;
>> +	int guc_log_size;
>>  	int use_mmio_flip;
>>  	int mmio_debug;
>>  	int edp_vswing;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

[Tvrtko Ursulin]

On 27/06/16 13:16, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> GuC Log buffer allocation was tied up with verbosity level kernel parameter
> i915.guc_log_level. User could be given a provision to enable logging at
> runtime and not necessarily during load time only. This patch will perform
> allocation of shared log buffer always but will initially enable logging on
> GuC side through init params based on i915.guc_log_level.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
>   2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 355b647..28a810f 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -826,9 +826,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 8fe96a2..db3c897 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -173,11 +173,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>   	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>   			GUC_CTL_VCS2_ENABLED;
>
> -	if (i915.guc_log_level >= 0) {
> -		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
> +	params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
> +
> +	if (i915.guc_log_level >= 0)
>   		params[GUC_CTL_DEBUG] =
>   			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> -	}
> +	else
> +		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>
>   	if (guc->ads_obj) {
>   		u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>

I did not manage to understand what is the benefit of always allocating 
the log buffer? If the user never enables logging it just wasted 11 
pages of memory, correct?

Looking at the later patches in the series, could you instead create the 
log buffer when logging is enabled via debugfs or implicitly via the 
relayfs access?

Or is the problem then that you would then have to reset the GuC to 
activate it?

Regards,

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

Re: [PATCH 02/11] drm/i915: Add GuC ukernel logging related fields to fw interface file

[Tvrtko Ursulin]

On 27/06/16 13:16, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> The first page of the GuC log buffer contains state info or meta data
> which is required to parse the logs contained in the subsequent pages.
> The structure representing the state info is added to interface file
> as Driver would need to handle log buffer flush interrupts from GuC.
> Added an enum for the different message/event types that can be send
> by the GuC ukernel to Host.
> Also added 2 new Host to GuC action types to inform GuC when Host has
> flushed the log buffer and forcefuly cause the GuC to send a new
> log buffer flush interrupt.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_fwif.h | 53 +++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 944786d..eb10324 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -418,15 +418,62 @@ struct guc_ads {
>   	u32 reserved2[4];
>   } __packed;
>
> +/* GuC logging structures */
> +
> +enum guc_log_buffer_type {
> +	GUC_ISR_LOG_BUFFER,
> +	GUC_DPC_LOG_BUFFER,
> +	GUC_CRASH_DUMP_LOG_BUFFER,
> +	GUC_MAX_LOG_BUFFER
> +};
> +
> +/*
> + * Below state is used for coordination of retrival of GuC logs
> + * by i915. read_ptr is where i915 read last. GuC keeps incrementing
> + * write_ptr on each log entry. When buffer gets half filled and i915
> + * has requested interrupt, GuC will set flush_to_file field and raise the
> + * interrupt. i915 should read the buffer and clear flush_to_file field.
> + * i915 should also update the read_ptr.
> +*/
> +struct guc_log_buffer_state {
> +	u32 marker[2];
> +	u32 read_ptr;
> +	u32 write_ptr;
> +	u32 size;
> +	u32 sampled_write_ptr;
> +	union {
> +		struct {
> +			u32 flush_to_file:1;
> +			u32 buffer_full_cnt:4;
> +			u32 reserved:27;
> +		};
> +		u32 flags;
> +	};
> +	u32 version;
> +} __packed;

Suggest documenting things like the unit of read/write_ptr, size, which 
files are NA/RO/RW for i915, what is sampled_write_ptr, what is the 
marker etc.

> +
> +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_LOGBUFFERFLUSH = 0x302,

Not that important but just strikes me it would be better to settle for 
either LOG_BUFFER_FLUSH or LOGBUFFERFLUSH and not use both conventions.

>   	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 +495,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
>

Regards,

Tvrtko

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

Re: [PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

[Goel, Akash]

On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 13:16, akash.goel@intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>
>> GuC Log buffer allocation was tied up with verbosity level kernel
>> parameter
>> i915.guc_log_level. User could be given a provision to enable logging at
>> runtime and not necessarily during load time only. This patch will
>> perform
>> allocation of shared log buffer always but will initially enable
>> logging on
>> GuC side through init params based on i915.guc_log_level.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
>>   drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 355b647..28a810f 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -826,9 +826,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 8fe96a2..db3c897 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -173,11 +173,13 @@ static void set_guc_init_params(struct
>> drm_i915_private *dev_priv)
>>       params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>>               GUC_CTL_VCS2_ENABLED;
>>
>> -    if (i915.guc_log_level >= 0) {
>> -        params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>> +    params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>> +
>> +    if (i915.guc_log_level >= 0)
>>           params[GUC_CTL_DEBUG] =
>>               i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>> -    }
>> +    else
>> +        params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>
>>       if (guc->ads_obj) {
>>           u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>>
>
> I did not manage to understand what is the benefit of always allocating
> the log buffer? If the user never enables logging it just wasted 11
> pages of memory, correct?
>
Yes if User never enables the logging at runtime, 11 RAM pages will be 
wasted.

Currently the pages are permanently pinned in GGTT also.
The GGTT address of log buffer is passed in the GuC firmware init 
params, at firmware loading time.

Probably this can be circumvented, if pages can be pinned right before
enabling logging (but using the same GGTT address).

> Looking at the later patches in the series, could you instead create the
> log buffer when logging is enabled via debugfs or implicitly via the
> relayfs access?
>
> Or is the problem then that you would then have to reset the GuC to
> activate it?

Yes GuC would have to be reset & firmware needs to be reloaded to pass 
the log buffer address.

Best regards
Akash

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

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

[Tvrtko Ursulin]

On 27/06/16 13:16, akash.goel@intel.com wrote:
> 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.
>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>   drivers/gpu/drm/i915/i915_irq.c  | 55 ++++++++++++++++++++++++++++++++--------
>   drivers/gpu/drm/i915/intel_drv.h |  6 +++++
>   3 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9ef4919..85a7103 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1806,6 +1806,7 @@ struct drm_i915_private {
>   	};
>   	u32 gt_irq_mask;
>   	u32 pm_irq_mask;
> +	u32 pm_ier_mask;
>   	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 4378a65..7316ab4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>   	__gen6_disable_pm_irq(dev_priv, mask);
>   }
>
> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
> +			       uint32_t reset_mask)

Kernel prefers u32. It is not that overall i915 is clean in that 
respect, but every time maintainers merge patches checkpatch shouts 
about it, and more noise tougher it is to spot more important issues. I 
would appreciate if u32 was used throughout.

>   {
>   	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_interrupts(struct drm_i915_private *dev_priv,
> +			       uint32_t enable_mask)
> +{
> +	uint32_t new_val;
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	new_val = dev_priv->pm_ier_mask;
> +	new_val |= enable_mask;
> +
> +	dev_priv->pm_ier_mask = new_val;
> +	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
> +	gen6_enable_pm_irq(dev_priv, enable_mask);

Hm, will this be confusing that we will have gen6_enable_pm_interrupts 
and gen6_enable_pm_irq, so extremely similar names and same parameters, 
but for different use?

Maybe rename the old one to gen6_unmask_pm_irq and name this one 
gen6_enable_pm_irq ? If there is really need to have both. Or add some 
kerneldoc explaining which one is used for what?

> +}
> +
> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
> +				uint32_t disable_mask)
> +{
> +	uint32_t new_val;
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	new_val = dev_priv->pm_ier_mask;
> +	new_val &= ~disable_mask;
> +
> +	dev_priv->pm_ier_mask = new_val;
> +	__gen6_disable_pm_irq(dev_priv, disable_mask);
> +	I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
> +}
> +
> +void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	gen6_reset_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>   	dev_priv->rps.pm_iir = 0;
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
> @@ -355,9 +393,7 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	WARN_ON(dev_priv->rps.pm_iir);
>   	WARN_ON(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);
> +	gen6_enable_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>
>   	spin_unlock_irq(&dev_priv->irq_lock);
>   }
> @@ -379,9 +415,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_interrupts(dev_priv, dev_priv->pm_rps_events);
>
>   	spin_unlock_irq(&dev_priv->irq_lock);
>
> @@ -3770,6 +3804,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>   		gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
>   	dev_priv->pm_irq_mask = 0xffffffff;
> +	dev_priv->pm_ier_mask = 0x0;
>   	GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
>   	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>   	/*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3156d8d..2a013fc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1060,6 +1060,12 @@ 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);
>   void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv);
>   void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv);
> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
> +			      uint32_t reset_mask);
> +void gen6_enable_pm_interrupts(struct drm_i915_private *dev_priv,
> +			       uint32_t enable_mask);
> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
> +				uint32_t disable_mask);
>   u32 gen6_sanitize_rps_pm_mask(struct drm_i915_private *dev_priv, u32 mask);
>   void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv);
>   void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv);
>

Regards,

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

Re: [PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

[Tvrtko Ursulin]

On 27/06/16 16:32, Goel, Akash wrote:
>
>
> On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote:
>>
>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>
>>> GuC Log buffer allocation was tied up with verbosity level kernel
>>> parameter
>>> i915.guc_log_level. User could be given a provision to enable logging at
>>> runtime and not necessarily during load time only. This patch will
>>> perform
>>> allocation of shared log buffer always but will initially enable
>>> logging on
>>> GuC side through init params based on i915.guc_log_level.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
>>>   drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
>>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 355b647..28a810f 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -826,9 +826,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 8fe96a2..db3c897 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> @@ -173,11 +173,13 @@ static void set_guc_init_params(struct
>>> drm_i915_private *dev_priv)
>>>       params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>>>               GUC_CTL_VCS2_ENABLED;
>>>
>>> -    if (i915.guc_log_level >= 0) {
>>> -        params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>> +    params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>> +
>>> +    if (i915.guc_log_level >= 0)
>>>           params[GUC_CTL_DEBUG] =
>>>               i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>>> -    }
>>> +    else
>>> +        params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>>
>>>       if (guc->ads_obj) {
>>>           u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>>>
>>
>> I did not manage to understand what is the benefit of always allocating
>> the log buffer? If the user never enables logging it just wasted 11
>> pages of memory, correct?
>>
> Yes if User never enables the logging at runtime, 11 RAM pages will be
> wasted.
>
> Currently the pages are permanently pinned in GGTT also.
> The GGTT address of log buffer is passed in the GuC firmware init
> params, at firmware loading time.
>
> Probably this can be circumvented, if pages can be pinned right before
> enabling logging (but using the same GGTT address).
>
>> Looking at the later patches in the series, could you instead create the
>> log buffer when logging is enabled via debugfs or implicitly via the
>> relayfs access?
>>
>> Or is the problem then that you would then have to reset the GuC to
>> activate it?
>
> Yes GuC would have to be reset & firmware needs to be reloaded to pass
> the log buffer address.

Right, as minimum I think commit message needs to explain that. The 
current explanation does not hold anyway since it is not possible to 
enable it via modifying the module parameter.

Btw have you considered keeping the module param as a global GuC logging 
enable and adding new code on top? So keep the current code to only 
allocate the buffer when module param is set, and then if it isn't fail 
the later userspace triggered attempts to start the logging (in debugfs 
or relayfs)?

Regards,

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

Re: [PATCH 01/11] drm/i915: Decouple GuC log setup from verbosity parameter

[Goel, Akash]

On 6/27/2016 9:26 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 16:32, Goel, Akash wrote:
>>
>>
>> On 6/27/2016 8:30 PM, Tvrtko Ursulin wrote:
>>>
>>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>
>>>> GuC Log buffer allocation was tied up with verbosity level kernel
>>>> parameter
>>>> i915.guc_log_level. User could be given a provision to enable
>>>> logging at
>>>> runtime and not necessarily during load time only. This patch will
>>>> perform
>>>> allocation of shared log buffer always but will initially enable
>>>> logging on
>>>> GuC side through init params based on i915.guc_log_level.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
>>>>   drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
>>>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> index 355b647..28a810f 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>> @@ -826,9 +826,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 8fe96a2..db3c897 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> @@ -173,11 +173,13 @@ static void set_guc_init_params(struct
>>>> drm_i915_private *dev_priv)
>>>>       params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>>>>               GUC_CTL_VCS2_ENABLED;
>>>>
>>>> -    if (i915.guc_log_level >= 0) {
>>>> -        params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>>> +    params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
>>>> +
>>>> +    if (i915.guc_log_level >= 0)
>>>>           params[GUC_CTL_DEBUG] =
>>>>               i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>>>> -    }
>>>> +    else
>>>> +        params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>>>
>>>>       if (guc->ads_obj) {
>>>>           u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>>>>
>>>
>>> I did not manage to understand what is the benefit of always allocating
>>> the log buffer? If the user never enables logging it just wasted 11
>>> pages of memory, correct?
>>>
>> Yes if User never enables the logging at runtime, 11 RAM pages will be
>> wasted.
>>
>> Currently the pages are permanently pinned in GGTT also.
>> The GGTT address of log buffer is passed in the GuC firmware init
>> params, at firmware loading time.
>>
>> Probably this can be circumvented, if pages can be pinned right before
>> enabling logging (but using the same GGTT address).
>>
>>> Looking at the later patches in the series, could you instead create the
>>> log buffer when logging is enabled via debugfs or implicitly via the
>>> relayfs access?
>>>
>>> Or is the problem then that you would then have to reset the GuC to
>>> activate it?
>>
>> Yes GuC would have to be reset & firmware needs to be reloaded to pass
>> the log buffer address.
>
> Right, as minimum I think commit message needs to explain that. The
> current explanation does not hold anyway since it is not possible to
> enable it via modifying the module parameter.

Right, there should have been an explanation citing the constraint in 
late allocation of log buffer when logging is enabled.
Sorry for missing.

>
> Btw have you considered keeping the module param as a global GuC logging
> enable and adding new code on top? So keep the current code to only
> allocate the buffer when module param is set, and then if it isn't fail
> the later userspace triggered attempts to start the logging (in debugfs
> or relayfs)?

Yes that was considered, keeping module param as the master control and 
allowing disable/enable of logging at runtime (through debugfs) only
when module param is set at boot time.

IIRC there was a request from Validation to keep logging control 
independent of boot time value of module param. So even if system
booted with guc_log_level as -1, still allow the logging to be enabled
at runtime later, through a debugfs interface 'i915_guc_log_control'.

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

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

[Goel, Akash]

On 6/27/2016 9:16 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 13:16, akash.goel@intel.com wrote:
>> 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.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>   drivers/gpu/drm/i915/i915_irq.c  | 55
>> ++++++++++++++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_drv.h |  6 +++++
>>   3 files changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 9ef4919..85a7103 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1806,6 +1806,7 @@ struct drm_i915_private {
>>       };
>>       u32 gt_irq_mask;
>>       u32 pm_irq_mask;
>> +    u32 pm_ier_mask;
>>       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 4378a65..7316ab4 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private
>> *dev_priv, uint32_t mask)
>>       __gen6_disable_pm_irq(dev_priv, mask);
>>   }
>>
>> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t reset_mask)
>
> Kernel prefers u32. It is not that overall i915 is clean in that
> respect, but every time maintainers merge patches checkpatch shouts
> about it, and more noise tougher it is to spot more important issues. I
> would appreciate if u32 was used throughout.

Fine, will use u32.

>
>>   {
>>       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_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t enable_mask)
>> +{
>> +    uint32_t new_val;
>> +
>> +    assert_spin_locked(&dev_priv->irq_lock);
>> +
>> +    new_val = dev_priv->pm_ier_mask;
>> +    new_val |= enable_mask;
>> +
>> +    dev_priv->pm_ier_mask = new_val;
>> +    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
>> +    gen6_enable_pm_irq(dev_priv, enable_mask);
>
> Hm, will this be confusing that we will have gen6_enable_pm_interrupts
> and gen6_enable_pm_irq, so extremely similar names and same parameters,
> but for different use?
Sorry for using confusing, ambiguous names.
>
> Maybe rename the old one to gen6_unmask_pm_irq and name this one
> gen6_enable_pm_irq ? If there is really need to have both. Or add some
> kerneldoc explaining which one is used for what?

Can I do like this, keep gen6_enable_pm_interrupts as is and rename 
gen6_enable_pm_irq to gen6_unmask_pm_irq.
Similarly also rename gen6_disable_pm_irq to gen6_mask_pm_irq.

Best regards
Akash
>
>> +}
>> +
>> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                uint32_t disable_mask)
>> +{
>> +    uint32_t new_val;
>> +
>> +    assert_spin_locked(&dev_priv->irq_lock);
>> +
>> +    new_val = dev_priv->pm_ier_mask;
>> +    new_val &= ~disable_mask;
>> +
>> +    dev_priv->pm_ier_mask = new_val;
>> +    __gen6_disable_pm_irq(dev_priv, disable_mask);
>> +    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
>> +}
>> +
>> +void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>> +{
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +    gen6_reset_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>>       dev_priv->rps.pm_iir = 0;
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>> @@ -355,9 +393,7 @@ void gen6_enable_rps_interrupts(struct
>> drm_i915_private *dev_priv)
>>       WARN_ON(dev_priv->rps.pm_iir);
>>       WARN_ON(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);
>> +    gen6_enable_pm_interrupts(dev_priv, dev_priv->pm_rps_events);
>>
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>   }
>> @@ -379,9 +415,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_interrupts(dev_priv, dev_priv->pm_rps_events);
>>
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>
>> @@ -3770,6 +3804,7 @@ static void gen8_gt_irq_postinstall(struct
>> drm_i915_private *dev_priv)
>>           gt_interrupts[0] |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>>
>>       dev_priv->pm_irq_mask = 0xffffffff;
>> +    dev_priv->pm_ier_mask = 0x0;
>>       GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
>>       GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
>>       /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 3156d8d..2a013fc 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1060,6 +1060,12 @@ 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);
>>   void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv);
>>   void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv);
>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                  uint32_t reset_mask);
>> +void gen6_enable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                   uint32_t enable_mask);
>> +void gen6_disable_pm_interrupts(struct drm_i915_private *dev_priv,
>> +                uint32_t disable_mask);
>>   u32 gen6_sanitize_rps_pm_mask(struct drm_i915_private *dev_priv, u32
>> mask);
>>   void intel_runtime_pm_disable_interrupts(struct drm_i915_private
>> *dev_priv);
>>   void intel_runtime_pm_enable_interrupts(struct drm_i915_private
>> *dev_priv);
>>
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160627]
[cannot apply to v4.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/akash-goel-intel-com/Support-for-sustained-capturing-of-GuC-firmware-logs/20160627-200950
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x0-06272056 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `create_buf_file_callback':
>> i915_guc_submission.c:(.text+0x28bef0): undefined reference to `relay_file_operations'
   drivers/built-in.o: In function `i915_guc_submission_init':
>> (.text+0x28cb08): undefined reference to `relay_open'
   drivers/built-in.o: In function `i915_guc_submission_fini':
>> (.text+0x28d522): undefined reference to `relay_close'
   drivers/built-in.o: In function `i915_guc_capture_logs':
>> (.text+0x28d835): undefined reference to `relay_switch_subbuf'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22333 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

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

[Tvrtko Ursulin]

On 27/06/16 17:35, Goel, Akash wrote:
> On 6/27/2016 9:16 PM, Tvrtko Ursulin wrote:
>>
>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>> 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.
>>>
>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>>   drivers/gpu/drm/i915/i915_irq.c  | 55
>>> ++++++++++++++++++++++++++++++++--------
>>>   drivers/gpu/drm/i915/intel_drv.h |  6 +++++
>>>   3 files changed, 52 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 9ef4919..85a7103 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1806,6 +1806,7 @@ struct drm_i915_private {
>>>       };
>>>       u32 gt_irq_mask;
>>>       u32 pm_irq_mask;
>>> +    u32 pm_ier_mask;
>>>       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 4378a65..7316ab4 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private
>>> *dev_priv, uint32_t mask)
>>>       __gen6_disable_pm_irq(dev_priv, mask);
>>>   }
>>>
>>> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>>> +                   uint32_t reset_mask)
>>
>> Kernel prefers u32. It is not that overall i915 is clean in that
>> respect, but every time maintainers merge patches checkpatch shouts
>> about it, and more noise tougher it is to spot more important issues. I
>> would appreciate if u32 was used throughout.
>
> Fine, will use u32.

Thanks!

>>>   {
>>>       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_interrupts(struct drm_i915_private *dev_priv,
>>> +                   uint32_t enable_mask)
>>> +{
>>> +    uint32_t new_val;
>>> +
>>> +    assert_spin_locked(&dev_priv->irq_lock);
>>> +
>>> +    new_val = dev_priv->pm_ier_mask;
>>> +    new_val |= enable_mask;
>>> +
>>> +    dev_priv->pm_ier_mask = new_val;
>>> +    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
>>> +    gen6_enable_pm_irq(dev_priv, enable_mask);
>>
>> Hm, will this be confusing that we will have gen6_enable_pm_interrupts
>> and gen6_enable_pm_irq, so extremely similar names and same parameters,
>> but for different use?
> Sorry for using confusing, ambiguous names.
>>
>> Maybe rename the old one to gen6_unmask_pm_irq and name this one
>> gen6_enable_pm_irq ? If there is really need to have both. Or add some
>> kerneldoc explaining which one is used for what?
>
> Can I do like this, keep gen6_enable_pm_interrupts as is and rename
> gen6_enable_pm_irq to gen6_unmask_pm_irq.
> Similarly also rename gen6_disable_pm_irq to gen6_mask_pm_irq.

Yes for mask/unmask, but I think the suffix really needs to be the same 
since it is the same functional family.

Regards,

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

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

[Goel, Akash]

On 6/28/2016 2:05 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 17:35, Goel, Akash wrote:
>> On 6/27/2016 9:16 PM, Tvrtko Ursulin wrote:
>>>
>>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>>> 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.
>>>>
>>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>>>   drivers/gpu/drm/i915/i915_irq.c  | 55
>>>> ++++++++++++++++++++++++++++++++--------
>>>>   drivers/gpu/drm/i915/intel_drv.h |  6 +++++
>>>>   3 files changed, 52 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 9ef4919..85a7103 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1806,6 +1806,7 @@ struct drm_i915_private {
>>>>       };
>>>>       u32 gt_irq_mask;
>>>>       u32 pm_irq_mask;
>>>> +    u32 pm_ier_mask;
>>>>       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 4378a65..7316ab4 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -336,14 +336,52 @@ void gen6_disable_pm_irq(struct drm_i915_private
>>>> *dev_priv, uint32_t mask)
>>>>       __gen6_disable_pm_irq(dev_priv, mask);
>>>>   }
>>>>
>>>> -void gen6_reset_rps_interrupts(struct drm_i915_private *dev_priv)
>>>> +void gen6_reset_pm_interrupts(struct drm_i915_private *dev_priv,
>>>> +                   uint32_t reset_mask)
>>>
>>> Kernel prefers u32. It is not that overall i915 is clean in that
>>> respect, but every time maintainers merge patches checkpatch shouts
>>> about it, and more noise tougher it is to spot more important issues. I
>>> would appreciate if u32 was used throughout.
>>
>> Fine, will use u32.
>
> Thanks!
>
>>>>   {
>>>>       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_interrupts(struct drm_i915_private *dev_priv,
>>>> +                   uint32_t enable_mask)
>>>> +{
>>>> +    uint32_t new_val;
>>>> +
>>>> +    assert_spin_locked(&dev_priv->irq_lock);
>>>> +
>>>> +    new_val = dev_priv->pm_ier_mask;
>>>> +    new_val |= enable_mask;
>>>> +
>>>> +    dev_priv->pm_ier_mask = new_val;
>>>> +    I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier_mask);
>>>> +    gen6_enable_pm_irq(dev_priv, enable_mask);
>>>
>>> Hm, will this be confusing that we will have gen6_enable_pm_interrupts
>>> and gen6_enable_pm_irq, so extremely similar names and same parameters,
>>> but for different use?
>> Sorry for using confusing, ambiguous names.
>>>
>>> Maybe rename the old one to gen6_unmask_pm_irq and name this one
>>> gen6_enable_pm_irq ? If there is really need to have both. Or add some
>>> kerneldoc explaining which one is used for what?
>>
>> Can I do like this, keep gen6_enable_pm_interrupts as is and rename
>> gen6_enable_pm_irq to gen6_unmask_pm_irq.
>> Similarly also rename gen6_disable_pm_irq to gen6_mask_pm_irq.
>
> Yes for mask/unmask, but I think the suffix really needs to be the same
> since it is the same functional family.
Fine, so will rename gen6_enable_pm_interrupts to gen6_enable_pm_irq,
and gen6_enable_pm_irq to gen6_unmask_pm_irq

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

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

[Chris Wilson]

On Mon, Jun 27, 2016 at 05:46:53PM +0530, akash.goel@intel.com wrote:
> +static void guc_remove_log_relay_file(struct intel_guc *guc)
> +{
> +	relay_close(guc->log_relay_chan);
> +}
> +
> +static void guc_create_log_relay_file(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct dentry *log_dir;
> +	struct rchan *guc_log_relay_chan;
> +	size_t n_subbufs, subbuf_size;
> +
> +	if (guc->log_relay_chan)
> +		return;
> +
> +	/* 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.
> +	 */

Ah. dev->primary->debugfs_root does not exist until the end of driver
loading.

You need to add an intel_guc_register() to the i915_driver_register()
after we call drm_dev_rigster() (that then calls this function).

Similarly, this needs to be torn down in unregister.

> +	if (!dev->primary->debugfs_root) {
> +		/* logging will remain off */
> +		i915.guc_log_level = -1;
> +		return;
> +	}
> +
> +	/* For now create the log file in /sys/kernel/debug/dri dir. */
> +	log_dir = dev->primary->debugfs_root->d_parent;

In future, this will be something like /sys/kernel/gpu/i915/guc_log, so
I don't see a good argument for not being more canonical in the debugfs
placement and using dev->primary->debugfs_root (i.e. /.../dri/0)

At the very least, you need to explain why we don't use dri/0/
-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 10/11] drm/i915: Support to create write combined type vmaps

[Chris Wilson]

On Mon, Jun 27, 2016 at 05:46:57PM +0530, akash.goel@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> vmaps has a provision for controlling the page protection bits, with which
> we can use to control the mapping type, e.g. WB, WC, UC or even WT.
> To allow the caller to choose their mapping type, we add a parameter to
> i915_gem_object_pin_map - but we still only allow one vmap to be cached
> per object. If the object is currently not pinned, then we recreate the
> previous vmap with the new access type, but if it was pinned we report an
> error. This effectively limits the access via i915_gem_object_pin_map to a
> single mapping type for the lifetime of the object. Not usually a problem,
> but something to be aware of when setting up the object's vmap.
> 
> We will want to vary the access type to enable WC mappings of ringbuffer
> and context objects on !llc platforms, as well as other objects where we
> need coherent access to the GPU's pages without going through the GTT
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  4 ++-
>  drivers/gpu/drm/i915/i915_gem.c         | 57 +++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c  |  2 +-
>  drivers/gpu/drm/i915/intel_lrc.c        |  8 ++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
>  5 files changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20c701c..3ef1ee5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3197,6 +3197,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>  /**
>   * i915_gem_object_pin_map - return a contiguous mapping of the entire object
>   * @obj - the object to map into kernel address space
> + * &use_wc - whether the mapping should be using WC or WB pgprot_t

s/&/@/ I think

>  /* get, pin, and map the pages of the object into kernel space */
> -void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
> +void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
>  {
> +	void *ptr;
> +	bool has_wc;
> +	bool pinned;
>  	int ret;
>  
>  	lockdep_assert_held(&obj->base.dev->struct_mutex);
> +	GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
>  
>  	ret = i915_gem_object_get_pages(obj);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	GEM_BUG_ON(obj->pages == NULL);
>  	i915_gem_object_pin_pages(obj);
>  
> -	if (!obj->mapping) {
> -		obj->mapping = i915_gem_object_map(obj);
> -		if (!obj->mapping) {
> -			i915_gem_object_unpin_pages(obj);
> -			return ERR_PTR(-ENOMEM);
> +	pinned = (obj->pages_pin_count > 1);

Too many ()

Hmm. It may look a bit dubious if I add my r-b here. But I didn't spot
any rebasing errors.
-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 06/11] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

[Goel, Akash]

On 6/28/2016 3:17 PM, Chris Wilson wrote:
> On Mon, Jun 27, 2016 at 05:46:53PM +0530, akash.goel@intel.com wrote:
>> +static void guc_remove_log_relay_file(struct intel_guc *guc)
>> +{
>> +	relay_close(guc->log_relay_chan);
>> +}
>> +
>> +static void guc_create_log_relay_file(struct intel_guc *guc)
>> +{
>> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +	struct drm_device *dev = dev_priv->dev;
>> +	struct dentry *log_dir;
>> +	struct rchan *guc_log_relay_chan;
>> +	size_t n_subbufs, subbuf_size;
>> +
>> +	if (guc->log_relay_chan)
>> +		return;
>> +
>> +	/* 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.
>> +	 */
>
> Ah. dev->primary->debugfs_root does not exist until the end of driver
> loading.
>
> You need to add an intel_guc_register() to the i915_driver_register()
> after we call drm_dev_rigster() (that then calls this function).
>
> Similarly, this needs to be torn down in unregister.

Yes, realized this today, that can’t get to the ‘dri’ directory until
the end of Driver load.
So will have to create the relay file after i915_driver_register().

>
>> +	if (!dev->primary->debugfs_root) {
>> +		/* logging will remain off */
>> +		i915.guc_log_level = -1;
>> +		return;
>> +	}
>> +
>> +	/* For now create the log file in /sys/kernel/debug/dri dir. */
>> +	log_dir = dev->primary->debugfs_root->d_parent;
>
> In future, this will be something like /sys/kernel/gpu/i915/guc_log, so
> I don't see a good argument for not being more canonical in the debugfs
> placement and using dev->primary->debugfs_root (i.e. /.../dri/0)

Yes can now use the dev->primary->debugfs_root itself.

Actually earlier 'i915_debugfs_files' were being created inside other
drm_minor directories also (i.e. dri/64 & dri/128), but now they are
restricted only to dri/0.

Best regards
Akash

> At the very least, you need to explain why we don't use dri/0/
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/11] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin]

On 27/06/16 13:16, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> There are certain types of interrupts which Host can recieve from GuC.
> GuC ukernel sends an interrupt to Host for certain events, like for
> example retrieve/consume the logs generated by ukernel.
> This patch adds support to receive interrupts from GuC but currently
> enables & partially handles only the interrupt sent by GuC ukernel.
> Future patches will add support for handling other interrupt types.
>
> 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)
>
> 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            | 95 ++++++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++
>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>   7 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 85a7103..20c701c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1808,6 +1808,7 @@ struct drm_i915_private {
>   	u32 pm_irq_mask;
>   	u32 pm_ier_mask;
>   	u32 pm_rps_events;
> +	u32 guc_events;
>   	u32 pipestat_irq_mask[I915_MAX_PIPES];
>
>   	struct i915_hotplug hotplug;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 28a810f..8105ddd 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1038,6 +1038,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;
> @@ -1064,6 +1066,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 7316ab4..3043e45 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
> @@ -422,6 +423,42 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>   	synchronize_irq(dev_priv->dev->irq);
>   }
>
> +void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> +{
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	gen6_reset_pm_interrupts(dev_priv, dev_priv->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(I915_READ(gen6_pm_iir(dev_priv)) &
> +					dev_priv->guc_events);
> +		dev_priv->guc.interrupts_enabled = true;
> +		gen6_enable_pm_interrupts(dev_priv, dev_priv->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;
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	cancel_work_sync(&dev_priv->guc.events_work);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	gen6_disable_pm_interrupts(dev_priv, dev_priv->guc_events);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	synchronize_irq(dev_priv->dev->irq);
> +}
> +
>   /**
>    * bdw_update_port_irq - update DE port interrupt
>    * @dev_priv: driver private
> @@ -1196,6 +1233,33 @@ out:
>   	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>   }
>
> +static void gen9_guc2host_events_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, guc.events_work);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	/* Speed up work cancelation during disabling guc interrupts. */
> +	if (!dev_priv->guc.interrupts_enabled) {
> +		spin_unlock_irq(&dev_priv->irq_lock);
> +		return;
> +	}
> +
> +	/* Though this work item gets synced during rpm suspend, but still need
> +	 * a rpm get/put to avoid the warning, as it could get executed in a
> +	 * window, where rpm ref count has dropped to zero but rpm suspend has
> +	 * not kicked in. Generally device is expected to be active only at this
> +	 * time so get/put should be really quick.
> +	 */
> +	intel_runtime_pm_get(dev_priv);
> +
> +	gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT);
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	/* TODO: Handle the events for which GuC interrupted host */
> +
> +	intel_runtime_pm_put(dev_priv);
> +}
>
>   /**
>    * ivybridge_parity_work - Workqueue called when a parity error interrupt
> @@ -1371,11 +1435,13 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
>   			DRM_ERROR("The master control interrupt lied (GT3)!\n");
>   	}
>
> -	if (master_ctl & GEN8_GT_PM_IRQ) {
> +	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
>   		gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
> -		if (gt_iir[2] & dev_priv->pm_rps_events) {
> +		if (gt_iir[2] & (dev_priv->pm_rps_events |
> +				 dev_priv->guc_events)) {
>   			I915_WRITE_FW(GEN8_GT_IIR(2),
> -				      gt_iir[2] & dev_priv->pm_rps_events);
> +				      gt_iir[2] & (dev_priv->pm_rps_events |
> +						   dev_priv->guc_events));
>   			ret = IRQ_HANDLED;
>   		} else
>   			DRM_ERROR("The master control interrupt lied (PM)!\n");
> @@ -1407,6 +1473,9 @@ static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
>
>   	if (gt_iir[2] & dev_priv->pm_rps_events)
>   		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> +
> +	if (gt_iir[2] & dev_priv->guc_events)
> +		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>   }
>
>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1653,6 +1722,20 @@ 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) {
> +		spin_lock(&dev_priv->irq_lock);
> +		if (dev_priv->guc.interrupts_enabled) {

So it is expected interrupts will always be enabled when 
i915.guc_log_level is set, correct?

Also do you need to check against dev_priv->guc.interrupts_enabled at 
all then? Or from an opposite angle, would you instead need to log the 
fact unexpected interrupt was received here?

> +			/* Process all the GuC to Host events in bottom half */
> +			gen6_disable_pm_irq(dev_priv,
> +				GEN9_GUC_TO_HOST_INT_EVENT);

Why it is important to disable the interrupt here? Not for the queue 
work I think.

Also, is it safe with regards to potentially losing the interrupt?

> +			queue_work(dev_priv->wq, &dev_priv->guc.events_work);

Because dev_priv->wq is a one a time in order wq so if something else is 
running on it and taking time, can that also be a cause of dropping an 
interrupt or being late with sending the flush signal to the guc and so 
losing some logs?

> +		}
> +		spin_unlock(&dev_priv->irq_lock);
> +	}
> +}
> +
>   static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
>   				     enum pipe pipe)
>   {
> @@ -3809,7 +3892,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_irq_mask, 0);
>   	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
> @@ -4594,6 +4677,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>
>   	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
>   	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
> +	INIT_WORK(&dev_priv->guc.events_work, gen9_guc2host_events_work);
> +
> +	if (HAS_GUC_UCODE(dev))
> +		dev_priv->guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>
>   	/* Let's track the enabled rps events */
>   	if (IS_VALLEYVIEW(dev_priv))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c6bfbf8..4441918 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5963,6 +5963,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)
> @@ -5974,6 +5975,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 2a013fc..6966ffe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1083,6 +1083,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 3e3e743..ae787e2 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -126,6 +126,11 @@ struct intel_guc {
>   	struct intel_guc_fw guc_fw;
>   	uint32_t log_flags;
>   	struct drm_i915_gem_object *log_obj;
> +	/*
> +	 * work, interrupts_enabled are protected by dev_priv->irq_lock
> +	 */
> +	struct work_struct events_work;
> +	bool interrupts_enabled;
>
>   	struct drm_i915_gem_object *ads_obj;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index db3c897..fcf36a2 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -448,6 +448,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;
>
> @@ -494,6 +495,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;
>

Regards,

Tvrtko

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

Re: [PATCH 10/11] drm/i915: Support to create write combined type vmaps

[Goel, Akash]

On 6/28/2016 3:22 PM, Chris Wilson wrote:
> On Mon, Jun 27, 2016 at 05:46:57PM +0530, akash.goel@intel.com wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 20c701c..3ef1ee5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3197,6 +3197,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>  /**
>>   * i915_gem_object_pin_map - return a contiguous mapping of the entire object
>>   * @obj - the object to map into kernel address space
>> + * &use_wc - whether the mapping should be using WC or WB pgprot_t
>
> s/&/@/ I think

Sorry my bad.

>
>>  /* get, pin, and map the pages of the object into kernel space */
>> -void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
>> +void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
>>  {
>> +	void *ptr;
>> +	bool has_wc;
>> +	bool pinned;
>>  	int ret;
>>
>>  	lockdep_assert_held(&obj->base.dev->struct_mutex);
>> +	GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
>>
>>  	ret = i915_gem_object_get_pages(obj);
>>  	if (ret)
>>  		return ERR_PTR(ret);
>>
>> +	GEM_BUG_ON(obj->pages == NULL);
>>  	i915_gem_object_pin_pages(obj);
>>
>> -	if (!obj->mapping) {
>> -		obj->mapping = i915_gem_object_map(obj);
>> -		if (!obj->mapping) {
>> -			i915_gem_object_unpin_pages(obj);
>> -			return ERR_PTR(-ENOMEM);
>> +	pinned = (obj->pages_pin_count > 1);
>
> Too many ()

Sorry is the above condition not correct ?
If pin count is more than 1 then it implies that pages have been pinned 
elsewhere also, so pages were already pinned before they were pinned
one more time, inside this function.
Please let me know, will fix it.

Best regards
Akash
>
> Hmm. It may look a bit dubious if I add my r-b here. But I didn't spot
> any rebasing errors.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/11] drm/i915: Support to create write combined type vmaps

[Chris Wilson]

On Tue, Jun 28, 2016 at 03:55:15PM +0530, Goel, Akash wrote:
> >>+	pinned = (obj->pages_pin_count > 1);
> >
> >Too many ()
> 
> Sorry is the above condition not correct ?
> If pin count is more than 1 then it implies that pages have been
> pinned elsewhere also, so pages were already pinned before they were
> pinned
> one more time, inside this function.

The definition of pinned is correct, just the () are redundant:
	pinned = obj->pages_pin_count > 1;
-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 04/11] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote:
>
> On 27/06/16 13:16, akash.goel@intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>
>> There are certain types of interrupts which Host can recieve from GuC.
>> GuC ukernel sends an interrupt to Host for certain events, like for
>> example retrieve/consume the logs generated by ukernel.
>> This patch adds support to receive interrupts from GuC but currently
>> enables & partially handles only the interrupt sent by GuC ukernel.
>> Future patches will add support for handling other interrupt types.
>>
>> 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)
>>
>> 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            | 95
>> ++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++
>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>>   7 files changed, 120 insertions(+), 4 deletions(-)
>>>>
>> +static void gen9_guc2host_events_work(struct work_struct *work)
>> +{
>> +    struct drm_i915_private *dev_priv =
>> +        container_of(work, struct drm_i915_private, guc.events_work);
>> +
>> +    spin_lock_irq(&dev_priv->irq_lock);
>> +    /* Speed up work cancelation during disabling guc interrupts. */
>> +    if (!dev_priv->guc.interrupts_enabled) {
>> +        spin_unlock_irq(&dev_priv->irq_lock);
>> +        return;
>> +    }
>> +
>> +    /* Though this work item gets synced during rpm suspend, but
>> still need
>> +     * a rpm get/put to avoid the warning, as it could get executed in a
>> +     * window, where rpm ref count has dropped to zero but rpm
>> suspend has
>> +     * not kicked in. Generally device is expected to be active only
>> at this
>> +     * time so get/put should be really quick.
>> +     */
>> +    intel_runtime_pm_get(dev_priv);
>> +
>> +    gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT);
>> +    spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> +    /* TODO: Handle the events for which GuC interrupted host */
>> +
>> +    intel_runtime_pm_put(dev_priv);
>> +}
>>
>>   /**
>>    * ivybridge_parity_work - Workqueue called when a parity error
>> interrupt
>> @@ -1371,11 +1435,13 @@ static irqreturn_t gen8_gt_irq_ack(struct
>> drm_i915_private *dev_priv,
>>               DRM_ERROR("The master control interrupt lied (GT3)!\n");
>>       }
>>
>> -    if (master_ctl & GEN8_GT_PM_IRQ) {
>> +    if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
>>           gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
>> -        if (gt_iir[2] & dev_priv->pm_rps_events) {
>> +        if (gt_iir[2] & (dev_priv->pm_rps_events |
>> +                 dev_priv->guc_events)) {
>>               I915_WRITE_FW(GEN8_GT_IIR(2),
>> -                      gt_iir[2] & dev_priv->pm_rps_events);
>> +                      gt_iir[2] & (dev_priv->pm_rps_events |
>> +                           dev_priv->guc_events));
>>               ret = IRQ_HANDLED;
>>           } else
>>               DRM_ERROR("The master control interrupt lied (PM)!\n");
>> @@ -1407,6 +1473,9 @@ static void gen8_gt_irq_handler(struct
>> drm_i915_private *dev_priv,
>>
>>       if (gt_iir[2] & dev_priv->pm_rps_events)
>>           gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>> +
>> +    if (gt_iir[2] & dev_priv->guc_events)
>> +        gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>>   }
>>
>>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>> @@ -1653,6 +1722,20 @@ 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) {
>> +        spin_lock(&dev_priv->irq_lock);
>> +        if (dev_priv->guc.interrupts_enabled) {
>
> So it is expected interrupts will always be enabled when
> i915.guc_log_level is set, correct?
>
Yes currently only when guc_log_level > 0, interrupt should be enabled.

But we need to disable/enable the interrupt upon suspend/resume and
across GPU reset.
So interrupt may not be always in a enabled state when guc_log_level>0.

> Also do you need to check against dev_priv->guc.interrupts_enabled at
> all then? Or from an opposite angle, would you instead need to log the
> fact unexpected interrupt was received here?

I think this check is needed, to avoid the race in disabling interrupt.
Please refer the sequence in interrupt disabling function (same as rps
disabling), there we first set the interrupts_enabled flag to false,
then wait for the work item to finish execution and then program the IMR 
register.

>
>> +            /* Process all the GuC to Host events in bottom half */
>> +            gen6_disable_pm_irq(dev_priv,
>> +                GEN9_GUC_TO_HOST_INT_EVENT);
>
> Why it is important to disable the interrupt here? Not for the queue
> work I think.

We want to & can handle one interrupt at a time, unless the queued work
item is executed we can't process the next interrupt, so better to keep 
the interrupt masked.
Sorry this is what is my understanding.

>
> Also, is it safe with regards to potentially losing the interrupt?
>
Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush
interrupt unless its gets an acknowledgement (flush signal) of the 
previous one from Host.

>> +            queue_work(dev_priv->wq, &dev_priv->guc.events_work);
>
> Because dev_priv->wq is a one a time in order wq so if something else is
> running on it and taking time, can that also be a cause of dropping an
> interrupt or being late with sending the flush signal to the guc and so
> losing some logs?

Its a Driver's private workqueue and Turbo work item is also queued
inside this workqueue which too needs to be executed without much delay.
But yes the flush work item can get substantially delayed in case if 
there are other work items queued before it, especially the 
mm.retire_work (but generally executes every ~1 second).

Best would be if the log buffer (44KB data) can be sampled in IRQ
context (or Tasklet context) itself.

Best regards
Akash
>
>> +        }
>> +        spin_unlock(&dev_priv->irq_lock);
>> +    }
>> +}
>> +
>>   static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
>>                        enum pipe pipe)
>>   {
>> @@ -3809,7 +3892,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_irq_mask, 0);
>>       GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
>> @@ -4594,6 +4677,10 @@ void intel_irq_init(struct drm_i915_private
>> *dev_priv)
>>
>>       INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
>>       INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
>> +    INIT_WORK(&dev_priv->guc.events_work, gen9_guc2host_events_work);
>> +
>> +    if (HAS_GUC_UCODE(dev))
>> +        dev_priv->guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>>
>>       /* Let's track the enabled rps events */
>>       if (IS_VALLEYVIEW(dev_priv))
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index c6bfbf8..4441918 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5963,6 +5963,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)
>> @@ -5974,6 +5975,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 2a013fc..6966ffe 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1083,6 +1083,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 3e3e743..ae787e2 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -126,6 +126,11 @@ struct intel_guc {
>>       struct intel_guc_fw guc_fw;
>>       uint32_t log_flags;
>>       struct drm_i915_gem_object *log_obj;
>> +    /*
>> +     * work, interrupts_enabled are protected by dev_priv->irq_lock
>> +     */
>> +    struct work_struct events_work;
>> +    bool interrupts_enabled;
>>
>>       struct drm_i915_gem_object *ads_obj;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index db3c897..fcf36a2 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -448,6 +448,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;
>>
>> @@ -494,6 +495,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;
>>
>
> Regards,
>
> Tvrtko
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/11] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin]

On 28/06/16 12:12, Goel, Akash wrote:
>
>
> On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote:
>>
>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>
>>> There are certain types of interrupts which Host can recieve from GuC.
>>> GuC ukernel sends an interrupt to Host for certain events, like for
>>> example retrieve/consume the logs generated by ukernel.
>>> This patch adds support to receive interrupts from GuC but currently
>>> enables & partially handles only the interrupt sent by GuC ukernel.
>>> Future patches will add support for handling other interrupt types.
>>>
>>> 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)
>>>
>>> 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            | 95
>>> ++++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>>>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>>>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++
>>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>>>   7 files changed, 120 insertions(+), 4 deletions(-)
>>>>>
>>> +static void gen9_guc2host_events_work(struct work_struct *work)
>>> +{
>>> +    struct drm_i915_private *dev_priv =
>>> +        container_of(work, struct drm_i915_private, guc.events_work);
>>> +
>>> +    spin_lock_irq(&dev_priv->irq_lock);
>>> +    /* Speed up work cancelation during disabling guc interrupts. */
>>> +    if (!dev_priv->guc.interrupts_enabled) {
>>> +        spin_unlock_irq(&dev_priv->irq_lock);
>>> +        return;
>>> +    }
>>> +
>>> +    /* Though this work item gets synced during rpm suspend, but
>>> still need
>>> +     * a rpm get/put to avoid the warning, as it could get executed
>>> in a
>>> +     * window, where rpm ref count has dropped to zero but rpm
>>> suspend has
>>> +     * not kicked in. Generally device is expected to be active only
>>> at this
>>> +     * time so get/put should be really quick.
>>> +     */
>>> +    intel_runtime_pm_get(dev_priv);
>>> +
>>> +    gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT);
>>> +    spin_unlock_irq(&dev_priv->irq_lock);
>>> +
>>> +    /* TODO: Handle the events for which GuC interrupted host */
>>> +
>>> +    intel_runtime_pm_put(dev_priv);
>>> +}
>>>
>>>   /**
>>>    * ivybridge_parity_work - Workqueue called when a parity error
>>> interrupt
>>> @@ -1371,11 +1435,13 @@ static irqreturn_t gen8_gt_irq_ack(struct
>>> drm_i915_private *dev_priv,
>>>               DRM_ERROR("The master control interrupt lied (GT3)!\n");
>>>       }
>>>
>>> -    if (master_ctl & GEN8_GT_PM_IRQ) {
>>> +    if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
>>>           gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
>>> -        if (gt_iir[2] & dev_priv->pm_rps_events) {
>>> +        if (gt_iir[2] & (dev_priv->pm_rps_events |
>>> +                 dev_priv->guc_events)) {
>>>               I915_WRITE_FW(GEN8_GT_IIR(2),
>>> -                      gt_iir[2] & dev_priv->pm_rps_events);
>>> +                      gt_iir[2] & (dev_priv->pm_rps_events |
>>> +                           dev_priv->guc_events));
>>>               ret = IRQ_HANDLED;
>>>           } else
>>>               DRM_ERROR("The master control interrupt lied (PM)!\n");
>>> @@ -1407,6 +1473,9 @@ static void gen8_gt_irq_handler(struct
>>> drm_i915_private *dev_priv,
>>>
>>>       if (gt_iir[2] & dev_priv->pm_rps_events)
>>>           gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>>> +
>>> +    if (gt_iir[2] & dev_priv->guc_events)
>>> +        gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>>>   }
>>>
>>>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>>> @@ -1653,6 +1722,20 @@ 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) {
>>> +        spin_lock(&dev_priv->irq_lock);
>>> +        if (dev_priv->guc.interrupts_enabled) {
>>
>> So it is expected interrupts will always be enabled when
>> i915.guc_log_level is set, correct?
>>
> Yes currently only when guc_log_level > 0, interrupt should be enabled.
>
> But we need to disable/enable the interrupt upon suspend/resume and
> across GPU reset.
> So interrupt may not be always in a enabled state when guc_log_level>0.
>
>> Also do you need to check against dev_priv->guc.interrupts_enabled at
>> all then? Or from an opposite angle, would you instead need to log the
>> fact unexpected interrupt was received here?
>
> I think this check is needed, to avoid the race in disabling interrupt.
> Please refer the sequence in interrupt disabling function (same as rps
> disabling), there we first set the interrupts_enabled flag to false,
> then wait for the work item to finish execution and then program the IMR
> register.

Right I see now that it is copy-pasted existing sequence. In this case I 
won't question it further. :)

>>
>>> +            /* Process all the GuC to Host events in bottom half */
>>> +            gen6_disable_pm_irq(dev_priv,
>>> +                GEN9_GUC_TO_HOST_INT_EVENT);
>>
>> Why it is important to disable the interrupt here? Not for the queue
>> work I think.
>
> We want to & can handle one interrupt at a time, unless the queued work
> item is executed we can't process the next interrupt, so better to keep
> the interrupt masked.
> Sorry this is what is my understanding.

So it is queued in hardware and will get asserted when unmasked?

>
>>
>> Also, is it safe with regards to potentially losing the interrupt?
>>
> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush
> interrupt unless its gets an acknowledgement (flush signal) of the
> previous one from Host.

Ah so the previous comment is really impossible? I mean the need to mask?

Possibly just put a comment up there explaining that.

>
>>> +            queue_work(dev_priv->wq, &dev_priv->guc.events_work);
>>
>> Because dev_priv->wq is a one a time in order wq so if something else is
>> running on it and taking time, can that also be a cause of dropping an
>> interrupt or being late with sending the flush signal to the guc and so
>> losing some logs?
>
> Its a Driver's private workqueue and Turbo work item is also queued
> inside this workqueue which too needs to be executed without much delay.
> But yes the flush work item can get substantially delayed in case if
> there are other work items queued before it, especially the
> mm.retire_work (but generally executes every ~1 second).
>
> Best would be if the log buffer (44KB data) can be sampled in IRQ
> context (or Tasklet context) itself.

I was just trying to understand if you perhaps need a dedicated wq. I 
don't have a feel at all on how much data guc logging generates per 
second. If the interrupt is low frequency even with a lot of cmd 
submission happening it could be fine like it is.

Regards,

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

Re: [PATCH 04/11] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 6/28/2016 7:14 PM, Tvrtko Ursulin wrote:
>
> On 28/06/16 12:12, Goel, Akash wrote:
>>
>>
>> On 6/28/2016 3:33 PM, Tvrtko Ursulin wrote:
>>>
>>> On 27/06/16 13:16, akash.goel@intel.com wrote:
>>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>
>>>> There are certain types of interrupts which Host can recieve from GuC.
>>>> GuC ukernel sends an interrupt to Host for certain events, like for
>>>> example retrieve/consume the logs generated by ukernel.
>>>> This patch adds support to receive interrupts from GuC but currently
>>>> enables & partially handles only the interrupt sent by GuC ukernel.
>>>> Future patches will add support for handling other interrupt types.
>>>>
>>>> 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)
>>>>
>>>> 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            | 95
>>>> ++++++++++++++++++++++++++++--
>>>>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>>>>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>>>>   drivers/gpu/drm/i915/intel_guc.h           |  5 ++
>>>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>>>>   7 files changed, 120 insertions(+), 4 deletions(-)
>>>>>>
>>>> +static void gen9_guc2host_events_work(struct work_struct *work)
>>>> +{
>>>> +    struct drm_i915_private *dev_priv =
>>>> +        container_of(work, struct drm_i915_private, guc.events_work);
>>>> +
>>>> +    spin_lock_irq(&dev_priv->irq_lock);
>>>> +    /* Speed up work cancelation during disabling guc interrupts. */
>>>> +    if (!dev_priv->guc.interrupts_enabled) {
>>>> +        spin_unlock_irq(&dev_priv->irq_lock);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* Though this work item gets synced during rpm suspend, but
>>>> still need
>>>> +     * a rpm get/put to avoid the warning, as it could get executed
>>>> in a
>>>> +     * window, where rpm ref count has dropped to zero but rpm
>>>> suspend has
>>>> +     * not kicked in. Generally device is expected to be active only
>>>> at this
>>>> +     * time so get/put should be really quick.
>>>> +     */
>>>> +    intel_runtime_pm_get(dev_priv);
>>>> +
>>>> +    gen6_enable_pm_irq(dev_priv, GEN9_GUC_TO_HOST_INT_EVENT);
>>>> +    spin_unlock_irq(&dev_priv->irq_lock);
>>>> +
>>>> +    /* TODO: Handle the events for which GuC interrupted host */
>>>> +
>>>> +    intel_runtime_pm_put(dev_priv);
>>>> +}
>>>>
>>>>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>>>> @@ -1653,6 +1722,20 @@ 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) {
>>>> +        spin_lock(&dev_priv->irq_lock);
>>>> +        if (dev_priv->guc.interrupts_enabled) {
>>>
>>> So it is expected interrupts will always be enabled when
>>> i915.guc_log_level is set, correct?
>>>
>> Yes currently only when guc_log_level > 0, interrupt should be enabled.
>>
>> But we need to disable/enable the interrupt upon suspend/resume and
>> across GPU reset.
>> So interrupt may not be always in a enabled state when guc_log_level>0.
>>
>>> Also do you need to check against dev_priv->guc.interrupts_enabled at
>>> all then? Or from an opposite angle, would you instead need to log the
>>> fact unexpected interrupt was received here?
>>
>> I think this check is needed, to avoid the race in disabling interrupt.
>> Please refer the sequence in interrupt disabling function (same as rps
>> disabling), there we first set the interrupts_enabled flag to false,
>> then wait for the work item to finish execution and then program the IMR
>> register.
>
> Right I see now that it is copy-pasted existing sequence. In this case I
> won't question it further. :)
>
>>>
>>>> +            /* Process all the GuC to Host events in bottom half */
>>>> +            gen6_disable_pm_irq(dev_priv,
>>>> +                GEN9_GUC_TO_HOST_INT_EVENT);
>>>
>>> Why it is important to disable the interrupt here? Not for the queue
>>> work I think.
>>
>> We want to & can handle one interrupt at a time, unless the queued work
>> item is executed we can't process the next interrupt, so better to keep
>> the interrupt masked.
>> Sorry this is what is my understanding.
>
> So it is queued in hardware and will get asserted when unmasked?
As per my understanding, if the interrupt is masked (IMR), it won't be
queued, will be ignored & so will not be asserted on unmasking.

If the interrupt wasn't masked, but was disabled (in IER) then it
will be asserted (in IIR) when its enabled.

>
>>
>>>
>>> Also, is it safe with regards to potentially losing the interrupt?
>>>
>> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush
>> interrupt unless its gets an acknowledgement (flush signal) of the
>> previous one from Host.
>
> Ah so the previous comment is really impossible? I mean the need to mask?

Sorry my comments were not fully correct. GuC can send a new flush 
interrupt, even if the previous one is pending, but that will be for a
different log buffer type (3 types of log buffer ISR, DPC, CRASH).
For the same buffer type, GuC won't send a new flush interrupt unless 
its gets an acknowledgement of the previous one from Host.

But as you said the workqueue is ordered and furthermore there is a
single instance of work item, so the serialization will be provided
implicitly and there is no real need to mask the interrupt.

As mentioned above, a new flush interrupt can come while the previous
one is being processed on Host but due to a single instance of work item 
either that new interrupt will not do anything effectively if work
item was in a pending state or will re queue the work item if it was
getting executed at that time.

Also the state of all 3 log buffer types are being parsed irrespective
for which one the interrupt actually came, and the whole buffer is being
captured (this is how it has been recommended to handle the flush
interrupts from Host side). So if a new interrupt comes while the work 
item was in a pending state, then effectively work of this new interrupt 
will also be done when work item is executed later.

So will remove the masking then ?

>
> Possibly just put a comment up there explaining that.
>
>>
>>>> +            queue_work(dev_priv->wq, &dev_priv->guc.events_work);
>>>
>>> Because dev_priv->wq is a one a time in order wq so if something else is
>>> running on it and taking time, can that also be a cause of dropping an
>>> interrupt or being late with sending the flush signal to the guc and so
>>> losing some logs?
>>
>> Its a Driver's private workqueue and Turbo work item is also queued
>> inside this workqueue which too needs to be executed without much delay.
>> But yes the flush work item can get substantially delayed in case if
>> there are other work items queued before it, especially the
>> mm.retire_work (but generally executes every ~1 second).
>>
>> Best would be if the log buffer (44KB data) can be sampled in IRQ
>> context (or Tasklet context) itself.
>
> I was just trying to understand if you perhaps need a dedicated wq. I
> don't have a feel at all on how much data guc logging generates per
> second. If the interrupt is low frequency even with a lot of cmd
> submission happening it could be fine like it is.
>
Actually with maximum verbosity level, I am seeing flush interrupt every 
ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done. 
But such may not happen in real life scenario.

I think, if needed, later on we can either have a dedicated high 
priority work queue for logging work or use the tasklet context to do
the processing.

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

Re: [PATCH 04/11] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin]

On 01/07/16 07:16, Goel, Akash wrote:

[snip]

>>>>
>>>>> +            /* Process all the GuC to Host events in bottom half */
>>>>> +            gen6_disable_pm_irq(dev_priv,
>>>>> +                GEN9_GUC_TO_HOST_INT_EVENT);
>>>>
>>>> Why it is important to disable the interrupt here? Not for the queue
>>>> work I think.
>>>
>>> We want to & can handle one interrupt at a time, unless the queued work
>>> item is executed we can't process the next interrupt, so better to keep
>>> the interrupt masked.
>>> Sorry this is what is my understanding.
>>
>> So it is queued in hardware and will get asserted when unmasked?
> As per my understanding, if the interrupt is masked (IMR), it won't be
> queued, will be ignored & so will not be asserted on unmasking.
>
> If the interrupt wasn't masked, but was disabled (in IER) then it
> will be asserted (in IIR) when its enabled.
>
>>
>>>
>>>>
>>>> Also, is it safe with regards to potentially losing the interrupt?
>>>>
>>> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush
>>> interrupt unless its gets an acknowledgement (flush signal) of the
>>> previous one from Host.
>>
>> Ah so the previous comment is really impossible? I mean the need to mask?
>
> Sorry my comments were not fully correct. GuC can send a new flush
> interrupt, even if the previous one is pending, but that will be for a
> different log buffer type (3 types of log buffer ISR, DPC, CRASH).
> For the same buffer type, GuC won't send a new flush interrupt unless
> its gets an acknowledgement of the previous one from Host.
>
> But as you said the workqueue is ordered and furthermore there is a
> single instance of work item, so the serialization will be provided
> implicitly and there is no real need to mask the interrupt.
>
> As mentioned above, a new flush interrupt can come while the previous
> one is being processed on Host but due to a single instance of work item
> either that new interrupt will not do anything effectively if work
> item was in a pending state or will re queue the work item if it was
> getting executed at that time.
>
> Also the state of all 3 log buffer types are being parsed irrespective
> for which one the interrupt actually came, and the whole buffer is being
> captured (this is how it has been recommended to handle the flush
> interrupts from Host side). So if a new interrupt comes while the work
> item was in a pending state, then effectively work of this new interrupt
> will also be done when work item is executed later.
>
> So will remove the masking then ?

I think so, because if I understood what you wrote, masking can lose us 
an interrupt.

>>
>> Possibly just put a comment up there explaining that.
>>
>>>
>>>>> +            queue_work(dev_priv->wq, &dev_priv->guc.events_work);
>>>>
>>>> Because dev_priv->wq is a one a time in order wq so if something
>>>> else is
>>>> running on it and taking time, can that also be a cause of dropping an
>>>> interrupt or being late with sending the flush signal to the guc and so
>>>> losing some logs?
>>>
>>> Its a Driver's private workqueue and Turbo work item is also queued
>>> inside this workqueue which too needs to be executed without much delay.
>>> But yes the flush work item can get substantially delayed in case if
>>> there are other work items queued before it, especially the
>>> mm.retire_work (but generally executes every ~1 second).
>>>
>>> Best would be if the log buffer (44KB data) can be sampled in IRQ
>>> context (or Tasklet context) itself.
>>
>> I was just trying to understand if you perhaps need a dedicated wq. I
>> don't have a feel at all on how much data guc logging generates per
>> second. If the interrupt is low frequency even with a lot of cmd
>> submission happening it could be fine like it is.
>>
> Actually with maximum verbosity level, I am seeing flush interrupt every
> ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done.
> But such may not happen in real life scenario.
>
> I think, if needed, later on we can either have a dedicated high
> priority work queue for logging work or use the tasklet context to do
> the processing.

Hm, do you need to add some DRM_ERROR or something if wq starts lagging 
behind the flush interrupts? How many missed flush interrupts can we 
afford before the logging buffer starts getting overwritten?

Regards,

Tvrtko



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

Re: [PATCH 04/11] drm/i915: Support for GuC interrupts

[Goel, Akash]

On 7/1/2016 2:17 PM, Tvrtko Ursulin wrote:
>
>
> On 01/07/16 07:16, Goel, Akash wrote:
>
> [snip]
>
>>>>>
>>>>>> +            /* Process all the GuC to Host events in bottom half */
>>>>>> +            gen6_disable_pm_irq(dev_priv,
>>>>>> +                GEN9_GUC_TO_HOST_INT_EVENT);
>>>>>
>>>>> Why it is important to disable the interrupt here? Not for the queue
>>>>> work I think.
>>>>
>>>> We want to & can handle one interrupt at a time, unless the queued work
>>>> item is executed we can't process the next interrupt, so better to keep
>>>> the interrupt masked.
>>>> Sorry this is what is my understanding.
>>>
>>> So it is queued in hardware and will get asserted when unmasked?
>> As per my understanding, if the interrupt is masked (IMR), it won't be
>> queued, will be ignored & so will not be asserted on unmasking.
>>
>> If the interrupt wasn't masked, but was disabled (in IER) then it
>> will be asserted (in IIR) when its enabled.
>>
>>>
>>>>
>>>>>
>>>>> Also, is it safe with regards to potentially losing the interrupt?
>>>>>
>>>> Particularly for the FLUSH_LOG_BUFFER case, GuC won't send a new flush
>>>> interrupt unless its gets an acknowledgement (flush signal) of the
>>>> previous one from Host.
>>>
>>> Ah so the previous comment is really impossible? I mean the need to
>>> mask?
>>
>> Sorry my comments were not fully correct. GuC can send a new flush
>> interrupt, even if the previous one is pending, but that will be for a
>> different log buffer type (3 types of log buffer ISR, DPC, CRASH).
>> For the same buffer type, GuC won't send a new flush interrupt unless
>> its gets an acknowledgement of the previous one from Host.
>>
>> But as you said the workqueue is ordered and furthermore there is a
>> single instance of work item, so the serialization will be provided
>> implicitly and there is no real need to mask the interrupt.
>>
>> As mentioned above, a new flush interrupt can come while the previous
>> one is being processed on Host but due to a single instance of work item
>> either that new interrupt will not do anything effectively if work
>> item was in a pending state or will re queue the work item if it was
>> getting executed at that time.
>>
>> Also the state of all 3 log buffer types are being parsed irrespective
>> for which one the interrupt actually came, and the whole buffer is being
>> captured (this is how it has been recommended to handle the flush
>> interrupts from Host side). So if a new interrupt comes while the work
>> item was in a pending state, then effectively work of this new interrupt
>> will also be done when work item is executed later.
>>
>> So will remove the masking then ?
>
> I think so, because if I understood what you wrote, masking can lose us
> an interrupt.
>

If a new flush interrupt comes while the work item was getting executed
then there is a potential of losing an opportunity to sample the log buffer.
Will not mask the interrupt.
Thanks for persisting on this.

>>>
>>> Possibly just put a comment up there explaining that.
>>>
>>>>
>>>>>> +            queue_work(dev_priv->wq, &dev_priv->guc.events_work);
>>>>>
>>>>> Because dev_priv->wq is a one a time in order wq so if something
>>>>> else is
>>>>> running on it and taking time, can that also be a cause of dropping an
>>>>> interrupt or being late with sending the flush signal to the guc
>>>>> and so
>>>>> losing some logs?
>>>>
>>>> Its a Driver's private workqueue and Turbo work item is also queued
>>>> inside this workqueue which too needs to be executed without much
>>>> delay.
>>>> But yes the flush work item can get substantially delayed in case if
>>>> there are other work items queued before it, especially the
>>>> mm.retire_work (but generally executes every ~1 second).
>>>>
>>>> Best would be if the log buffer (44KB data) can be sampled in IRQ
>>>> context (or Tasklet context) itself.
>>>
>>> I was just trying to understand if you perhaps need a dedicated wq. I
>>> don't have a feel at all on how much data guc logging generates per
>>> second. If the interrupt is low frequency even with a lot of cmd
>>> submission happening it could be fine like it is.
>>>
>> Actually with maximum verbosity level, I am seeing flush interrupt every
>> ms, with 'gem_exec_nop' IGT, as there are lot of submissions being done.
>> But such may not happen in real life scenario.
>>
>> I think, if needed, later on we can either have a dedicated high
>> priority work queue for logging work or use the tasklet context to do
>> the processing.
>
> Hm, do you need to add some DRM_ERROR or something if wq starts lagging
> behind the flush interrupts? How many missed flush interrupts can we
> afford before the logging buffer starts getting overwritten?
>

Actually if GuC is producing logs at such a fast rate then we can't 
afford to miss even a single interrupt, if we don't want to lose any logs.
When the 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
acknowledgement from Host, so that it does not end up doing any 
overwrite causing loss of logs.

There is a buffer_full_cnt field in the state structure which GuC 
firmware increments every time it detects a potential log buffer 
overflow. Probably this can be shown via debugfs ?

The return value of queue_work() can be checked in the interrupt, if it 
is zero then it means the work item is still in the queue so the 
previous flush hasn't been processed yet, this can give an inkling of lag.

I had initially thought that creating a dedicated workqueue for flush
work might be considered an overkill.
Will proceed as per your suggestion.

Best regards
Akash


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

[PATCH 10/11] drm/i915: Support to create write combined type vmaps

[akash.goel]

From: Chris Wilson <chris@chris-wilson.co.uk>

vmaps has a provision for controlling the page protection bits, with which
we can use to control the mapping type, e.g. WB, WC, UC or even WT.
To allow the caller to choose their mapping type, we add a parameter to
i915_gem_object_pin_map - but we still only allow one vmap to be cached
per object. If the object is currently not pinned, then we recreate the
previous vmap with the new access type, but if it was pinned we report an
error. This effectively limits the access via i915_gem_object_pin_map to a
single mapping type for the lifetime of the object. Not usually a problem,
but something to be aware of when setting up the object's vmap.

We will want to vary the access type to enable WC mappings of ringbuffer
and context objects on !llc platforms, as well as other objects where we
need coherent access to the GPU's pages without going through the GTT

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++-
 drivers/gpu/drm/i915/i915_gem.c         | 57 +++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c        |  8 ++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
 5 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18947ba..2a0eb01 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3144,6 +3144,7 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 /**
  * i915_gem_object_pin_map - return a contiguous mapping of the entire object
  * @obj - the object to map into kernel address space
+ * &use_wc - whether the mapping should be using WC or WB pgprot_t
  *
  * Calls i915_gem_object_pin_pages() to prevent reaping of the object's
  * pages and then returns a contiguous mapping of the backing storage into
@@ -3155,7 +3156,8 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
  * Returns the pointer through which to access the mapped object, or an
  * ERR_PTR() on error.
  */
-void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
+void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
+					 bool use_wc);
 
 /**
  * i915_gem_object_unpin_map - releases an earlier mapping
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 343d881..cafbbdc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2225,10 +2225,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	list_del(&obj->global_list);
 
 	if (obj->mapping) {
-		if (is_vmalloc_addr(obj->mapping))
-			vunmap(obj->mapping);
+		void *ptr = (void *)((uintptr_t)obj->mapping & ~1);
+		if (is_vmalloc_addr(ptr))
+			vunmap(ptr);
 		else
-			kunmap(kmap_to_page(obj->mapping));
+			kunmap(kmap_to_page(ptr));
 		obj->mapping = NULL;
 	}
 
@@ -2401,7 +2402,8 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 }
 
 /* The 'mapping' part of i915_gem_object_pin_map() below */
-static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
+static void *i915_gem_object_map(const struct drm_i915_gem_object *obj,
+				bool use_wc)
 {
 	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
 	struct sg_table *sgt = obj->pages;
@@ -2413,7 +2415,7 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 	void *addr;
 
 	/* A single page can always be kmapped */
-	if (n_pages == 1)
+	if (n_pages == 1 && !use_wc)
 		return kmap(sg_page(sgt->sgl));
 
 	if (n_pages > ARRAY_SIZE(stack_pages)) {
@@ -2429,7 +2431,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 	/* Check that we have the expected number of pages */
 	GEM_BUG_ON(i != n_pages);
 
-	addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
+	addr = vmap(pages, n_pages, VM_NO_GUARD,
+		    use_wc ? pgprot_writecombine(PAGE_KERNEL_IO) : PAGE_KERNEL);
 
 	if (pages != stack_pages)
 		drm_free_large(pages);
@@ -2438,27 +2441,55 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 }
 
 /* get, pin, and map the pages of the object into kernel space */
-void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
+void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
 {
+	void *ptr;
+	bool has_wc;
+	bool pinned;
 	int ret;
 
 	lockdep_assert_held(&obj->base.dev->struct_mutex);
+	GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
 
 	ret = i915_gem_object_get_pages(obj);
 	if (ret)
 		return ERR_PTR(ret);
 
+	GEM_BUG_ON(obj->pages == NULL);
 	i915_gem_object_pin_pages(obj);
 
-	if (!obj->mapping) {
-		obj->mapping = i915_gem_object_map(obj);
-		if (!obj->mapping) {
-			i915_gem_object_unpin_pages(obj);
-			return ERR_PTR(-ENOMEM);
+	pinned = (obj->pages_pin_count > 1);
+	ptr = (void *)((uintptr_t)obj->mapping & ~1);
+	has_wc = (uintptr_t)obj->mapping & 1;
+
+	if (ptr && has_wc != use_wc) {
+		if (pinned) {
+			ret = -EBUSY;
+			goto err;
+		}
+
+		if (is_vmalloc_addr(ptr))
+			vunmap(ptr);
+		else
+			kunmap(kmap_to_page(ptr));
+		ptr = obj->mapping = NULL;
+	}
+
+	if (!ptr) {
+		ptr = i915_gem_object_map(obj, use_wc);
+		if (!ptr) {
+			ret = -ENOMEM;
+			goto err;
 		}
+
+		obj->mapping = (void *)((uintptr_t)ptr | use_wc);
 	}
 
-	return obj->mapping;
+	return ptr;
+
+err:
+	i915_gem_object_unpin_pages(obj);
+	return ERR_PTR(ret);
 }
 
 void i915_vma_move_to_active(struct i915_vma *vma,
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 80bbe43..edcadce 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -115,7 +115,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 	if (ret)
 		return ERR_PTR(ret);
 
-	addr = i915_gem_object_pin_map(obj);
+	addr = i915_gem_object_pin_map(obj, false);
 	mutex_unlock(&dev->struct_mutex);
 
 	return addr;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5c191a1..a605e2b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -952,7 +952,7 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 	if (ret)
 		goto err;
 
-	vaddr = i915_gem_object_pin_map(ce->state);
+	vaddr = i915_gem_object_pin_map(ce->state, false);
 	if (IS_ERR(vaddr)) {
 		ret = PTR_ERR(vaddr);
 		goto unpin_ctx_obj;
@@ -1939,7 +1939,7 @@ lrc_setup_hws(struct intel_engine_cs *engine,
 	/* The HWSP is part of the default context object in LRC mode. */
 	engine->status_page.gfx_addr = i915_gem_obj_ggtt_offset(dctx_obj) +
 				       LRC_PPHWSP_PN * PAGE_SIZE;
-	hws = i915_gem_object_pin_map(dctx_obj);
+	hws = i915_gem_object_pin_map(dctx_obj, false);
 	if (IS_ERR(hws))
 		return PTR_ERR(hws);
 	engine->status_page.page_addr = hws + LRC_PPHWSP_PN * PAGE_SIZE;
@@ -2291,7 +2291,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 		return ret;
 	}
 
-	vaddr = i915_gem_object_pin_map(ctx_obj);
+	vaddr = i915_gem_object_pin_map(ctx_obj, false);
 	if (IS_ERR(vaddr)) {
 		ret = PTR_ERR(vaddr);
 		DRM_DEBUG_DRIVER("Could not map object pages! (%d)\n", ret);
@@ -2525,7 +2525,7 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv,
 		if (!ctx_obj)
 			continue;
 
-		vaddr = i915_gem_object_pin_map(ctx_obj);
+		vaddr = i915_gem_object_pin_map(ctx_obj, false);
 		if (WARN_ON(IS_ERR(vaddr)))
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8d35a39..f1ef666 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2131,7 +2131,7 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_i915_private *dev_priv,
 		if (ret)
 			goto err_unpin;
 
-		addr = i915_gem_object_pin_map(obj);
+		addr = i915_gem_object_pin_map(obj, false);
 		if (IS_ERR(addr)) {
 			ret = PTR_ERR(addr);
 			goto err_unpin;
-- 
1.9.2

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