Re: [Intel-gfx] [PATCH 2/2] drm/i915: More use of GT specific print helpers

[John Harrison <john.c.harrison@intel.com>]

On 10/9/2023 12:54, Andi Shyti wrote:
> Hi John,
>
> ...
>
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -71,6 +71,7 @@
>>   #include "gem/i915_gem_pm.h"
>>   #include "gt/intel_gt.h"
>>   #include "gt/intel_gt_pm.h"
>> +#include "gt/intel_gt_print.h"
>>   #include "gt/intel_rc6.h"
>>   
>>   #include "pxp/intel_pxp.h"
>> @@ -429,7 +430,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>>   	for_each_gt(gt, i915, id) {
>>   		ret = intel_pcode_init(gt->uncore);
>>   		if (ret) {
>> -			drm_err(&gt->i915->drm, "gt%d: intel_pcode_init failed %d\n", id, ret);
>> +			gt_err(gt, "intel_pcode_init failed %d\n", ret);
> using gt_*() print functions in the upper layers looks a bit
> wrong to me. If we need GT printing, the prints need to be done
> inside the function called, in this case would be
> intel_pcode_init().
It is less wrong that using gt->i915->drm as a parameter and 'gt%d' in 
the format string. That is the whole point of the helper. The code has 
access to a gt object so it should use the gt helper to make use of that 
object rather than unrolling it and diving in to the gt internals.

As for moving the error message inside the init function itself. That is 
maybe a valid change but that potentially counts as a functional change 
and should be done by someone who actually knows the code. All I'm doing 
is improving the code layering by using the correct helper to hide the 
internal details of an object this layer should not know about.

John.

>
> Andi