[v3,09/12] drm/i915/guc: Make GuC log related functions depend only on log level
diff mbox

Message ID 1515082914-4111-10-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com Jan. 4, 2018, 4:21 p.m. UTC
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@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 | 17 +++++++----------
 drivers/gpu/drm/i915/intel_uc.c      | 15 ++++++++-------
 3 files changed, 17 insertions(+), 18 deletions(-)

Comments

Michal Wajdeczko Jan. 4, 2018, 5:15 p.m. UTC | #1
On Thu, 04 Jan 2018 17:21:51 +0100, Sagar Arun Kamble  
<sagar.a.kamble@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@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 | 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.

> +		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:
sagar.a.kamble@intel.com Jan. 5, 2018, 4:58 a.m. UTC | #2
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@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@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 | 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:

Patch
diff mbox

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) {
+		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: