[PATCH igt 0/9] msm: recovery and mapping updates

[PATCH igt 0/9] msm: recovery and mapping updates

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

Mostly updates to address flakes, and updates related to newer kernels.
Plus a few additional sub-tests.

Rob Clark (9):
  drm/recovery: Wait for devcore to become available
  drm/recovery: add iova-fault-stress test
  msm/mapping: update for sparse
  msm/mapping: Handle BO names with spaces
  msm/mapping: Add additional subtests
  msm/mapping: Wait for devcore to become available
  msm/mapping: Handle larger gmem file size
  msm/mapping: Add wait for stall-on-fault to re-arm
  msm/mapping: Free devcore

 tests/msm/msm_mapping.c  | 90 +++++++++++++++++++++++++++++++++++-----
 tests/msm/msm_recovery.c | 39 ++++++++++++-----
 2 files changed, 109 insertions(+), 20 deletions(-)

-- 
2.50.0

[PATCH igt 1/9] drm/recovery: Wait for devcore to become available

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

The devcore could take some time to show up, so add a igt_wait() with
timeout so we don't fail the test if the devcore is not immediately
available.

This addresses a source of flakeyness.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_recovery.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
index fbe26e687ed3..433392079bd4 100644
--- a/tests/msm/msm_recovery.c
+++ b/tests/msm/msm_recovery.c
@@ -44,8 +44,12 @@ read_and_clear_devcore(void)
 	glob_t glob_buf = {0};
 	int ret, fd;
 
-	ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL, &glob_buf);
-	if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
+	/* The devcore shows up asynchronously, so it might not be
+	 * immediately available:
+	 */
+	if (!igt_wait(glob("/sys/class/devcoredump/devcd*/data",
+			   GLOB_NOSORT, NULL, &glob_buf) != GLOB_NOMATCH,
+		      1000, 100))
 		return;
 
 	fd = open(glob_buf.gl_pathv[0], O_RDWR);
-- 
2.50.0

[PATCH igt 2/9] drm/recovery: add iova-fault-stress test

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add a test which triggers many faults to test robustness.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_recovery.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
index 433392079bd4..8a83a6f24c41 100644
--- a/tests/msm/msm_recovery.c
+++ b/tests/msm/msm_recovery.c
@@ -195,6 +195,23 @@ do_parallel_test(struct msm_pipe *pipe, int child)
 	igt_msm_cmd_free(cmd);
 }
 
+static void
+do_fault_test(struct msm_pipe *pipe, bool stress)
+{
+	struct msm_cmd *cmd =
+		igt_msm_cmd_new(pipe, 0x10000);
+	unsigned cnt = stress ? 0x10000/16 : 1;
+
+	for (unsigned i = 0; i < cnt; i++) {
+		msm_cmd_pkt7(cmd, CP_MEM_WRITE, 3);
+		msm_cmd_emit(cmd, 0xdeaddead);           /* ADDR_LO */
+		msm_cmd_emit(cmd, 0x1);                  /* ADDR_HI */
+		msm_cmd_emit(cmd, 0x123);                /* VAL */
+	}
+
+	igt_wait_and_close(igt_msm_cmd_submit(cmd));
+}
+
 /*
  * Tests for drm/msm hangcheck, recovery, and fault handling
  */
@@ -242,18 +259,16 @@ igt_main
 
 	igt_describe("Test iova fault handling");
 	igt_subtest("iova-fault") {
-		struct msm_cmd *cmd;
-
 		igt_require(dev->gen >= 6);
 
-		cmd = igt_msm_cmd_new(pipe, 0x1000);
+		do_fault_test(pipe, false);
+	}
 
-		msm_cmd_pkt7(cmd, CP_MEM_WRITE, 3);
-		msm_cmd_emit(cmd, 0xdeaddead);           /* ADDR_LO */
-		msm_cmd_emit(cmd, 0x1);                  /* ADDR_HI */
-		msm_cmd_emit(cmd, 0x123);                /* VAL */
+	igt_describe("Test iova fault handling (stress)");
+	igt_subtest("iova-fault-stress") {
+		igt_require(dev->gen >= 6);
 
-		igt_wait_and_close(igt_msm_cmd_submit(cmd));
+		do_fault_test(pipe, true);
 	}
 
 	igt_fixture {
-- 
2.50.0

[PATCH igt 3/9] msm/mapping: update for sparse

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The debugfs format changes a bit.  Update the igt test to handle both
cases.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index ac20ca91d613..f536db82aba2 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -148,6 +148,10 @@ get_bo_addr(int drm_fd, const char *name)
 
 			ret = sscanf(line, "      vmas: [gpu: aspace=%"PRIx64", %"PRIx64",mapped,inuse=1]",
 					&dummy, &addr);
+			if (ret != 2) {
+				ret = sscanf(line, "      vmas: [gpu: vm=%"PRIx64", %"PRIx64", mapped]",
+						&dummy, &addr);
+			}
 			igt_fail_on(ret != 2);
 
 			return addr;
-- 
2.50.0

[PATCH igt 4/9] msm/mapping: Handle BO names with spaces

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

Instead of trimming at the first space, scan from the end of the string
until we reach the first non-space char.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index f536db82aba2..5afbcd081fc7 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -101,13 +101,14 @@ endswith(const char *str, const char *end)
 
 	/* Trim trailing whitespace: */
 	if (p) {
-		char *c = p;
-		while (c) {
+		char *c = p + strlen(p) - 1;
+		while (c > p ) {
 			if (isspace(*c)) {
 				*c = '\0';
+			} else {
 				break;
 			}
-			c++;
+			c--;
 		}
 	}
 
-- 
2.50.0

[PATCH igt 5/9] msm/mapping: Add additional subtests

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Test a few additional buffers that userspace should not have access to.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index 5afbcd081fc7..846385bb5206 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -253,6 +253,36 @@ igt_main
 		do_mapping_test(pipe, "shadow", false);
 	}
 
+	igt_describe("Test pwrup_reglist mapping, should be inaccessible");
+	igt_subtest("pwrup_reglist") {
+		do_mapping_test(pipe, "pwrup_reglist", true);
+		do_mapping_test(pipe, "pwrup_reglist", false);
+	}
+
+	igt_describe("Test memptrs mapping, should be inaccessible");
+	igt_subtest("memptrs") {
+		/*
+		 * This test will fail on older GPUs without HW_APRIV, but
+		 * there isn't a good way to test that from userspace, short
+		 * of maintaining a giant table.  Probably just easier to
+		 * list it in xfails or skips for those GPUs.
+		 */
+		do_mapping_test(pipe, "memptrs", true);
+		do_mapping_test(pipe, "memptrs", false);
+	}
+
+	igt_describe("Test 'preempt_record ring0' mapping, should be inaccessible");
+	igt_subtest("preempt_record_ring0") {
+		do_mapping_test(pipe, "preempt_record ring0", true);
+		do_mapping_test(pipe, "preempt_record ring0", false);
+	}
+
+	igt_describe("Test 'preempt_smmu_info ring0' mapping, should be inaccessible");
+	igt_subtest("preempt_smmu_info_ring0") {
+		do_mapping_test(pipe, "preempt_smmu_info ring0", true);
+		do_mapping_test(pipe, "preempt_smmu_info ring0", false);
+	}
+
 	igt_fixture {
 		igt_msm_pipe_close(pipe);
 		igt_msm_dev_close(dev);
-- 
2.50.0

[PATCH igt 6/9] msm/mapping: Wait for devcore to become available

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

The devcore could take some time to show up, so add a igt_wait() with
timeout so we don't fail the test if the devcore is not immediately
available.

This addresses a source of flakeyness.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index 846385bb5206..03a9e814c931 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -42,14 +42,18 @@
  */
 
 static char *
-get_and_clear_devcore(void)
+get_and_clear_devcore(int timeout_ms)
 {
 	glob_t glob_buf = {0};
 	char *buf = NULL;
-	int ret, fd;
+	int fd;
 
-	ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL, &glob_buf);
-	if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
+	/* The devcore shows up asynchronously, so it might not be
+	 * immediately available:
+	 */
+	if (!igt_wait(glob("/sys/class/devcoredump/devcd*/data",
+			   GLOB_NOSORT, NULL, &glob_buf) != GLOB_NOMATCH,
+		      timeout_ms, 100))
 		return NULL;
 
 	fd = open(glob_buf.gl_pathv[0], O_RDWR);
@@ -175,7 +179,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
 	int fence_fd, ret;
 
 	/* Clear any existing devcore's: */
-	while ((devcore = get_and_clear_devcore())) {
+	while ((devcore = get_and_clear_devcore(0))) {
 		free(devcore);
 	}
 
@@ -208,7 +212,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
 	/* And now we should have gotten a devcore from the iova fault
 	 * triggered by the read or write:
 	 */
-	devcore = get_and_clear_devcore();
+	devcore = get_and_clear_devcore(1000);
 	igt_fail_on(!devcore);
 
 	/* Make sure the devcore is from iova fault: */
-- 
2.50.0

[PATCH igt 7/9] msm/mapping: Handle larger gmem file size

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

If running on a system with full desktop environment, the file will be
much larger than 0x4000 bytes.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index 03a9e814c931..978ea18375dd 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -122,7 +122,7 @@ endswith(const char *str, const char *end)
 static uint64_t
 get_bo_addr(int drm_fd, const char *name)
 {
-	char buf[0x4000];
+	char buf[0x80000];
 	char *p = buf;
 
 	igt_debugfs_read(drm_fd, "gem", buf);
-- 
2.50.0

[PATCH igt 8/9] msm/mapping: Add wait for stall-on-fault to re-arm

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

Newer kernels disable stall-on-fault for a grace period, to avoid a
flood of faults causing instability with memory translations that
the hw attempts with the translation stalled.  Fortunately it adds a
debugfs file so we can know how long we need to wait for stall-on-
fault to be re-enabled.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index 978ea18375dd..7e2f5c7eadc8 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -74,6 +74,30 @@ get_and_clear_devcore(int timeout_ms)
 	return buf;
 }
 
+static void
+wait_for_stall_on_fault(int drm_fd)
+{
+	char buf[64] = "\0";
+
+	do {
+		int us;
+
+		igt_debugfs_read(drm_fd, "stall_reenable_time_us", buf);
+		if (!strlen(buf)) {
+			/* Not supported on older kernels: */
+			return;
+		}
+
+		us = atoi(buf);
+		if (!us) {
+			/* Done waiting: */
+			return;
+		}
+
+		usleep(us);
+	} while (true);
+}
+
 /*
  * Helper to find named buffer address
  */
@@ -224,6 +248,11 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
 	ret = sscanf(s, "  - iova=%"PRIx64, &fault_addr);
 	igt_fail_on(ret != 1);
 	igt_fail_on(addr != fault_addr);
+
+	/* Wait for stall-on-fault to re-enable, otherwise the next sub-test
+	 * would not generate a devcore:
+	 */
+	wait_for_stall_on_fault(pipe->dev->fd);
 }
 
 /*
-- 
2.50.0

[PATCH igt 9/9] msm/mapping: Free devcore

[Rob Clark]

From: Rob Clark <rob.clark@oss.qualcomm.com>

We shouldn't leak the devcore.

Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
---
 tests/msm/msm_mapping.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
index 7e2f5c7eadc8..b735d5f014c3 100644
--- a/tests/msm/msm_mapping.c
+++ b/tests/msm/msm_mapping.c
@@ -249,6 +249,8 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
 	igt_fail_on(ret != 1);
 	igt_fail_on(addr != fault_addr);
 
+	free(devcore);
+
 	/* Wait for stall-on-fault to re-enable, otherwise the next sub-test
 	 * would not generate a devcore:
 	 */
-- 
2.50.0

Re: [PATCH igt 1/9] drm/recovery: Wait for devcore to become available

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:08:55 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>

Please change subject, it is now:
[PATCH igt 1/9] drm/recovery: Wait for devcore to become available

and it should be:
[PATCH igt 1/9] tests/msm/msm_recovery: Wait for devcore to become available

or little shorter:
[PATCH igt 1/9] tests/msm/msm_recovery: Wait for devcore

After a resend I will ack it, I have also few nits for other
patches.

Regards,
Kamil

> 
> The devcore could take some time to show up, so add a igt_wait() with
> timeout so we don't fail the test if the devcore is not immediately
> available.
> 
> This addresses a source of flakeyness.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_recovery.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> index fbe26e687ed3..433392079bd4 100644
> --- a/tests/msm/msm_recovery.c
> +++ b/tests/msm/msm_recovery.c
> @@ -44,8 +44,12 @@ read_and_clear_devcore(void)
>  	glob_t glob_buf = {0};
>  	int ret, fd;
>  
> -	ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL, &glob_buf);
> -	if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
> +	/* The devcore shows up asynchronously, so it might not be
> +	 * immediately available:
> +	 */
> +	if (!igt_wait(glob("/sys/class/devcoredump/devcd*/data",
> +			   GLOB_NOSORT, NULL, &glob_buf) != GLOB_NOMATCH,
> +		      1000, 100))
>  		return;
>  
>  	fd = open(glob_buf.gl_pathv[0], O_RDWR);
> -- 
> 2.50.0
> 

Re: [PATCH igt 2/9] drm/recovery: add iova-fault-stress test

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:08:56 -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>

This 'From: ' do no match your s-o-b below, please fix it.

Please run checkpatch.pl from Linux kernel on your patches.
Few helpfull options could be found in CONTRIBUTING.md

Also improve subject to:
[PATCH igt 2/9] tests/msm/msm_recovery: add iova-fault-stress test

> 
> Add a test which triggers many faults to test robustness.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_recovery.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> index 433392079bd4..8a83a6f24c41 100644
> --- a/tests/msm/msm_recovery.c
> +++ b/tests/msm/msm_recovery.c
> @@ -195,6 +195,23 @@ do_parallel_test(struct msm_pipe *pipe, int child)
>  	igt_msm_cmd_free(cmd);
>  }
>  
> +static void
> +do_fault_test(struct msm_pipe *pipe, bool stress)
> +{
> +	struct msm_cmd *cmd =
> +		igt_msm_cmd_new(pipe, 0x10000);

Why not in one line? Imho this is better:

	struct msm_cmd *cmd = igt_msm_cmd_new(pipe, 0x10000);

> +	unsigned cnt = stress ? 0x10000/16 : 1;

Use 'unsigned int' here, also

Place space around '/', also make 0x10000 a const or define it.

For example:
	const unsigned int f_cnt = 0x10000;
	struct msm_cmd *cmd = igt_msm_cmd_new(pipe, f_cnt);
	unsigned int cnt = stress ? f_cnt / 16 : 1;

> +
> +	for (unsigned i = 0; i < cnt; i++) {

Use 'unsigned int' here:

	for (unsigned int i = 0; i < cnt; i++) {

Regards,
Kamil

> +		msm_cmd_pkt7(cmd, CP_MEM_WRITE, 3);
> +		msm_cmd_emit(cmd, 0xdeaddead);           /* ADDR_LO */
> +		msm_cmd_emit(cmd, 0x1);                  /* ADDR_HI */
> +		msm_cmd_emit(cmd, 0x123);                /* VAL */
> +	}
> +
> +	igt_wait_and_close(igt_msm_cmd_submit(cmd));
> +}
> +
>  /*
>   * Tests for drm/msm hangcheck, recovery, and fault handling
>   */
> @@ -242,18 +259,16 @@ igt_main
>  
>  	igt_describe("Test iova fault handling");
>  	igt_subtest("iova-fault") {
> -		struct msm_cmd *cmd;
> -
>  		igt_require(dev->gen >= 6);
>  
> -		cmd = igt_msm_cmd_new(pipe, 0x1000);
> +		do_fault_test(pipe, false);
> +	}
>  
> -		msm_cmd_pkt7(cmd, CP_MEM_WRITE, 3);
> -		msm_cmd_emit(cmd, 0xdeaddead);           /* ADDR_LO */
> -		msm_cmd_emit(cmd, 0x1);                  /* ADDR_HI */
> -		msm_cmd_emit(cmd, 0x123);                /* VAL */
> +	igt_describe("Test iova fault handling (stress)");
> +	igt_subtest("iova-fault-stress") {
> +		igt_require(dev->gen >= 6);
>  
> -		igt_wait_and_close(igt_msm_cmd_submit(cmd));
> +		do_fault_test(pipe, true);
>  	}
>  
>  	igt_fixture {
> -- 
> 2.50.0
> 

Re: [PATCH igt 3/9] msm/mapping: update for sparse

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:08:57 -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>

Fix 'From: ' e-mail address to match s-o-b.
Also add 'tests/' prefix:

[PATCH igt 3/9] tests/msm/msm_mapping: update for sparse

> 
> The debugfs format changes a bit.  Update the igt test to handle both
> cases.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index ac20ca91d613..f536db82aba2 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -148,6 +148,10 @@ get_bo_addr(int drm_fd, const char *name)
>  
>  			ret = sscanf(line, "      vmas: [gpu: aspace=%"PRIx64", %"PRIx64",mapped,inuse=1]",
>  					&dummy, &addr);
> +			if (ret != 2) {
> +				ret = sscanf(line, "      vmas: [gpu: vm=%"PRIx64", %"PRIx64", mapped]",
> +						&dummy, &addr);

Align to 'line'.

Regards,
Kamil

> +			}
>  			igt_fail_on(ret != 2);
>  
>  			return addr;
> -- 
> 2.50.0
> 

Re: [PATCH igt 4/9] msm/mapping: Handle BO names with spaces

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:08:58 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>
> 
> Instead of trimming at the first space, scan from the end of the string
> until we reach the first non-space char.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index f536db82aba2..5afbcd081fc7 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -101,13 +101,14 @@ endswith(const char *str, const char *end)
>  
>  	/* Trim trailing whitespace: */
>  	if (p) {
> -		char *c = p;
> -		while (c) {
> +		char *c = p + strlen(p) - 1;

Add empty line here.

> +		while (c > p ) {

Remove space after 'p':

		while (c > p) {

Regards,
Kamil

>  			if (isspace(*c)) {
>  				*c = '\0';
> +			} else {
>  				break;
>  			}
> -			c++;
> +			c--;
>  		}
>  	}
>  
> -- 
> 2.50.0
> 

Re: [PATCH igt 5/9] msm/mapping: Add additional subtests

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:08:59 -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>

Fix 'From: ' e-mail, also please add to all subjects 'tests/' prefix:

[PATCH igt 5/9] tests/msm/msm_mapping: Add additional subtests

Please apply that also to other tests in series.

Btw, why not a little more desciription in subject?

[PATCH igt 5/9] tests/msm/msm_mapping: Test inaccessible buffers

Regards,
Kamil

> 
> Test a few additional buffers that userspace should not have access to.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index 5afbcd081fc7..846385bb5206 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -253,6 +253,36 @@ igt_main
>  		do_mapping_test(pipe, "shadow", false);
>  	}
>  
> +	igt_describe("Test pwrup_reglist mapping, should be inaccessible");
> +	igt_subtest("pwrup_reglist") {
> +		do_mapping_test(pipe, "pwrup_reglist", true);
> +		do_mapping_test(pipe, "pwrup_reglist", false);
> +	}
> +
> +	igt_describe("Test memptrs mapping, should be inaccessible");
> +	igt_subtest("memptrs") {
> +		/*
> +		 * This test will fail on older GPUs without HW_APRIV, but
> +		 * there isn't a good way to test that from userspace, short
> +		 * of maintaining a giant table.  Probably just easier to
> +		 * list it in xfails or skips for those GPUs.
> +		 */
> +		do_mapping_test(pipe, "memptrs", true);
> +		do_mapping_test(pipe, "memptrs", false);
> +	}
> +
> +	igt_describe("Test 'preempt_record ring0' mapping, should be inaccessible");
> +	igt_subtest("preempt_record_ring0") {
> +		do_mapping_test(pipe, "preempt_record ring0", true);
> +		do_mapping_test(pipe, "preempt_record ring0", false);
> +	}
> +
> +	igt_describe("Test 'preempt_smmu_info ring0' mapping, should be inaccessible");
> +	igt_subtest("preempt_smmu_info_ring0") {
> +		do_mapping_test(pipe, "preempt_smmu_info ring0", true);
> +		do_mapping_test(pipe, "preempt_smmu_info ring0", false);
> +	}
> +
>  	igt_fixture {
>  		igt_msm_pipe_close(pipe);
>  		igt_msm_dev_close(dev);
> -- 
> 2.50.0
> 

Re: [PATCH igt 6/9] msm/mapping: Wait for devcore to become available

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:09:00 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>

Is this the same as 1/9 patch?

One more nit just spotted, it is useally 'i-g-t' after PATCH:

[PATCH i-g-t 6/9] tests/msm/msm_mapping: Wait for devcore to become available

Regards,
Kamil

> 
> The devcore could take some time to show up, so add a igt_wait() with
> timeout so we don't fail the test if the devcore is not immediately
> available.
> 
> This addresses a source of flakeyness.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index 846385bb5206..03a9e814c931 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -42,14 +42,18 @@
>   */
>  
>  static char *
> -get_and_clear_devcore(void)
> +get_and_clear_devcore(int timeout_ms)
>  {
>  	glob_t glob_buf = {0};
>  	char *buf = NULL;
> -	int ret, fd;
> +	int fd;
>  
> -	ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL, &glob_buf);
> -	if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
> +	/* The devcore shows up asynchronously, so it might not be
> +	 * immediately available:
> +	 */
> +	if (!igt_wait(glob("/sys/class/devcoredump/devcd*/data",
> +			   GLOB_NOSORT, NULL, &glob_buf) != GLOB_NOMATCH,
> +		      timeout_ms, 100))
>  		return NULL;
>  
>  	fd = open(glob_buf.gl_pathv[0], O_RDWR);
> @@ -175,7 +179,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
>  	int fence_fd, ret;
>  
>  	/* Clear any existing devcore's: */
> -	while ((devcore = get_and_clear_devcore())) {
> +	while ((devcore = get_and_clear_devcore(0))) {
>  		free(devcore);
>  	}
>  
> @@ -208,7 +212,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
>  	/* And now we should have gotten a devcore from the iova fault
>  	 * triggered by the read or write:
>  	 */
> -	devcore = get_and_clear_devcore();
> +	devcore = get_and_clear_devcore(1000);
>  	igt_fail_on(!devcore);
>  
>  	/* Make sure the devcore is from iova fault: */
> -- 
> 2.50.0
> 

Re: [PATCH igt 7/9] msm/mapping: Handle larger gmem file size

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:09:01 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>
> 
> If running on a system with full desktop environment, the file will be
> much larger than 0x4000 bytes.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index 03a9e814c931..978ea18375dd 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -122,7 +122,7 @@ endswith(const char *str, const char *end)
>  static uint64_t
>  get_bo_addr(int drm_fd, const char *name)
>  {
> -	char buf[0x4000];
> +	char buf[0x80000];

If it is that big why not malloc() it?

Regards,
Kamil

>  	char *p = buf;
>  
>  	igt_debugfs_read(drm_fd, "gem", buf);
> -- 
> 2.50.0
> 

Re: [PATCH igt 8/9] msm/mapping: Add wait for stall-on-fault to re-arm

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:09:02 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>
> 
> Newer kernels disable stall-on-fault for a grace period, to avoid a
> flood of faults causing instability with memory translations that
> the hw attempts with the translation stalled.  Fortunately it adds a

Is it system-wide or only for msm driver?

Please improve subject:

[PATCH igt 8/9] tests/msm/msm_mapping: Add wait for stall-on-fault to re-arm

imho it could be shorter:

[PATCH igt 8/9] tests/msm/msm_mapping: Wait for stall-on-fault

Regards,
Kamil

> debugfs file so we can know how long we need to wait for stall-on-
> fault to be re-enabled.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index 978ea18375dd..7e2f5c7eadc8 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -74,6 +74,30 @@ get_and_clear_devcore(int timeout_ms)
>  	return buf;
>  }
>  
> +static void
> +wait_for_stall_on_fault(int drm_fd)
> +{
> +	char buf[64] = "\0";
> +
> +	do {
> +		int us;
> +
> +		igt_debugfs_read(drm_fd, "stall_reenable_time_us", buf);
> +		if (!strlen(buf)) {
> +			/* Not supported on older kernels: */
> +			return;
> +		}
> +
> +		us = atoi(buf);
> +		if (!us) {
> +			/* Done waiting: */
> +			return;
> +		}
> +
> +		usleep(us);
> +	} while (true);
> +}
> +
>  /*
>   * Helper to find named buffer address
>   */
> @@ -224,6 +248,11 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
>  	ret = sscanf(s, "  - iova=%"PRIx64, &fault_addr);
>  	igt_fail_on(ret != 1);
>  	igt_fail_on(addr != fault_addr);
> +
> +	/* Wait for stall-on-fault to re-enable, otherwise the next sub-test
> +	 * would not generate a devcore:
> +	 */
> +	wait_for_stall_on_fault(pipe->dev->fd);
>  }
>  
>  /*
> -- 
> 2.50.0
> 

Re: [PATCH igt 9/9] msm/mapping: Free devcore

[Kamil Konieczny]

Hi Rob,
On 2025-06-30 at 11:09:03 -0700, Rob Clark wrote:
> From: Rob Clark <rob.clark@oss.qualcomm.com>

imho:

[PATCH i-g-t 9/9] tests/msm/msm_mapping: Fix memory leak

Also imho this should be your first patch, so fixes first,
but it is up to you.

Regards,
Kamil

> 
> We shouldn't leak the devcore.
> 
> Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> ---
>  tests/msm/msm_mapping.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> index 7e2f5c7eadc8..b735d5f014c3 100644
> --- a/tests/msm/msm_mapping.c
> +++ b/tests/msm/msm_mapping.c
> @@ -249,6 +249,8 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
>  	igt_fail_on(ret != 1);
>  	igt_fail_on(addr != fault_addr);
>  
> +	free(devcore);
> +
>  	/* Wait for stall-on-fault to re-enable, otherwise the next sub-test
>  	 * would not generate a devcore:
>  	 */
> -- 
> 2.50.0
> 

Re: [PATCH igt 7/9] msm/mapping: Handle larger gmem file size

[Rob Clark]

On Tue, Jul 1, 2025 at 9:21 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2025-06-30 at 11:09:01 -0700, Rob Clark wrote:
> > From: Rob Clark <rob.clark@oss.qualcomm.com>
> >
> > If running on a system with full desktop environment, the file will be
> > much larger than 0x4000 bytes.
> >
> > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > ---
> >  tests/msm/msm_mapping.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > index 03a9e814c931..978ea18375dd 100644
> > --- a/tests/msm/msm_mapping.c
> > +++ b/tests/msm/msm_mapping.c
> > @@ -122,7 +122,7 @@ endswith(const char *str, const char *end)
> >  static uint64_t
> >  get_bo_addr(int drm_fd, const char *name)
> >  {
> > -     char buf[0x4000];
> > +     char buf[0x80000];
>
> If it is that big why not malloc() it?

in kernel, for sure.  But userspace stack size is multiple MB at least

BR,
-R

> Regards,
> Kamil
>
> >       char *p = buf;
> >
> >       igt_debugfs_read(drm_fd, "gem", buf);
> > --
> > 2.50.0
> >

Re: [PATCH igt 8/9] msm/mapping: Add wait for stall-on-fault to re-arm

[Rob Clark]

On Tue, Jul 1, 2025 at 9:24 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2025-06-30 at 11:09:02 -0700, Rob Clark wrote:
> > From: Rob Clark <rob.clark@oss.qualcomm.com>
> >
> > Newer kernels disable stall-on-fault for a grace period, to avoid a
> > flood of faults causing instability with memory translations that
> > the hw attempts with the translation stalled.  Fortunately it adds a
>
> Is it system-wide or only for msm driver?

specific to msm

BR,
-R

> Please improve subject:
>
> [PATCH igt 8/9] tests/msm/msm_mapping: Add wait for stall-on-fault to re-arm
>
> imho it could be shorter:
>
> [PATCH igt 8/9] tests/msm/msm_mapping: Wait for stall-on-fault
>
> Regards,
> Kamil
>
> > debugfs file so we can know how long we need to wait for stall-on-
> > fault to be re-enabled.
> >
> > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > ---
> >  tests/msm/msm_mapping.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > index 978ea18375dd..7e2f5c7eadc8 100644
> > --- a/tests/msm/msm_mapping.c
> > +++ b/tests/msm/msm_mapping.c
> > @@ -74,6 +74,30 @@ get_and_clear_devcore(int timeout_ms)
> >       return buf;
> >  }
> >
> > +static void
> > +wait_for_stall_on_fault(int drm_fd)
> > +{
> > +     char buf[64] = "\0";
> > +
> > +     do {
> > +             int us;
> > +
> > +             igt_debugfs_read(drm_fd, "stall_reenable_time_us", buf);
> > +             if (!strlen(buf)) {
> > +                     /* Not supported on older kernels: */
> > +                     return;
> > +             }
> > +
> > +             us = atoi(buf);
> > +             if (!us) {
> > +                     /* Done waiting: */
> > +                     return;
> > +             }
> > +
> > +             usleep(us);
> > +     } while (true);
> > +}
> > +
> >  /*
> >   * Helper to find named buffer address
> >   */
> > @@ -224,6 +248,11 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
> >       ret = sscanf(s, "  - iova=%"PRIx64, &fault_addr);
> >       igt_fail_on(ret != 1);
> >       igt_fail_on(addr != fault_addr);
> > +
> > +     /* Wait for stall-on-fault to re-enable, otherwise the next sub-test
> > +      * would not generate a devcore:
> > +      */
> > +     wait_for_stall_on_fault(pipe->dev->fd);
> >  }
> >
> >  /*
> > --
> > 2.50.0
> >

Re: [PATCH igt 8/9] msm/mapping: Add wait for stall-on-fault to re-arm

[Kamil Konieczny]

Hi Rob,
On 2025-07-01 at 11:02:07 -0700, Rob Clark wrote:
> On Tue, Jul 1, 2025 at 9:24 AM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Rob,
> > On 2025-06-30 at 11:09:02 -0700, Rob Clark wrote:
> > > From: Rob Clark <rob.clark@oss.qualcomm.com>
> > >
> > > Newer kernels disable stall-on-fault for a grace period, to avoid a
> > > flood of faults causing instability with memory translations that
> > > the hw attempts with the translation stalled.  Fortunately it adds a
> >
> > Is it system-wide or only for msm driver?
> 
> specific to msm
> 
> BR,
> -R

Ok, so please write it explicitly, like for example:

Newer msm driver disables stall-on-fault for a grace period, [...rest...]

Please also give a link to lore with a change in msm driver.

Regards,
Kamil

> 
> > Please improve subject:
> >
> > [PATCH igt 8/9] tests/msm/msm_mapping: Add wait for stall-on-fault to re-arm
> >
> > imho it could be shorter:
> >
> > [PATCH igt 8/9] tests/msm/msm_mapping: Wait for stall-on-fault
> >
> > Regards,
> > Kamil
> >
> > > debugfs file so we can know how long we need to wait for stall-on-
> > > fault to be re-enabled.
> > >
> > > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > > ---
> > >  tests/msm/msm_mapping.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > > index 978ea18375dd..7e2f5c7eadc8 100644
> > > --- a/tests/msm/msm_mapping.c
> > > +++ b/tests/msm/msm_mapping.c
> > > @@ -74,6 +74,30 @@ get_and_clear_devcore(int timeout_ms)
> > >       return buf;
> > >  }
> > >
> > > +static void
> > > +wait_for_stall_on_fault(int drm_fd)
> > > +{
> > > +     char buf[64] = "\0";
> > > +
> > > +     do {
> > > +             int us;
> > > +
> > > +             igt_debugfs_read(drm_fd, "stall_reenable_time_us", buf);
> > > +             if (!strlen(buf)) {
> > > +                     /* Not supported on older kernels: */
> > > +                     return;
> > > +             }
> > > +
> > > +             us = atoi(buf);
> > > +             if (!us) {
> > > +                     /* Done waiting: */
> > > +                     return;
> > > +             }
> > > +
> > > +             usleep(us);
> > > +     } while (true);
> > > +}
> > > +
> > >  /*
> > >   * Helper to find named buffer address
> > >   */
> > > @@ -224,6 +248,11 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
> > >       ret = sscanf(s, "  - iova=%"PRIx64, &fault_addr);
> > >       igt_fail_on(ret != 1);
> > >       igt_fail_on(addr != fault_addr);
> > > +
> > > +     /* Wait for stall-on-fault to re-enable, otherwise the next sub-test
> > > +      * would not generate a devcore:
> > > +      */
> > > +     wait_for_stall_on_fault(pipe->dev->fd);
> > >  }
> > >
> > >  /*
> > > --
> > > 2.50.0
> > >

Re: [PATCH igt 7/9] msm/mapping: Handle larger gmem file size

[Kamil Konieczny]

Hi Rob,
On 2025-07-01 at 11:01:06 -0700, Rob Clark wrote:
> On Tue, Jul 1, 2025 at 9:21 AM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Rob,
> > On 2025-06-30 at 11:09:01 -0700, Rob Clark wrote:
> > > From: Rob Clark <rob.clark@oss.qualcomm.com>
> > >
> > > If running on a system with full desktop environment, the file will be
> > > much larger than 0x4000 bytes.
> > >
> > > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > > ---
> > >  tests/msm/msm_mapping.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > > index 03a9e814c931..978ea18375dd 100644
> > > --- a/tests/msm/msm_mapping.c
> > > +++ b/tests/msm/msm_mapping.c
> > > @@ -122,7 +122,7 @@ endswith(const char *str, const char *end)
> > >  static uint64_t
> > >  get_bo_addr(int drm_fd, const char *name)
> > >  {
> > > -     char buf[0x4000];
> > > +     char buf[0x80000];
> >
> > If it is that big why not malloc() it?
> 
> in kernel, for sure.  But userspace stack size is multiple MB at least
> 
> BR,
> -R

You are reading "gem" debugfs below, so this buffer in igt is in
userspace, no need for it to be on stack. Btw you just discovered
a bug in igt, imho this function should give sizeof(buf) to caller
here to igt_debugfs_read():

igt_debugfs_read(drm_fd, "gem", buf, sizeof(buf));

Also imho it should be calloc() to avoid garbage later on in buf.
Or if it should read all of debugfs node, make it like:

int igt_debugfs_read_all(int drm_fd, char *, char **buf);

and let it alloc *buf, and later realloc this *buf if needed.

Regards,
Kamil

> 
> > Regards,
> > Kamil
> >
> > >       char *p = buf;
> > >
> > >       igt_debugfs_read(drm_fd, "gem", buf);
> > > --
> > > 2.50.0
> > >

Re: [PATCH igt 6/9] msm/mapping: Wait for devcore to become available

[Rob Clark]

On Tue, Jul 1, 2025 at 9:20 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2025-06-30 at 11:09:00 -0700, Rob Clark wrote:
> > From: Rob Clark <rob.clark@oss.qualcomm.com>
>
> Is this the same as 1/9 patch?

No, but similar.  Both tests have a similar, but subtly different,
function to read the devcore.  (In the mapping test, we don't have to
read the whole file, but we want to return the first part so the test
can parse it.. in the recovery test we don't need to parse it, but
want to explicitly read the entire devcore.)

BR,
-R

>
> One more nit just spotted, it is useally 'i-g-t' after PATCH:
>
> [PATCH i-g-t 6/9] tests/msm/msm_mapping: Wait for devcore to become available
>
> Regards,
> Kamil
>
> >
> > The devcore could take some time to show up, so add a igt_wait() with
> > timeout so we don't fail the test if the devcore is not immediately
> > available.
> >
> > This addresses a source of flakeyness.
> >
> > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > ---
> >  tests/msm/msm_mapping.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > index 846385bb5206..03a9e814c931 100644
> > --- a/tests/msm/msm_mapping.c
> > +++ b/tests/msm/msm_mapping.c
> > @@ -42,14 +42,18 @@
> >   */
> >
> >  static char *
> > -get_and_clear_devcore(void)
> > +get_and_clear_devcore(int timeout_ms)
> >  {
> >       glob_t glob_buf = {0};
> >       char *buf = NULL;
> > -     int ret, fd;
> > +     int fd;
> >
> > -     ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL, &glob_buf);
> > -     if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
> > +     /* The devcore shows up asynchronously, so it might not be
> > +      * immediately available:
> > +      */
> > +     if (!igt_wait(glob("/sys/class/devcoredump/devcd*/data",
> > +                        GLOB_NOSORT, NULL, &glob_buf) != GLOB_NOMATCH,
> > +                   timeout_ms, 100))
> >               return NULL;
> >
> >       fd = open(glob_buf.gl_pathv[0], O_RDWR);
> > @@ -175,7 +179,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
> >       int fence_fd, ret;
> >
> >       /* Clear any existing devcore's: */
> > -     while ((devcore = get_and_clear_devcore())) {
> > +     while ((devcore = get_and_clear_devcore(0))) {
> >               free(devcore);
> >       }
> >
> > @@ -208,7 +212,7 @@ do_mapping_test(struct msm_pipe *pipe, const char *buffername, bool write)
> >       /* And now we should have gotten a devcore from the iova fault
> >        * triggered by the read or write:
> >        */
> > -     devcore = get_and_clear_devcore();
> > +     devcore = get_and_clear_devcore(1000);
> >       igt_fail_on(!devcore);
> >
> >       /* Make sure the devcore is from iova fault: */
> > --
> > 2.50.0
> >

Re: [PATCH igt 7/9] msm/mapping: Handle larger gmem file size

[Rob Clark]

On Wed, Jul 2, 2025 at 12:48 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
> On 2025-07-01 at 11:01:06 -0700, Rob Clark wrote:
> > On Tue, Jul 1, 2025 at 9:21 AM Kamil Konieczny
> > <kamil.konieczny@linux.intel.com> wrote:
> > >
> > > Hi Rob,
> > > On 2025-06-30 at 11:09:01 -0700, Rob Clark wrote:
> > > > From: Rob Clark <rob.clark@oss.qualcomm.com>
> > > >
> > > > If running on a system with full desktop environment, the file will be
> > > > much larger than 0x4000 bytes.
> > > >
> > > > Signed-off-by: Rob Clark <rob.clark@oss.qualcomm.com>
> > > > ---
> > > >  tests/msm/msm_mapping.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tests/msm/msm_mapping.c b/tests/msm/msm_mapping.c
> > > > index 03a9e814c931..978ea18375dd 100644
> > > > --- a/tests/msm/msm_mapping.c
> > > > +++ b/tests/msm/msm_mapping.c
> > > > @@ -122,7 +122,7 @@ endswith(const char *str, const char *end)
> > > >  static uint64_t
> > > >  get_bo_addr(int drm_fd, const char *name)
> > > >  {
> > > > -     char buf[0x4000];
> > > > +     char buf[0x80000];
> > >
> > > If it is that big why not malloc() it?
> >
> > in kernel, for sure.  But userspace stack size is multiple MB at least
> >
> > BR,
> > -R
>
> You are reading "gem" debugfs below, so this buffer in igt is in
> userspace, no need for it to be on stack. Btw you just discovered
> a bug in igt, imho this function should give sizeof(buf) to caller
> here to igt_debugfs_read():
>
> igt_debugfs_read(drm_fd, "gem", buf, sizeof(buf));

Actually igt_debugfs_read() is a convenience wrapper macro for
__igt_debugfs_read() which does just this.  But makes
igt_debugfs_read() more convenient for stack allocated buffers.

BR,
-R

>
> Also imho it should be calloc() to avoid garbage later on in buf.
> Or if it should read all of debugfs node, make it like:
>
> int igt_debugfs_read_all(int drm_fd, char *, char **buf);
>
> and let it alloc *buf, and later realloc this *buf if needed.
>
> Regards,
> Kamil
>
> >
> > > Regards,
> > > Kamil
> > >
> > > >       char *p = buf;
> > > >
> > > >       igt_debugfs_read(drm_fd, "gem", buf);
> > > > --
> > > > 2.50.0
> > > >