[PATCH] drm/i915/gvt: Add gvt_debug to classify GVT-g log messages
Liu Shuo
shuo.a.liu at gmail.com
Thu Aug 31 12:41:00 UTC 2017
Thanks Zhenyu for review.
On Thu 31.Aug'17 at 10:56:11 +0800, Zhenyu Wang wrote:
>On 2017.08.29 20:33:23 +0800, Shuo Liu wrote:
>> Add a silimar log mechanism as like drm. Classify GVT-g log messages
>> as different categories by differnt log functions.
>>
>> The reason for split debug code into a module is: i915 module depends
>> on kvmgt/xengt module in init code. If put debug code in i915 module,
>> i915 and kvmgt/xengt will has loop dependence.
>>
>> Signed-off-by: Shuo Liu <shuo.a.liu at intel.com>
>> ---
>> drivers/gpu/drm/i915/Kconfig | 9 ++++++++
>> drivers/gpu/drm/i915/gvt/Makefile | 1 +
>> drivers/gpu/drm/i915/gvt/debug.h | 45 +++++++++++++++++++++++++++---------
>> drivers/gpu/drm/i915/gvt/gvt_debug.c | 41 ++++++++++++++++++++++++++++++++
>> 4 files changed, 85 insertions(+), 11 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/gvt/gvt_debug.c
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index a5cd5da..9883061 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -124,6 +124,15 @@ config DRM_I915_GVT_KVMGT
>> Choose this option if you want to enable KVMGT support for
>> Intel GVT-g.
>>
>> +config DRM_I915_GVT_DEBUG
>> + tristate "Enable debug support for Intel GVT-g"
>> + depends on DRM_I915
>> + depends on DRM_I915_GVT
>
>just depends on DRM_I915_GVT
OK, will remove DRM_I915.
>
>> + default n
>> + help
>> + Choose this option if you want to enable debug support for
>> + Intel GVT-g.
>> +
>> menu "drm/i915 Debugging"
>> depends on DRM_I915
>> depends on EXPERT
>> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
>> index f5486cb9..2b5955a 100644
>> --- a/drivers/gpu/drm/i915/gvt/Makefile
>> +++ b/drivers/gpu/drm/i915/gvt/Makefile
>> @@ -6,3 +6,4 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
>> ccflags-y += -I$(src) -I$(src)/$(GVT_DIR)
>> i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
>> obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o
>> +obj-$(CONFIG_DRM_I915_GVT_DEBUG) += $(GVT_DIR)/gvt_debug.o
>
>Let's align file naming, just use "debug.c".
Changing name is just for the output module. gvt_debug.ko is more easy
to understand.
>
>> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
>> index b0cff4d..f33a55f 100644
>> --- a/drivers/gpu/drm/i915/gvt/debug.h
>> +++ b/drivers/gpu/drm/i915/gvt/debug.h
>> @@ -24,42 +24,65 @@
>> #ifndef __GVT_DEBUG_H__
>> #define __GVT_DEBUG_H__
>>
>> +#define GVT_MSG_NONE 0x00
>> +#define GVT_MSG_CORE 0x01
>> +#define GVT_MSG_IRQ 0x02
>> +#define GVT_MSG_MM 0x04
>> +#define GVT_MSG_MMIO 0x08
>> +#define GVT_MSG_DPY 0x10
>> +#define GVT_MSG_EL 0x20
>> +#define GVT_MSG_SCHED 0x40
>> +#define GVT_MSG_RENDER 0x80
>
>alignment
will fix. thanks.
>
>> +#define GVT_MSG_CMD 0x100
>> +
>> +#if IS_ENABLED(CONFIG_DRM_I915_GVT_DEBUG)
>> +__printf(3, 4)
>> +void gvt_printk(const char *level, unsigned int category,
>> + const char *format, ...);
>> +#define gvt_err(fmt, args...) \
>> + gvt_printk(KERN_ERR, GVT_MSG_NONE, fmt, ##args)
>> +#else
>> +static inline __printf(3, 4)
>> +void gvt_printk(const char *level, unsigned int category,
>> + const char *format, ...) {}
>> #define gvt_err(fmt, args...) \
>> DRM_ERROR("gvt: "fmt, ##args)
>> +#endif
>
>gvt_err() should always print so DRM_ERROR is fine.
The reason gvt_err switch here is when enable GVT_DEBUG, i think it's
better align error log with others categories. But i am OK with
always be DRM_ERROR.
>GVT_DEBUG should make below vgpu dbg info optional instead, e.g
>phrase out all those if !GVT_DEBUG.
>
>oh, looks if !GVT_DEBUG this would just fail to build?
>
Yes, it will fail. We need gvt_dbg_* defined anyway.
>>
>> #define gvt_vgpu_err(fmt, args...) \
>> do { \
>> if (IS_ERR_OR_NULL(vgpu)) \
>> - DRM_DEBUG_DRIVER("gvt: "fmt, ##args); \
>> + gvt_printk(KERN_WARNING, GVT_MSG_CORE, fmt, ##args); \
>> else \
>> - DRM_DEBUG_DRIVER("gvt: vgpu %d: "fmt, vgpu->id, ##args);\
>> + gvt_printk(KERN_WARNING, GVT_MSG_CORE, \
>> + "vgpu %d: "fmt, vgpu->id, ##args); \
>> } while (0)
>>
>> #define gvt_dbg_core(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: core: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_CORE, "core: "fmt, ##args)
>>
>> #define gvt_dbg_irq(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: irq: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_IRQ, "irq: "fmt, ##args)
>>
>> #define gvt_dbg_mm(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: mm: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_MM, "mm: "fmt, ##args)
>>
>> #define gvt_dbg_mmio(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: mmio: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_MMIO, "mmio: "fmt, ##args)
>>
>> #define gvt_dbg_dpy(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: dpy: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_DPY, "dpy: "fmt, ##args)
>>
>> #define gvt_dbg_el(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: el: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_EL, "el: "fmt, ##args)
>>
>> #define gvt_dbg_sched(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: sched: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_SCHED, "sched: "fmt, ##args)
>>
>> #define gvt_dbg_render(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: render: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_RENDER, "render: "fmt, ##args)
>>
>> #define gvt_dbg_cmd(fmt, args...) \
>> - DRM_DEBUG_DRIVER("gvt: cmd: "fmt, ##args)
>> + gvt_printk(KERN_DEBUG, GVT_MSG_CMD, "cmd: "fmt, ##args)
>>
>> #endif
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt_debug.c b/drivers/gpu/drm/i915/gvt/gvt_debug.c
>> new file mode 100644
>> index 0000000..8fe8b2f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gvt/gvt_debug.c
>> @@ -0,0 +1,41 @@
>> +#include <linux/module.h>
>> +#include "debug.h"
>> +
>> +static unsigned int gvt_debug;
>> +
>> +module_param_named(debug, gvt_debug, int, 0600);
>> +MODULE_PARM_DESC(debug, "Enable Gvt-g debug output, where each bit enables a category.\n"
>> + "\t\tBit 0 (0x01) will enable CORE messages (GVT-g core message)\n"
>> + "\t\tBit 1 (0x02) will enable IRQ messages (GVT-g interrupt message)\n"
>> + "\t\tBit 2 (0x04) will enable MM messages (GVT-g memory management message)\n"
>> + "\t\tBit 3 (0x08) will enable MMIO messages (GVT-g MMIO message)\n"
>> + "\t\tBit 4 (0x10) will enable DPY messages (GVT-g display message)\n"
>> + "\t\tBit 5 (0x20) will enable EL messages (GVT-g execlist message)\n"
>> + "\t\tBit 6 (0x40) will enable SCHED messages (GVT-g schedule message)\n"
>> + "\t\tBit 7 (0x80) will enable RENDER messages (GVT-g render message)\n"
>> + "\t\tBit 8 (0x100) will enable CMD messages (GVT-g command message)");
>> +
>> +void gvt_printk(const char *level, unsigned int category,
>> + const char *format, ...)
>> +{
>> + struct va_format vaf;
>> + va_list args;
>> +
>> + if (category != GVT_MSG_NONE && !(gvt_debug & category))
>> + return;
>> +
>> + va_start(args, format);
>> + vaf.fmt = format;
>> + vaf.va = &args;
>> +
>> + printk("%s[gvt:%ps]%s %pV",
>> + level, __builtin_return_address(0),
>> + strcmp(level, KERN_ERR) == 0 ? " *ERROR*" : "", &vaf);
>> +
>> + va_end(args);
>> +}
>> +EXPORT_SYMBOL_GPL(gvt_printk);
>
>Not required for other module, this is only for gvt internal usage.
Fully agree with you to build it in gvt module.
But here is limitation:
1) gvt in i915 module, while MPT in another module kvmgt
2) i915 module with gvt code has dependency on kvmgt
3) both i915 module with gvt and kvmgt has dependency on gvt_printk
If we build gvt_printk into i915, it will deadlock while i915 modprobe.
Any comments on this?
>
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_DESCRIPTION("GVT-g debug driver");
>> +MODULE_LICENSE("GPL");
>
>required?
>
>--
>Open Source Technology Center, Intel ltd.
>
>$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
>_______________________________________________
>intel-gvt-dev mailing list
>intel-gvt-dev at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
More information about the intel-gvt-dev
mailing list