| Message ID | 20180510143113.13219-1-piotr.piorkowski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, 10 May 2018 16:31:13 +0200, Piotr Piorkowski <piotr.piorkowski@intel.com> wrote: > From: Piotr Piórkowski <piotr.piorkowski@intel.com> > > Currently we are using modparam as placeholder for GuC log level. > Stop doing this and keep runtime GuC level in intel_guc_log struct. > > Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_guc.c | 8 +++----- > drivers/gpu/drm/i915/intel_guc_log.c | 17 ++++++++--------- > drivers/gpu/drm/i915/intel_guc_log.h | 3 ++- > drivers/gpu/drm/i915/intel_uc.c | 2 +- > 4 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c > b/drivers/gpu/drm/i915/intel_guc.c > index 116f4ccf1bbd..4b489b67663e 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -203,13 +203,11 @@ void intel_guc_fini(struct intel_guc *guc) > guc_shared_data_destroy(guc); > } > -static u32 get_log_control_flags(void) > +static u32 get_log_control_flags(struct intel_guc_log *log) I would rather pass *guc here, as this is still intel_guc.c file, not intel_guc_log.* > { > - u32 level = i915_modparams.guc_log_level; > + u32 level = log->level; As you still have dedicated function to read level (see below) maybe you should use it > u32 flags = 0; > - GEM_BUG_ON(level < 0); > - > if (!GUC_LOG_LEVEL_IS_ENABLED(level)) > flags |= GUC_LOG_DEFAULT_DISABLED; > @@ -250,7 +248,7 @@ void intel_guc_init_params(struct intel_guc *guc) > params[GUC_CTL_LOG_PARAMS] = guc->log.flags; > - params[GUC_CTL_DEBUG] = get_log_control_flags(); > + params[GUC_CTL_DEBUG] = get_log_control_flags(&guc->log); As you touch function signature, maybe it is good time to rename it to: static u32 guc_ctl_debug_flags(guc) { } as we may want to do similar thing for other complex params ;) > /* If GuC submission is enabled, set up additional parameters here */ > if (USES_GUC_SUBMISSION(dev_priv)) { > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 401e1704d61e..b3819680070a 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -475,11 +475,13 @@ int intel_guc_log_create(struct intel_guc_log *log) > offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT; > log->flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; > + GEM_BUG_ON(i915_modparams.guc_log_level < 0); Maybe we don't need this GEM_BUG_ON as we have one in sanitize_options_early. > + log->level = i915_modparams.guc_log_level; > + > return 0; > err: > - /* logging will be off */ > - i915_modparams.guc_log_level = 0; > + DRM_DEBUG_DRIVER("Failed to allocate GuC log buffer. %d\n", ret); > return ret; > } > @@ -488,12 +490,10 @@ void intel_guc_log_destroy(struct intel_guc_log > *log) > i915_vma_unpin_and_release(&log->vma); > } > -int intel_guc_log_level_get(struct intel_guc_log *log) > +u32 intel_guc_log_level_get(struct intel_guc_log *log) > { > GEM_BUG_ON(!log->vma); > - GEM_BUG_ON(i915_modparams.guc_log_level < 0); > - > - return i915_modparams.guc_log_level; > + return log->level; I guess this function can be now defined as inline in .h > } > int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) > @@ -504,7 +504,6 @@ int intel_guc_log_level_set(struct intel_guc_log > *log, u64 val) > BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0); > GEM_BUG_ON(!log->vma); > - GEM_BUG_ON(i915_modparams.guc_log_level < 0); > /* > * GuC is recognizing log levels starting from 0 to max, we're using 0 > @@ -515,7 +514,7 @@ int intel_guc_log_level_set(struct intel_guc_log > *log, u64 val) > mutex_lock(&dev_priv->drm.struct_mutex); > - if (i915_modparams.guc_log_level == val) { > + if (log->level == val) { > ret = 0; > goto out_unlock; > } > @@ -530,7 +529,7 @@ int intel_guc_log_level_set(struct intel_guc_log > *log, u64 val) > goto out_unlock; > } > - i915_modparams.guc_log_level = val; > + log->level = val; > out_unlock: > mutex_unlock(&dev_priv->drm.struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h > b/drivers/gpu/drm/i915/intel_guc_log.h > index fa80535a6f9d..db21241588e6 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -59,6 +59,7 @@ struct intel_guc; > struct intel_guc_log { > u32 flags; > + u32 level; [bikeshed] I would reorder above, as 'level' is more important than static flags ... btw, maybe we can replace that 'flags' with static function in intel_guc.c similar to existing one like: static u32 guc_ctl_log_flags(guc) { } > struct i915_vma *vma; > struct { > void *buf_addr; > @@ -80,7 +81,7 @@ void intel_guc_log_init_early(struct intel_guc_log > *log); > int intel_guc_log_create(struct intel_guc_log *log); > void intel_guc_log_destroy(struct intel_guc_log *log); > -int intel_guc_log_level_get(struct intel_guc_log *log); > +u32 intel_guc_log_level_get(struct intel_guc_log *log); > int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val); > bool intel_guc_log_relay_enabled(const struct intel_guc_log *log); > int intel_guc_log_relay_open(struct intel_guc_log *log); > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 1cffaf7b5dbe..897eaad09c7d 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -208,7 +208,7 @@ void intel_uc_init_mmio(struct drm_i915_private > *dev_priv) > static void guc_capture_load_err_log(struct intel_guc *guc) > { > - if (!guc->log.vma || !i915_modparams.guc_log_level) > + if (!guc->log.vma || !guc->log.level) > return; > if (!guc->load_err_log) Thanks, Michal
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 116f4ccf1bbd..4b489b67663e 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -203,13 +203,11 @@ void intel_guc_fini(struct intel_guc *guc) guc_shared_data_destroy(guc); } -static u32 get_log_control_flags(void) +static u32 get_log_control_flags(struct intel_guc_log *log) { - u32 level = i915_modparams.guc_log_level; + u32 level = log->level; u32 flags = 0; - GEM_BUG_ON(level < 0); - if (!GUC_LOG_LEVEL_IS_ENABLED(level)) flags |= GUC_LOG_DEFAULT_DISABLED; @@ -250,7 +248,7 @@ void intel_guc_init_params(struct intel_guc *guc) params[GUC_CTL_LOG_PARAMS] = guc->log.flags; - params[GUC_CTL_DEBUG] = get_log_control_flags(); + params[GUC_CTL_DEBUG] = get_log_control_flags(&guc->log); /* If GuC submission is enabled, set up additional parameters here */ if (USES_GUC_SUBMISSION(dev_priv)) { diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 401e1704d61e..b3819680070a 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -475,11 +475,13 @@ int intel_guc_log_create(struct intel_guc_log *log) offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT; log->flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; + GEM_BUG_ON(i915_modparams.guc_log_level < 0); + log->level = i915_modparams.guc_log_level; + return 0; err: - /* logging will be off */ - i915_modparams.guc_log_level = 0; + DRM_DEBUG_DRIVER("Failed to allocate GuC log buffer. %d\n", ret); return ret; } @@ -488,12 +490,10 @@ void intel_guc_log_destroy(struct intel_guc_log *log) i915_vma_unpin_and_release(&log->vma); } -int intel_guc_log_level_get(struct intel_guc_log *log) +u32 intel_guc_log_level_get(struct intel_guc_log *log) { GEM_BUG_ON(!log->vma); - GEM_BUG_ON(i915_modparams.guc_log_level < 0); - - return i915_modparams.guc_log_level; + return log->level; } int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) @@ -504,7 +504,6 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0); GEM_BUG_ON(!log->vma); - GEM_BUG_ON(i915_modparams.guc_log_level < 0); /* * GuC is recognizing log levels starting from 0 to max, we're using 0 @@ -515,7 +514,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) mutex_lock(&dev_priv->drm.struct_mutex); - if (i915_modparams.guc_log_level == val) { + if (log->level == val) { ret = 0; goto out_unlock; } @@ -530,7 +529,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) goto out_unlock; } - i915_modparams.guc_log_level = val; + log->level = val; out_unlock: mutex_unlock(&dev_priv->drm.struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index fa80535a6f9d..db21241588e6 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -59,6 +59,7 @@ struct intel_guc; struct intel_guc_log { u32 flags; + u32 level; struct i915_vma *vma; struct { void *buf_addr; @@ -80,7 +81,7 @@ void intel_guc_log_init_early(struct intel_guc_log *log); int intel_guc_log_create(struct intel_guc_log *log); void intel_guc_log_destroy(struct intel_guc_log *log); -int intel_guc_log_level_get(struct intel_guc_log *log); +u32 intel_guc_log_level_get(struct intel_guc_log *log); int intel_guc_log_level_set(struct intel_guc_log *log, u64 control_val); bool intel_guc_log_relay_enabled(const struct intel_guc_log *log); int intel_guc_log_relay_open(struct intel_guc_log *log); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 1cffaf7b5dbe..897eaad09c7d 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -208,7 +208,7 @@ void intel_uc_init_mmio(struct drm_i915_private *dev_priv) static void guc_capture_load_err_log(struct intel_guc *guc) { - if (!guc->log.vma || !i915_modparams.guc_log_level) + if (!guc->log.vma || !guc->log.level) return; if (!guc->load_err_log)