Re: [PATCH v3 4/8] drm/i915/gt/selftests: Use drm_* prints in aggregated gt selftests

[Krzysztof Niemiec <krzysztof.niemiec@intel.com>]

Hi Krzysztof,

On 2026-08-28 at 10:03:35 +0000, Krzysztof Karas wrote:
> Move away from generic pr prints to device context aware drm
> versions.
> 
> Assisted-by: GitHub-Copilot:claude-opus-5
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/selftest_context.c    |  33 +-
>  drivers/gpu/drm/i915/gt/selftest_engine_cs.c  |   8 +-
>  drivers/gpu/drm/i915/gt/selftest_engine_pm.c  |  58 +--
>  drivers/gpu/drm/i915/gt/selftest_execlists.c  | 353 +++++++++++-------
>  drivers/gpu/drm/i915/gt/selftest_gt_pm.c      |  32 +-
>  drivers/gpu/drm/i915/gt/selftest_migrate.c    |  65 ++--
>  drivers/gpu/drm/i915/gt/selftest_mocs.c       |  12 +-
>  drivers/gpu/drm/i915/gt/selftest_reset.c      |  28 +-
>  drivers/gpu/drm/i915/gt/selftest_slpc.c       | 103 +++--
>  drivers/gpu/drm/i915/gt/selftest_tlb.c        |  39 +-
>  .../gpu/drm/i915/gt/selftest_workarounds.c    | 104 ++++--
>  11 files changed, 513 insertions(+), 322 deletions(-)
> 

<snip>

> diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> index 21e5ed9f72a3..97dc44cf615f 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> @@ -72,6 +72,8 @@ static int wait_for_reset(struct intel_engine_cs *engine,
>  			  struct i915_request *rq,
>  			  unsigned long timeout)
>  {
> +	struct drm_i915_private *i915 = engine->i915;
> +
>  	timeout += jiffies;
>  
>  	do {
> @@ -89,20 +91,18 @@ static int wait_for_reset(struct intel_engine_cs *engine,
>  	} while (time_before(jiffies, timeout));
>  
>  	if (rq->fence.error != -EIO) {
> -		pr_err("%s: hanging request %llx:%lld not reset\n",
> -		       engine->name,
> -		       rq->fence.context,
> -		       rq->fence.seqno);
> +		drm_err(&i915->drm,
> +			"%s: hanging request %llx:%lld not reset\n",
> +			engine->name, rq->fence.context, rq->fence.seqno);
>  		return -EINVAL;
>  	}
>  
>  	/* Give the request a jiffy to complete after flushing the worker */
>  	if (i915_request_wait(rq, 0,
>  			      max(0l, (long)(timeout - jiffies)) + 1) < 0) {
> -		pr_err("%s: hanging request %llx:%lld did not complete\n",
> -		       engine->name,
> -		       rq->fence.context,
> -		       rq->fence.seqno);
> +		drm_err(&i915->drm,
> +			"%s: hanging request %llx:%lld did not complete\n",
> +			engine->name, rq->fence.context, rq->fence.seqno);
>  		return -ETIME;
>  	}
>  
> @@ -328,6 +328,7 @@ static int live_unlite_preempt(void *arg)
>  static int live_unlite_ring(void *arg)
>  {
>  	struct intel_gt *gt = arg;
> +	struct drm_i915_private *i915 = gt->i915;
>  	struct intel_engine_cs *engine;
>  	struct igt_spinner spin;
>  	enum intel_engine_id id;
> @@ -419,12 +420,10 @@ static int live_unlite_ring(void *arg)
>  			n++;
>  		}
>  		intel_engine_flush_submission(engine);
> -		pr_debug("%s: Filled ring with %d nop tails {size:%x, tail:%x, emit:%x, rq.tail:%x}\n",
> -			 engine->name, n,
> -			 ce[0]->ring->size,
> -			 ce[0]->ring->tail,
> -			 ce[0]->ring->emit,
> -			 rq->tail);
> +		drm_dbg(&i915->drm,
> +			"%s: Filled ring with %d nop tails {size:%x, tail:%x, emit:%x, rq.tail:%x}\n",
> +			engine->name, n, ce[0]->ring->size, ce[0]->ring->tail,
> +			ce[0]->ring->emit, rq->tail);
>  		GEM_BUG_ON(intel_ring_direction(ce[0]->ring,
>  						rq->tail,
>  						ce[0]->ring->tail) <= 0);

Here...

> @@ -444,15 +443,16 @@ static int live_unlite_ring(void *arg)
>  		err = wait_for_submit(engine, rq, HZ / 2);
>  		i915_request_put(rq);
>  		if (err) {
> -			pr_err("%s: preemption request was not submitted\n",
> -			       engine->name);
> +			drm_err(&i915->drm,
> +				"%s: preemption request was not submitted\n",
> +				engine->name);
>  			err = -ETIME;
>  		}
>  
> -		pr_debug("%s: ring[0]:{ tail:%x, emit:%x }, ring[1]:{ tail:%x, emit:%x }\n",
> -			 engine->name,
> -			 ce[0]->ring->tail, ce[0]->ring->emit,
> -			 ce[1]->ring->tail, ce[1]->ring->emit);
> +		drm_dbg(&i915->drm,
> +			"%s: ring[0]:{ tail:%x, emit:%x }, ring[1]:{ tail:%x, emit:%x }\n",
> +			engine->name, ce[0]->ring->tail, ce[0]->ring->emit,
> +			ce[1]->ring->tail, ce[1]->ring->emit);
>  

here...
<snip within same file>

>  
> @@ -2800,6 +2854,7 @@ static int __live_preempt_ring(struct intel_engine_cs *engine,
>  			       struct igt_spinner *spin,
>  			       int queue_sz, int ring_sz)
>  {
> +	struct drm_i915_private *i915 = engine->i915;
>  	struct intel_context *ce[2] = {};
>  	struct i915_request *rq;
>  	struct igt_live_test t;
> @@ -2867,12 +2922,10 @@ static int __live_preempt_ring(struct intel_engine_cs *engine,
>  		n++;
>  	}
>  	intel_engine_flush_submission(engine);
> -	pr_debug("%s: Filled %d with %d nop tails {size:%x, tail:%x, emit:%x, rq.tail:%x}\n",
> -		 engine->name, queue_sz, n,
> -		 ce[0]->ring->size,
> -		 ce[0]->ring->tail,
> -		 ce[0]->ring->emit,
> -		 rq->tail);
> +	drm_dbg(&i915->drm,
> +		"%s: Filled %d with %d nop tails {size:%x, tail:%x, emit:%x, rq.tail:%x}\n",
> +		engine->name, queue_sz, n, ce[0]->ring->size, ce[0]->ring->tail,
> +		ce[0]->ring->emit, rq->tail);

and here..

>  	i915_request_put(rq);
>  
>  	/* Create a second request to preempt the first ring */
> @@ -2889,15 +2942,16 @@ static int __live_preempt_ring(struct intel_engine_cs *engine,
>  	err = wait_for_submit(engine, rq, HZ / 2);
>  	i915_request_put(rq);
>  	if (err) {
> -		pr_err("%s: preemption request was not submitted\n",
> -		       engine->name);
> +		drm_err(&i915->drm,
> +			"%s: preemption request was not submitted\n",
> +			engine->name);
>  		err = -ETIME;
>  	}
>  
> -	pr_debug("%s: ring[0]:{ tail:%x, emit:%x }, ring[1]:{ tail:%x, emit:%x }\n",
> -		 engine->name,
> -		 ce[0]->ring->tail, ce[0]->ring->emit,
> -		 ce[1]->ring->tail, ce[1]->ring->emit);
> +	drm_dbg(&i915->drm,
> +		"%s: ring[0]:{ tail:%x, emit:%x }, ring[1]:{ tail:%x, emit:%x }\n",
> +		engine->name, ce[0]->ring->tail, ce[0]->ring->emit,
> +		ce[1]->ring->tail, ce[1]->ring->emit);
>  

and here.

Maybe it's worth keeping the arguments in these snippets stacked like
they were stacked before for readability? You greedily rearrange arguments
all over your series, which I don't think matters that much, but here it
is particularly easy on the eyes before your patch, and kind of lost that
quality. Giga nit though so don't address this if you don't feel like it


> diff --git a/drivers/gpu/drm/i915/gt/selftest_mocs.c b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> index d73e438fb85f..770516c6c7da 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> @@ -13,6 +13,8 @@
>  #include "selftests/igt_spinner.h"
>  #include "selftests/intel_scheduler_helpers.h"
>  
> +#include <drm/drm_print.h>
> +

Same nit with drm includes as before

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 2cfc23c58e90..938218e0d5bc 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -14,11 +14,14 @@
>  #include "selftests/igt_atomic.h"
>  #include "selftests/igt_spinner.h"
>  
> +#include <drm/drm_print.h>
> +

Include nit again

> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> index c3c918248989..531dbc08c8c3 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> @@ -3,6 +3,10 @@
>   * Copyright © 2021 Intel Corporation
>   */
>  
> +#include <drm/drm_print.h>
> +
> +#include "uc/intel_guc.h"
> +
>  #define NUM_STEPS 5
>  #define H2G_DELAY 50000
>  #define delay_for_h2g() usleep_range(H2G_DELAY, H2G_DELAY + 10000)
> @@ -23,13 +27,22 @@ struct slpc_thread {
>  	int result;
>  };
>  
> +static struct intel_gt *selftest_slpc_to_gt(struct intel_guc_slpc *slpc)
> +{
> +	struct intel_guc *guc = container_of(slpc, struct intel_guc, slpc);
> +
> +	return guc_to_gt(guc);
> +}
> +
>  static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)
>  {
> +	struct intel_gt *gt = selftest_slpc_to_gt(slpc);
>  	int ret;
>  
>  	ret = intel_guc_slpc_set_min_freq(slpc, freq);
>  	if (ret)
> -		pr_err("Could not set min frequency to [%u]\n", freq);
> +		drm_err(&gt->i915->drm, "Could not set min frequency to [%u]\n",
> +			freq);
>  	else /* Delay to ensure h2g completes */
>  		delay_for_h2g();
>  
> @@ -38,12 +51,13 @@ static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)
>  
>  static int slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 freq)
>  {
> +	struct intel_gt *gt = selftest_slpc_to_gt(slpc);
>  	int ret;
>  
>  	ret = intel_guc_slpc_set_max_freq(slpc, freq);
>  	if (ret)
> -		pr_err("Could not set maximum frequency [%u]\n",
> -		       freq);
> +		drm_err(&gt->i915->drm,
> +			"Could not set maximum frequency [%u]\n", freq);

Here you could use just directly

	gt_err(gt, "Could not set...

Since gt_err is just [1]

#define gt_err(_gt, _fmt, ...) \
	drm_err(&(_gt)->i915->drm, "GT%u: " _fmt, (_gt)->info.id, ##__VA_ARGS__)

and it seems like there's a decently accessible reference to gt all over
this file, so I'd consider converting the drm_prints to gt_prints all
over too.

[1] https://elixir.bootlin.com/linux/v7.2/source/drivers/gpu/drm/i915/gt/intel_gt_print.h#L13-L14

>  	else /* Delay to ensure h2g completes */
>  		delay_for_h2g();
>  
> @@ -52,18 +66,19 @@ static int slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 freq)
>  
>  static int slpc_set_freq(struct intel_gt *gt, u32 freq)
>  {
> +	struct drm_i915_private *i915 = gt->i915;
>  	int err;
>  	struct intel_guc_slpc *slpc = &gt_to_guc(gt)->slpc;
>  
>  	err = slpc_set_max_freq(slpc, freq);
>  	if (err) {
> -		pr_err("Unable to update max freq");
> +		drm_err(&i915->drm, "Unable to update max freq");

Like here for example, you could get rid of the i915 struct entirely and
just do 'gt_err(gt, "Unable to...', since at this point you're rewriting
the macro by hand anyway.

The gt_prints give even more context for the error (in the form of the GT
id) compared to drm_prints, so I think they are be preferable for that
reason in places where we can access the GT ref. So I'd look for other
places where you could use gt prints too.

>  		return err;
>  	}
>  
>  	err = slpc_set_min_freq(slpc, freq);
>  	if (err) {
> -		pr_err("Unable to update min freq");
> +		drm_err(&i915->drm, "Unable to update min freq");
>  		return err;
>  	}
>  

> diff --git a/drivers/gpu/drm/i915/gt/selftest_tlb.c b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> index a5184f09d1de..89420f07f681 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_tlb.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> @@ -20,6 +20,8 @@
>  #include "selftests/igt_flush_test.h"
>  #include "selftests/i915_random.h"
>  
> +#include <drm/drm_print.h>
> +

Nit for include

> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 4e4c420ae67f..8b74d62e7ca5 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -16,6 +16,8 @@
>  #include "selftests/intel_scheduler_helpers.h"
>  #include "selftests/mock_drm.h"
>  
> +#include <drm/drm_print.h>
> +

Nit for include

>  #include "gem/selftests/igt_gem_utils.h"
>  #include "gem/selftests/mock_context.h"
>  

Thanks
Krzysztof