[Intel-gfx] [PATCH 05/11] drm/i915/guc: Make GuC log related functions depend only on log level
Sagar Arun Kamble
sagar.a.kamble at intel.com
Wed Oct 18 15:57:58 UTC 2017
On 10/18/2017 6:37 PM, Tvrtko Ursulin wrote:
>
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
>> With guc_log_level parameter sanitized we can remove the GuC submission
>> checks from flush_guc_logs and i915_guc_log_register/unregister and
>> intel_uc_fini_hw. It is important to note that GuC log runtime/relay
>> channel 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 and driver
>> will try to destroy during cleanup/fini_hw where relay will be NULL
>> already.
>>
>> With this patch GuC interrupts are enabled first after GuC load if
>> logging
>> is enabled. Earlier they were enabled only when submission was getting
>> 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. 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: Updated guc_log_unregister again. Made all GuC log related functions
>> depend only guc_log_level. Updated uC init w.r.t enabling of GuC
>> interrupts.
>>
>> 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 | 12 ++++--------
>> drivers/gpu/drm/i915/intel_uc.c | 21 ++++++++++++---------
>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 31f25e5..959057a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -274,7 +274,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 (i915_modparams.guc_log_level >= 0)
>> + intel_disable_guc_interrupts(guc);
>
> I don't like this as much since we have a general direction of having
> less usage of modparams sprinkled around. Ideally we would take a
> snapshot after sanitizing so it remains immutable for the lifetime of
> the driver.
Yes. Will update.
>
> For this particular case it would be nice I think if in the guc object
> you would store something like bool guc->interrupts_needed.
>
> Whether or not then to act on that from inside
> intel_(en|dis)ble_guc_interrupts would be open for discussion.
ok
>
> Your thoughts?
>
>> ctx = dev_priv->kernel_context;
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 200f0a1..f87e9f5 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -504,8 +504,7 @@ static void guc_log_capture_logs(struct intel_guc
>> *guc)
>> static void guc_flush_logs(struct intel_guc *guc)
>> {
>> - if (!i915_modparams.enable_guc_submission ||
>> - (i915_modparams.guc_log_level < 0))
>> + if (i915_modparams.guc_log_level < 0)
>
> This would then become guc->log_level.
yes
>
>> return;
>> /* First disable the interrupts, will be renabled afterwards */
>> @@ -645,8 +644,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 (!i915_modparams.enable_guc_submission ||
>> - (i915_modparams.guc_log_level < 0))
>> + if (i915_modparams.guc_log_level < 0)
>> return;
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -656,12 +654,10 @@ void i915_guc_log_register(struct
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>> {
>> - if (!i915_modparams.enable_guc_submission)
>> - return;
>> -
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> /* GuC logging is currently the only user of Guc2Host
>> interrupts */
>> - intel_disable_guc_interrupts(&dev_priv->guc);
>> + if (i915_modparams.guc_log_level >= 0)
>> + intel_disable_guc_interrupts(&dev_priv->guc);
>> 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 8feefcd..95c5ec4 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -212,18 +212,18 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> goto err_log_capture;
>> + if (i915_modparams.guc_log_level >= 0)
>> + intel_enable_guc_interrupts(guc);
>> +
>> ret = guc_enable_communication(guc);
>> if (ret)
>> - goto err_log_capture;
>> + goto err_interrupts;
>> intel_huc_auth(&dev_priv->huc);
>> if (i915_modparams.enable_guc_submission) {
>> - if (i915_modparams.guc_log_level >= 0)
>> - intel_enable_guc_interrupts(guc);
>> -
>> ret = i915_guc_submission_enable(dev_priv);
>> if (ret)
>> - goto err_interrupts;
>> + goto err_comm;
>> }
>> dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version
>> %u.%u])\n",
>> @@ -243,9 +243,11 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> * nonfatal error (i.e. it doesn't prevent driver load, but
>> * marks the GPU as wedged until reset).
>> */
>> -err_interrupts:
>> +err_comm:
>> guc_disable_communication(guc);
>> - intel_disable_guc_interrupts(guc);
>> +err_interrupts:
>> + if (i915_modparams.guc_log_level >= 0)
>> + intel_disable_guc_interrupts(guc);
>> err_log_capture:
>> guc_capture_load_err_log(guc);
>> err_submission:
>> @@ -285,10 +287,11 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> guc_disable_communication(&dev_priv->guc);
>> - if (i915_modparams.enable_guc_submission) {
>> + if (i915_modparams.guc_log_level >= 0)
>> intel_disable_guc_interrupts(&dev_priv->guc);
>> +
>> + if (i915_modparams.enable_guc_submission)
>> i915_guc_submission_fini(dev_priv);
>> - }
>> i915_ggtt_disable_guc(dev_priv);
>> }
>>
>
> Open for discussion I think, but it feels even visually like to much
> modparams.
yes. will update.
Thanks.
>
> Regards,
>
> Tvrtko
More information about the Intel-gfx
mailing list