[PATCH 0/2] More print message helper updates

[PATCH 0/2] More print message helper updates

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

There was an update a while back to use sub-system specific print
helpers that implicitly add sub-system specific information to the
print. It seems a bunch of GT related messages got missed in that
update. So update them now.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>


John Harrison (2):
  drm/i915/gt: More use of GT specific print helpers
  drm/i915: More use of GT specific print helpers

 drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 29 +++++++++------------
 drivers/gpu/drm/i915/gt/intel_gsc.c         | 11 ++++----
 drivers/gpu/drm/i915/gt/intel_gt_print.h    |  3 +++
 drivers/gpu/drm/i915/gt/intel_reset.c       | 26 ++++++++----------
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 13 ++++-----
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c   |  8 +++---
 drivers/gpu/drm/i915/i915_driver.c          |  3 ++-
 drivers/gpu/drm/i915/i915_perf.c            |  8 +++---
 8 files changed, 48 insertions(+), 53 deletions(-)

-- 
2.41.0

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

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

A bunch of print messages got missed in the update to using sub-system
specific helpers. So update those.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 29 +++++++++------------
 drivers/gpu/drm/i915/gt/intel_gsc.c         | 11 ++++----
 drivers/gpu/drm/i915/gt/intel_gt_print.h    |  3 +++
 drivers/gpu/drm/i915/gt/intel_reset.c       | 26 ++++++++----------
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 13 ++++-----
 5 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index b1a1d07e2e217..179d9546865b0 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -316,10 +316,9 @@ u32 intel_engine_context_size(struct intel_gt *gt, u8 class)
 			 * out in the wash.
 			 */
 			cxt_size = intel_uncore_read(uncore, CXT_SIZE) + 1;
-			drm_dbg(&gt->i915->drm,
-				"graphics_ver = %d CXT_SIZE = %d bytes [0x%08x]\n",
-				GRAPHICS_VER(gt->i915), cxt_size * 64,
-				cxt_size - 1);
+			gt_dbg(gt, "graphics_ver = %d CXT_SIZE = %d bytes [0x%08x]\n",
+			       GRAPHICS_VER(gt->i915), cxt_size * 64,
+			       cxt_size - 1);
 			return round_up(cxt_size * 64, PAGE_SIZE);
 		case 3:
 		case 2:
@@ -788,7 +787,7 @@ static void engine_mask_apply_media_fuses(struct intel_gt *gt)
 
 		if (!(BIT(i) & vdbox_mask)) {
 			gt->info.engine_mask &= ~BIT(_VCS(i));
-			drm_dbg(&i915->drm, "vcs%u fused off\n", i);
+			gt_dbg(gt, "vcs%u fused off\n", i);
 			continue;
 		}
 
@@ -796,8 +795,7 @@ static void engine_mask_apply_media_fuses(struct intel_gt *gt)
 			gt->info.vdbox_sfc_access |= BIT(i);
 		logical_vdbox++;
 	}
-	drm_dbg(&i915->drm, "vdbox enable: %04x, instances: %04lx\n",
-		vdbox_mask, VDBOX_MASK(gt));
+	gt_dbg(gt, "vdbox enable: %04x, instances: %04lx\n", vdbox_mask, VDBOX_MASK(gt));
 	GEM_BUG_ON(vdbox_mask != VDBOX_MASK(gt));
 
 	for (i = 0; i < I915_MAX_VECS; i++) {
@@ -808,11 +806,10 @@ static void engine_mask_apply_media_fuses(struct intel_gt *gt)
 
 		if (!(BIT(i) & vebox_mask)) {
 			gt->info.engine_mask &= ~BIT(_VECS(i));
-			drm_dbg(&i915->drm, "vecs%u fused off\n", i);
+			gt_dbg(gt, "vecs%u fused off\n", i);
 		}
 	}
-	drm_dbg(&i915->drm, "vebox enable: %04x, instances: %04lx\n",
-		vebox_mask, VEBOX_MASK(gt));
+	gt_dbg(gt, "vebox enable: %04x, instances: %04lx\n", vebox_mask, VEBOX_MASK(gt));
 	GEM_BUG_ON(vebox_mask != VEBOX_MASK(gt));
 }
 
