[igt-dev] [PATCH v2 0/8] tests/panfrost: Misc fixes/improvements

[igt-dev] [PATCH v2 0/8] tests/panfrost: Misc fixes/improvements

[Boris Brezillon]

Hello,

This is a collection of fixes for the panfrost testsuite:
 * submit tests are now passing on Bifrost
 * the prime test is fixed to handle the case where the KMS device is
   assigned card1

There's also an extra test to make sure we don't crash the kernel
when closing the DRM FD while jobs are still in-flight.

Regards,

Boris

Boris Brezillon (8):
  tests/panfrost: Make sure we open a DUMB capable node for prime tests
  lib/panfrost: Handle the NULL case in igt_panfrost_free_bo()
  lib/panfrost: Add a helper to create a job loop
  lib/panfrost: Add a helper to create a NULL job
  tests/panfrost: Simplify submit tests
  tests/panfrost: Extend the pan-reset test
  lib/panfrost: Get rid of igt_panfrost_trivial_job()
  tests/panfrost: Test FD-close while jobs are still in-flight

 lib/igt_panfrost.c      | 197 ++++++++++++++++++----------------------
 lib/igt_panfrost.h      |   5 +
 tests/panfrost_prime.c  |  25 ++++-
 tests/panfrost_submit.c |  99 ++++++++++----------
 4 files changed, 162 insertions(+), 164 deletions(-)

-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 1/8] tests/panfrost: Make sure we open a DUMB capable node for prime tests

[Boris Brezillon]

v2:
* Use __drm_open_driver_another()

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 tests/panfrost_prime.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/tests/panfrost_prime.c b/tests/panfrost_prime.c
index 351d46f2f7e6..dbc741a9eacb 100644
--- a/tests/panfrost_prime.c
+++ b/tests/panfrost_prime.c
@@ -35,13 +35,33 @@
 #include <sys/ioctl.h>
 #include "panfrost_drm.h"
 
+static bool igt_has_dumb(int fd)
+{
+	uint64_t value = 0;
+	int ret;
+
+	ret = drmGetCap(fd, DRM_CAP_DUMB_BUFFER, &value);
+	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
+	return value == 1;
+}
+
+static bool igt_has_prime(int fd, uint64_t flags)
+{
+	uint64_t value = 0;
+	int ret;
+
+	ret = drmGetCap(fd, DRM_CAP_PRIME, &value);
+	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
+	return (value & flags) == flags;
+}
+
 igt_main
 {
 	int fd, kms_fd;
 
 	igt_fixture {
-		kms_fd = drm_open_driver_master(DRIVER_ANY);
 		fd = drm_open_driver(DRIVER_PANFROST);
+		kms_fd = __drm_open_driver_another(1, DRIVER_ANY);
 	}
 
 	igt_subtest("gem-prime-import") {
@@ -50,6 +70,9 @@ igt_main
 	        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
 		int dmabuf_fd;
 
+		igt_skip_on(!igt_has_dumb(kms_fd) ||
+			    !igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));
+
 		/* Just to be sure that when we import the dumb buffer it has
 		 * a non-NULL address.
 		 */
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 1/8] tests/panfrost: Make sure we open a DUMB capable node for prime tests

[Petri Latvala]

On Tue, Jun 22, 2021 at 11:02:14AM +0200, Boris Brezillon wrote:
> v2:
> * Use __drm_open_driver_another()
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  tests/panfrost_prime.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/panfrost_prime.c b/tests/panfrost_prime.c
> index 351d46f2f7e6..dbc741a9eacb 100644
> --- a/tests/panfrost_prime.c
> +++ b/tests/panfrost_prime.c
> @@ -35,13 +35,33 @@
>  #include <sys/ioctl.h>
>  #include "panfrost_drm.h"
>  
> +static bool igt_has_dumb(int fd)
> +{
> +	uint64_t value = 0;
> +	int ret;
> +
> +	ret = drmGetCap(fd, DRM_CAP_DUMB_BUFFER, &value);
> +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> +	return value == 1;
> +}
> +
> +static bool igt_has_prime(int fd, uint64_t flags)
> +{
> +	uint64_t value = 0;
> +	int ret;
> +
> +	ret = drmGetCap(fd, DRM_CAP_PRIME, &value);
> +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> +	return (value & flags) == flags;
> +}
> +
>  igt_main
>  {
>  	int fd, kms_fd;
>  
>  	igt_fixture {
> -		kms_fd = drm_open_driver_master(DRIVER_ANY);
>  		fd = drm_open_driver(DRIVER_PANFROST);
> +		kms_fd = __drm_open_driver_another(1, DRIVER_ANY);
>  	}
>  
>  	igt_subtest("gem-prime-import") {
> @@ -50,6 +70,9 @@ igt_main
>  	        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
>  		int dmabuf_fd;
>  
> +		igt_skip_on(!igt_has_dumb(kms_fd) ||
> +			    !igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));

For better logs in case any of these trigger, use separate clauses:

igt_require(igt_has_dumb(kms_fd));
igt_require(igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));


