Intel gpu-tool tests

[yanko-yankulov]

Hi all, @markjdb

I have been playing with the intel-gpu tests, trying to figure out some of the simpler issues. @markjdb really hope you don't mind me piggybacking on your work, just trying to help and liked the challenge. If you have already stumbled and fixed the things I will post, all the better and please ignore by ramblings :).

So 3 proposed patches for the moment.

First one is trivial - fix shmem_file_setup proto to avoid going in the negative values and crashing the kernel - triggered by gem_mmap if I am not mistaken

From e6e5aa9aa2d2b7bb5b262efee239dbb1d7ea9591 Mon Sep 17 00:00:00 2001
From: Yanko Yankulov <[email protected]>
Date: Mon, 13 Feb 2017 20:06:18 +0200
Subject: [PATCH 1/3] fix shmem_file_setup parameter types

Fixing a crash with huge gem mmaps
---
 sys/compat/linuxkpi/common/include/linux/fs.h | 2 +-
 sys/compat/linuxkpi/common/src/linux_fs.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sys/compat/linuxkpi/common/include/linux/fs.h b/sys/compat/linuxkpi/common/include/linux/fs.h
index f2592e90b3f..c4be5a9bd07 100644
--- a/sys/compat/linuxkpi/common/include/linux/fs.h
+++ b/sys/compat/linuxkpi/common/include/linux/fs.h
@@ -331,7 +331,7 @@ shmem_read_mapping_page(struct address_space *as, int idx)
        return (shmem_read_mapping_page_gfp(as, idx, 0));
 }
 
-extern struct linux_file *shmem_file_setup(char *name, int size, int flags);
+extern struct linux_file *shmem_file_setup(char *name, loff_t size, unsigned long flags);
 
 static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) {}
 static inline gfp_t mapping_gfp_mask(struct address_space *m)
diff --git a/sys/compat/linuxkpi/common/src/linux_fs.c b/sys/compat/linuxkpi/common/src/linux_fs.c
index 0d303e836c3..30b25bbc8fe 100644
--- a/sys/compat/linuxkpi/common/src/linux_fs.c
+++ b/sys/compat/linuxkpi/common/src/linux_fs.c
@@ -316,7 +316,7 @@ linux_get_new_vnode(void)
 }
 
 struct linux_file *
-shmem_file_setup(char *name, int size, int flags)
+shmem_file_setup(char *name, loff_t size, unsigned long flags)
 {
        struct linux_file *filp;
        struct vnode *vp;
-- 
2.11.0

Second - unfortunately I don't remember the test that triggered this, and we might need a more general solution, but good news is that simple fix as this actually works :)

commit 2d6d83bba04e7a90df1c74e05da1a156f8c8b476
Author: Yanko Yankulov <[email protected]>
Date:   Sun Feb 12 17:32:20 2017 +0200

    ioctl: translate ERESTARTSYS to ERESTART

diff --git a/sys/compat/linuxkpi/common/src/linux_compat.c b/sys/compat/linuxkpi/common/src/linux_compat.c
index 323929ef7f8..d7a3d3aa331 100644
--- a/sys/compat/linuxkpi/common/src/linux_compat.c
+++ b/sys/compat/linuxkpi/common/src/linux_compat.c
@@ -1056,6 +1056,9 @@ linux_dev_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag,
                current->bsd_ioctl_len = -1;
        }
        
+       if( error == ERESTARTSYS )
+               error = ERESTART;
+       
        return (error);
 }

The third one I have mentioned in #117, just adding it here for completeness.

commit 13382d4a45aa7e917c0bea1fe2ebf14c0b9171a6
Author: Yanko Yankulov <[email protected]>
Date:   Sat Feb 11 19:42:26 2017 +0200

    don't call vm_ops->open

diff --git a/sys/compat/linuxkpi/common/src/linux_compat.c b/sys/compat/linuxkpi/common/src/linux_compat.c
index d7a3d3aa331..8f6c093f0bd 100644
--- a/sys/compat/linuxkpi/common/src/linux_compat.c
+++ b/sys/compat/linuxkpi/common/src/linux_compat.c
@@ -820,7 +820,6 @@ linux_cdev_pager_ctor(void *handle, vm_ooffset_t size, vm_prot_t prot,
        vmap->vm_private_data = handle;
 
        *color = 0;
-       vmap->vm_ops->open(vmap);
        return (0);
 }
 

Currently I am tracing a panic (handle not found in linux_cdev_handle_find) reliably triggered by gem_fence_upload.

With 6a3c029 reverted the panic does not happen. It is really weird, but I believe I have some insight on what is causing it - The tests calls multiple mmaps/munmaps in parallel. Each thread has it's own gem object on which it operates. However a set of simple debug printfs shows that linux_cdev_handle_remove gets called from a thread different then the one that has created the handle, while the original thread is trying to use it, leading to a race in the creation/deletion. I am not 100% sure, but I am inclined to attribute this behavior to the deferred deallocation of vm_maps in vm_map_entry_delete.

So with the patch reverted, the test passes. I am just wondering if this is not hiding an unpleasant surprise further down the road?

Any comments and suggestions are appreciated,

Best,
Yanko

[yanko-yankulov]

So to answer my own question, reverting 6a3c029 did bite me. Without it gem_mmap_gtt panics with " Assertion fs->first_pindex <= pager_last failed at /storage/freebsd-graphics/freebsd-base-graphics/sys/vm/vm_fault.c:387". This is going to be fun ... :)

[markjdb]

Oh, by all means please report any issues you find with the GPU tests. I certainly haven't run them all.