@@ -838,7 +835,7 @@ static void engine_mask_apply_compute_fuses(struct intel_gt *gt)
 	 */
 	for_each_clear_bit(i, &ccs_mask, I915_MAX_CCS) {
 		info->engine_mask &= ~BIT(_CCS(i));
-		drm_dbg(&i915->drm, "ccs%u fused off\n", i);
+		gt_dbg(gt, "ccs%u fused off\n", i);
 	}
 }
 
@@ -866,8 +863,8 @@ static void engine_mask_apply_copy_fuses(struct intel_gt *gt)
 						   _BCS(instance));
 
 		if (mask & info->engine_mask) {
-			drm_dbg(&i915->drm, "bcs%u fused off\n", instance);
-			drm_dbg(&i915->drm, "bcs%u fused off\n", instance + 1);
+			gt_dbg(gt, "bcs%u fused off\n", instance);
+			gt_dbg(gt, "bcs%u fused off\n", instance + 1);
 
 			info->engine_mask &= ~mask;
 		}
@@ -907,8 +904,7 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
 	 *    submission, which will wake up the GSC power well.
 	 */
 	if (__HAS_ENGINE(info->engine_mask, GSC0) && !intel_uc_wants_gsc_uc(&gt->uc)) {
-		drm_notice(&gt->i915->drm,
-			   "No GSC FW selected, disabling GSC CS and media C6\n");
+		gt_notice(gt, "No GSC FW selected, disabling GSC CS and media C6\n");
 		info->engine_mask &= ~BIT(GSC0);
 	}
 
@@ -1097,8 +1093,7 @@ static int init_status_page(struct intel_engine_cs *engine)
 	 */
 	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
 	if (IS_ERR(obj)) {
-		drm_err(&engine->i915->drm,
-			"Failed to allocate status page\n");
+		gt_err(engine->gt, "Failed to allocate status page\n");
 		return PTR_ERR(obj);
 	}
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index bcc3605158dbd..6d440de8ba017 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -11,6 +11,7 @@
 #include "gem/i915_gem_region.h"
 #include "gt/intel_gsc.h"
 #include "gt/intel_gt.h"
+#include "gt/intel_gt_print.h"
 
 #define GSC_BAR_LENGTH  0x00000FFC
 
@@ -49,13 +50,13 @@ gsc_ext_om_alloc(struct intel_gsc *gsc, struct intel_gsc_intf *intf, size_t size
 					  I915_BO_ALLOC_CONTIGUOUS |
 					  I915_BO_ALLOC_CPU_CLEAR);
 	if (IS_ERR(obj)) {
-		drm_err(&gt->i915->drm, "Failed to allocate gsc memory\n");
+		gt_err(gt, "Failed to allocate gsc memory\n");
 		return PTR_ERR(obj);
 	}
 
 	err = i915_gem_object_pin_pages_unlocked(obj);
 	if (err) {
-		drm_err(&gt->i915->drm, "Failed to pin pages for gsc memory\n");
+		gt_err(gt, "Failed to pin pages for gsc memory\n");
 		goto out_put;
 	}
 
@@ -286,12 +287,12 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 	int ret;
 
 	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
-		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out of range", intf_id);
+		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
 		return;
 	}
 
 	if (!HAS_HECI_GSC(gt->i915)) {
-		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
+		gt_warn_once(gt, "GSC irq: not supported");
 		return;
 	}
 
@@ -300,7 +301,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 
 	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
 	if (ret)
-		drm_err_ratelimited(&gt->i915->drm, "error handling GSC irq: %d\n", ret);
+		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
 }
 
 void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_print.h b/drivers/gpu/drm/i915/gt/intel_gt_print.h
index 55a336a9ff061..7fdc78c79273e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_print.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_print.h
@@ -16,6 +16,9 @@
 #define gt_warn(_gt, _fmt, ...) \
 	drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
 