Either way, LGTM
Acked-by: Petri Latvala <petri.latvala@intel.com>

> +
>  		/* Just to be sure that when we import the dumb buffer it has
>  		 * a non-NULL address.
>  		 */
> -- 
> 2.31.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 1/8] tests/panfrost: Make sure we open a DUMB capable node for prime tests

[Boris Brezillon]

Hi Petri,

On Tue, 22 Jun 2021 12:42:01 +0300
Petri Latvala <petri.latvala@intel.com> wrote:

> On Tue, Jun 22, 2021 at 11:02:14AM +0200, Boris Brezillon wrote:
> > v2:
> > * Use __drm_open_driver_another()
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  tests/panfrost_prime.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/panfrost_prime.c b/tests/panfrost_prime.c
> > index 351d46f2f7e6..dbc741a9eacb 100644
> > --- a/tests/panfrost_prime.c
> > +++ b/tests/panfrost_prime.c
> > @@ -35,13 +35,33 @@
> >  #include <sys/ioctl.h>
> >  #include "panfrost_drm.h"
> >  
> > +static bool igt_has_dumb(int fd)
> > +{
> > +	uint64_t value = 0;
> > +	int ret;
> > +
> > +	ret = drmGetCap(fd, DRM_CAP_DUMB_BUFFER, &value);
> > +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> > +	return value == 1;
> > +}
> > +
> > +static bool igt_has_prime(int fd, uint64_t flags)
> > +{
> > +	uint64_t value = 0;
> > +	int ret;
> > +
> > +	ret = drmGetCap(fd, DRM_CAP_PRIME, &value);
> > +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> > +	return (value & flags) == flags;
> > +}
> > +
> >  igt_main
> >  {
> >  	int fd, kms_fd;
> >  
> >  	igt_fixture {
> > -		kms_fd = drm_open_driver_master(DRIVER_ANY);
> >  		fd = drm_open_driver(DRIVER_PANFROST);
> > +		kms_fd = __drm_open_driver_another(1, DRIVER_ANY);
> >  	}
> >  
> >  	igt_subtest("gem-prime-import") {
> > @@ -50,6 +70,9 @@ igt_main
> >  	        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
> >  		int dmabuf_fd;
> >  
> > +		igt_skip_on(!igt_has_dumb(kms_fd) ||
> > +			    !igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));  
> 
> For better logs in case any of these trigger, use separate clauses:
> 
> igt_require(igt_has_dumb(kms_fd));
> igt_require(igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));
> 
> 
> Either way, LGTM
> Acked-by: Petri Latvala <petri.latvala@intel.com>

Thanks for this ack. I haven't contributed to igt in a while, and I
don't remember the process to push patches once they've received
acks/reviews. Should I push directly to the igt/master on gitlab (in
which case I'd need to be added to the project), or should I let
someone else push things for me.

Regards,

Boris

> 
> > +
> >  		/* Just to be sure that when we import the dumb buffer it has
> >  		 * a non-NULL address.
> >  		 */
> > -- 
> > 2.31.1
> >   

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 1/8] tests/panfrost: Make sure we open a DUMB capable node for prime tests

[Petri Latvala]

On Thu, Jun 24, 2021 at 04:29:43PM +0200, Boris Brezillon wrote:
> Hi Petri,
> 
> On Tue, 22 Jun 2021 12:42:01 +0300
> Petri Latvala <petri.latvala@intel.com> wrote:
> 
> > On Tue, Jun 22, 2021 at 11:02:14AM +0200, Boris Brezillon wrote:
> > > v2:
> > > * Use __drm_open_driver_another()
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  tests/panfrost_prime.c | 25 ++++++++++++++++++++++++-
> > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/panfrost_prime.c b/tests/panfrost_prime.c
> > > index 351d46f2f7e6..dbc741a9eacb 100644
> > > --- a/tests/panfrost_prime.c
> > > +++ b/tests/panfrost_prime.c
> > > @@ -35,13 +35,33 @@
> > >  #include <sys/ioctl.h>
> > >  #include "panfrost_drm.h"
> > >  
> > > +static bool igt_has_dumb(int fd)
> > > +{
> > > +	uint64_t value = 0;
> > > +	int ret;
> > > +
> > > +	ret = drmGetCap(fd, DRM_CAP_DUMB_BUFFER, &value);
> > > +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> > > +	return value == 1;
> > > +}
> > > +
> > > +static bool igt_has_prime(int fd, uint64_t flags)
> > > +{
> > > +	uint64_t value = 0;
> > > +	int ret;
> > > +
> > > +	ret = drmGetCap(fd, DRM_CAP_PRIME, &value);
> > > +	igt_assert(ret == 0 || errno == EINVAL || errno == EOPNOTSUPP);
> > > +	return (value & flags) == flags;
> > > +}
> > > +
> > >  igt_main
> > >  {
> > >  	int fd, kms_fd;
> > >  
> > >  	igt_fixture {
> > > -		kms_fd = drm_open_driver_master(DRIVER_ANY);
> > >  		fd = drm_open_driver(DRIVER_PANFROST);
> > > +		kms_fd = __drm_open_driver_another(1, DRIVER_ANY);
> > >  	}
> > >  
> > >  	igt_subtest("gem-prime-import") {
> > > @@ -50,6 +70,9 @@ igt_main
> > >  	        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
> > >  		int dmabuf_fd;
> > >  
> > > +		igt_skip_on(!igt_has_dumb(kms_fd) ||
> > > +			    !igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));  
> > 
> > For better logs in case any of these trigger, use separate clauses:
> > 
> > igt_require(igt_has_dumb(kms_fd));
> > igt_require(igt_has_prime(kms_fd, DRM_PRIME_CAP_EXPORT));
> > 
> > 
> > Either way, LGTM
> > Acked-by: Petri Latvala <petri.latvala@intel.com>
> 
> Thanks for this ack. I haven't contributed to igt in a while, and I
> don't remember the process to push patches once they've received
> acks/reviews. Should I push directly to the igt/master on gitlab (in
> which case I'd need to be added to the project), or should I let
> someone else push things for me.

After acks/reviews if the CI doesn't give a fuzz you just push. You
have enough activity now to qualify for direct access so... well you
already have the access. Granted 2 days ago by Daniel Stone.


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 2/8] lib/panfrost: Handle the NULL case in igt_panfrost_free_bo()

[Boris Brezillon]

So we don't have to bother checking the value before calling this
function.

v2:
* No changes

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 lib/igt_panfrost.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/igt_panfrost.c b/lib/igt_panfrost.c
index 8b0c2b7743ea..4bbb5ddf4076 100644
--- a/lib/igt_panfrost.c
+++ b/lib/igt_panfrost.c
@@ -72,6 +72,9 @@ igt_panfrost_gem_new(int fd, size_t size)
 void
 igt_panfrost_free_bo(int fd, struct panfrost_bo *bo)
 {
+        if (!bo)
+                return;
+
         if (bo->map)
                 munmap(bo->map, bo->size);
         gem_close(fd, bo->handle);
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 3/8] lib/panfrost: Add a helper to create a job loop

[Boris Brezillon]

Useful to trigger job timeouts and test the kernel driver timeout
handling logic.

v2:
* Add comments explaining how the job loop is formed
* Add a helper to retrieve job headers

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 lib/igt_panfrost.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_panfrost.h |  4 +++
 2 files changed, 81 insertions(+)

diff --git a/lib/igt_panfrost.c b/lib/igt_panfrost.c
index 4bbb5ddf4076..5b65537b7ced 100644
--- a/lib/igt_panfrost.c
+++ b/lib/igt_panfrost.c
@@ -130,6 +130,83 @@ void igt_panfrost_bo_mmap(int fd, struct panfrost_bo *bo)
         igt_assert(bo->map);
 }
 
+struct mali_job_descriptor_header *
+igt_panfrost_job_loop_get_job_header(struct panfrost_submit *submit,
+                                     unsigned job_idx)
+{
+        unsigned job_offset = ALIGN(sizeof(struct mali_job_descriptor_header) +
+                                    sizeof(struct mali_payload_set_value),
+                                    64) *
+                              job_idx;
+
+        igt_assert(job_idx <= 1);
+
+        return submit->submit_bo->map + job_offset;
+}
+
+struct panfrost_submit *igt_panfrost_job_loop(int fd)
+{
+        /* We create 2 WRITE_VALUE jobs pointing to each other to form a loop.
+         * Each WRITE_VALUE job resets the ->exception_status field of the
+         * other job to allow re-execution (if we don't do that we end up with
+         * an INVALID_DATA fault on the second execution).
+         */
+        struct panfrost_submit *submit;
+        struct mali_job_descriptor_header header = {
+                .job_type = JOB_TYPE_SET_VALUE,
+                .job_barrier = 1,
+                .unknown_flags = 5,
+                .job_index = 1,
+                .job_descriptor_size = 1,
+        };
+
+        /* .unknow = 3 means write 0 at the address specified in .out */
+        struct mali_payload_set_value payload = {
+                .unknown = 3,
+        };
+        uint32_t *bos;
+        unsigned job1_offset = ALIGN(sizeof(header) + sizeof(payload), 64);
+        unsigned job0_offset = 0;
+
+        submit = malloc(sizeof(*submit));
+	memset(submit, 0, sizeof(*submit));
+
+        submit->submit_bo = igt_panfrost_gem_new(fd, ALIGN(sizeof(header) + sizeof(payload), 64) * 2);
+        igt_panfrost_bo_mmap(fd, submit->submit_bo);
+
+        /* Job 0 points to job 1 and has its WRITE_VALUE pointer pointing to
+         * job 1 execption_status field.
+         */
+        header.next_job_64 = submit->submit_bo->offset + job1_offset;
+        payload.out = submit->submit_bo->offset + job1_offset +
+                      offsetof(struct mali_job_descriptor_header, exception_status);
+        memcpy(submit->submit_bo->map + job0_offset, &header, sizeof(header));
+        memcpy(submit->submit_bo->map + job0_offset + sizeof(header), &payload, sizeof(payload));
+
+        /* Job 1 points to job 0 and has its WRITE_VALUE pointer pointing to
+         * job 0 execption_status field.
+         */
+        header.next_job_64 = submit->submit_bo->offset + job0_offset;
+        payload.out = submit->submit_bo->offset + job0_offset +
+                      offsetof(struct mali_job_descriptor_header, exception_status);
+        memcpy(submit->submit_bo->map + job1_offset, &header, sizeof(header));
+        memcpy(submit->submit_bo->map + job1_offset + sizeof(header), &payload, sizeof(payload));
+
+        submit->args = malloc(sizeof(*submit->args));
+        memset(submit->args, 0, sizeof(*submit->args));
+        submit->args->jc = submit->submit_bo->offset;
+
+        bos = malloc(sizeof(*bos) * 1);
+        bos[0] = submit->submit_bo->handle;
+
+        submit->args->bo_handles = to_user_pointer(bos);
+        submit->args->bo_handle_count = 1;
+
+        igt_assert_eq(drmSyncobjCreate(fd, DRM_SYNCOBJ_CREATE_SIGNALED, &submit->args->out_sync), 0);
+
+        return submit;
+}
+
 struct panfrost_submit *igt_panfrost_trivial_job(int fd, bool do_crash, int width, int height, uint32_t color)
 {
         struct panfrost_submit *submit;
diff --git a/lib/igt_panfrost.h b/lib/igt_panfrost.h
index cc7998dcb4bf..3ab4baf892f3 100644
--- a/lib/igt_panfrost.h
+++ b/lib/igt_panfrost.h
@@ -47,7 +47,11 @@ struct panfrost_submit {
 struct panfrost_bo *igt_panfrost_gem_new(int fd, size_t size);
 void igt_panfrost_free_bo(int fd, struct panfrost_bo *bo);
 
+struct mali_job_descriptor_header *
+igt_panfrost_job_loop_get_job_header(struct panfrost_submit *submit,
+                                     unsigned job_idx);
 struct panfrost_submit *igt_panfrost_trivial_job(int fd, bool do_crash, int width, int height, uint32_t color);
+struct panfrost_submit *igt_panfrost_job_loop(int fd);
 void igt_panfrost_free_job(int fd, struct panfrost_submit *submit);
 
 /* IOCTL wrappers */
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 4/8] lib/panfrost: Add a helper to create a NULL job

[Boris Brezillon]

Useful when we just want to make sure the scheduler has executed a
job (way simpler than creating actual jobs).

v2:
* No changes

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 lib/igt_panfrost.c | 33 +++++++++++++++++++++++++++++++++
 lib/igt_panfrost.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/lib/igt_panfrost.c b/lib/igt_panfrost.c
index 5b65537b7ced..1026454f7cab 100644
--- a/lib/igt_panfrost.c
+++ b/lib/igt_panfrost.c
@@ -207,6 +207,39 @@ struct panfrost_submit *igt_panfrost_job_loop(int fd)
         return submit;
 }
 
+struct panfrost_submit *igt_panfrost_null_job(int fd)
+{
+        struct panfrost_submit *submit;
+        struct mali_job_descriptor_header header = {
+                .job_type = JOB_TYPE_NULL,
+                .job_index = 1,
+                .job_descriptor_size = 1,
+        };
+        uint32_t *bos;
+
+        submit = malloc(sizeof(*submit));
+	memset(submit, 0, sizeof(*submit));
+
+        submit->submit_bo = igt_panfrost_gem_new(fd, sizeof(header));
+        igt_panfrost_bo_mmap(fd, submit->submit_bo);
+
+        memcpy(submit->submit_bo->map, &header, sizeof(header));
+
+        submit->args = malloc(sizeof(*submit->args));
+        memset(submit->args, 0, sizeof(*submit->args));
+        submit->args->jc = submit->submit_bo->offset;
+
+        bos = malloc(sizeof(*bos) * 1);
+        bos[0] = submit->submit_bo->handle;
+
+        submit->args->bo_handles = to_user_pointer(bos);
+        submit->args->bo_handle_count = 1;
+
+        igt_assert_eq(drmSyncobjCreate(fd, DRM_SYNCOBJ_CREATE_SIGNALED, &submit->args->out_sync), 0);
+
+        return submit;
+}
+
 struct panfrost_submit *igt_panfrost_trivial_job(int fd, bool do_crash, int width, int height, uint32_t color)
 {
         struct panfrost_submit *submit;
diff --git a/lib/igt_panfrost.h b/lib/igt_panfrost.h
index 3ab4baf892f3..3f69bcbefb75 100644
--- a/lib/igt_panfrost.h
+++ b/lib/igt_panfrost.h
@@ -52,6 +52,7 @@ igt_panfrost_job_loop_get_job_header(struct panfrost_submit *submit,
                                      unsigned job_idx);
 struct panfrost_submit *igt_panfrost_trivial_job(int fd, bool do_crash, int width, int height, uint32_t color);
 struct panfrost_submit *igt_panfrost_job_loop(int fd);
+struct panfrost_submit *igt_panfrost_null_job(int fd);
 void igt_panfrost_free_job(int fd, struct panfrost_submit *submit);
 
 /* IOCTL wrappers */
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 5/8] tests/panfrost: Simplify submit tests

[Boris Brezillon]

Use NULL jobs and WRITE_VALUE job loops to test job submission. This
way we get tests that pass on both Midgard and Bifrost without pulling
the pan_pack() infrastructure from mesa. Note that we'll have to do that
at some point if we want to add complex tests, but we're not there yet.

v2:
* Move of the pan-reset changes out of this commit

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 tests/panfrost_submit.c | 71 ++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 47 deletions(-)

diff --git a/tests/panfrost_submit.c b/tests/panfrost_submit.c
index 13ce85b73d9e..abc274921f2b 100644
--- a/tests/panfrost_submit.c
+++ b/tests/panfrost_submit.c
@@ -57,30 +57,9 @@ abs_timeout(uint64_t duration)
         return (uint64_t)current.tv_sec * NSECS_PER_SEC + current.tv_nsec + duration;
 }
 
-static void check_error(int fd, struct panfrost_submit *submit)
+static void check_done(struct mali_job_descriptor_header *header)
 {
-	struct mali_job_descriptor_header *header;
-
-        header = submit->submit_bo->map;
-        igt_assert_eq_u64(header->fault_pointer, 0);
-}
-
-static void check_fb(int fd, struct panfrost_bo *bo)
-{
-        int gpu_prod_id = igt_panfrost_get_param(fd, DRM_PANFROST_PARAM_GPU_PROD_ID);
-        __uint32_t *fbo;
-        int i;
-
-        fbo = bo->map;
-
-        if (gpu_prod_id >= 0x0750) {
-                for (i = 0; i < ALIGN(WIDTH, 16) * HEIGHT; i++)
-                        igt_assert_eq_u32(fbo[i], CLEAR_COLOR);
-        } else {
-                // Mask the alpha away because on <=T720 we don't know how to have it
-                for (i = 0; i < ALIGN(WIDTH, 16) * HEIGHT; i++)
-                        igt_assert_eq_u32(fbo[i], CLEAR_COLOR & 0x00ffffff);
-        }
+        igt_assert(header->exception_status == 1 && header->fault_pointer == 0);
 }
 
 igt_main
@@ -94,15 +73,12 @@ igt_main
         igt_subtest("pan-submit") {
                 struct panfrost_submit *submit;
 
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_null_job(fd);
 
-                igt_panfrost_bo_mmap(fd, submit->fbo);
                 do_ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args);
                 igt_assert(syncobj_wait(fd, &submit->args->out_sync, 1,
                                         abs_timeout(SHORT_TIME_NSEC), 0, NULL));
-                check_error(fd, submit);
-                check_fb(fd, submit->fbo);
+                check_done(submit->submit_bo->map);
                 igt_panfrost_free_job(fd, submit);
         }
 
@@ -114,64 +90,65 @@ igt_main
         igt_subtest("pan-submit-error-bad-in-syncs") {
                 struct panfrost_submit *submit;
 
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_null_job(fd);
                 submit->args->in_syncs = 0ULL;
                 submit->args->in_sync_count = 1;
 
                 do_ioctl_err(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args, EFAULT);
+                igt_panfrost_free_job(fd, submit);
         }
 
         igt_subtest("pan-submit-error-bad-bo-handles") {
                 struct panfrost_submit *submit;
 
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_null_job(fd);
                 submit->args->bo_handles = 0ULL;
                 submit->args->bo_handle_count = 1;
 
                 do_ioctl_err(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args, EFAULT);
+                igt_panfrost_free_job(fd, submit);
         }
 
         igt_subtest("pan-submit-error-bad-requirements") {
                 struct panfrost_submit *submit;
 
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_null_job(fd);
                 submit->args->requirements = 2;
 
                 do_ioctl_err(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args, EINVAL);
+                igt_panfrost_free_job(fd, submit);
         }
 
         igt_subtest("pan-submit-error-bad-out-sync") {
                 struct panfrost_submit *submit;
 
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_null_job(fd);
                 submit->args->out_sync = -1;
 
                 do_ioctl_err(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args, ENODEV);
+                igt_panfrost_free_job(fd, submit);
         }
 
         igt_subtest("pan-reset") {
                 struct panfrost_submit *submit;
+                struct mali_job_descriptor_header *headers[2];
 
-                submit = igt_panfrost_trivial_job(fd, true, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
+                submit = igt_panfrost_job_loop(fd);
+                headers[0] = igt_panfrost_job_loop_get_job_header(submit, 0);
+                headers[1] = igt_panfrost_job_loop_get_job_header(submit, 1);
                 do_ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args);
-                /* Expect for this job to timeout */
+
+                /* The job should stay active for BAD_JOB_TIME_NSEC until the
+                 * scheduler consider it as a GPU hang and reset the GPU.
+                 * After the reset, the job fence is signaled.
+                 */
                 igt_assert(!syncobj_wait(fd, &submit->args->out_sync, 1,
                                          abs_timeout(SHORT_TIME_NSEC), 0, NULL));
-                igt_panfrost_free_job(fd, submit);
-
-                submit = igt_panfrost_trivial_job(fd, false, WIDTH, HEIGHT,
-                                                  CLEAR_COLOR);
-                igt_panfrost_bo_mmap(fd, submit->fbo);
-                do_ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args);
-                /* This one should work */
                 igt_assert(syncobj_wait(fd, &submit->args->out_sync, 1,
                                         abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL));
-                check_fb(fd, submit->fbo);
+
+                /* At least one job header of the job loop should have its exception status set to 0 */
+                igt_assert(headers[0]->exception_status != 1 || headers[1]->exception_status != 1);
                 igt_panfrost_free_job(fd, submit);
         }
 
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 6/8] tests/panfrost: Extend the pan-reset test

[Boris Brezillon]

Extend the pan-reset test to make sure jobs from other context get
resumed after a reset.

v2:
* New patch

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 tests/panfrost_submit.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/tests/panfrost_submit.c b/tests/panfrost_submit.c
index abc274921f2b..6c47d1f2d3e9 100644
--- a/tests/panfrost_submit.c
+++ b/tests/panfrost_submit.c
@@ -130,26 +130,31 @@ igt_main
         }
 
         igt_subtest("pan-reset") {
-                struct panfrost_submit *submit;
-                struct mali_job_descriptor_header *headers[2];
+                int tmpfd = drm_open_driver(DRIVER_PANFROST);
+                struct panfrost_submit *submit[2];
+                struct mali_job_descriptor_header *headers[3];
 
-                submit = igt_panfrost_job_loop(fd);
-                headers[0] = igt_panfrost_job_loop_get_job_header(submit, 0);
-                headers[1] = igt_panfrost_job_loop_get_job_header(submit, 1);
-                do_ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, submit->args);
-
-                /* The job should stay active for BAD_JOB_TIME_NSEC until the
-                 * scheduler consider it as a GPU hang and reset the GPU.
-                 * After the reset, the job fence is signaled.
-                 */
-                igt_assert(!syncobj_wait(fd, &submit->args->out_sync, 1,
+                submit[0] = igt_panfrost_job_loop(fd);
+                submit[1] = igt_panfrost_null_job(tmpfd);
+                headers[0] = igt_panfrost_job_loop_get_job_header(submit[0], 0);
+                headers[1] = igt_panfrost_job_loop_get_job_header(submit[0], 1);
+                headers[2] = submit[1]->submit_bo->map;
+                do_ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, submit[0]->args);
+                do_ioctl(tmpfd, DRM_IOCTL_PANFROST_SUBMIT, submit[1]->args);
+                /* First job should timeout, second job should complete right after the timeout */
+                igt_assert(!syncobj_wait(fd, &submit[0]->args->out_sync, 1,
                                          abs_timeout(SHORT_TIME_NSEC), 0, NULL));
-                igt_assert(syncobj_wait(fd, &submit->args->out_sync, 1,
+                igt_assert(syncobj_wait(fd, &submit[0]->args->out_sync, 1,
                                         abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL));
+                igt_assert(syncobj_wait(tmpfd, &submit[1]->args->out_sync, 1,
+                                        abs_timeout(SHORT_TIME_NSEC), 0, NULL));
 
                 /* At least one job header of the job loop should have its exception status set to 0 */
                 igt_assert(headers[0]->exception_status != 1 || headers[1]->exception_status != 1);
-                igt_panfrost_free_job(fd, submit);
+                check_done(headers[2]);
+                igt_panfrost_free_job(fd, submit[0]);
+                igt_panfrost_free_job(tmpfd, submit[1]);
+                close(tmpfd);
         }
 
         igt_fixture {
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 7/8] lib/panfrost: Get rid of igt_panfrost_trivial_job()

[Boris Brezillon]

We no longer use fragment jobs to test job submission, let's get rid
of igt_panfrost_trivial_job(). If we ever need to issue fragment
jobs again, we'll probably have to pull the pan_pack() infra from
mesa and make the helper deal with Bifrost specificities.

v2:
* Add R-b

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
---
 lib/igt_panfrost.c | 136 ---------------------------------------------
 1 file changed, 136 deletions(-)

diff --git a/lib/igt_panfrost.c b/lib/igt_panfrost.c
index 1026454f7cab..7651f0f6c8a3 100644
--- a/lib/igt_panfrost.c
+++ b/lib/igt_panfrost.c
@@ -240,142 +240,6 @@ struct panfrost_submit *igt_panfrost_null_job(int fd)
         return submit;
 }
 
-struct panfrost_submit *igt_panfrost_trivial_job(int fd, bool do_crash, int width, int height, uint32_t color)
-{
-        struct panfrost_submit *submit;
-        struct mali_job_descriptor_header header = {
-                .job_type = JOB_TYPE_FRAGMENT,
-                .job_index = 1,
-                .job_descriptor_size = 1,
-        };
-        struct mali_payload_fragment payload = {
-                .min_tile_coord = MALI_COORDINATE_TO_TILE_MIN(0, 0),
-                .max_tile_coord = MALI_COORDINATE_TO_TILE_MAX(ALIGN(width, 16), height),
-        };
-        struct bifrost_framebuffer mfbd_framebuffer = {
-            .unk0 = 0x0,
-            .unknown1 = 0x0,
-            .tiler_meta = 0xff00000000,
-            .width1 = MALI_POSITIVE(ALIGN(width, 16)),
-            .height1 = MALI_POSITIVE(height),
-            .width2 = MALI_POSITIVE(ALIGN(width, 16)),
-            .height2 = MALI_POSITIVE(height),
-            .unk1 = 0x1080,
-            .unk2 = 0x0,
-            .rt_count_1 = MALI_POSITIVE(1),
-            .rt_count_2 = 1,
-            .unk3 = 0x100,
-            .clear_stencil = 0x0,
-            .clear_depth = 0.000000,
-            .unknown2 = 0x1f,
-        };
-        struct mali_single_framebuffer sfbd_framebuffer = {
-            .unknown2 = 0x1f,
-            .width = MALI_POSITIVE(width),
-            .height = MALI_POSITIVE(height),
-            .stride = width * 4,
-            .resolution_check = ((width + height) / 3) << 4,
-            .tiler_flags = 0xfff,
-            .clear_color_1 = color,
-            .clear_color_2 = color,
-            .clear_color_3 = color,
-            .clear_color_4 = color,
-            .clear_flags = 0x101100 | MALI_CLEAR_SLOW,
-            .format = 0xb84e0281,
-        };
-        struct mali_rt_format fmt = {
-                .unk1 = 0x4000000,
-                .unk2 = 0x1,
-                .nr_channels = MALI_POSITIVE(4),
-                .flags = do_crash ? 0x444 | (1 << 8) : 0x444,
-                .swizzle = MALI_CHANNEL_BLUE | (MALI_CHANNEL_GREEN << 3) | (MALI_CHANNEL_RED << 6) | (MALI_CHANNEL_ONE << 9),
-                .unk4 = 0x8,
-        };
-        struct bifrost_render_target rts = {
-                .format = fmt,
-                .chunknown = {
-                    .unk = 0x0,
-                    .pointer = 0x0,
-                },
-                .framebuffer_stride = ALIGN(width, 16) * 4 / 16,
-                .clear_color_1 = color,
-                .clear_color_2 = color,
-                .clear_color_3 = color,
-                .clear_color_4 = color,
-        };
-        int gpu_prod_id = igt_panfrost_get_param(fd, DRM_PANFROST_PARAM_GPU_PROD_ID);
-        uint32_t *known_unknown;
-        uint32_t *bos;
-
-        submit = malloc(sizeof(*submit));
-
-        submit->fbo = igt_panfrost_gem_new(fd, ALIGN(width, 16) * height * 4);
-        rts.framebuffer = submit->fbo->offset;
-        sfbd_framebuffer.framebuffer = submit->fbo->offset;
-
-        submit->tiler_heap_bo = igt_panfrost_gem_new(fd, 32768 * 128);
-        mfbd_framebuffer.tiler_heap_start = submit->tiler_heap_bo->offset;
-        mfbd_framebuffer.tiler_heap_end = submit->tiler_heap_bo->offset + 32768 * 128;
-        sfbd_framebuffer.tiler_heap_free = mfbd_framebuffer.tiler_heap_start;
-        sfbd_framebuffer.tiler_heap_end = mfbd_framebuffer.tiler_heap_end;
-
-        submit->tiler_scratch_bo = igt_panfrost_gem_new(fd, 128 * 128 * 128);
-        mfbd_framebuffer.tiler_scratch_start = submit->tiler_scratch_bo->offset;
-        mfbd_framebuffer.tiler_scratch_middle = submit->tiler_scratch_bo->offset + 0xf0000;
-        sfbd_framebuffer.unknown_address_0 = mfbd_framebuffer.tiler_scratch_start;
-
-        submit->scratchpad_bo = igt_panfrost_gem_new(fd, 64 * 4096);
-        igt_panfrost_bo_mmap(fd, submit->scratchpad_bo);
-        mfbd_framebuffer.scratchpad = submit->scratchpad_bo->offset;
-        sfbd_framebuffer.unknown_address_1 = submit->scratchpad_bo->offset;
-        sfbd_framebuffer.unknown_address_2 = submit->scratchpad_bo->offset + 512;
-
-        known_unknown = ((void*)submit->scratchpad_bo->map) + 512;
-        *known_unknown = 0xa0000000;
-
-        if (gpu_prod_id >= 0x0750) {
-            submit->fb_bo = igt_panfrost_gem_new(fd, sizeof(mfbd_framebuffer) + sizeof(struct bifrost_render_target));
-            igt_panfrost_bo_mmap(fd, submit->fb_bo);
-            memcpy(submit->fb_bo->map, &mfbd_framebuffer, sizeof(mfbd_framebuffer));
-            memcpy(submit->fb_bo->map + sizeof(mfbd_framebuffer), &rts, sizeof(struct bifrost_render_target));
-            payload.framebuffer = submit->fb_bo->offset | MALI_MFBD;
-        } else {
-            // We don't know yet how to cause a hang on <=T720
-            // Should probably use an infinite loop to hang the GPU
-            igt_require(!do_crash);
-            submit->fb_bo = igt_panfrost_gem_new(fd, sizeof(sfbd_framebuffer));
-            igt_panfrost_bo_mmap(fd, submit->fb_bo);
-            memcpy(submit->fb_bo->map, &sfbd_framebuffer, sizeof(sfbd_framebuffer));
-            payload.framebuffer = submit->fb_bo->offset | MALI_SFBD;
-        }
-
-        submit->submit_bo = igt_panfrost_gem_new(fd, sizeof(header) + sizeof(payload) + 1024000);
-        igt_panfrost_bo_mmap(fd, submit->submit_bo);
-
-        memcpy(submit->submit_bo->map, &header, sizeof(header));
-        memcpy(submit->submit_bo->map + sizeof(header), &payload, sizeof(payload));
-
-        submit->args = malloc(sizeof(*submit->args));
-        memset(submit->args, 0, sizeof(*submit->args));
-        submit->args->jc = submit->submit_bo->offset;
-        submit->args->requirements = PANFROST_JD_REQ_FS;
-
-        bos = malloc(sizeof(*bos) * 6);
-        bos[0] = submit->fbo->handle;
-        bos[1] = submit->tiler_heap_bo->handle;
-        bos[2] = submit->tiler_scratch_bo->handle;
-        bos[3] = submit->scratchpad_bo->handle;
-        bos[4] = submit->fb_bo->handle;
-        bos[5] = submit->submit_bo->handle;
-
-        submit->args->bo_handles = to_user_pointer(bos);
-        submit->args->bo_handle_count = 6;
-
-        igt_assert_eq(drmSyncobjCreate(fd, DRM_SYNCOBJ_CREATE_SIGNALED, &submit->args->out_sync), 0);
-
-        return submit;
-}
-
 void igt_panfrost_free_job(int fd, struct panfrost_submit *submit)
 {
         free(from_user_pointer(submit->args->bo_handles));
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v2 8/8] tests/panfrost: Test FD-close while jobs are still in-flight

[Boris Brezillon]

We had use-after-free faults in the past because the MMU context
while released while jobs were still in flight, and the job cleanup
path assumed the context was still present. Add a test to make sure this
won't happen again.

v2:
* Add R-b

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
---
 tests/panfrost_submit.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/panfrost_submit.c b/tests/panfrost_submit.c
index 6c47d1f2d3e9..070d78625ebf 100644
--- a/tests/panfrost_submit.c
+++ b/tests/panfrost_submit.c
@@ -157,6 +157,17 @@ igt_main
                 close(tmpfd);
         }
 
+        igt_subtest("pan-submit-and-close") {
+                /* We need our own FD because we close it right after the job submission */
+                int tmpfd = drm_open_driver(DRIVER_PANFROST);
+                struct panfrost_submit *submit;
+
+                submit = igt_panfrost_job_loop(tmpfd);
+                do_ioctl(tmpfd, DRM_IOCTL_PANFROST_SUBMIT, submit->args);
+                igt_panfrost_free_job(tmpfd, submit);
+                close(tmpfd);
+        }
+
         igt_fixture {
                 close(fd);
         }
-- 
2.31.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v2 0/8] tests/panfrost: Misc fixes/improvements

[Alyssa Rosenzweig]

Series is r-b me, thank you!

On Tue, Jun 22, 2021 at 11:02:13AM +0200, Boris Brezillon wrote:
> Hello,
> 
> This is a collection of fixes for the panfrost testsuite:
>  * submit tests are now passing on Bifrost
>  * the prime test is fixed to handle the case where the KMS device is
>    assigned card1
> 
> There's also an extra test to make sure we don't crash the kernel
> when closing the DRM FD while jobs are still in-flight.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (8):
>   tests/panfrost: Make sure we open a DUMB capable node for prime tests
>   lib/panfrost: Handle the NULL case in igt_panfrost_free_bo()
>   lib/panfrost: Add a helper to create a job loop
>   lib/panfrost: Add a helper to create a NULL job
>   tests/panfrost: Simplify submit tests
>   tests/panfrost: Extend the pan-reset test
>   lib/panfrost: Get rid of igt_panfrost_trivial_job()
>   tests/panfrost: Test FD-close while jobs are still in-flight
> 
>  lib/igt_panfrost.c      | 197 ++++++++++++++++++----------------------
>  lib/igt_panfrost.h      |   5 +
>  tests/panfrost_prime.c  |  25 ++++-
>  tests/panfrost_submit.c |  99 ++++++++++----------
>  4 files changed, 162 insertions(+), 164 deletions(-)
> 
> -- 
> 2.31.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev