[igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This also adds coverage for codepaths related to reading back the
devcore file from sysfs, to help catch issues like
https://gitlab.freedesktop.org/drm/msm/-/issues/20

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 tests/msm/msm_recovery.c | 42 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
index e01087b4..4c2f2846 100644
--- a/tests/msm/msm_recovery.c
+++ b/tests/msm/msm_recovery.c
@@ -22,14 +22,54 @@
  */
 
 #include <fcntl.h>
+#include <glob.h>
 
 #include "igt.h"
 #include "igt_msm.h"
+#include "igt_io.h"
 
 static struct msm_device *dev;
 static struct msm_bo *scratch_bo;
 static uint32_t *scratch;
 
+/*
+ * Helper to read and clear devcore.  We want to read it completely to ensure
+ * we catch any kernel side regressions like:
+ * https://gitlab.freedesktop.org/drm/msm/-/issues/20
+ */
+
+static void
+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)
+		return;
+
+	fd = open(glob_buf.gl_pathv[0], O_RDWR);
+
+	if (fd >= 0) {
+		char buf[0x1000];
+
+		/*
+		 * We want to read the entire file but we can throw away the
+		 * contents.. we just want to make sure that we exercise the
+		 * kernel side codepaths hit when reading the devcore from
+		 * sysfs
+		 */
+		do {
+			ret = igt_readn(fd, buf, sizeof(buf));
+		} while (ret > 0);
+
+		/* Clear the devcore: */
+		igt_writen(fd, "1", 1);
+	}
+
+	globfree(&glob_buf);
+}
+
 /*
  * Helpers for cmdstream packet building:
  */
@@ -102,6 +142,8 @@ do_hang_test(struct msm_pipe *pipe)
 			continue;
 		igt_assert_eq(scratch[1+i], 2+i);
 	}
+
+	read_and_clear_devcore();
 }
 
 /*
-- 
2.38.1

[igt-dev] ✗ Fi.CI.BUILD: failure for tests/msm: Read the devcore back in recovery tests

[Patchwork]

== Series Details ==

Series: tests/msm: Read the devcore back in recovery tests
URL   : https://patchwork.freedesktop.org/series/111828/
State : failure

== Summary ==

Applying: tests/msm: Read the devcore back in recovery tests
Using index info to reconstruct a base tree...
M	tests/msm/msm_recovery.c
Falling back to patching base and 3-way merge...
Auto-merging tests/msm/msm_recovery.c
CONFLICT (content): Merge conflict in tests/msm/msm_recovery.c
Patch failed at 0001 tests/msm: Read the devcore back in recovery tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[Kamil Konieczny]

Hi Rob,

before sending again please rebase on current igt (see logs from
gitlab).
I have also one small nit below.

On 2022-12-11 at 11:53:02 -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> This also adds coverage for codepaths related to reading back the
> devcore file from sysfs, to help catch issues like
> https://gitlab.freedesktop.org/drm/msm/-/issues/20
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  tests/msm/msm_recovery.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> index e01087b4..4c2f2846 100644
> --- a/tests/msm/msm_recovery.c
> +++ b/tests/msm/msm_recovery.c
> @@ -22,14 +22,54 @@
>   */
>  
>  #include <fcntl.h>
> +#include <glob.h>
>  
>  #include "igt.h"
>  #include "igt_msm.h"
> +#include "igt_io.h"
>  
>  static struct msm_device *dev;
>  static struct msm_bo *scratch_bo;
>  static uint32_t *scratch;
>  
> +/*
> + * Helper to read and clear devcore.  We want to read it completely to ensure
> + * we catch any kernel side regressions like:
> + * https://gitlab.freedesktop.org/drm/msm/-/issues/20
> + */
> +
> +static void
> +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)
> +		return;
> +
> +	fd = open(glob_buf.gl_pathv[0], O_RDWR);
> +
> +	if (fd >= 0) {
> +		char buf[0x1000];
> +
> +		/*
> +		 * We want to read the entire file but we can throw away the
> +		 * contents.. we just want to make sure that we exercise the
> +		 * kernel side codepaths hit when reading the devcore from
> +		 * sysfs
> +		 */
> +		do {
> +			ret = igt_readn(fd, buf, sizeof(buf));
> +		} while (ret > 0);
> +
> +		/* Clear the devcore: */
> +		igt_writen(fd, "1", 1);

Put here:
		fclose(fd);

Regards,
Kamil

> +	}
> +
> +	globfree(&glob_buf);
> +}
> +
>  /*
>   * Helpers for cmdstream packet building:
>   */
> @@ -102,6 +142,8 @@ do_hang_test(struct msm_pipe *pipe)
>  			continue;
>  		igt_assert_eq(scratch[1+i], 2+i);
>  	}
> +
> +	read_and_clear_devcore();
>  }
>  
>  /*
> -- 
> 2.38.1
> 

Re: [igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[Rob Clark]

On Mon, Dec 12, 2022 at 6:15 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
>
> before sending again please rebase on current igt (see logs from
> gitlab).

sure.. but I was based on ToT as of the 7th, and it rebased cleanly..
not sure if you had some concern than I'm overlooking

> I have also one small nit below.
>
> On 2022-12-11 at 11:53:02 -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > This also adds coverage for codepaths related to reading back the
> > devcore file from sysfs, to help catch issues like
> > https://gitlab.freedesktop.org/drm/msm/-/issues/20
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  tests/msm/msm_recovery.c | 42 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> > index e01087b4..4c2f2846 100644
> > --- a/tests/msm/msm_recovery.c
> > +++ b/tests/msm/msm_recovery.c
> > @@ -22,14 +22,54 @@
> >   */
> >
> >  #include <fcntl.h>
> > +#include <glob.h>
> >
> >  #include "igt.h"
> >  #include "igt_msm.h"
> > +#include "igt_io.h"
> >
> >  static struct msm_device *dev;
> >  static struct msm_bo *scratch_bo;
> >  static uint32_t *scratch;
> >
> > +/*
> > + * Helper to read and clear devcore.  We want to read it completely to ensure
> > + * we catch any kernel side regressions like:
> > + * https://gitlab.freedesktop.org/drm/msm/-/issues/20
> > + */
> > +
> > +static void
> > +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)
> > +             return;
> > +
> > +     fd = open(glob_buf.gl_pathv[0], O_RDWR);
> > +
> > +     if (fd >= 0) {
> > +             char buf[0x1000];
> > +
> > +             /*
> > +              * We want to read the entire file but we can throw away the
> > +              * contents.. we just want to make sure that we exercise the
> > +              * kernel side codepaths hit when reading the devcore from
> > +              * sysfs
> > +              */
> > +             do {
> > +                     ret = igt_readn(fd, buf, sizeof(buf));
> > +             } while (ret > 0);
> > +
> > +             /* Clear the devcore: */
> > +             igt_writen(fd, "1", 1);
>
> Put here:
>                 fclose(fd);
>

oh, right, good catch

BR,
-R

> Regards,
> Kamil
>
> > +     }
> > +
> > +     globfree(&glob_buf);
> > +}
> > +
> >  /*
> >   * Helpers for cmdstream packet building:
> >   */
> > @@ -102,6 +142,8 @@ do_hang_test(struct msm_pipe *pipe)
> >                       continue;
> >               igt_assert_eq(scratch[1+i], 2+i);
> >       }
> > +
> > +     read_and_clear_devcore();
> >  }
> >
> >  /*
> > --
> > 2.38.1
> >

Re: [igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[Kamil Konieczny]

Hi Rob,

On 2022-12-12 at 13:54:39 -0800, Rob Clark wrote:
> On Mon, Dec 12, 2022 at 6:15 AM Kamil Konieczny
> <kamil.konieczny@linux.intel.com> wrote:
> >
> > Hi Rob,
> >
> > before sending again please rebase on current igt (see logs from
> > gitlab).
> 
> sure.. but I was based on ToT as of the 7th, and it rebased cleanly..
> not sure if you had some concern than I'm overlooking

Well it depends on previous ones, especially this one:

[1/2] lib: Add helper to wait and close fence fd

so please either send it as part of whole series or wait for
others to be merged.
Btw it would help if you at least note such dependancy in cover
letter or in patch description after "---" like:

---
some notes that will not go into git repo, for example:
This patch depends on "lib: Add helper to wait and close fence fd"

It will help maintainters to look at this only after other parts
will go.

Regards,
Kamil

> 
> > I have also one small nit below.
> >
> > On 2022-12-11 at 11:53:02 -0800, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > This also adds coverage for codepaths related to reading back the
> > > devcore file from sysfs, to help catch issues like
> > > https://gitlab.freedesktop.org/drm/msm/-/issues/20
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  tests/msm/msm_recovery.c | 42 ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> > > index e01087b4..4c2f2846 100644
> > > --- a/tests/msm/msm_recovery.c
> > > +++ b/tests/msm/msm_recovery.c
> > > @@ -22,14 +22,54 @@
> > >   */
> > >
> > >  #include <fcntl.h>
> > > +#include <glob.h>
> > >
> > >  #include "igt.h"
> > >  #include "igt_msm.h"
> > > +#include "igt_io.h"
> > >
> > >  static struct msm_device *dev;
> > >  static struct msm_bo *scratch_bo;
> > >  static uint32_t *scratch;
> > >
> > > +/*
> > > + * Helper to read and clear devcore.  We want to read it completely to ensure
> > > + * we catch any kernel side regressions like:
> > > + * https://gitlab.freedesktop.org/drm/msm/-/issues/20
> > > + */
> > > +
> > > +static void
> > > +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)
> > > +             return;
> > > +
> > > +     fd = open(glob_buf.gl_pathv[0], O_RDWR);
> > > +
> > > +     if (fd >= 0) {
> > > +             char buf[0x1000];
> > > +
> > > +             /*
> > > +              * We want to read the entire file but we can throw away the
> > > +              * contents.. we just want to make sure that we exercise the
> > > +              * kernel side codepaths hit when reading the devcore from
> > > +              * sysfs
> > > +              */
> > > +             do {
> > > +                     ret = igt_readn(fd, buf, sizeof(buf));
> > > +             } while (ret > 0);
> > > +
> > > +             /* Clear the devcore: */
> > > +             igt_writen(fd, "1", 1);
> >
> > Put here:
> >                 fclose(fd);
> >
> 
> oh, right, good catch
> 
> BR,
> -R
> 
> > Regards,
> > Kamil
> >
> > > +     }
> > > +
> > > +     globfree(&glob_buf);
> > > +}
> > > +
> > >  /*
> > >   * Helpers for cmdstream packet building:
> > >   */
> > > @@ -102,6 +142,8 @@ do_hang_test(struct msm_pipe *pipe)
> > >                       continue;
> > >               igt_assert_eq(scratch[1+i], 2+i);
> > >       }
> > > +
> > > +     read_and_clear_devcore();
> > >  }
> > >
> > >  /*
> > > --
> > > 2.38.1
> > >

Re: [igt-dev] [PATCH igt] tests/msm: Read the devcore back in recovery tests

[Rob Clark]

On Tue, Dec 13, 2022 at 3:14 AM Kamil Konieczny
<kamil.konieczny@linux.intel.com> wrote:
>
> Hi Rob,
>
> On 2022-12-12 at 13:54:39 -0800, Rob Clark wrote:
> > On Mon, Dec 12, 2022 at 6:15 AM Kamil Konieczny
> > <kamil.konieczny@linux.intel.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > before sending again please rebase on current igt (see logs from
> > > gitlab).
> >
> > sure.. but I was based on ToT as of the 7th, and it rebased cleanly..
> > not sure if you had some concern than I'm overlooking
>
> Well it depends on previous ones, especially this one:
>
> [1/2] lib: Add helper to wait and close fence fd
>

there is a trivial conflict to resolve if this is merged before that
series.. but I can deal with that before pushing, once patches get
r-b's ;-)

BR,
-R

> so please either send it as part of whole series or wait for
> others to be merged.
> Btw it would help if you at least note such dependancy in cover
> letter or in patch description after "---" like:
>
> ---
> some notes that will not go into git repo, for example:
> This patch depends on "lib: Add helper to wait and close fence fd"
>
> It will help maintainters to look at this only after other parts
> will go.
>
> Regards,
> Kamil
>
> >
> > > I have also one small nit below.
> > >
> > > On 2022-12-11 at 11:53:02 -0800, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > This also adds coverage for codepaths related to reading back the
> > > > devcore file from sysfs, to help catch issues like
> > > > https://gitlab.freedesktop.org/drm/msm/-/issues/20
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >  tests/msm/msm_recovery.c | 42 ++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/tests/msm/msm_recovery.c b/tests/msm/msm_recovery.c
> > > > index e01087b4..4c2f2846 100644
> > > > --- a/tests/msm/msm_recovery.c
> > > > +++ b/tests/msm/msm_recovery.c
> > > > @@ -22,14 +22,54 @@
> > > >   */
> > > >
> > > >  #include <fcntl.h>
> > > > +#include <glob.h>
> > > >
> > > >  #include "igt.h"
> > > >  #include "igt_msm.h"
> > > > +#include "igt_io.h"
> > > >
> > > >  static struct msm_device *dev;
> > > >  static struct msm_bo *scratch_bo;
> > > >  static uint32_t *scratch;
> > > >
> > > > +/*
> > > > + * Helper to read and clear devcore.  We want to read it completely to ensure
> > > > + * we catch any kernel side regressions like:
> > > > + * https://gitlab.freedesktop.org/drm/msm/-/issues/20
> > > > + */
> > > > +
> > > > +static void
> > > > +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)
> > > > +             return;
> > > > +
> > > > +     fd = open(glob_buf.gl_pathv[0], O_RDWR);
> > > > +
> > > > +     if (fd >= 0) {
> > > > +             char buf[0x1000];
> > > > +
> > > > +             /*
> > > > +              * We want to read the entire file but we can throw away the
> > > > +              * contents.. we just want to make sure that we exercise the
> > > > +              * kernel side codepaths hit when reading the devcore from
> > > > +              * sysfs
> > > > +              */
> > > > +             do {
> > > > +                     ret = igt_readn(fd, buf, sizeof(buf));
> > > > +             } while (ret > 0);
> > > > +
> > > > +             /* Clear the devcore: */
> > > > +             igt_writen(fd, "1", 1);
> > >
> > > Put here:
> > >                 fclose(fd);
> > >
> >
> > oh, right, good catch
> >
> > BR,
> > -R
> >
> > > Regards,
> > > Kamil
> > >
> > > > +     }
> > > > +
> > > > +     globfree(&glob_buf);
> > > > +}
> > > > +
> > > >  /*
> > > >   * Helpers for cmdstream packet building:
> > > >   */
> > > > @@ -102,6 +142,8 @@ do_hang_test(struct msm_pipe *pipe)
> > > >                       continue;
> > > >               igt_assert_eq(scratch[1+i], 2+i);
> > > >       }
> > > > +
> > > > +     read_and_clear_devcore();
> > > >  }
> > > >
> > > >  /*
> > > > --
> > > > 2.38.1
> > > >