FreeBSDDesktop / kms-drm

the DRM part of the linuxkpi-based KMS
63 stars 26 forks source link

Fix I915_USERPTR_UNSYNCHRONIZED for Vulkan on old Haswell cards #205

Closed amshafer closed 4 years ago

amshafer commented 4 years ago

This lets the unsynchronized DRM_IOCTL_I915_GEM_USERPTR hack work in mesa on 7th generation Haswell graphics. This was needed to get Vulkan working on the Toshiba Satellite L55t-A5232 with current.

The userptr ioctl isn't needed anymore thanks to the softpin feature in mesa 19.0 and later. The issue is that softpin requires hardware of the 8th generation or later, along with support for 48-bit addresses. The old and terrible Toshiba laptop I am testing on does not have either of these things, so this patch was required.

Please let me know what you all think about this, and if it needs improvement. I'm happy to provide any details or testing.

i915_gem_userptr_get_pages will allocate and pin the number of pages requested. If this does not immediately pin all of the pages it will defer to __i915_gem_userptr_get_pages_worker to complete the request. A section of __i915_gem_userptr_get_pages_worker was halfway ifdef-ed out, but almost all of the linuxkpi functions were implemented. That section is in charge of pinning the remaining pages and without it the ioctl fails.

Questions

The only linuxkpi function not implemented was mmget_not_zero, which is supposed to be in linux's mm.h. I added it above __i915_gem_userptr_get_pages_worker, as that is the only function that uses it. Maybe it would be better included in the main linuxkpi? Please let me know where it best belongs. If it needs to be committed to current I have a tiny patch to do so laying around.

get_user_pages_remote seemed to have an inconsistent number of arguments. The drm code has the number used by current versions of Linux, but the linuxkpi doesn't expect the last argument. Maybe this needs to be updated in the linuxkpi? I'm happy to do so if required.

jbeich commented 4 years ago

This also fixes I915_USERPTR_UNSYNCHRONIZED on Skylake as used by lang/intel-compute-runtime, multimedia/libva-intel-media-driver (QSV), x11-drivers/xf86-video-intel (SNA).

valpackett commented 4 years ago

Nice!

Maybe it would be better included in the main linuxkpi?

Since it's a trivial function that just calls a different one, maybe it doesn't count as "GPL licensed"… But we do have linuxkpi_gplv2 here in this repo for code copied from Linux, it would be easy and safe to put it there.

The drm code has the number used by current versions of Linux, but the linuxkpi doesn't expect the last argument.

Probably should be updated, especially if there are no other users of this. /cc @hselasky

hselasky commented 4 years ago

Sounds OK. Please make a patch!

hselasky commented 4 years ago

Sorry I see the patch now. Will submit this upstream first, then we need to wait for MFC, and then you can patch kms! Thank you!

hselasky commented 4 years ago

Here, try this patch: https://svnweb.freebsd.org/changeset/base/357077 And wait one week. And this should be ready to go in. Please also update the patch and commit message using the function from the LinuxKPI.

amshafer commented 4 years ago

Thanks guys! Updated the diff and I'll do some more testing this week before this gets committed.

This mmget_not_zero is pretty much exactly identical to the Linux one, but there's really no other way to write that. I assume that's not a problem.

hselasky commented 4 years ago

It is fine. It's just a refcounting mechanism.

valpackett commented 4 years ago

@hselasky so what about the extra arg in get_user_pages_remote?

hselasky commented 4 years ago

Let's leave that API AS-IS for now. It is NULL anyway - right?

amshafer commented 4 years ago

Yup it is NULL.

After a week, I grabbed the latest current and tested this again and had no issues. Anything else needed from me before this is good to go?

hselasky commented 4 years ago

Just let me MFC that kernel patch to 12 and 11 first. I'll do it later today.

hselasky commented 4 years ago

Here you go: https://svnweb.freebsd.org/changeset/base/357435 https://svnweb.freebsd.org/changeset/base/357436

hselasky commented 4 years ago

Can this patch be applied to other branches aswell?

amshafer commented 4 years ago

Thanks! It looks like it can to drm-v4.16 and drm-v4.16-fbsd12.0, but I can test that if you want?

hselasky commented 4 years ago

Yes, please test!

amshafer commented 4 years ago

drm 4.16 works fine, tested with vulkan direct to display. The first time I ran it I saw a syncobj time out, but I ran 20 or 30 times after that and couldn't get it to happen again. Not sure why that happened.

I realized that I am running CURRENT on my only laptop with intel graphics. Is there a way I can test drm-v4.16-fbsd12.0 and drm-v5.0-fbsd12.1 without having to rollback to 12.0 and 12.1? I get compilation errors atm. I could try bhyve passthrough or something but I'm not sure if that works.

The commits are in my tree: drm-v4.16 commit drm-v4.16-fbsd12.0 commit drm-v5.0-fbsd12.1 commit

hselasky commented 4 years ago

Hi, If you have 12/11 user-space, you can boot any newer kernel.

--HPS

hselasky commented 4 years ago

Just make two new pull requests and add me.

amshafer commented 4 years ago

Created one for drm-v4.16. This PR and #207 should both be ready to be merged.

I saw the mailing list post about how mmget_not_zero is not available on releases, so I'm thinking the best course of action for those is to add mmget_not_zero above __i915_gem_userptr_get_pages_worker. Hopefully I can get that tested in drm-v4.16-fbsd12.0 and drm-v5.0-fbsd12.1 this afternoon. I can also take a look at drm-v4.11 since that was mentioned too.

hselasky commented 4 years ago

The code should be wrapped with something like :

if __FreeBSD_version >= xxxx && __FreeBSD_version < yyyy

I think. Else it won't build on 11-stable.

Just check the definition in sys/sys/param.h in -stable . The release branches use 000 in the end.

amshafer commented 4 years ago

So how should I choose a __FreeBSD_version to use? I'm a bit unfamiliar with it having only now read that section of the porter's handbook. Should yyyy be a version number bumped because of this change or should I just use the latest __FreeBSD_version?

hselasky commented 4 years ago

@jbeich : I can bump those version numbers -current, -11 and -12 to indicated the LinuxKPI change and give a list here? What do you think?

hselasky commented 4 years ago

@amshafer : I'll give you the right numbers once we agree here. For now, just just the last number and add a boundary for the releases, so for 11.x you use >= 11xxxx000 && < 11xxxxyyyy

hselasky commented 4 years ago

I will make the patch which fix this issue for ports build and -stable during the day.

hselasky commented 4 years ago

These changes are now pushed. Please verify:

d7cd40063..6d68e697c drm-v4.16 -> drm-v4.16 f3206bf81..99da0bacc drm-v4.16-fbsd12.0 -> drm-v4.16-fbsd12.0 6706271dc..92a0a5d00 drm-v5.0 -> drm-v5.0 eebd254ce..847921ac8 drm-v5.0-fbsd12.1 -> drm-v5.0-fbsd12.1

zeising commented 4 years ago

I've picked this change into drm-v4.11-fbsd11.2 as well: https://github.com/FreeBSDDesktop/kms-drm/commit/c1b4d3a9c858d1a8a8a042fb3129eea76e432520

amshafer commented 4 years ago

graphics/mesa-dri: Fixing vulkan by switching to unsynchronized userptr ioctls

For anyone else using vulkan, this small patch to mesa is also needed.