On Mon, 18 Dec 2017, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxx>
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
