Re: [PATCH 12/13] drm/xe/tests: Add migration packet test

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

On 7/6/2026 6:42 PM, Michal Wajdeczko wrote:
> 
> 
> On 7/3/2026 3:44 PM, Michał Winiarski wrote:
>> On Tue, Jun 30, 2026 at 03:26:31PM +0200, Michal Wajdeczko wrote:
>>> One of our migration data packet (descriptor) is based on the KLV
>>> encoding. Add a simple descriptor initialization test, as we plan
>>> to use new KLV helper functions there.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> ---
>>>  .../gpu/drm/xe/tests/xe_sriov_packet_kunit.c  | 83 +++++++++++++++++++
>>>  drivers/gpu/drm/xe/xe_sriov_packet.c          |  4 +
>>>  2 files changed, 87 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/xe/tests/xe_sriov_packet_kunit.c
>>>
>>> diff --git a/drivers/gpu/drm/xe/tests/xe_sriov_packet_kunit.c b/drivers/gpu/drm/xe/tests/xe_sriov_packet_kunit.c
>>> new file mode 100644
>>> index 000000000000..9af066875671
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/tests/xe_sriov_packet_kunit.c
>>> @@ -0,0 +1,83 @@
>>> +// SPDX-License-Identifier: GPL-2.0 AND MIT
>>> +/*
>>> + * Copyright © 2026 Intel Corporation
>>> + */
>>> +
>>> +#include <kunit/test.h>
>>> +#include <kunit/test-bug.h>
>>> +
>>> +#include "xe_device.h"
>>> +#include "xe_guc_klv_helpers.h"
>>> +#include "xe_kunit_helpers.h"
>>> +#include "xe_pci_test.h"
>>> +
>>> +#define TEST_VF		VFID(1)
>>> +
>>> +static int sriov_packet_test_init(struct kunit *test)
>>> +{
>>> +	struct xe_pci_fake_data fake = {
>>> +		.sriov_mode = XE_SRIOV_MODE_PF,
>>> +		.platform = XE_PANTHERLAKE, /* we need MEMIRQ */
>>> +		.subplatform = XE_SUBPLATFORM_NONE,
>>> +		.graphics_verx100 = 3000,
>>> +		.media_verx100 = 3000,
>>> +	};
>>> +	struct xe_device *xe;
>>> +
>>> +	test->priv = &fake;
>>> +	xe_kunit_helper_xe_device_test_init(test);
>>> +	xe = test->priv;
>>> +
>>> +	/* pretend we can support at least VF1 */
>>> +	xe->sriov.pf.device_total_vfs = 1;
>>> +	xe->sriov.pf.driver_max_vfs = 1;
>>> +
>>> +	KUNIT_ASSERT_EQ(test, 0, xe_sriov_init(xe));
>>> +	KUNIT_ASSERT_TRUE(test, xe_sriov_pf_migration_supported(xe));
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void test_descriptor_init(struct kunit *test)
>>> +{
>>> +	struct xe_device *xe = test->priv;
>>> +	struct xe_sriov_packet **desc;
>>> +
>>> +	guard(mutex)(pf_migration_mutex(xe, TEST_VF));
>>
>> Sashiko complains that we'll never release the lock if we're mixing
>> scope-managed locks with KUNIT_ASSERT.
>> We might want to convert this to test-managed resource.
> 
> hmm, while this could be done here this way, it might be not so trivial
> in cases where we would also mix some original code that uses guard(mutex)
> before calling other function which we replaced with the stub and trigger
> the KUNIT_ASSERT from there.
> 
> maybe special steps should be taken only when locking a global mutex,
> which is not the case here? but it's not a rocket science, so yeah,
> let's do it
> 

hmm, on the first ASSERT we will get this WARN anyway:


====================================
WARNING: kunit_try_catch/18 still has locks held!
7.2.0-rc2-g1fa71d6adb71-dirty #1279 Tainted: G                 N
------------------------------------
1 lock held by kunit_try_catch/18:
 #0: 00000000644e21d8 (&migration->lock){+.+.}-{3:3}, at: test_descriptor_init+0x1a6/0xdd4


and doing mutex unlock in a cleanup action may result in:


=====================================
WARNING: bad unlock balance detected!
7.2.0-rc2-g1fa71d6adb71-dirty #1279 Tainted: G                 N
-------------------------------------
kunit_try_catch/21 is trying to release lock (&migration->lock) at:
[<00000000609192c5>] kunit_remove_resource+0x145/0x250
but there are no more locks to release!


so the best option seems to be avoid ASSERT and just return

>>
>> LGTM otherwise:
>> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> thanks!
> 
>>
>> Thanks,
>> -Michał
>>
>>> +
>>> +	KUNIT_ASSERT_EQ(test, 0, pf_descriptor_init(xe, TEST_VF));
>>> +	desc = pf_pick_descriptor(xe, TEST_VF);
>>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, *desc);
>>> +	KUNIT_EXPECT_NE(test, (*desc)->hdr.version, 0);
>>> +	KUNIT_EXPECT_EQ(test, (*desc)->hdr.version, XE_SRIOV_PACKET_SUPPORTED_VERSION);
>>> +	KUNIT_EXPECT_EQ(test, (*desc)->hdr.type, XE_SRIOV_PACKET_TYPE_DESCRIPTOR);
>>> +	KUNIT_EXPECT_NE(test, (*desc)->hdr.size, 0);
>>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, (*desc)->vaddr);
>>> +	KUNIT_EXPECT_EQ(test, 0, xe_sriov_packet_process_descriptor(xe, TEST_VF, *desc));
>>> +
>>> +	switch ((*desc)->hdr.version) {
>>> +	case 1:
>>> +		/* v1 is KLV based */
>>> +		KUNIT_EXPECT_TRUE(test, IS_ALIGNED((*desc)->hdr.size, sizeof(u32)));
>>> +		/* v1 has at least DEVID and REVID KLVs */
>>> +		KUNIT_EXPECT_LE(test, 2,
>>> +				xe_guc_klv_count((*desc)->vaddr,
>>> +						 (*desc)->hdr.size / sizeof(u32)));
>>> +		break;
>>> +	default:
>>> +		kunit_skip(test, "missing test code for version %u\n", (*desc)->hdr.version);
>>> +	}
>>> +}
>>> +
>>> +static struct kunit_case sriov_packet_test_cases[] = {
>>> +	KUNIT_CASE(test_descriptor_init),
>>> +	{}
>>> +};
>>> +
>>> +static struct kunit_suite sriov_packet_suite = {
>>> +	.name = "sriov_packet",
>>> +	.test_cases = sriov_packet_test_cases,
>>> +	.init = sriov_packet_test_init,
>>> +};
>>> +
>>> +kunit_test_suite(sriov_packet_suite);
>>> diff --git a/drivers/gpu/drm/xe/xe_sriov_packet.c b/drivers/gpu/drm/xe/xe_sriov_packet.c
>>> index e581e8e6c1d1..558a3697d639 100644
>>> --- a/drivers/gpu/drm/xe/xe_sriov_packet.c
>>> +++ b/drivers/gpu/drm/xe/xe_sriov_packet.c
>>> @@ -516,3 +516,7 @@ int xe_sriov_packet_save_init(struct xe_device *xe, unsigned int vfid)
>>>  
>>>  	return 0;
>>>  }
>>> +
>>> +#if IS_BUILTIN(CONFIG_DRM_XE_KUNIT_TEST)
>>> +#include "tests/xe_sriov_packet_kunit.c"
>>> +#endif
>>> -- 
>>> 2.47.1
>>>
>