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