Re: [Intel-gfx] [PATCH 1/4] drm/i915/guc: Speed up GuC log dumps

Daniele Ceraolo Spurio
2021-12-20T14:43:37Z

On 12/10/2021 10:58 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.
>
> v2: Don't return error codes from size query. Report overflow in the
> error dump as well (review feedback from Daniele).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> ---
> drivers/gpu/drm/i915/gt/intel_gt_debugfs.h | 21 +++++--
> .../drm/i915/gt/uc/intel_guc_log_debugfs.c | 58 ++++++++++++++++++-
> 2 files changed, 71 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..17e79b735cfe 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 8fd068049376..ddfbe334689f 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,22 +10,74 @@
> #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 PAGE_SIZE;
> +
> + /* "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 PAGE_SIZE;
> +
> + if (!log->vma)
> + return PAGE_SIZE;
> +
> + 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;
>
> - return intel_guc_log_dump(m->private, &p, false);
> + 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 PAGE_SIZE;
> +
> + 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)
> {
> struct drm_printer p = drm_seq_file_printer(m);
>
> + 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 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)
> {