[Intel-xe] [PATCH v2 2/8] drm/xe/irq: Improve warnings and assertions for GT handling

[Lucas De Marchi]

On Wed, Apr 12, 2023 at 03:52:42PM -0700, Matt Roper wrote:
>Improve the assertions and warnings for GT-related interrupt handling to
>help us catch any areas where we're trying to process interrupts in
>relation to the wrong GT.
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_irq.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index 7e78eecba62a..ca86c99f8007 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -220,16 +220,22 @@ gt_engine_identity(struct xe_device *xe,
> static void
> gt_other_irq_handler(struct xe_gt *gt, const u8 instance, const u16 iir)
> {
>-	if (instance == OTHER_GUC_INSTANCE && !xe_gt_is_media_type(gt))
>-		return xe_guc_irq_handler(&gt->uc.guc, iir);
>-	if (instance == OTHER_MEDIA_GUC_INSTANCE && xe_gt_is_media_type(gt))
>-		return xe_guc_irq_handler(&gt->uc.guc, iir);
>-
>-	if (instance != OTHER_GUC_INSTANCE &&
>-	    instance != OTHER_MEDIA_GUC_INSTANCE) {
>-		WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
>-			  instance, iir);
>+	struct xe_device *xe = gt_to_xe(gt);
>+
>+	if (instance == OTHER_GUC_INSTANCE) {
>+		drm_WARN_ON_ONCE(&xe->drm, xe_gt_is_media_type(gt));
>+		xe_guc_irq_handler(&gt->uc.guc, iir);
>+		return;
>+	}
>+
>+	if (instance == OTHER_MEDIA_GUC_INSTANCE) {
>+		drm_WARN_ON_ONCE(&xe->drm, !xe_gt_is_media_type(gt));
>+		xe_guc_irq_handler(&gt->uc.guc, iir);
>+		return;
> 	}
>+
>+	drm_warn_once(&xe->drm, "Unhandled 'other' interrupt on GT%d: instance=0x%x, iir=0x%x\n",
>+		      gt->info.id, instance, iir);


do we need 3 warnings in the irq path? Since we are calling the same
function we could probably reduce it to:

	static void
	gt_other_irq_handler(struct xe_gt *gt, const u8 instance, const u16 iir)
	{
		if (likely((instance == OTHER_GUC_INSTANCE && !xe_gt_is_media_type(gt)) ||
			   (instance == OTHER_MEDIA_GUC_INSTANCE && xe_gt_is_media_type(gt)))
			return xe_guc_irq_handler(&gt->uc.guc, iir);

		drm_warn_once(&xe->drm, "Unhandled 'other' interrupt on GT%d: type=0x%x, instance=0x%x, iir=0x%x\n",
			      gt->info.id, gt->info.type, instance, iir);
	}

the info on the other 2 warnings can be derived from this single one
above and it's not a warning that we want to see really.

likely() is generally frowned upon in i915/xe nowadays, but on the irq
path and situations like this I think it also helps documenting that the
warning is really a rare exception.

Lucas De Marchi

> }
>
> static struct xe_gt *find_media_gt(struct xe_device *xe)
>@@ -307,8 +313,11 @@ static void gt_irq_handler(struct xe_device *xe, struct xe_gt *full_gt,
> 			}
>
> 			hwe = xe_gt_hw_engine(gt, class, instance, false);
>-			if (!hwe)
>+			if (!hwe) {
>+				drm_warn_once(&xe->drm, "Interrupt for unknown engine on GT%d: class=%d instance=%d\n",
>+					      gt->info.id, class, instance);
> 				continue;
>+			}
>
> 			xe_hw_engine_handle_irq(hwe, intr_vec);
> 		}
>-- 
>2.39.2
>