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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux