Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix I915_USERPTR_UNSYNCHRONIZED for Vulkan on old Haswell cards #205

Closed
wants to merge 2 commits into from
Closed

Conversation

amshafer
Copy link

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.

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

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.

__i915_gem_userptr_get_pages_worker was halfway ifdef-ed out, but
all of the linuxkpi functions were implemented. The only exception
was mmget_not_zero, which is supposed to be in mm.h. I added it
above this function, but it should probably be included in the
FreeBSD src.
@jbeich
Copy link

jbeich commented Jan 24, 2020

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
Copy link
Contributor

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
Copy link
Contributor

Sounds OK. Please make a patch!

@hselasky
Copy link
Contributor

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
Copy link
Contributor

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.

mmget_not_zero is now in the main linuxkpi, in mm_types.h which
is included from mm.h. There is no longer any reason why it
should be in the i915 code.
@amshafer
Copy link
Author

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
Copy link
Contributor

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

@valpackett
Copy link
Contributor

@hselasky so what about the extra arg in get_user_pages_remote?

@hselasky
Copy link
Contributor

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

@amshafer
Copy link
Author

amshafer commented Feb 2, 2020

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
Copy link
Contributor

hselasky commented Feb 2, 2020

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

@hselasky
Copy link
Contributor

hselasky commented Feb 3, 2020

@hselasky
Copy link
Contributor

hselasky commented Feb 3, 2020

Can this patch be applied to other branches aswell?

@amshafer
Copy link
Author

amshafer commented Feb 3, 2020

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
Copy link
Contributor

hselasky commented Feb 3, 2020

Yes, please test!

@amshafer
Copy link
Author

amshafer commented Feb 3, 2020

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
Copy link
Contributor

hselasky commented Feb 3, 2020

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

--HPS

@hselasky
Copy link
Contributor

hselasky commented Feb 4, 2020

Just make two new pull requests and add me.

@amshafer
Copy link
Author

amshafer commented Feb 4, 2020

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
Copy link
Contributor

hselasky commented Feb 4, 2020

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
Copy link
Author

amshafer commented Feb 4, 2020

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
Copy link
Contributor

hselasky commented Feb 5, 2020

@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
Copy link
Contributor

hselasky commented Feb 5, 2020

@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
Copy link
Contributor

hselasky commented Feb 6, 2020

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

@hselasky
Copy link
Contributor

hselasky commented Feb 6, 2020

These changes are now pushed. Please verify:

d7cd400..6d68e69 drm-v4.16 -> drm-v4.16
f3206bf..99da0ba drm-v4.16-fbsd12.0 -> drm-v4.16-fbsd12.0
6706271..92a0a5d drm-v5.0 -> drm-v5.0
eebd254..847921a drm-v5.0-fbsd12.1 -> drm-v5.0-fbsd12.1

@hselasky hselasky closed this Feb 6, 2020
@zeising
Copy link
Member

zeising commented Feb 21, 2020

I've picked this change into drm-v4.11-fbsd11.2 as well:
c1b4d3a

@amshafer
Copy link
Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants