diff mbox series

[2/4] drm/i915/guc: Speed up GuC log dumps

Message ID 20211210044022.1842938-3-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Assorted fixes/tweaks to GuC support | expand

Commit Message

John Harrison Dec. 10, 2021, 4:40 a.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

Add support for telling the debugfs interface the size of the GuC log
dump in advance. Without that, the underlying framework keeps calling
the 'show' function with larger and larger buffer allocations until it
fits. That means reading the log from graphics memory many times - 16
times with the full 18MB log size.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_debugfs.h    | 21 ++++++--
 .../drm/i915/gt/uc/intel_guc_log_debugfs.c    | 54 +++++++++++++++++--
 2 files changed, 67 insertions(+), 8 deletions(-)

Comments

Daniele Ceraolo Spurio Dec. 11, 2021, 12:23 a.m. UTC | #1 
On 12/9/2021 8:40 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Add support for telling the debugfs interface the size of the GuC log
> dump in advance. Without that, the underlying framework keeps calling
> the 'show' function with larger and larger buffer allocations until it
> fits. That means reading the log from graphics memory many times - 16
> times with the full 18MB log size.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_debugfs.h    | 21 ++++++--
>   .../drm/i915/gt/uc/intel_guc_log_debugfs.c    | 54 +++++++++++++++++--
>   2 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
> index e307ceb99031..1adea367d99b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
> @@ -10,11 +10,7 @@
>   
>   struct intel_gt;
>   
> -#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(__name)				\
> -	static int __name ## _open(struct inode *inode, struct file *file) \
> -{									\
> -	return single_open(file, __name ## _show, inode->i_private);	\
> -}									\
> +#define __GT_DEBUGFS_ATTRIBUTE_FOPS(__name)				\
>   static const struct file_operations __name ## _fops = {			\
>   	.owner = THIS_MODULE,						\
>   	.open = __name ## _open,					\
> @@ -23,6 +19,21 @@ static const struct file_operations __name ## _fops = {			\
>   	.release = single_release,					\
>   }
>   
> +#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(__name)			\
> +static int __name ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +	return single_open(file, __name ## _show, inode->i_private);	\
> +}									\
> +__GT_DEBUGFS_ATTRIBUTE_FOPS(__name)
> +
> +#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(__name, __size_vf)	\
> +static int __name ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +    return single_open_size(file, __name ## _show, inode->i_private,	\
> +			    __size_vf(inode->i_private));		\
> +}									\
> +__GT_DEBUGFS_ATTRIBUTE_FOPS(__name)
> +
>   void intel_gt_debugfs_register(struct intel_gt *gt);
>   
>   struct intel_gt_debugfs_file {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> index 46026c2c1722..da7dd099fd93 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> @@ -10,14 +10,62 @@
>   #include "intel_guc.h"
>   #include "intel_guc_log.h"
>   #include "intel_guc_log_debugfs.h"
> +#include "intel_uc.h"
> +
> +static u32 obj_to_guc_log_dump_size(struct drm_i915_gem_object *obj)
> +{
> +	u32 size;
> +
> +	if (!obj)
> +		return -ENODEV;
> +
> +	/* "0x%08x 0x%08x 0x%08x 0x%08x\n" => 16 bytes -> 44 chars => x2.75 */
> +	size = ((obj->base.size * 11) + 3) / 4;
> +
> +	/* Add padding for final blank line, any extra header info, etc. */
> +	size = PAGE_ALIGN(size + PAGE_SIZE);
> +
> +	return size;
> +}
> +
> +static u32 guc_log_dump_size(struct intel_guc_log *log)
> +{
> +	struct intel_guc *guc = log_to_guc(log);
> +
> +	if (!intel_guc_is_supported(guc))
> +		return -ENODEV;
> +
> +	if (!log->vma)
> +		return -ENODEV;

You're returning these error codes as a u32 and passing them as a size 
to single_open_size, so they're going to be read as a massive positive 
number. Same for the error code from obj_to_guc_log_dump_size. Either 
return a safe size on error (PAGE_SIZE?) or make the 
DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE smarter and don't even 
attempt to open the file if the function that calculates the size 
returns zero or an error.

> +
> +	return obj_to_guc_log_dump_size(log->vma->obj);
> +}
>   
>   static int guc_log_dump_show(struct seq_file *m, void *data)
>   {
>   	struct drm_printer p = drm_seq_file_printer(m);
> +	int ret;
> +
> +	ret = intel_guc_log_dump(m->private, &p, false);
> +
> +	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) && seq_has_overflowed(m))
> +		pr_warn_once("preallocated size:%zx for %s exceeded\n",
> +			     m->size, __func__);

Do we need this debug log in guc_load_err_log_dump_show as well? or 
maybe just move it at the end of intel_guc_log_dump?

Daniele

