Re: [PATCH 05/11] drm/i915/guc: Make GuC log related functions depend only on log level

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@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.

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.

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.

>   		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.

Regards,

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