6a3c029 is certainly needed; without it it's quite easy to panic the system. But it's not completely correct. Consider that we may mmap a GEM object twice. Without 6a3c029 we'd acquire two references on the cdev reference, but internally, both mappings are of the same VM object. So when both mappings are destroyed with munmap(), the VM object will only be destroyed upon the second unmap, and that would cause us to release only one reference on the cdev handle. Later if we mmap the same GEM object again, we may end up using a stale cdev handle.

So, the code still isn't right. Consider that the cdev handle contains info about the mapping, such as its size. But obviously this might differ between multiple mappings which share a handle, so a comparison by the handle pointer (the GEM object address) isn't sufficient. However, 6a3c029 fixes panics seen when using Xorg, so it provides some stability at least.

Your first two patches seem ok to me. Do you have them in a branch somewhere so that I can just push them tonight? I'll take a closer look at the third one.

[markjdb]

I should point out that the issue I described above isn't particularly easy to solve IMHO. The problem is that FreeBSD's VM hides the details of userland mappings from the driver layer, while Linux does not try to be abstract and passes everything right through. One might consider this a defect in FreeBSD, but the lack of abstractions leads to ugly complexity in the GPU drivers. For instance, i915 allows one to create GEM objects backed by malloc'ed memory; in the kernel, the userland address of that memory is needed in order to actually look up the backing pages. For this reason, i915 has to register so-called MMU notifiers to handle the possibility that the user process has munmap()ed the memory backing a GEM object. In FreeBSD, this wouldn't be necessary - the GEM object could be defined using a reference to the anonymous VM object backing the malloc'ed memory, and addressed using pindexes. Then userland can do whatever it wants with its mapping without disrupting the kernel.

Anyway. These divergences are quite difficult and in some cases impossible to solve in the LinuxKPI layer. Surgical changes to the drives themselves are needed in some cases.

[yanko-yankulov]

Hi @markjdb,

Thanks for all the info. I will go though it on fresh head tomorrow. But it definitely seems there will be some head-banging ahead. Will see if I will come up with a solution.

Regarding the patches - just pushed them - c766964. Yey my first github fork :).

Thanks

[yanko-yankulov]

Hi,

I believe I have v1 of a working solution (b7f5b8e). Both tests pass and I am currently using it on my desktop.

So basically I have restored the ref-counting of the linux_cdev_handles, but the refcount is now held by the pager. This solves the multiple mmaps case and the deferred pager destruction case that I hit initially.

I have also made two other changes (should probably split them in different commits)

1. Instead of holding the whole VMA struct in the handle, hold explicitly only the things that we need - vm_private_data, vm_pgoff, vm_ops and possibly the vm_file ( for the GEM case with shared objects the saved vm_file is zeroed for the moment, in hope that it will not be needed. If/when a case arises another extension will be needed ). The size of the VMA is calculated from the vm_obj, so with this change different mmaps with different sizes should work. I went through the fault handlers in the drm code, and couldn't find use for any of the other fields, but might have missed something.

2. I have also changed the vm_obj handle to be the lcdev_handle ( same uniqueness guarantee ), in hope to decrease the number of lookups of lcdev_handles and satisfy an aesthetic urge. It should be better unless calls to unmap_mapping_range() on not mapped objects are at least 2 times more frequent then combined mmap/fault/unmap + unmap_mapping_range() on actually mapped objects.

TODO: Redo the refcount with atomic ops, to eliminate some the exclusive locks I have added.

Looking forward to any comments when you got the time.

[yanko-yankulov]

And by 'both tests pass' I mean gem_mmap_gtt fails the same way as before the patch :)

[markjdb]

ACK. I haven't looked at this yet, but will soon.

[yanko-yankulov]

Hi @markjdb,

Thanks for spending the time.

I have pushed an updated version with decreased wlocks (075295b)

I have also pushed two independent relatively trivial commits - 65db404 and 3bcceb6, solving a witness complain/potential crash and an assert failure, so you might want to check them out too.

[yanko-yankulov]

More spam :)

I have rebased the patches on the current master.

a846ccf don't call vm_ops->open
9577ef9 rework linux_cdev_handler
32675cc move linux_set_current after WUNLOCK
0692384 sleep_wq can be NULL on finish_wait

The next set enables most of the kms* tests, that would hang before them - its a debugfs issue:
457ba7f enable __wait_event_interruptible_lock_irq
b53f1f8 debugfs_fill: ERESTARTSYS->ERESTART
706cd59 more ugly hacks to enable debugfs

The last one is really ugly, and I am a bit ashamed for really proposing it, but I can't come up with a better solution for the moment. The case is that some of the debugfs functions would not use seq_file but access the user pointer directly. Any ideas are welcome.

The last set for the moment was needed for kms_mmap_write_crc. With this the test pass, but I suspect the dmabuf mmap code may need some massaging ( cache mode for example )

35ec5e1 add dummy file struct to shmem_file
f9ac66d linux_dmabuf allow userland access
34f001e linux_dma_buf do mmap

Best,
Yanko

[yanko-yankulov]

Updated the commit ids in the prev comment. I have introduced a bug with the first implementation of the debugfs ugliness. The private_data of the debugfs file can be seq_file or simple_attr. I will need to document this somewhere, as it is not obvious.

Now fixed and tiny-bit decreased the ugliness of the patch :).

[yanko-yankulov]

The debugfs patch is still wrong and there is at least one debugfs file in i915 driver (i915_error_state) that does not like us overwriting the filp->private_data with a pointer to an sbuf.

I have implemented a test rework of the debugfs/seqfile/simple_attr to use the user pointer directly without going through an sbuf at all. Everything seems to work, but I don't know if there is some requirement that I am missing, that warranted the use of the sbufs in the first place. The one thing I could come up with is that we loose the ability to call debugfs fileops from the kernel, but this doesn't look like a needed feature.