diff mbox

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

Message ID 1467029818-3417-7-git-send-email-akash.goel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

akash.goel@intel.com June 27, 2016, 12:16 p.m. UTC 
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(-)

Comments

kernel test robot June 27, 2016, 2:23 p.m. UTC | #1 
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
kernel test robot June 27, 2016, 5:50 p.m. UTC | #2 
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
Chris Wilson June 28, 2016, 9:47 a.m. UTC | #3 
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
akash.goel@intel.com June 28, 2016, 10:01 a.m. UTC | #4 
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
>
diff mbox

Patch

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
 	 */