diff mbox series

[v2,6/6] drm/i915/uc: do not free err log on uc_fini

Message ID 20200312011631.15262-7-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series Re-org uC debugfs files and move them under GT | expand

Commit Message

Daniele Ceraolo Spurio March 12, 2020, 1:16 a.m. UTC 
we do call uc_fini if there is an issue while loading the GuC, so we
can't delete in there the logs we need to debug the load failure.
Moving the log free to driver remove ensures the logs stick around ong
enough for us to dump them.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c    | 3 +--
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++--
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

John Harrison March 25, 2020, 5:58 p.m. UTC | #1 
On 3/11/2020 18:16, Daniele Ceraolo Spurio wrote:
> we do call uc_fini if there is an issue while loading the GuC, so we
> can't delete in there the logs we need to debug the load failure.
> Moving the log free to driver remove ensures the logs stick around ong
> enough for us to dump them.
I think this could be worded better and has a couple of typos.

Otherwise it looks plausible.
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c    | 3 +--
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++--
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 +
>   3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 3dea8881e915..eda66b0d44bd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -635,8 +635,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
>   {
>   	__intel_gt_disable(gt);
>   
> -	intel_uc_fini_hw(&gt->uc);
> -	intel_uc_fini(&gt->uc);
> +	intel_uc_driver_remove(&gt->uc);
>   
>   	intel_engines_release(gt);
>   }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index a4cbe06e06bd..b11e564ef22e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -131,6 +131,13 @@ static void __uc_free_load_err_log(struct intel_uc *uc)
>   		i915_gem_object_put(log);
>   }
>   
> +void intel_uc_driver_remove(struct intel_uc *uc)
> +{
> +	intel_uc_fini_hw(uc);
> +	intel_uc_fini(uc);
> +	__uc_free_load_err_log(uc);
> +}
> +
>   static inline bool guc_communication_enabled(struct intel_guc *guc)
>   {
>   	return intel_guc_ct_enabled(&guc->ct);
> @@ -311,8 +318,6 @@ static void __uc_fini(struct intel_uc *uc)
>   {
>   	intel_huc_fini(&uc->huc);
>   	intel_guc_fini(&uc->guc);
> -
> -	__uc_free_load_err_log(uc);
>   }
>   
>   static int __uc_sanitize(struct intel_uc *uc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 5ae7b50b7dc1..9c954c589edf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -34,6 +34,7 @@ struct intel_uc {
>   
>   void intel_uc_init_early(struct intel_uc *uc);
>   void intel_uc_driver_late_release(struct intel_uc *uc);
> +void intel_uc_driver_remove(struct intel_uc *uc);
>   void intel_uc_init_mmio(struct intel_uc *uc);
>   void intel_uc_reset_prepare(struct intel_uc *uc);
>   void intel_uc_suspend(struct intel_uc *uc);
Daniele Ceraolo Spurio March 25, 2020, 6:03 p.m. UTC | #2 
On 3/25/20 10:58 AM, John Harrison wrote:
> On 3/11/2020 18:16, Daniele Ceraolo Spurio wrote:
>> we do call uc_fini if there is an issue while loading the GuC, so we
>> can't delete in there the logs we need to debug the load failure.
>> Moving the log free to driver remove ensures the logs stick around ong
>> enough for us to dump them.
> I think this could be worded better and has a couple of typos.
> 

How about:

"We need to keep the GuC error logs around to debug the load failure, so 
we can't clean them in the error unwind, which includes uc_fini(). 
Moving the cleanup to driver remove ensures that the logs stick around 
long enough for us to dump them."

?

Daniele

> Otherwise it looks plausible.
> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> 
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_gt.c    | 3 +--
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++--
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 +
>>   3 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index 3dea8881e915..eda66b0d44bd 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -635,8 +635,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
>>   {
>>       __intel_gt_disable(gt);
>> -    intel_uc_fini_hw(&gt->uc);
>> -    intel_uc_fini(&gt->uc);
>> +    intel_uc_driver_remove(&gt->uc);
>>       intel_engines_release(gt);
>>   }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index a4cbe06e06bd..b11e564ef22e 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -131,6 +131,13 @@ static void __uc_free_load_err_log(struct 
>> intel_uc *uc)
>>           i915_gem_object_put(log);
>>   }
>> +void intel_uc_driver_remove(struct intel_uc *uc)
>> +{
>> +    intel_uc_fini_hw(uc);
>> +    intel_uc_fini(uc);
>> +    __uc_free_load_err_log(uc);
>> +}
>> +
>>   static inline bool guc_communication_enabled(struct intel_guc *guc)
>>   {
>>       return intel_guc_ct_enabled(&guc->ct);
>> @@ -311,8 +318,6 @@ static void __uc_fini(struct intel_uc *uc)
>>   {
>>       intel_huc_fini(&uc->huc);
>>       intel_guc_fini(&uc->guc);
>> -
>> -    __uc_free_load_err_log(uc);
>>   }
>>   static int __uc_sanitize(struct intel_uc *uc)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 5ae7b50b7dc1..9c954c589edf 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -34,6 +34,7 @@ struct intel_uc {
>>   void intel_uc_init_early(struct intel_uc *uc);
>>   void intel_uc_driver_late_release(struct intel_uc *uc);
>> +void intel_uc_driver_remove(struct intel_uc *uc);
>>   void intel_uc_init_mmio(struct intel_uc *uc);
>>   void intel_uc_reset_prepare(struct intel_uc *uc);
>>   void intel_uc_suspend(struct intel_uc *uc);
>
John Harrison March 25, 2020, 6:05 p.m. UTC | #3 
On 3/25/2020 11:03, Daniele Ceraolo Spurio wrote:
> On 3/25/20 10:58 AM, John Harrison wrote:
>> On 3/11/2020 18:16, Daniele Ceraolo Spurio wrote:
>>> we do call uc_fini if there is an issue while loading the GuC, so we
>>> can't delete in there the logs we need to debug the load failure.
>>> Moving the log free to driver remove ensures the logs stick around ong
>>> enough for us to dump them.
>> I think this could be worded better and has a couple of typos.
>>
>
> How about:
>
> "We need to keep the GuC error logs around to debug the load failure, 
> so we can't clean them in the error unwind, which includes uc_fini(). 
> Moving the cleanup to driver remove ensures that the logs stick around 
> long enough for us to dump them."
>
> ?
>
> Daniele
>
Sounds good to me :).

Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 3dea8881e915..eda66b0d44bd 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -635,8 +635,7 @@  void intel_gt_driver_remove(struct intel_gt *gt)
 {
 	__intel_gt_disable(gt);
 
-	intel_uc_fini_hw(&gt->uc);
-	intel_uc_fini(&gt->uc);
+	intel_uc_driver_remove(&gt->uc);
 
 	intel_engines_release(gt);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index a4cbe06e06bd..b11e564ef22e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -131,6 +131,13 @@  static void __uc_free_load_err_log(struct intel_uc *uc)
 		i915_gem_object_put(log);
 }
 
+void intel_uc_driver_remove(struct intel_uc *uc)
+{
+	intel_uc_fini_hw(uc);
+	intel_uc_fini(uc);
+	__uc_free_load_err_log(uc);
+}
+
 static inline bool guc_communication_enabled(struct intel_guc *guc)
 {
 	return intel_guc_ct_enabled(&guc->ct);
@@ -311,8 +318,6 @@  static void __uc_fini(struct intel_uc *uc)
 {
 	intel_huc_fini(&uc->huc);
 	intel_guc_fini(&uc->guc);
-
-	__uc_free_load_err_log(uc);
 }
 
 static int __uc_sanitize(struct intel_uc *uc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 5ae7b50b7dc1..9c954c589edf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -34,6 +34,7 @@  struct intel_uc {
 
 void intel_uc_init_early(struct intel_uc *uc);
 void intel_uc_driver_late_release(struct intel_uc *uc);
+void intel_uc_driver_remove(struct intel_uc *uc);
 void intel_uc_init_mmio(struct intel_uc *uc);
 void intel_uc_reset_prepare(struct intel_uc *uc);
 void intel_uc_suspend(struct intel_uc *uc);