Re: [PATCH 10/13] drm/xe/tests: Add object encoding helper test

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 7/3/2026 10:47 PM, Michał Winiarski wrote:
> On Tue, Jun 30, 2026 at 03:26:29PM +0200, Michal Wajdeczko wrote:
>> We will soon be encoding complex objects as KLVs using our helper
>> function. Add few simple tests to make sure this helper function
>> works as expected.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  .../drm/xe/tests/xe_guc_klv_helpers_kunit.c   | 162 ++++++++++++++++++
>>  1 file changed, 162 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_klv_helpers_kunit.c b/drivers/gpu/drm/xe/tests/xe_guc_klv_helpers_kunit.c
>> index c2d5f0098816..9a8acc85e9f1 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_guc_klv_helpers_kunit.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_guc_klv_helpers_kunit.c
>> @@ -7,6 +7,7 @@
>>  #include <kunit/test-bug.h>
>>  
>>  #define TEST_KEY	(GUC_KLV_RESERVED_RANGE_START + 0x3de)
>> +#define TEST_GROUP_KEY	(GUC_KLV_RESERVED_RANGE_START + 0x3f0)
>>  #define TEST_PAD	0xdeadbeef
>>  
>>  static void test_count(struct kunit *test)
>> @@ -129,11 +130,172 @@ static void test_encode_string(struct kunit *test)
>>  			    xe_guc_klv_encode_string(failed, ARRAY_SIZE(klvs), key, string));
>>  }
>>  
>> +struct some_object {
>> +	u32 value1;
>> +	u64 value2;
>> +} __packed;
>> +
>> +static u32 *obj_raw_encoder(u32 *klvs, u32 avail, const void *arg)
>> +{
>> +	const struct some_object *obj = arg;
>> +	size_t sz = sizeof(*obj);
>> +	u32 dwords = to_num_dwords(sz);
>> +
>> +	if (IS_ERR(klvs))
>> +		return klvs;
>> +	if (dwords > avail)
>> +		return ERR_PTR(-ENOSPC);
>> +	memcpy(klvs, obj, sz);
>> +	return klvs + dwords;
>> +}
>> +
>> +static u32 *obj_klv_encoder(u32 *klvs, u32 avail, const void *arg)
>> +{
>> +	const struct some_object *obj = arg;
>> +	u32 *end = klvs + avail;
>> +
>> +	klvs = xe_guc_klv_encode_u32(klvs, end - klvs, TEST_KEY + 1, obj->value1);
>> +	klvs = xe_guc_klv_encode_u64(klvs, end - klvs, TEST_KEY + 2, obj->value2);
> 
> Sashiko spotted that we should check for errors between the encode
> calls, and that's probably a good idea.

the complain is about the calculation of the avail parameter, as with klvs
pointer being faulty, result of subtraction is undefined

but

the assumption is that all encode helpers shall ignore avail parameter if
passed klvs pointer is invalid, hence passing garbage as avail should be safe

and this allows us to write simpler code, as any encoding failure must result
in ERR_PTR, which when passed to the next encoder, shall be immediately returned
repeated by the subsequent encoders, without looking at other parameters

thus we may only need to check the result of the final encoder

and here, this is a pure example encoder code, for the test purposed we check
the final encoding result in the code below, for object encoding test purposes,
we don't care about internal encoding steps taken

> 
> With that:
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> Thanks,
> -Michał
> 
>> +	return klvs;
>> +}
>> +
>> +static u32 *obj_nested_encoder(u32 *klvs, u32 avail, const void *arg)
>> +{
>> +	u32 *end = klvs + avail;
>> +
>> +	klvs = xe_guc_klv_encode_object(klvs, end - klvs, TEST_GROUP_KEY + 1,
>> +					arg, obj_klv_encoder);
>> +	klvs = xe_guc_klv_encode_object(klvs, end - klvs, TEST_GROUP_KEY + 2,
>> +					arg, obj_klv_encoder);
>> +	return klvs;
>> +}
>> +
>> +static void test_encode_object_raw(struct kunit *test)
>> +{
>> +	const struct some_object obj = {
>> +		.value1 = 0xdead1234,
>> +		.value2 = 0xdead87654321dead,
>> +	};
>> +	u32 payload = to_num_dwords(sizeof(obj));
>> +	u32 *err = ERR_PTR(-ENOSPC);
>> +	u16 key = TEST_KEY;
>> +	u32 klvs[16];
>> +	u32 n;
>> +
>> +	/* too small, must fail */
>> +	for (n = 0; n < GUC_KLV_LEN_MIN + payload; n++) {
>> +		memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +		KUNIT_EXPECT_PTR_EQ_MSG(test, ERR_PTR(-ENOSPC),
>> +					xe_guc_klv_encode_object(klvs, n, key, &obj,
>> +								 obj_raw_encoder),
>> +					"buf size=%u dwords", n);
>> +	}
>> +
>> +	/* must pass */
>> +	memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test,
>> +				     xe_guc_klv_encode_object(klvs, GUC_KLV_LEN_MIN + payload,
>> +							      key, &obj, obj_raw_encoder));
>> +	KUNIT_EXPECT_EQ(test, klvs[0], PREP_GUC_KLV(key, payload));
>> +	KUNIT_EXPECT_MEMEQ(test, &klvs[1], &obj, sizeof(obj));
>> +
>> +	/* already failed, must fail */
>> +	KUNIT_ASSERT_PTR_EQ(test, err,
>> +			    xe_guc_klv_encode_object(err, ARRAY_SIZE(klvs), key,
>> +						     &obj, obj_raw_encoder));
>> +}
>> +
>> +static void test_encode_object_klv(struct kunit *test)
>> +{
>> +	const struct some_object obj = {
>> +		.value1 = 0xdead1234,
>> +		.value2 = 0xdead87654321dead,
>> +	};
>> +	u16 key = TEST_GROUP_KEY;
>> +	u32 payload = 0;
>> +	u32 klvs[16];
>> +	u32 n;
>> +
>> +	payload += GUC_KLV_LEN_MIN + to_num_dwords(sizeof(obj.value1));
>> +	payload += GUC_KLV_LEN_MIN + to_num_dwords(sizeof(obj.value2));
>> +
>> +	/* too small, must fail */
>> +	for (n = 0; n < GUC_KLV_LEN_MIN + payload; n++) {
>> +		memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +		KUNIT_EXPECT_PTR_EQ_MSG(test, ERR_PTR(-ENOSPC),
>> +					xe_guc_klv_encode_object(klvs, n, key, &obj,
>> +								 obj_klv_encoder),
>> +					"buf size=%u dwords", n);
>> +	}
>> +
>> +	/* must pass */
>> +	memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test,
>> +				     xe_guc_klv_encode_object(klvs, GUC_KLV_LEN_MIN + payload,
>> +							      key, &obj, obj_klv_encoder));
>> +	KUNIT_EXPECT_EQ(test, klvs[0], PREP_GUC_KLV(key, payload));
>> +	KUNIT_EXPECT_EQ(test, klvs[1], PREP_GUC_KLV(TEST_KEY + 1, 1));
>> +	KUNIT_EXPECT_EQ(test, klvs[2], obj.value1);
>> +	KUNIT_EXPECT_EQ(test, klvs[3], PREP_GUC_KLV(TEST_KEY + 2, 2));
>> +	KUNIT_EXPECT_EQ(test, klvs[4], lower_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[5], upper_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[6], TEST_PAD);
>> +}
>> +
>> +static void test_encode_object_nested(struct kunit *test)
>> +{
>> +	const struct some_object obj = {
>> +		.value1 = 0xdead1234,
>> +		.value2 = 0xdead87654321dead,
>> +	};
>> +	u16 key = TEST_GROUP_KEY;
>> +	u32 payload = 0;
>> +	u32 klvs[16];
>> +	u32 n;
>> +
>> +	payload += GUC_KLV_LEN_MIN;
>> +	payload += GUC_KLV_LEN_MIN + to_num_dwords(sizeof(obj.value1));
>> +	payload += GUC_KLV_LEN_MIN + to_num_dwords(sizeof(obj.value2));
>> +	payload *= 2;
>> +
>> +	/* too small, must fail */
>> +	for (n = 0; n < GUC_KLV_LEN_MIN + payload; n++) {
>> +		memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +		KUNIT_EXPECT_PTR_EQ_MSG(test, ERR_PTR(-ENOSPC),
>> +					xe_guc_klv_encode_object(klvs, n, key, &obj,
>> +								 obj_nested_encoder),
>> +					"buf size=%u dwords", n);
>> +	}
>> +
>> +	/* must pass */
>> +	memset32(klvs, TEST_PAD, ARRAY_SIZE(klvs));
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test,
>> +				     xe_guc_klv_encode_object(klvs, GUC_KLV_LEN_MIN + payload,
>> +							      key, &obj, obj_nested_encoder));
>> +	KUNIT_EXPECT_EQ(test, klvs[0], PREP_GUC_KLV(key, payload));
>> +	KUNIT_EXPECT_EQ(test, klvs[1], PREP_GUC_KLV(TEST_GROUP_KEY + 1, 5));
>> +	KUNIT_EXPECT_EQ(test, klvs[2], PREP_GUC_KLV(TEST_KEY + 1, 1));
>> +	KUNIT_EXPECT_EQ(test, klvs[3], obj.value1);
>> +	KUNIT_EXPECT_EQ(test, klvs[4], PREP_GUC_KLV(TEST_KEY + 2, 2));
>> +	KUNIT_EXPECT_EQ(test, klvs[5], lower_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[6], upper_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[7], PREP_GUC_KLV(TEST_GROUP_KEY + 2, 5));
>> +	KUNIT_EXPECT_EQ(test, klvs[8], PREP_GUC_KLV(TEST_KEY + 1, 1));
>> +	KUNIT_EXPECT_EQ(test, klvs[9], obj.value1);
>> +	KUNIT_EXPECT_EQ(test, klvs[10], PREP_GUC_KLV(TEST_KEY + 2, 2));
>> +	KUNIT_EXPECT_EQ(test, klvs[11], lower_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[12], upper_32_bits(obj.value2));
>> +	KUNIT_EXPECT_EQ(test, klvs[13], TEST_PAD);
>> +}
>> +
>>  static struct kunit_case guc_klv_helpers_test_cases[] = {
>>  	KUNIT_CASE(test_count),
>>  	KUNIT_CASE(test_encode_u32),
>>  	KUNIT_CASE(test_encode_u64),
>>  	KUNIT_CASE(test_encode_string),
>> +	KUNIT_CASE(test_encode_object_raw),
>> +	KUNIT_CASE(test_encode_object_klv),
>> +	KUNIT_CASE(test_encode_object_nested),
>>  	{}
>>  };
>>  
>> -- 
>> 2.47.1
>>