[Intel-gfx] [PATCH v3 09/12] drm/i915/guc: Make GuC log related functions depend only on log level
Sagar Arun Kamble
sagar.a.kamble at intel.com
Fri Jan 5 04:58:47 UTC 2018
On 1/4/2018 10:45 PM, Michal Wajdeczko wrote:
> On Thu, 04 Jan 2018 17:21:51 +0100, Sagar Arun Kamble
> <sagar.a.kamble at intel.com> wrote:
>
>> With GuC log level set properly only for cases where GuC is loaded we
>> can
>> remove the GuC submission checks from flush_guc_logs and
>> guc_log_register,
>> unregister and uc_fini_hw functions. It is important to note that GuC
>> log
>> runtime data has to be freed during driver unregister.
>> Freeing of that data can't be gated by guc_log_level check because if we
>> free GuC log runtime only when log level >=0 then it will not be
>> destroyed
>> when logging is disabled after enabling before driver unload.
>>
>> Also, with this patch GuC interrupts are enabled first after GuC load if
>> logging is enabled. GuC to Host interrupts will be needed for GuC CT
>> buffer recv mechanism and hence we will be adding support to control
>> that
>> interrupt based on ref. taken by Log or CT recv feature in next
>> patch. To
>> prepare for that all interrupt updates are now gated by GuC log level
>> checks.
>>
>> v2: Rebase. Updated check in i915_guc_log_unregister to be based on
>> guc_log_level. (Michal Wajdeczko)
>>
>> v3: Rebase. Made all GuC log related functions depend only log level.
>> Updated uC init w.r.t enabling of GuC interrupts. Commit message update.
>> Rebase w.r.t guc_log_level immutable changes. (Tvrtko)
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_guc.c | 3 ++-
>> drivers/gpu/drm/i915/intel_guc_log.c | 17 +++++++----------
>> drivers/gpu/drm/i915/intel_uc.c | 15 ++++++++-------
>> 3 files changed, 17 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index d351642..36d1bca 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -406,7 +406,8 @@ int intel_guc_suspend(struct drm_i915_private
>> *dev_priv)
>> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> return 0;
>> - intel_disable_guc_interrupts(guc);
>> + if (guc->log.level >= 0)
>> + intel_disable_guc_interrupts(guc);
>> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>> /* any value greater than GUC_POWER_D0 */
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index d979830..7bc0065 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -484,8 +484,7 @@ static void guc_log_capture_logs(struct intel_guc
>> *guc)
>> static void guc_flush_logs(struct intel_guc *guc)
>> {
>> - if (!USES_GUC_SUBMISSION(dev_priv) ||
>> - guc->log.level < 0)
>> + if (guc->log.level < 0)
>> return;
>> /* First disable the interrupts, will be renabled afterwards */
>> @@ -613,8 +612,7 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>> {
>> - if (!USES_GUC_SUBMISSION(dev_priv) ||
>> - dev_priv->guc.log.level < 0)
>> + if (dev_priv->guc.log.level < 0)
>> return;
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -624,14 +622,13 @@ void i915_guc_log_register(struct
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>> {
>> - if (!USES_GUC_SUBMISSION(dev_priv))
>> - return;
>> -
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> /* GuC logging is currently the only user of Guc2Host interrupts */
>> - intel_runtime_pm_get(dev_priv);
>> - intel_disable_guc_interrupts(&dev_priv->guc);
>> - intel_runtime_pm_put(dev_priv);
>> + if (dev_priv->guc.log.level >= 0) {
>
> Can we move "if (guc->log.level >= 0)" condition check to
> intel_guc_[disable|enable]_interrupts functions? Then we
> should be able to avoid repeating this check over and over
> and it will be also easier for use once we add new CTB check.
>
I will create a new wrapper function for this logging specific check.
Will not move the check to
guc_enable|disable_intr as it will be low level handler to
enable/disable interrupts to be used
by logging, CTB.
>> + intel_runtime_pm_get(dev_priv);
>> + intel_disable_guc_interrupts(&dev_priv->guc);
>> + intel_runtime_pm_put(dev_priv);
>> + }
>> intel_guc_log_runtime_destroy(&dev_priv->guc);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index e2e2020..fb5edcc 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -318,9 +318,12 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> goto err_log_capture;
>> + if (guc->log.level >= 0)
>> + intel_enable_guc_interrupts(guc);
>> +
>> ret = guc_enable_communication(guc);
>> if (ret)
>> - goto err_log_capture;
>> + goto err_interrupts;
>> if (USES_HUC(dev_priv)) {
>> ret = intel_huc_auth(huc);
>> @@ -329,12 +332,9 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> }
>> if (USES_GUC_SUBMISSION(dev_priv)) {
>> - if (guc->log.level >= 0)
>> - intel_enable_guc_interrupts(guc);
>> -
>> ret = intel_guc_submission_enable(guc);
>> if (ret)
>> - goto err_interrupts;
>> + goto err_communication;
>> }
>> dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
>> @@ -349,10 +349,11 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> /*
>> * We've failed to load the firmware :(
>> */
>> -err_interrupts:
>> - intel_disable_guc_interrupts(guc);
>> err_communication:
>> guc_disable_communication(guc);
>> +err_interrupts:
>> + if (guc->log.level >= 0)
>> + intel_disable_guc_interrupts(guc);
>> err_log_capture:
>> guc_capture_load_err_log(guc);
>> err_out:
More information about the Intel-gfx
mailing list