Re: [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 10/11/2017 9:10 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:53:59 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> With guc_log_level parameter sanitized and GuC interrupts control
>> functions made self sufficient w.r.t interrupts state, we can remove
>> the enable_guc_submission checks from flush_guc_logs and
>> i915_guc_log_register/unregister and intel_uc_fini_hw.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_log.c | 9 ++-------
>>  drivers/gpu/drm/i915/intel_uc.c      | 3 +--
>>  2 files changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 0f201c0..fb5eb2b9 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>>  {
>>      struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> -    if (!i915_guc_submission_enabled(guc) ||
>> -        i915_modparams.guc_log_level < 0)
>> +    if (i915_modparams.guc_log_level < 0)
>>          return;
>>     /* First disable the interrupts, will be renabled afterwards */
>> @@ -646,8 +645,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_guc_submission_enabled(&dev_priv->guc) ||
>> -        i915_modparams.guc_log_level < 0)
>> +    if (i915_modparams.guc_log_level < 0)
>
> I would expect above two fixes in patch 2/21
Since there are multiple places needing the update and for better 
division I had created separate patches.
Will keep same for now.
>
>>          return;
>>     mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -657,9 +655,6 @@ void i915_guc_log_register(struct 
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>>  {
>> -    if (!i915_guc_submission_enabled(&dev_priv->guc))
>> -        return;
>
> Hmm, as i915_guc_log_unregister() is called unconditionally by
> i915_driver_unregister so maybe here we should have at least
>
>     if (i915_modparams.guc_log_level < 0)
>         return;
Yes. Will add this.
>
>> -
>>      mutex_lock(&dev_priv->drm.struct_mutex);
>>      /* GuC logging is currently the only user of Guc2Host interrupts */
>>      gen9_disable_guc_interrupts(dev_priv);
>
> What about dropping this gen9_disable_guc_interrupts from here
> and rely on the intel_uc_fini_hw() that will do the same?
>
This needs to be handled separately as if we remove it from here we will 
have to synchronize the logging irq
with relay runtime destruction. Will defer this for now.
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 3cf3cbd..974434e 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *dev_priv)
>>     guc_disable_communication(guc);
>> -    if (i915_guc_submission_enabled(guc))
>> -        gen9_disable_guc_interrupts(dev_priv);
>> +    gen9_disable_guc_interrupts(dev_priv);
>>      i915_guc_submission_fini(dev_priv);
>>     i915_ggtt_disable_guc(dev_priv);
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx