Re: [PATCH v3 2/8] drm/i915/selftests: Use drm_* prints in live selftests

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

Hi Krzysztof,

On 2026-08-28 at 10:03:33 +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>
> ---
>  .../gpu/drm/i915/selftests/i915_gem_evict.c   |  75 +++--
>  			i915_request_put(last);

<snip>

> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index e6bd06774816..0c855f00c3ae 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -25,6 +25,8 @@
>  #include <linux/list_sort.h>
>  #include <linux/prime_numbers.h>
>  
> +#include <drm/drm_print.h>
> +
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_internal.h"
>  #include "gem/i915_gem_lmem.h"
> @@ -34,6 +36,7 @@
>  #include "gt/intel_gpu_commands.h"
>  #include "gt/intel_gtt.h"
>  
> +#include "../i915_drv.h"

Is the ../ really necessary here? It builds for me if i just switch to
#include "i915_drv.h", other includes treat driver/gpu/drm/i915 as the
working dir. I've seen both styles used in the selftests but it just
looks a bit out of place here.

<snip>

> diff --git a/drivers/gpu/drm/i915/selftests/i915_perf.c b/drivers/gpu/drm/i915/selftests/i915_perf.c
> index e9469e27f42a..d2a1a901d47b 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_perf.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_perf.c
> @@ -14,6 +14,8 @@
>  #include "igt_flush_test.h"
>  #include "lib_sw_fence.h"
>  
> +#include <drm/drm_print.h>
> +

Giga nit, but should this be before the "" includes?

<snip>

> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 514d2200751b..1f58024150fa 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> @@ -26,9 +26,12 @@
>  
>  #include "gt/intel_gt.h"
>  
> +#include <drm/drm_print.h>
> +

Should this be before the "" includes?

<not snipping here, same file below>

>  static int intel_fw_table_check(const struct intel_forcewake_range *ranges,
>  				unsigned int num_ranges,
> -				bool is_watertight)
> +				bool is_watertight,
> +				const struct drm_device *drm)
>  {
>  	unsigned int i;
>  	s32 prev;
> @@ -36,22 +39,22 @@ static int intel_fw_table_check(const struct intel_forcewake_range *ranges,
>  	for (i = 0, prev = -1; i < num_ranges; i++, ranges++) {
>  		/* Check that the table is watertight */
>  		if (is_watertight && (prev + 1) != (s32)ranges->start) {
> -			pr_err("%s: entry[%d]:(%x, %x) is not watertight to previous (%x)\n",
> -			       __func__, i, ranges->start, ranges->end, prev);
> +			drm_err(drm, "%s: entry[%d]:(%x, %x) is not watertight to previous (%x)\n",
> +				__func__, i, ranges->start, ranges->end, prev);
>  			return -EINVAL;
>  		}
>  
>  		/* Check that the table never goes backwards */
>  		if (prev >= (s32)ranges->start) {
> -			pr_err("%s: entry[%d]:(%x, %x) is less than the previous (%x)\n",
> -			       __func__, i, ranges->start, ranges->end, prev);
> +			drm_err(drm, "%s: entry[%d]:(%x, %x) is less than the previous (%x)\n",
> +				__func__, i, ranges->start, ranges->end, prev);
>  			return -EINVAL;
>  		}
>  
>  		/* Check that the entry is valid */
>  		if (ranges->start >= ranges->end) {
> -			pr_err("%s: entry[%d]:(%x, %x) has negative length\n",
> -			       __func__, i, ranges->start, ranges->end);
> +			drm_err(drm, "%s: entry[%d]:(%x, %x) has negative length\n",
> +				__func__, i, ranges->start, ranges->end);
>  			return -EINVAL;
>  		}
>  
> @@ -126,7 +129,8 @@ int intel_uncore_mock_selftests(void)
>  	for (i = 0; i < ARRAY_SIZE(fw); i++) {
>  		err = intel_fw_table_check(fw[i].ranges,
>  					   fw[i].num_ranges,
> -					   fw[i].is_watertight);
> +					   fw[i].is_watertight,
> +					   NULL);

I'll join Andi here in that this looks a bit weird. Especially since the
next function in the file intel_shadow_table_check() keeps using
pr_err(). Either convert that too or don't do either.

<snippnig the rest>

Thanks
Krzysztof