| Message ID | 20171218155132.19236-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-12-18 15:51:31) > We dump device flags in few places (init_early, debugfs, gpu_error) > using different functions. Lets add reusable function to avoid > code duplication. > > add/remove: 1/0 grow/shrink: 0/3 up/down: 1296/-3572 (-2276) > Function old new delta > intel_device_info_dump_flags - 1296 +1296 > i915_capabilities 2435 1353 -1082 > i915_error_state_to_str 6642 5507 -1135 > intel_device_info_dump 1507 152 -1355 > Total: Before=1287992, After=1285716, chg -0.18% > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 5 ++--- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_gpu_error.c | 6 +++--- > drivers/gpu/drm/i915/intel_device_info.c | 20 ++++++++++++++++---- > 4 files changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 0ddce72..e384e28 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -58,14 +58,13 @@ static int i915_capabilities(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > const struct intel_device_info *info = INTEL_INFO(dev_priv); > + struct drm_printer p = drm_seq_file_printer(m); > > seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv)); > seq_printf(m, "platform: %s\n", intel_platform_name(info->platform)); > seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv)); > > -#define PRINT_FLAG(x) seq_printf(m, #x ": %s\n", yesno(info->x)) > - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); > -#undef PRINT_FLAG > + intel_device_info_dump_flags(info, &p); > > kernel_param_lock(THIS_MODULE); > #define PRINT_PARAM(T, x, ...) seq_print_param(m, #x, #T, &i915_modparams.x); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1aba565..26ebc70 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -4165,6 +4165,8 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) > const char *intel_platform_name(enum intel_platform platform); > void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); > void intel_device_info_dump(struct drm_i915_private *dev_priv); > +void intel_device_info_dump_flags(const struct intel_device_info *info, > + struct drm_printer *p); > > /* modesetting */ > extern void intel_modeset_init_hw(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index aba50aa..664f55c 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -564,9 +564,9 @@ static void print_error_obj(struct drm_i915_error_state_buf *m, > static void err_print_capabilities(struct drm_i915_error_state_buf *m, > const struct intel_device_info *info) > { > -#define PRINT_FLAG(x) err_printf(m, #x ": %s\n", yesno(info->x)) > - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); > -#undef PRINT_FLAG > + struct drm_printer p = i915_error_printer(m); > + > + intel_device_info_dump_flags(info, &p); > } > > static __always_inline void err_print_param(struct drm_i915_error_state_buf *m, > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index f478be3..5a385a9 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -22,6 +22,8 @@ > * > */ > > +#include <drm/drm_print.h> > + > #include "i915_drv.h" > > #define PLATFORM_NAME(x) [INTEL_##x] = #x > @@ -67,6 +69,14 @@ const char *intel_platform_name(enum intel_platform platform) > return platform_names[platform]; > } > > +void intel_device_info_dump_flags(const struct intel_device_info *info, > + struct drm_printer *p) > +{ > +#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name)); > + DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); > +#undef PRINT_FLAG > +} > + > void intel_device_info_dump(struct drm_i915_private *dev_priv) > { > const struct intel_device_info *info = &dev_priv->info; > @@ -76,10 +86,12 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv) > info->gen, > dev_priv->drm.pdev->device, > dev_priv->drm.pdev->revision); > -#define PRINT_FLAG(name) \ > - DRM_DEBUG_DRIVER("i915 device info: " #name ": %s", yesno(info->name)) > - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); > -#undef PRINT_FLAG > + > + if (drm_debug & DRM_UT_DRIVER) { > + struct drm_printer p = drm_debug_printer("i915 device info: "); > + > + intel_device_info_dump_flags(info, &p); > + } Looks good, I'm thinking we might push the drm_printer to the caller of intel_device_info_dump(), but this is already a substantial improvement Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Mon, 18 Dec 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-12-18 15:51:31) >> We dump device flags in few places (init_early, debugfs, gpu_error) >> using different functions. Lets add reusable function to avoid >> code duplication. >> >> add/remove: 1/0 grow/shrink: 0/3 up/down: 1296/-3572 (-2276) >> Function old new delta >> intel_device_info_dump_flags - 1296 +1296 >> i915_capabilities 2435 1353 -1082 >> i915_error_state_to_str 6642 5507 -1135 >> intel_device_info_dump 1507 152 -1355 >> Total: Before=1287992, After=1285716, chg -0.18% >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 5 ++--- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/i915_gpu_error.c | 6 +++--- >> drivers/gpu/drm/i915/intel_device_info.c | 20 ++++++++++++++++---- >> 4 files changed, 23 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 0ddce72..e384e28 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -58,14 +58,13 @@ static int i915_capabilities(struct seq_file *m, void *data) >> { >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> const struct intel_device_info *info = INTEL_INFO(dev_priv); >> + struct drm_printer p = drm_seq_file_printer(m); >> >> seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv)); >> seq_printf(m, "platform: %s\n", intel_platform_name(info->platform)); >> seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv)); >> >> -#define PRINT_FLAG(x) seq_printf(m, #x ": %s\n", yesno(info->x)) >> - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); >> -#undef PRINT_FLAG >> + intel_device_info_dump_flags(info, &p); >> >> kernel_param_lock(THIS_MODULE); >> #define PRINT_PARAM(T, x, ...) seq_print_param(m, #x, #T, &i915_modparams.x); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 1aba565..26ebc70 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -4165,6 +4165,8 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) >> const char *intel_platform_name(enum intel_platform platform); >> void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); >> void intel_device_info_dump(struct drm_i915_private *dev_priv); >> +void intel_device_info_dump_flags(const struct intel_device_info *info, >> + struct drm_printer *p); >> >> /* modesetting */ >> extern void intel_modeset_init_hw(struct drm_device *dev); >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c >> index aba50aa..664f55c 100644 >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >> @@ -564,9 +564,9 @@ static void print_error_obj(struct drm_i915_error_state_buf *m, >> static void err_print_capabilities(struct drm_i915_error_state_buf *m, >> const struct intel_device_info *info) >> { >> -#define PRINT_FLAG(x) err_printf(m, #x ": %s\n", yesno(info->x)) >> - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); >> -#undef PRINT_FLAG >> + struct drm_printer p = i915_error_printer(m); >> + >> + intel_device_info_dump_flags(info, &p); >> } >> >> static __always_inline void err_print_param(struct drm_i915_error_state_buf *m, >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> index f478be3..5a385a9 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -22,6 +22,8 @@ >> * >> */ >> >> +#include <drm/drm_print.h> >> + >> #include "i915_drv.h" >> >> #define PLATFORM_NAME(x) [INTEL_##x] = #x >> @@ -67,6 +69,14 @@ const char *intel_platform_name(enum intel_platform platform) >> return platform_names[platform]; >> } >> >> +void intel_device_info_dump_flags(const struct intel_device_info *info, >> + struct drm_printer *p) >> +{ >> +#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name)); >> + DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); >> +#undef PRINT_FLAG >> +} >> + >> void intel_device_info_dump(struct drm_i915_private *dev_priv) >> { >> const struct intel_device_info *info = &dev_priv->info; >> @@ -76,10 +86,12 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv) >> info->gen, >> dev_priv->drm.pdev->device, >> dev_priv->drm.pdev->revision); >> -#define PRINT_FLAG(name) \ >> - DRM_DEBUG_DRIVER("i915 device info: " #name ": %s", yesno(info->name)) >> - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); >> -#undef PRINT_FLAG >> + >> + if (drm_debug & DRM_UT_DRIVER) { >> + struct drm_printer p = drm_debug_printer("i915 device info: "); >> + >> + intel_device_info_dump_flags(info, &p); >> + } > > > Looks good, I'm thinking we might push the drm_printer to the caller of > intel_device_info_dump(), but this is already a substantial improvement Param ordering seems a bit surprising in both patches. Target first? BR, Jani. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Jani Nikula (2017-12-18 17:37:13) > On Mon, 18 Dec 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Looks good, I'm thinking we might push the drm_printer to the caller of > > intel_device_info_dump(), but this is already a substantial improvement > > Param ordering seems a bit surprising in both patches. Target first? The param order at the moment tries to be (object-doing-the-dump, stream-as-parameter). I don't think it is quite so backwards as it first appears, when compared to printf(stderr, fmt,...) or even seq_printf(m, fmt, ..). In both of those printf cases, it is the stream that can be argued is providing the object (even more so if you create your own stream using cookie_io_functions_t). But for us, it is clearer to argue that the object we control is the object providing the method to output into the stream. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 0ddce72..e384e28 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -58,14 +58,13 @@ static int i915_capabilities(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); const struct intel_device_info *info = INTEL_INFO(dev_priv); + struct drm_printer p = drm_seq_file_printer(m); seq_printf(m, "gen: %d\n", INTEL_GEN(dev_priv)); seq_printf(m, "platform: %s\n", intel_platform_name(info->platform)); seq_printf(m, "pch: %d\n", INTEL_PCH_TYPE(dev_priv)); -#define PRINT_FLAG(x) seq_printf(m, #x ": %s\n", yesno(info->x)) - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); -#undef PRINT_FLAG + intel_device_info_dump_flags(info, &p); kernel_param_lock(THIS_MODULE); #define PRINT_PARAM(T, x, ...) seq_print_param(m, #x, #T, &i915_modparams.x); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1aba565..26ebc70 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -4165,6 +4165,8 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev) const char *intel_platform_name(enum intel_platform platform); void intel_device_info_runtime_init(struct drm_i915_private *dev_priv); void intel_device_info_dump(struct drm_i915_private *dev_priv); +void intel_device_info_dump_flags(const struct intel_device_info *info, + struct drm_printer *p); /* modesetting */ extern void intel_modeset_init_hw(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index aba50aa..664f55c 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -564,9 +564,9 @@ static void print_error_obj(struct drm_i915_error_state_buf *m, static void err_print_capabilities(struct drm_i915_error_state_buf *m, const struct intel_device_info *info) { -#define PRINT_FLAG(x) err_printf(m, #x ": %s\n", yesno(info->x)) - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); -#undef PRINT_FLAG + struct drm_printer p = i915_error_printer(m); + + intel_device_info_dump_flags(info, &p); } static __always_inline void err_print_param(struct drm_i915_error_state_buf *m, diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index f478be3..5a385a9 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -22,6 +22,8 @@ * */ +#include <drm/drm_print.h> + #include "i915_drv.h" #define PLATFORM_NAME(x) [INTEL_##x] = #x @@ -67,6 +69,14 @@ const char *intel_platform_name(enum intel_platform platform) return platform_names[platform]; } +void intel_device_info_dump_flags(const struct intel_device_info *info, + struct drm_printer *p) +{ +#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name)); + DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); +#undef PRINT_FLAG +} + void intel_device_info_dump(struct drm_i915_private *dev_priv) { const struct intel_device_info *info = &dev_priv->info; @@ -76,10 +86,12 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv) info->gen, dev_priv->drm.pdev->device, dev_priv->drm.pdev->revision); -#define PRINT_FLAG(name) \ - DRM_DEBUG_DRIVER("i915 device info: " #name ": %s", yesno(info->name)) - DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG); -#undef PRINT_FLAG + + if (drm_debug & DRM_UT_DRIVER) { + struct drm_printer p = drm_debug_printer("i915 device info: "); + + intel_device_info_dump_flags(info, &p); + } } static void gen10_sseu_info_init(struct drm_i915_private *dev_priv)
We dump device flags in few places (init_early, debugfs, gpu_error) using different functions. Lets add reusable function to avoid code duplication. add/remove: 1/0 grow/shrink: 0/3 up/down: 1296/-3572 (-2276) Function old new delta intel_device_info_dump_flags - 1296 +1296 i915_capabilities 2435 1353 -1082 i915_error_state_to_str 6642 5507 -1135 intel_device_info_dump 1507 152 -1355 Total: Before=1287992, After=1285716, chg -0.18% Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_debugfs.c | 5 ++--- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gpu_error.c | 6 +++--- drivers/gpu/drm/i915/intel_device_info.c | 20 ++++++++++++++++---- 4 files changed, 23 insertions(+), 10 deletions(-)