[Intel-gfx] [PATCH 01/15] drm/i915/guc: Tidy guc_log_control
Sagar Arun Kamble
sagar.a.kamble at intel.com
Mon Mar 5 05:29:19 UTC 2018
On 3/2/2018 5:22 PM, Michał Winiarski wrote:
> On Fri, Mar 02, 2018 at 04:39:38PM +0530, Sagar Arun Kamble wrote:
>>
>> On 2/27/2018 6:22 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.
>>>
>>> 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>
>>> ---
>>> drivers/gpu/drm/i915/i915_debugfs.c | 11 ++----
>>> drivers/gpu/drm/i915/intel_guc_log.c | 75 +++++++++++++++++++-----------------
>>> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
>>> 3 files changed, 46 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 33fbf3965309..58983cafaece 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)
>> Should we name this i915_guc_log_level_get instead? and other related
>> functions too?
> I chose symmetry here, note that the debugfs file is still named
> i915_guc_log_control at this point. This changes later in the series though.
>
>>> {
>>> 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 7b5074e2120c..22a05320817b 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>>> @@ -657,52 +657,55 @@ 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)
>> Should we be passing guc_log as parameter and implement guc_log_to_guc()
>> function.
> This is the top-level interface exported for GuC users. In other words - callers
> of this function shouldn't have to know about struct guc_log (and the fact that
> it's located inside struct intel_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_IS_ENABLED(x) (x > 0)
>>> +#define GUC_LOG_LEVEL_TO_VERBOSITY(x) (GUC_LOG_IS_ENABLED(x) ? x - 1 : 0)
>> This is bit misleading, can we make this macro return -1 if logging is to be
>> disabled. That way guc_log_control can be invoked with
>> single signed 32bit parameter.
> Note that guc_log_control is the function operating directly on GuC interface.
> This Host2GuC action really takes 3 arguments (2 parameters here) - enable,
> default_logging_enable, verbosity.
> As a consequence, I'd like to avoid placing any logic there. The macros are
> taking care of translation from guc_log_level modparam to values understood by
> GuC (host2guc params).
>
> I agree that the naming is confusing here.
> I'll go with LOG_LEVEL_TO_ENABLED(x) and LOG_LEVEL_TO_VERBOSITY(x) in second
> spin as suggested by Michał.
>
>>> +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;
>>> 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 disablded.
>>> + */
>>> + if (GUC_LOG_LEVEL_TO_VERBOSITY(val) < GUC_LOG_VERBOSITY_MIN ||
>> This check seems unnecessary as we currently don't have negative output for
>> G_L_L_T_V macro.
>> If we add negative value there, will need to remove this check.
> Yeah, agree. That's an error on my part, I wanted to do input validation here.
> This should probably be something more like:
> if (val < VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MIN) ||
I think we should drop the min side check because val will never be
negative and if we want to keep the check
then it should be
#define GUC_LOG_LEVEL_DISABED 0
if (val < GUC_LOG_LEVEL_DISABLED) ||
Since we want to invoke guc_log_control to disable the logging.
> val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
>
> -Michał
>
>>> + GUC_LOG_LEVEL_TO_VERBOSITY(val) > 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, GUC_LOG_IS_ENABLED(val),
>>> + GUC_LOG_LEVEL_TO_VERBOSITY(val));
>>> intel_runtime_pm_put(dev_priv);
>>> - mutex_unlock(&dev_priv->drm.struct_mutex);
>>> + if (ret)
>>> + goto out_unlock;
>>> - if (ret < 0) {
>>> - DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
>>> - return ret;
>>> - }
>>> + i915_modparams.guc_log_level = val;
>>> - if (enable_logging) {
>>> - i915_modparams.guc_log_level = 1 + verbosity;
>>> + mutex_unlock(&dev_priv->drm.struct_mutex);
>>> - /*
>>> - * 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.
>>> - */
>>> + if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
>>> ret = guc_log_late_setup(guc);
>>> - if (ret < 0) {
>>> - DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
>>> - return ret;
>>> - }
>>> + if (ret)
>>> + goto out;
>>> /* GuC logging is currently the only user of Guc2Host interrupts */
>>> mutex_lock(&dev_priv->drm.struct_mutex);
>>> @@ -710,7 +713,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 (!GUC_LOG_IS_ENABLED(val) && 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
>>> @@ -718,11 +721,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
>>
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list