Igt-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
To: Shekhar Chauhan <shekhar.chauhan@intel.com>
Cc: <igt-dev@lists.freedesktop.org>, <harish.chegondi@intel.com>,
	<riana.tauro@intel.com>
Subject: Re: [PATCH] tests/intel/xe_oa: Add unbind/rebind test with an open OA stream
Date: Fri, 30 Oct 2026 10:56:34 -0700	[thread overview]
Message-ID: <auTaUn1QJSGSBjxQ@soc-5CG1426VCC.clients.intel.com> (raw)
In-Reply-To: <90bc9be0-75bf-4cd7-8095-e6c5c7f97c3a@intel.com>

On Fri, Oct 30, 2026 at 01:00:37PM +0530, Shekhar Chauhan wrote:
>
>On 10/27/2026 5:00, Umesh Nerlige Ramappa wrote:
>>Open an OA stream, unbind and rebind the xe device, and log the state
>>of the stream fd (poll/read/ioctls) after removal and after probe.
>>Close stream either before or after the rebind.
>>
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>
>The test, in itself seems fine to me but there are certain suggestions:
>    1. After close/rebind of the GPU, we poke/check/probe the IOCTLs 
>from probe_stream_after_removal() but we never defined what the 
>expected behaviour is. I mean, how do we know that this is what is 
>supposed to be the case, at the end of this subtests execution.  Can we 
>have some asserts?  Although, since this is a new test being added, I 
>do not know that the kernel is even addressing something like this or 
>not, like, how should the XeKMD respond when we re-poke/probe a stream 
>after it's device is gone.

It should be ENODEV, I can add an assert for that.

>
>    2. Because we have a random decide between close and open in 
>main(), I feel not everything will be tested everytime CI runs this.  
>Maybe we can have both of them run definitely.

Initially I had 2 tests - one for each case, but I ran into (3) below 
when running the whole test suite. As a quick fix, I used one test at 
the end with random case. As for definitively testing, we rely on 
multiple runs on CI, so it should be fine. It's following other tests in 
this file that only run random cases (like buffer sizes).  The idea is 
that multiple runs in CI will catch issues anyways.

>
>    3. (AI-Suggestion) If we unbind the device, we did not update the 
>primary set-up of the device for this test in igt_main(). Right now, 
>it's the last subtest, so no one uses the GPU after this, but 
>tomorrow, if a new test is added after this test, then that new 
>subtest might fail without cause.

Right, that is not addressed here. For now we keep this as the last test 
so that our local runs don't fail. CI only runs this in shards (only one 
test at a time), so we will not see an issue even if this weren't the 
last test case.

I will take another look to see if we can restore the entire OA setup 
after the test, so that subsequent tests will pass in local runs.  If 
not, then we can do it in a follow up series.

Thanks,
Umesh