+#define gt_warn_once(_gt, _fmt, ...) \
+	drm_warn_once(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
+
 #define gt_notice(_gt, _fmt, ...) \
 	drm_notice(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
 
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index a21e939fdbf6c..d5ed904f355d5 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -26,6 +26,7 @@
 #include "intel_engine_regs.h"
 #include "intel_gt.h"
 #include "intel_gt_pm.h"
+#include "intel_gt_print.h"
 #include "intel_gt_requests.h"
 #include "intel_mchbar_regs.h"
 #include "intel_pci_config.h"
@@ -592,10 +593,10 @@ static int gen8_engine_reset_prepare(struct intel_engine_cs *engine)
 	ret = __intel_wait_for_register_fw(uncore, reg, mask, ack,
 					   700, 0, NULL);
 	if (ret)
-		drm_err(&engine->i915->drm,
-			"%s reset request timed out: {request: %08x, RESET_CTL: %08x}\n",
-			engine->name, request,
-			intel_uncore_read_fw(uncore, reg));
+		gt_err(engine->gt,
+		       "%s reset request timed out: {request: %08x, RESET_CTL: %08x}\n",
+		       engine->name, request,
+		       intel_uncore_read_fw(uncore, reg));
 
 	return ret;
 }
@@ -1199,17 +1200,16 @@ void intel_gt_reset(struct intel_gt *gt,
 		goto unlock;
 
 	if (reason)
-		drm_notice(&gt->i915->drm,
-			   "Resetting chip for %s\n", reason);
+		gt_notice(gt, "Resetting chip for %s\n", reason);
 	atomic_inc(&gt->i915->gpu_error.reset_count);
 
 	awake = reset_prepare(gt);
 
 	if (!intel_has_gpu_reset(gt)) {
 		if (gt->i915->params.reset)
-			drm_err(&gt->i915->drm, "GPU reset not supported\n");
+			gt_err(gt, "GPU reset not supported\n");
 		else
-			drm_dbg(&gt->i915->drm, "GPU reset disabled\n");
+			gt_dbg(gt, "GPU reset disabled\n");
 		goto error;
 	}
 
@@ -1217,7 +1217,7 @@ void intel_gt_reset(struct intel_gt *gt,
 		intel_runtime_pm_disable_interrupts(gt->i915);
 
 	if (do_reset(gt, stalled_mask)) {
-		drm_err(&gt->i915->drm, "Failed to reset chip\n");
+		gt_err(gt, "Failed to reset chip\n");
 		goto taint;
 	}
 
@@ -1236,9 +1236,7 @@ void intel_gt_reset(struct intel_gt *gt,
 	 */
 	ret = intel_gt_init_hw(gt);
 	if (ret) {
-		drm_err(&gt->i915->drm,
-			"Failed to initialise HW following reset (%d)\n",
-			ret);
+		gt_err(gt, "Failed to initialise HW following reset (%d)\n", ret);
 		goto taint;
 	}
 
@@ -1605,9 +1603,7 @@ static void intel_wedge_me(struct work_struct *work)
 {
 	struct intel_wedge_me *w = container_of(work, typeof(*w), work.work);
 
-	drm_err(&w->gt->i915->drm,
-		"%s timed out, cancelling all in-flight rendering.\n",
-		w->name);
+	gt_err(w->gt, "%s timed out, cancelling all in-flight rendering.\n", w->name);
 	intel_gt_set_wedged(w->gt);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index b86a10b1f534f..192ac0e59afa1 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -11,6 +11,7 @@
 #include "intel_gpu_commands.h"
 #include "intel_gt.h"
 #include "intel_gt_mcr.h"
+#include "intel_gt_print.h"
 #include "intel_gt_regs.h"
 #include "intel_ring.h"
 #include "intel_workarounds.h"
@@ -119,8 +120,8 @@ static void wa_init_finish(struct i915_wa_list *wal)
 	if (!wal->count)
 		return;
 
-	drm_dbg(&wal->gt->i915->drm, "Initialized %u %s workarounds on %s\n",
-		wal->wa_count, wal->name, wal->engine_name);
+	gt_dbg(wal->gt, "Initialized %u %s workarounds on %s\n",
+	       wal->wa_count, wal->name, wal->engine_name);
 }
 
 static enum forcewake_domains
@@ -1780,10 +1781,10 @@ wa_verify(struct intel_gt *gt, const struct i915_wa *wa, u32 cur,
 	  const char *name, const char *from)
 {
 	if ((cur ^ wa->set) & wa->read) {
-		drm_err(&gt->i915->drm,
-			"%s workaround lost on %s! (reg[%x]=0x%x, relevant bits were 0x%x vs expected 0x%x)\n",
-			name, from, i915_mmio_reg_offset(wa->reg),
-			cur, cur & wa->read, wa->set & wa->read);
+		gt_err(gt,
+		       "%s workaround lost on %s! (reg[%x]=0x%x, relevant bits were 0x%x vs expected 0x%x)\n",
+		       name, from, i915_mmio_reg_offset(wa->reg),
+		       cur, cur & wa->read, wa->set & wa->read);
 
 		return false;
 	}
-- 
2.41.0

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

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Update a bunch of GT related print messages in non-GT files to use the
GT specific helpers.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 8 +++-----
 drivers/gpu/drm/i915/i915_driver.c        | 3 ++-
 drivers/gpu/drm/i915/i915_perf.c          | 8 ++++----
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 0d3b22a743659..453d855dd1de7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -68,8 +68,7 @@ static void gsc_work(struct work_struct *work)
 				 * A proxy failure right after firmware load means the proxy-init
 				 * step has failed so mark GSC as not usable after this
 				 */
-				drm_err(&gt->i915->drm,
-					"GSC proxy handler failed to init\n");
+				gt_err(gt, "GSC proxy handler failed to init\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
 			}
 			goto out_put;
@@ -83,11 +82,10 @@ static void gsc_work(struct work_struct *work)
 			 * status register to check if the proxy init was actually successful
 			 */
 			if (intel_gsc_uc_fw_proxy_init_done(gsc, false)) {
-				drm_dbg(&gt->i915->drm, "GSC Proxy initialized\n");
+				gt_dbg(gt, "GSC Proxy initialized\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
 			} else {
-				drm_err(&gt->i915->drm,
-					"GSC status reports proxy init not complete\n");
+				gt_err(gt, "GSC status reports proxy init not complete\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
 			}
 		}
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index e5a94b08d5efe..944ab895da72e 100644
--- 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);
 			return ret;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 1347e4ec9dd5a..8f7ab64feec0d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -206,6 +206,7 @@
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_clock_utils.h"
 #include "gt/intel_gt_mcr.h"
+#include "gt/intel_gt_print.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_lrc.h"
 #include "gt/intel_lrc_reg.h"
@@ -1659,9 +1660,8 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
 	free_noa_wait(stream);
 
 	if (perf->spurious_report_rs.missed) {
-		drm_notice(&gt->i915->drm,
-			   "%d spurious OA report notices suppressed due to ratelimiting\n",
-			   perf->spurious_report_rs.missed);
+		gt_notice(gt, "%d spurious OA report notices suppressed due to ratelimiting\n",
+			  perf->spurious_report_rs.missed);
 	}
 }
 
@@ -1852,7 +1852,7 @@ static int alloc_oa_buffer(struct i915_perf_stream *stream)
 	 */
 	ret = i915_vma_pin(vma, 0, SZ_16M, PIN_GLOBAL | PIN_HIGH);
 	if (ret) {
-		drm_err(&gt->i915->drm, "Failed to pin OA buffer %d\n", ret);
+		gt_err(gt, "Failed to pin OA buffer %d\n", ret);
 		goto err_unref;
 	}
 
-- 
2.41.0

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

[Andi Shyti]

Hi John,

...

>  	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
> -		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out of range", intf_id);
> +		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
>  		return;
>  	}
>  
>  	if (!HAS_HECI_GSC(gt->i915)) {
> -		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
> +		gt_warn_once(gt, "GSC irq: not supported");
>  		return;
>  	}
>  
> @@ -300,7 +301,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  
>  	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>  	if (ret)
> -		drm_err_ratelimited(&gt->i915->drm, "error handling GSC irq: %d\n", ret);
> +		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }
>  
>  void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_print.h b/drivers/gpu/drm/i915/gt/intel_gt_print.h
> index 55a336a9ff061..7fdc78c79273e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_print.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_print.h
> @@ -16,6 +16,9 @@
>  #define gt_warn(_gt, _fmt, ...) \
>  	drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>  
> +#define gt_warn_once(_gt, _fmt, ...) \
> +	drm_warn_once(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
> +

I would add the gt_warn_once() part in a different patch.

Andi

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

[Andi Shyti]

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

Andi

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

[John Harrison]

On 10/9/2023 12:50, Andi Shyti wrote:
> Hi John,
>
> ...
>
>>   	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
>> -		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out of range", intf_id);
>> +		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
>>   		return;
>>   	}
>>   
>>   	if (!HAS_HECI_GSC(gt->i915)) {
>> -		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
>> +		gt_warn_once(gt, "GSC irq: not supported");
>>   		return;
>>   	}
>>   
>> @@ -300,7 +301,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>>   
>>   	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>>   	if (ret)
>> -		drm_err_ratelimited(&gt->i915->drm, "error handling GSC irq: %d\n", ret);
>> +		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>>   }
>>   
>>   void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_print.h b/drivers/gpu/drm/i915/gt/intel_gt_print.h
>> index 55a336a9ff061..7fdc78c79273e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_print.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_print.h
>> @@ -16,6 +16,9 @@
>>   #define gt_warn(_gt, _fmt, ...) \
>>   	drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>   
>> +#define gt_warn_once(_gt, _fmt, ...) \
>> +	drm_warn_once(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>> +
> I would add the gt_warn_once() part in a different patch.
But this is the patch that uses it. You should not add dead code. The 
only exception being if it is something large and complex that needs to 
be added in stages for ease of code review. But this really doesn't 
count as large or complex!

John.

>
> Andi

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

[John Harrison]

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

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

[Andi Shyti]

Hi John,

...

> > >   	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
> > > -		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out of range", intf_id);
> > > +		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
> > >   		return;
> > >   	}
> > >   	if (!HAS_HECI_GSC(gt->i915)) {
> > > -		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
> > > +		gt_warn_once(gt, "GSC irq: not supported");
> > >   		return;
> > >   	}
> > > @@ -300,7 +301,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
> > >   	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> > >   	if (ret)
> > > -		drm_err_ratelimited(&gt->i915->drm, "error handling GSC irq: %d\n", ret);
> > > +		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
> > >   }
> > >   void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_print.h b/drivers/gpu/drm/i915/gt/intel_gt_print.h
> > > index 55a336a9ff061..7fdc78c79273e 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_print.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_print.h
> > > @@ -16,6 +16,9 @@
> > >   #define gt_warn(_gt, _fmt, ...) \
> > >   	drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
> > > +#define gt_warn_once(_gt, _fmt, ...) \
> > > +	drm_warn_once(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
> > > +
> > I would add the gt_warn_once() part in a different patch.
> But this is the patch that uses it. You should not add dead code. The only
> exception being if it is something large and complex that needs to be added
> in stages for ease of code review. But this really doesn't count as large or
> complex!

I wouldn't call it dead code if it's used right after... you
could also put all the *_warn_* changes in different patch.

Anyway, I don't have a strong opinion for such a straight forward
patch, so that I'm fine with it as it is:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

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

[John Harrison]

On 10/9/2023 13:02, Andi Shyti wrote:
> Hi John,
>
> ...
>
>>>>    	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
>>>> -		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out of range", intf_id);
>>>> +		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
>>>>    		return;
>>>>    	}
>>>>    	if (!HAS_HECI_GSC(gt->i915)) {
>>>> -		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
>>>> +		gt_warn_once(gt, "GSC irq: not supported");
>>>>    		return;
>>>>    	}
>>>> @@ -300,7 +301,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>>>>    	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>>>>    	if (ret)
>>>> -		drm_err_ratelimited(&gt->i915->drm, "error handling GSC irq: %d\n", ret);
>>>> +		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>>>>    }
>>>>    void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_print.h b/drivers/gpu/drm/i915/gt/intel_gt_print.h
>>>> index 55a336a9ff061..7fdc78c79273e 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_print.h
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_print.h
>>>> @@ -16,6 +16,9 @@
>>>>    #define gt_warn(_gt, _fmt, ...) \
>>>>    	drm_warn(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>>> +#define gt_warn_once(_gt, _fmt, ...) \
>>>> +	drm_warn_once(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)
>>>> +
>>> I would add the gt_warn_once() part in a different patch.
>> But this is the patch that uses it. You should not add dead code. The only
>> exception being if it is something large and complex that needs to be added
>> in stages for ease of code review. But this really doesn't count as large or
>> complex!
> I wouldn't call it dead code if it's used right after... you
> could also put all the *_warn_* changes in different patch.
I did start splitting it into errors vs debugs but then decided it 
wasn't worth the effort ;)

>
> Anyway, I don't have a strong opinion for such a straight forward
> patch, so that I'm fine with it as it is:
>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Thanks :).

>
> Andi

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

[Andi Shyti]

Hi John,

On Mon, Oct 09, 2023 at 12:57:55PM -0700, John Harrison wrote:
> 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.

yes, it's an improvement

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

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

maybe one day we need to revisit all the gt dependency in the
higher levels and the i915 dependencies in gt.

Thanks,
Andi