[igt-dev] [CI 1/2] tests/i915/query: sanity check reported GTT alignment

[Matthew Auld]

Hi,

On 19/10/2022 14:33, Kamil Konieczny wrote:
> Hi Matthew,
> 
> On 2022-10-17 at 10:01:10 +0100, Matthew Auld wrote:
>> Ensure the kernel is reporting "normal" values here, based on our
>> current expectations.
>>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
>> Cc: Nirmoy Das <nirmoy.das at intel.com>
>> Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>
> - ^
> imho you should remove this when sending patch.

Hmm, what should be removed here? Do you mean the r-b tag?

> Please also use
> [PATCH i-g-t] or [PATCH i-g-t vN] in Subject: line (with
> N=version > 1) instead of [CI 1/2]. It would also help if you
> could send series bigger than one patch with cover letter
> (well, now it can stay as is).

The patch should be unchanged, I'm just re-sending with [CI] and 
--suppress-cc=all since I just want to get some CI results before 
merging (i.e not requesting review from the mailing list), now that the 
corresponding kernel patches are merged. The patch has already been 
reviewed.

> 
>> ---
>>   tests/i915/i915_query.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tests/i915/i915_query.c b/tests/i915/i915_query.c
>> index 4e43c7cd..bff073d2 100644
>> --- a/tests/i915/i915_query.c
>> +++ b/tests/i915/i915_query.c
>> @@ -638,6 +638,8 @@ static void upload(int fd, struct igt_list_head *handles, uint32_t num_handles)
>>   	free(exec);
>>   }
>>   
>> +/** XXX: remove once we sync the uapi header */
>> +#define gtt_alignment rsvd0
>>   static void test_query_regions_sanity_check(int fd)
>>   {
>>   	struct drm_i915_query_memory_regions *regions;
>> @@ -664,6 +666,11 @@ static void test_query_regions_sanity_check(int fd)
>>   		struct drm_i915_gem_memory_class_instance r1 = info.region;
>>   		int j;
>>   
>> +		if (info.gtt_alignment) {
>> +			igt_assert_lte_u64(4096, info.gtt_alignment);
>> +			igt_assert(is_power_of_two(info.gtt_alignment));
>> +		}
>> +
>>   		if (r1.memory_class == I915_MEMORY_CLASS_SYSTEM) {
>>   			igt_assert_eq(r1.memory_instance, 0);
>>   			found_system = true;
>> @@ -674,6 +681,9 @@ static void test_query_regions_sanity_check(int fd)
>>   			igt_assert(info.unallocated_cpu_visible_size == 0 ||
>>   				   info.unallocated_cpu_visible_size ==
>>   				   info.unallocated_size);
>> +
>> +			igt_assert(info.gtt_alignment == 0 ||
>> +				   info.gtt_alignment == 4096);
> -------------------------------------------------------- ^
> Why not copy your check from above ? This is restricting to only
> 4KB or 0 and can break for bigger values.

We want it to break for bigger values. This check is for system memory, 
and likely means we have a silly bug since we should not expect anything 
other than 4K (or zero if the kernel doesn't support the field).

> 
> Regards,
> Kamil
> 
>>   		} else {
>>   			igt_assert(info.probed_cpu_visible_size <= info.probed_size);
>>   			igt_assert(info.unallocated_size <= info.probed_size);
>> -- 
>> 2.37.3
>>