>
>-shekhar
>
>>---
>>  tests/intel/xe_oa.c | 127 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>
>>diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>>index b9f62c1154fe..8a56efa43dfd 100644
>>--- a/tests/intel/xe_oa.c
>>+++ b/tests/intel/xe_oa.c
>>@@ -22,6 +22,7 @@
>>  #include "drm.h"
>>  #include "igt.h"
>>  #include "igt_device.h"
>>+#include "igt_pci.h"
>>  #include "igt_syncobj.h"
>>  #include "igt_sysfs.h"
>>  #include "intel_wa.h"
>>@@ -2725,6 +2726,125 @@ test_enable_disable(const struct drm_xe_oa_unit *oau)
>>  	__perf_close(stream_fd);
>>  }
>>+static void probe_stream_after_removal(const char *when)
>>+{
>>+	struct drm_xe_oa_stream_info info = {};
>>+	uint8_t buf[4096];
>>+	struct pollfd pfd = { .fd = stream_fd, .events = POLLIN };
>>+	int ret;
>>+
>>+	ret = poll(&pfd, 1, 1000);
>>+	igt_info("[%s] poll: ret=%d revents=0x%x errno=%s\n", when, ret, pfd.revents,
>>+		 ret < 0 ? strerror(errno) : "-");
>>+
>>+	errno = 0;
>>+	ret = read(stream_fd, buf, sizeof(buf));
>>+	igt_info("[%s] read: ret=%d errno=%s\n", when, ret, ret < 0 ? strerror(errno) : "-");
>>+
>>+	errno = 0;
>>+	ret = igt_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_INFO, &info);
>>+	igt_info("[%s] ioctl INFO: ret=%d errno=%s oa_buf_size=%" PRIu64 "\n", when, ret,
>>+		 ret < 0 ? strerror(errno) : "-", (uint64_t)info.oa_buf_size);
>>+
>>+	errno = 0;
>>+	ret = igt_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_DISABLE, 0);
>>+	igt_info("[%s] ioctl DISABLE: ret=%d errno=%s\n", when, ret,
>>+		 ret < 0 ? strerror(errno) : "-");
>>+
>>+	errno = 0;
>>+	ret = igt_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>>+	igt_info("[%s] ioctl ENABLE: ret=%d errno=%s\n", when, ret,
>>+		 ret < 0 ? strerror(errno) : "-");
>>+}
>>+
>>+/**
>>+ * SUBTEST: unbind-rebind-stream
>>+ * Description: Open an OA stream and test the effects of unbind and rebind
>>+ */
>>+static void
>>+test_unbind_rebind(const struct drm_xe_oa_unit *oau, bool close_before_rebind)
>>+{
>>+	struct intel_xe_perf_metric_set *test_set = oa_unit_metric_set(oau);
>>+	uint64_t fmt = test_set->perf_oa_format;
>>+	uint64_t properties[] = {
>>+		DRM_XE_OA_PROPERTY_OA_UNIT_ID, oau->oa_unit_id,
>>+		DRM_XE_OA_PROPERTY_SAMPLE_OA, true,
>>+		DRM_XE_OA_PROPERTY_OA_METRIC_SET, test_set->perf_oa_metrics_set,
>>+		DRM_XE_OA_PROPERTY_OA_FORMAT, __ff(fmt),
>>+		DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, oa_exponent_default,
>>+	};
>>+	struct intel_xe_oa_open_prop param = {
>>+		.num_properties = ARRAY_SIZE(properties) / 2,
>>+		.properties_ptr = to_user_pointer(properties),
>>+	};
>>+	size_t format_size = get_oa_format(fmt).size;
>>+	uint8_t buf[format_size * 4];
>>+	char pci_slot[NAME_MAX];
>>+	int ret;
>>+
>>+	igt_device_get_pci_slot_name(drm_fd, pci_slot);
>>+	igt_info("Device %s\n", pci_slot);
>>+
>>+	stream_fd = __perf_open(drm_fd, &param, false);
>>+	set_fd_flags(stream_fd, O_CLOEXEC | O_NONBLOCK);
>>+	do_ioctl(stream_fd, DRM_XE_OBSERVATION_IOCTL_ENABLE, 0);
>>+
>>+	/* Make sure the stream is live before pulling the device */
>>+	{
>>+		struct pollfd pfd = { .fd = stream_fd, .events = POLLIN };
>>+
>>+		igt_assert_lt(0, poll(&pfd, 1, 2000));
>>+	}
>>+	ret = read(stream_fd, buf, sizeof(buf));
>>+	igt_info("Pre-unbind read: ret=%d\n", ret);
>>+	igt_assert_lt(0, ret);
>>+
>>+	/* Guard against a wedged unbind/bind */
>>+	igt_set_timeout(60, "xe unbind timed out (wedged?)");
>>+	igt_info("Unbinding xe from %s with stream open\n", pci_slot);
>>+	ret = igt_pci_driver_unbind("xe", pci_slot);
>>+	igt_reset_timeout();
>>+	igt_assert_f(ret == 0, "xe unbind failed: %s\n", strerror(-ret));
>>+	igt_info("Unbind complete\n");
>>+
>>+	probe_stream_after_removal("after-unbind");
>>+
>>+	if (close_before_rebind) {
>>+		igt_info("Closing stream fd while device is unbound\n");
>>+		igt_set_timeout(60, "stream close timed out (wedged?)");
>>+		__perf_close(stream_fd);
>>+		igt_reset_timeout();
>>+		igt_info("Stream close complete\n");
>>+	}
>>+
>>+	/* The drm fd also refers to the removed device */
>>+	xe_device_put(drm_fd);
>>+	drm_close_driver(drm_fd);
>>+	drm_fd = -1;
>>+
>>+	igt_set_timeout(60, "xe bind timed out (wedged?)");
>>+	igt_info("Rebinding xe to %s\n", pci_slot);
>>+	ret = igt_pci_driver_bind("xe", pci_slot);
>>+	igt_reset_timeout();
>>+	igt_assert_f(ret == 0, "xe bind failed: %s\n", strerror(-ret));
>>+	igt_info("Bind complete\n");
>>+
>>+	if (!close_before_rebind) {
>>+		probe_stream_after_removal("after-rebind");
>>+
>>+		igt_info("Closing stream fd after rebind\n");
>>+		igt_set_timeout(60, "stream close timed out (wedged?)");
>>+		__perf_close(stream_fd);
>>+		igt_reset_timeout();
>>+		igt_info("Stream close complete\n");
>>+	}
>>+
>>+	/* Restore state for the remaining subtests/fixtures */
>>+	drm_fd = drm_open_driver(DRIVER_XE);
>>+	xe_device_get(drm_fd);
>>+	igt_info("Reopened xe device after rebind\n");
>>+}
>>+
>>  /**
>>   * SUBTEST: short-reads
>>   * Description: Test behavior for short reads
>>@@ -5221,6 +5341,13 @@ int igt_main_args("b:td", long_options, help_str, opt_handler, NULL)
>>  		}
>>  	}
>>+	igt_subtest_with_dynamic("unbind-rebind-stream") {
>>+		bool close_before_rebind = random() & 0x1;
>>+
>>+		igt_dynamic_f("%s", close_before_rebind ? "close" : "open")
>>+			test_unbind_rebind(oau0, close_before_rebind);
>>+	}
>>+
>>  	igt_fixture() {
>>  		/* leave sysctl options in their default state... */
>>  		write_u64_file("/proc/sys/dev/xe/observation_paranoid", 1);
>
>-- 
>Shekhar Chauhan
>Linux Graphics Software Engineer
>Intel Corporation
>

      reply	other threads:[~2026-10-30 17:57 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-26 23:30 [PATCH] tests/intel/xe_oa: Add unbind/rebind test with an open OA stream Umesh Nerlige Ramappa
2026-10-27  0:31 ` ✓ Xe.CI.BAT: success for " Patchwork
2026-10-27  0:45 ` ✓ i915.CI.BAT: " Patchwork
2026-10-27  6:26 ` ✗ i915.CI.Full: failure " Patchwork
2026-10-27  7:04 ` ✗ Xe.CI.FULL: " Patchwork
2026-10-30  7:30 ` [PATCH] " Shekhar Chauhan
2026-10-30 17:56   ` Umesh Nerlige Ramappa [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=auTaUn1QJSGSBjxQ@soc-5CG1426VCC.clients.intel.com \
    --to=umesh.nerlige.ramappa@intel.com \
    --cc=harish.chegondi@intel.com \
    --cc=igt-dev@lists.freedesktop.org \
    --cc=riana.tauro@intel.com \
    --cc=shekhar.chauhan@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox