[Intel-gfx] [PATCH v2 01/15] drm/i915/guc: Tidy guc_log_control
Sagar Arun Kamble
sagar.a.kamble at intel.com
Fri Mar 9 05:46:27 UTC 2018
On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> We plan to decouple log runtime (mapping + relay) from verbosity control.
> Let's tidy the code now to reduce the churn in the following patches.
>
> v2: Tidy macros, keep debug messages, use helper var for enable,
> correct typo (Michał)
> Fix incorrect input validaction (Sagar)
>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 11 ++---
> drivers/gpu/drm/i915/intel_guc_log.c | 80 +++++++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
> 3 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 89f7ff2c652e..fa0755fe10d0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2500,13 +2500,10 @@ static int i915_guc_log_control_get(void *data, u64 *val)
> {
> struct drm_i915_private *dev_priv = data;
>
> - if (!HAS_GUC(dev_priv))
> + if (!USES_GUC(dev_priv))
> return -ENODEV;
>
> - if (!dev_priv->guc.log.vma)
> - return -EINVAL;
> -
> - *val = i915_modparams.guc_log_level;
> + *val = intel_guc_log_control_get(&dev_priv->guc);
>
> return 0;
> }
> @@ -2515,10 +2512,10 @@ static int i915_guc_log_control_set(void *data, u64 val)
> {
> struct drm_i915_private *dev_priv = data;
>
> - if (!HAS_GUC(dev_priv))
> + if (!USES_GUC(dev_priv))
> return -ENODEV;
>
> - return intel_guc_log_control(&dev_priv->guc, val);
> + return intel_guc_log_control_set(&dev_priv->guc, val);
> }
>
> DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops,
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index c0c2e7d1c7d7..7e59fb07b06b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -659,51 +659,63 @@ void intel_guc_log_destroy(struct intel_guc *guc)
> i915_vma_unpin_and_release(&guc->log.vma);
> }
>
> -int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> +int intel_guc_log_control_get(struct intel_guc *guc)
> +{
> + GEM_BUG_ON(!guc->log.vma);
> + GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> +
> + return i915_modparams.guc_log_level;
> +}
> +
> +#define GUC_LOG_LEVEL_DISABLED 0
> +#define LOG_LEVEL_TO_ENABLED(x) ((x) > 0)
> +#define LOG_LEVEL_TO_VERBOSITY(x) ({ \
> + typeof(x) _x = (x); \
> + LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0; \
> +})
> +#define VERBOSITY_TO_LOG_LEVEL(x) ((x) + 1)
> +int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - bool enable_logging = control_val > 0;
> - u32 verbosity;
> + bool enabled = LOG_LEVEL_TO_ENABLED(val);
> int ret;
>
> - if (!guc->log.vma)
> - return -ENODEV;
> + BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
> + GEM_BUG_ON(!guc->log.vma);
> + GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>
> - BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
> - if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
> + /*
> + * GuC is recognizing log levels starting from 0 to max, we're using 0
> + * as indication that logging should be disabled.
> + */
> + if (val < GUC_LOG_LEVEL_DISABLED ||
> + val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
> return -EINVAL;
>
> - /* This combination doesn't make sense & won't have any effect */
> - if (!enable_logging && !i915_modparams.guc_log_level)
> - return 0;
> + mutex_lock(&dev_priv->drm.struct_mutex);
>
> - verbosity = enable_logging ? control_val - 1 : 0;
> + if (i915_modparams.guc_log_level == val) {
> + ret = 0;
> + goto out_unlock;
> + }
>
> - ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> - if (ret)
> - return ret;
> intel_runtime_pm_get(dev_priv);
> - ret = guc_log_control(guc, enable_logging, verbosity);
> + ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
> intel_runtime_pm_put(dev_priv);
> - mutex_unlock(&dev_priv->drm.struct_mutex);
> -
> - if (ret < 0) {
> - DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
> - return ret;
> + if (ret) {
> + DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
> + goto out_unlock;
> }
>
> - if (enable_logging) {
> - i915_modparams.guc_log_level = 1 + verbosity;
> + i915_modparams.guc_log_level = val;
>
> - /*
> - * If log was disabled at boot time, then the relay channel file
> - * wouldn't have been created by now and interrupts also would
> - * not have been enabled. Try again now, just in case.
> - */
> + mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> + if (enabled && !guc_log_has_runtime(guc)) {
> ret = guc_log_late_setup(guc);
> - if (ret < 0) {
> + if (ret) {
> DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> - return ret;
> + goto out;
> }
>
> /* GuC logging is currently the only user of Guc2Host interrupts */
> @@ -712,7 +724,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> gen9_enable_guc_interrupts(dev_priv);
> intel_runtime_pm_put(dev_priv);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> - } else {
> + } else if (!enabled && guc_log_has_runtime(guc)) {
> /*
> * Once logging is disabled, GuC won't generate logs & send an
> * interrupt. But there could be some data in the log buffer
> @@ -720,11 +732,13 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> * buffer state and then collect the left over logs.
> */
> guc_flush_logs(guc);
> -
> - /* As logging is disabled, update log level to reflect that */
> - i915_modparams.guc_log_level = 0;
> }
>
> + return 0;
> +
> +out_unlock:
> + mutex_unlock(&dev_priv->drm.struct_mutex);
> +out:
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index dab0e949567a..141ce9ca22ce 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -64,7 +64,8 @@ void intel_guc_log_destroy(struct intel_guc *guc);
> void intel_guc_log_init_early(struct intel_guc *guc);
> int intel_guc_log_relay_create(struct intel_guc *guc);
> void intel_guc_log_relay_destroy(struct intel_guc *guc);
> -int intel_guc_log_control(struct intel_guc *guc, u64 control_val);
> +int intel_guc_log_control_get(struct intel_guc *guc);
> +int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val);
> void i915_guc_log_register(struct drm_i915_private *dev_priv);
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
>
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list