| Message ID | 20190725205106.36148-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/uc: Don't sanitize guc_log_level modparam | expand |
Quoting Michal Wajdeczko (2019-07-25 21:51:06) > We are already storing runtime value of log level in private > field, so there is no need to modify modparam. There is an aspect of communicating the clamped value back to the user. Does that have any value or alternative? -Chris
On Thu, 25 Jul 2019 23:44:08 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-07-25 21:51:06) >> We are already storing runtime value of log level in private >> field, so there is no need to modify modparam. > > There is an aspect of communicating the clamped value back to the user. > Does that have any value or alternative? Actual (clamped or default) value of the GuC log level is exposed in i915_guc_log_level debugfs entry. User can modify it from there too. Michal
Quoting Michal Wajdeczko (2019-07-26 05:47:03) > On Thu, 25 Jul 2019 23:44:08 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-07-25 21:51:06) > >> We are already storing runtime value of log level in private > >> field, so there is no need to modify modparam. > > > > There is an aspect of communicating the clamped value back to the user. > > Does that have any value or alternative? > > Actual (clamped or default) value of the GuC log level is exposed in > i915_guc_log_level debugfs entry. User can modify it from there too. Why? :) I fail to see why we have two methods of setting a variable, if we want a callback on modparam we can supply a param_ops.set()... Just being a nuisance, we should be planning sysfs/gt/ controls. -Chris
On Fri, 26 Jul 2019 10:21:57 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-07-26 05:47:03) >> On Thu, 25 Jul 2019 23:44:08 +0200, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >> > Quoting Michal Wajdeczko (2019-07-25 21:51:06) >> >> We are already storing runtime value of log level in private >> >> field, so there is no need to modify modparam. >> > >> > There is an aspect of communicating the clamped value back to the >> user. >> > Does that have any value or alternative? >> >> Actual (clamped or default) value of the GuC log level is exposed in >> i915_guc_log_level debugfs entry. User can modify it from there too. > > Why? :) I fail to see why we have two methods of setting a variable, if > we want a callback on modparam we can supply a param_ops.set()... Single modparam value may not work in the future as we may have to support multiple devices on single platform. There will be more similar changes around other guc modparams in a moment. Michal
Quoting Michal Wajdeczko (2019-07-26 10:48:31) > On Fri, 26 Jul 2019 10:21:57 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-07-26 05:47:03) > >> On Thu, 25 Jul 2019 23:44:08 +0200, Chris Wilson > >> <chris@chris-wilson.co.uk> wrote: > >> > >> > Quoting Michal Wajdeczko (2019-07-25 21:51:06) > >> >> We are already storing runtime value of log level in private > >> >> field, so there is no need to modify modparam. > >> > > >> > There is an aspect of communicating the clamped value back to the > >> user. > >> > Does that have any value or alternative? > >> > >> Actual (clamped or default) value of the GuC log level is exposed in > >> i915_guc_log_level debugfs entry. User can modify it from there too. > > > > Why? :) I fail to see why we have two methods of setting a variable, if > > we want a callback on modparam we can supply a param_ops.set()... > > Single modparam value may not work in the future as we may have to support > multiple devices on single platform. There will be more similar changes > around other guc modparams in a moment. Exactly, and debugfs doesn't exist. -Chris
Quoting Michal Wajdeczko (2019-07-25 21:51:06) > We are already storing runtime value of log level in private > field, so there is no need to modify modparam. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Michal is persuasive that having already broken the link between the modparam and the actual value set via debugfs, no one can currently trust the value reported by modparam. In that respect, this should not have noticeable userspace consequences and is just another step along the road of removing the modparam entirely. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c index 77fda1e85d3b..3460deca12c8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c @@ -443,6 +443,29 @@ static void guc_log_capture_logs(struct intel_guc_log *log) guc_action_flush_log_complete(guc); } +static u32 __get_default_log_level(struct intel_guc_log *log) +{ + /* A negative value means "use platform/config default" */ + if (i915_modparams.guc_log_level < 0) { + return (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || + IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) ? + GUC_LOG_LEVEL_MAX : GUC_LOG_LEVEL_NON_VERBOSE; + } + + if (i915_modparams.guc_log_level > GUC_LOG_LEVEL_MAX) { + DRM_WARN("Incompatible option detected: %s=%d, %s!\n", + "guc_log_level", i915_modparams.guc_log_level, + "verbosity too high"); + return (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || + IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) ? + GUC_LOG_LEVEL_MAX : GUC_LOG_LEVEL_DISABLED; + } + + GEM_BUG_ON(i915_modparams.guc_log_level < GUC_LOG_LEVEL_DISABLED); + GEM_BUG_ON(i915_modparams.guc_log_level > GUC_LOG_LEVEL_MAX); + return i915_modparams.guc_log_level; +} + int intel_guc_log_create(struct intel_guc_log *log) { struct intel_guc *guc = log_to_guc(log); @@ -482,7 +505,11 @@ int intel_guc_log_create(struct intel_guc_log *log) log->vma = vma; - log->level = i915_modparams.guc_log_level; + log->level = __get_default_log_level(log); + DRM_DEBUG_DRIVER("guc_log_level=%d (%s, verbose:%s, verbosity:%d)\n", + log->level, enableddisabled(log->level), + yesno(GUC_LOG_LEVEL_IS_VERBOSE(log->level)), + GUC_LOG_LEVEL_TO_VERBOSITY(log->level)); return 0; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index b1815abecf30..4ace47c0978f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -74,23 +74,6 @@ static int __get_platform_enable_guc(struct intel_uc *uc) return enable_guc; } -static int __get_default_guc_log_level(struct intel_uc *uc) -{ - int guc_log_level; - - if (!intel_uc_fw_supported(&uc->guc.fw) || !intel_uc_is_using_guc(uc)) - guc_log_level = GUC_LOG_LEVEL_DISABLED; - else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || - IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) - guc_log_level = GUC_LOG_LEVEL_MAX; - else - guc_log_level = GUC_LOG_LEVEL_NON_VERBOSE; - - /* Any platform specific fine-tuning can be done here */ - - return guc_log_level; -} - /** * sanitize_options_early - sanitize uC related modparam options * @uc: the intel_uc structure @@ -100,13 +83,6 @@ static int __get_default_guc_log_level(struct intel_uc *uc) * modparam varies between platforms and it is hardcoded in driver code. * Any other modparam value is only monitored against availability of the * related hardware or firmware definitions. - * - * In case of "guc_log_level" option this function will attempt to modify - * it only if it was initially set to "auto(-1)" or if initial value was - * "enable(1..4)" on platforms without the GuC. Default value for this - * modparam varies between platforms and is usually set to "disable(0)" - * unless GuC is enabled on given platform and the driver is compiled with - * debug config when this modparam will default to "enable(1..4)". */ static void sanitize_options_early(struct intel_uc *uc) { @@ -149,34 +125,8 @@ static void sanitize_options_early(struct intel_uc *uc) i915_modparams.enable_guc &= ~ENABLE_GUC_SUBMISSION; } - /* A negative value means "use platform/config default" */ - if (i915_modparams.guc_log_level < 0) - i915_modparams.guc_log_level = - __get_default_guc_log_level(uc); - - if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(uc)) { - DRM_WARN("Incompatible option detected: guc_log_level=%d, " - "but GuC is not enabled!\n", - i915_modparams.guc_log_level); - i915_modparams.guc_log_level = 0; - } - - if (i915_modparams.guc_log_level > GUC_LOG_LEVEL_MAX) { - DRM_WARN("Incompatible option detected: %s=%d, %s!\n", - "guc_log_level", i915_modparams.guc_log_level, - "verbosity too high"); - i915_modparams.guc_log_level = GUC_LOG_LEVEL_MAX; - } - - DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n", - i915_modparams.guc_log_level, - yesno(i915_modparams.guc_log_level), - yesno(GUC_LOG_LEVEL_IS_VERBOSE(i915_modparams.guc_log_level)), - GUC_LOG_LEVEL_TO_VERBOSITY(i915_modparams.guc_log_level)); - /* Make sure that sanitization was done */ GEM_BUG_ON(i915_modparams.enable_guc < 0); - GEM_BUG_ON(i915_modparams.guc_log_level < 0); } void intel_uc_init_early(struct intel_uc *uc)
We are already storing runtime value of log level in private field, so there is no need to modify modparam. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 29 ++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 50 ---------------------- 2 files changed, 28 insertions(+), 51 deletions(-)