> +
> +	return ret;
> +}
> +DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(guc_log_dump, guc_log_dump_size);
> +
> +static u32 guc_load_err_dump_size(struct intel_guc_log *log)
> +{
> +	struct intel_guc *guc = log_to_guc(log);
> +	struct intel_uc *uc = container_of(guc, struct intel_uc, guc);
> +
> +	if (!intel_guc_is_supported(guc))
> +		return -ENODEV;
>   
> -	return intel_guc_log_dump(m->private, &p, false);
> +	return obj_to_guc_log_dump_size(uc->load_err_log);
>   }
> -DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(guc_log_dump);
>   
>   static int guc_load_err_log_dump_show(struct seq_file *m, void *data)
>   {
> @@ -25,7 +73,7 @@ static int guc_load_err_log_dump_show(struct seq_file *m, void *data)
>   
>   	return intel_guc_log_dump(m->private, &p, true);
>   }
> -DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(guc_load_err_log_dump);
> +DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(guc_load_err_log_dump, guc_load_err_dump_size);
>   
>   static int guc_log_level_get(void *data, u64 *val)
>   {
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
index e307ceb99031..1adea367d99b 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.h
@@ -10,11 +10,7 @@ 
 
 struct intel_gt;
 
-#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(__name)				\
-	static int __name ## _open(struct inode *inode, struct file *file) \
-{									\
-	return single_open(file, __name ## _show, inode->i_private);	\
-}									\
+#define __GT_DEBUGFS_ATTRIBUTE_FOPS(__name)				\
 static const struct file_operations __name ## _fops = {			\
 	.owner = THIS_MODULE,						\
 	.open = __name ## _open,					\
@@ -23,6 +19,21 @@  static const struct file_operations __name ## _fops = {			\
 	.release = single_release,					\
 }
 
+#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(__name)			\
+static int __name ## _open(struct inode *inode, struct file *file)	\
+{									\
+	return single_open(file, __name ## _show, inode->i_private);	\
+}									\
+__GT_DEBUGFS_ATTRIBUTE_FOPS(__name)
+
+#define DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(__name, __size_vf)	\
+static int __name ## _open(struct inode *inode, struct file *file)	\
+{									\
+    return single_open_size(file, __name ## _show, inode->i_private,	\
+			    __size_vf(inode->i_private));		\
+}									\
+__GT_DEBUGFS_ATTRIBUTE_FOPS(__name)
+
 void intel_gt_debugfs_register(struct intel_gt *gt);
 
 struct intel_gt_debugfs_file {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
index 46026c2c1722..da7dd099fd93 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
@@ -10,14 +10,62 @@ 
 #include "intel_guc.h"
 #include "intel_guc_log.h"
 #include "intel_guc_log_debugfs.h"
+#include "intel_uc.h"
+
+static u32 obj_to_guc_log_dump_size(struct drm_i915_gem_object *obj)
+{
+	u32 size;
+
+	if (!obj)
+		return -ENODEV;
+
+	/* "0x%08x 0x%08x 0x%08x 0x%08x\n" => 16 bytes -> 44 chars => x2.75 */
+	size = ((obj->base.size * 11) + 3) / 4;
+
+	/* Add padding for final blank line, any extra header info, etc. */
+	size = PAGE_ALIGN(size + PAGE_SIZE);
+
+	return size;
+}
+
+static u32 guc_log_dump_size(struct intel_guc_log *log)
+{
+	struct intel_guc *guc = log_to_guc(log);
+
+	if (!intel_guc_is_supported(guc))
+		return -ENODEV;
+
+	if (!log->vma)
+		return -ENODEV;
+
+	return obj_to_guc_log_dump_size(log->vma->obj);
+}
 
 static int guc_log_dump_show(struct seq_file *m, void *data)
 {
 	struct drm_printer p = drm_seq_file_printer(m);
+	int ret;
+
+	ret = intel_guc_log_dump(m->private, &p, false);
+
+	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) && seq_has_overflowed(m))
+		pr_warn_once("preallocated size:%zx for %s exceeded\n",
+			     m->size, __func__);
+
+	return ret;
+}
+DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(guc_log_dump, guc_log_dump_size);
+
+static u32 guc_load_err_dump_size(struct intel_guc_log *log)
+{
+	struct intel_guc *guc = log_to_guc(log);
+	struct intel_uc *uc = container_of(guc, struct intel_uc, guc);
+
+	if (!intel_guc_is_supported(guc))
+		return -ENODEV;
 
-	return intel_guc_log_dump(m->private, &p, false);
+	return obj_to_guc_log_dump_size(uc->load_err_log);
 }
-DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(guc_log_dump);
 
 static int guc_load_err_log_dump_show(struct seq_file *m, void *data)
 {
@@ -25,7 +73,7 @@  static int guc_load_err_log_dump_show(struct seq_file *m, void *data)
 
 	return intel_guc_log_dump(m->private, &p, true);
 }
-DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(guc_load_err_log_dump);
+DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE_WITH_SIZE(guc_load_err_log_dump, guc_load_err_dump_size);
 
 static int guc_log_level_get(void *data, u64 *val)
 {