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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 28 Aug 2026, Andi Shyti <andi.shyti@kernel.org> wrote:
> Hi Krzysztof,
>
> ...
>
>>  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)

First, we generally don't pass around struct drm_device *, it's struct
drm_i915_private * in i915.

Second, the highest level context parameters should be first. This one
looks like an afterthought.

>>  {
>>  	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);
>
> arrghh! Here we are again at the drm_err(NULL,...) case.
>
> You could eventually do in intel_fw_table_check()
>
> 	if (drm)
> 		drm_err(drm, ...);
> 	else
> 		pr_err(...);

drm_err() handles NULL drm_device just fine.

>
> but I think it's not worth the effort, just leave pr_err().
> Besides, passing the drm device is quite out of the scope of the
> function itself, it's just odd!

But agreed.

BR,
Jani.

>
> Andi
>
>>  		if (err)
>>  			return err;
>>  	}

-- 
Jani Nikula, Intel