ValveSoftware / wine

Wine with a bit of extra spice
Other
1.3k stars 244 forks source link

win32u: Add driverVersion from Vulkan to registry #191

Closed Samsuper12 closed 1 year ago

Samsuper12 commented 1 year ago

UE4 (4.27) crashes if Intel GPU doesn't have a driverVersion string in the registry.

Affected games: Stray, System Shock (2023), Dead By Daylight and Warhammer 40,000: Boltgun.

Samsuper12 commented 1 year ago

More about it: https://github.com/doitsujin/dxvk/pull/3480 https://gitlab.freedesktop.org/mesa/mesa/-/issues/7928#note_1695389

kisak-valve commented 1 year ago

Hello @Samsuper12, at a glance this doesn't look like it would be Proton specific. Can you send it upstream?

Samsuper12 commented 1 year ago

Hello @Samsuper12, at a glance this doesn't look like it would be Proton specific. Can you send it upstream?

Hi! Only ValveSoftware/Wine has the add_remaining_gpus_via_vulkan function. Without it, this PR has no sense.

gofman commented 1 year ago

add_remaining_gpus_via_vulkan() is orthogonal to what patch is doing. Yes, upstream doesn't have this workaround for broken xrandr on Wayland, but driver version can still be added in a similar way. Then, it is the matter of properly updating the patch addding that add_remaining_gpus_via_vulkan() in Proton when picking from upstream. So it is something really better to send upstream first.

Then, especially since the part of version string is hardcoded anyway, why do we need getting the rest from Vulkan at all and not hardcode the string alltogether simplifying the thing? What UE4 actually expects there to work properly?

The other question is which driver version string is there across different GPUs (IIRC that was driver specific, in any case worth verification). It is easy to regress things by putting wrong registry values on different GPUs. If keeping just one variant (like we do elsewhere sometimes), it probably makes sense to stick to what AMD has there for now.

Samsuper12 commented 1 year ago

Yes, upstream doesn't have this workaround for broken xrandr on Wayland, but driver version can still be added in a similar way.

The driverVersion from get_gpu_properties_from_vulkan() is always zero. I'll check later what I can do with it.

Then, especially since the part of version string is hardcoded anyway, why do we need getting the rest from Vulkan at all and not hardcode the string alltogether simplifying the thing?

I suppose some applications or games can detect Mesa by this string. If not now, but in the future. The string contains very important build information that can be helpful. But, yeah, I agree, it can be hard coded at all, like amd_ags (gpu_info->driverVersion = "21.30.25.05-211005a-372402E-RadeonSoftware";)

What UE4 actually expects there to work properly?

Any string. Even "0" . But in theory, build number + version: link. ( Access to UE4 sources is required.)

The other question is which driver version string is there across different GPUs (IIRC that was driver specific, in any case worth verification). It is easy to regress things by putting wrong registry values on different GPUs. If keeping just one variant (like we do elsewhere sometimes), it probably makes sense to stick to what AMD has there for now.

I thought about it. I don't know Proton workflow perfectly. Maybe some users can help with testing these changes. Anyway, we can just write something like:

if (vendor_id == 0x8086)  { // Intel ID
    set_reg_value(..., "31.0.101.3790") // DriverVersion from a real Intel GPU driver.
}
gofman commented 1 year ago

get_gpu_properties_from_vulkan() always returns zero from driverVersion. I'll check later what I can do with it. That is the matter of having a fallback path. Note that upstream requires general GPU device information working even for GPUs without Vulkan support. So if we would really benefit from the Vulkan driver version here (which I doubt) it would be a matter of still having some hardcoded fallback or not adding the key at all.

I suppose some applications or games can detect Mesa by this string. If not now, but in the future. The string contains very important build information that can be helpful. But, yeah, I agree, it can be hard coded at all, like amd_ags

We need to match typical Windows configuration here. Special paths in games which mind Proton or rare Mesa configurations on Windows are doomed be broken over time anyway.

I checked on Windows 10 /AMD and I have 31.0.14051.5006 here. That matches what we currently have in dlls/wbemprox/builtin.c both in upstream Wine and latest Proton. I guess hardcoding it and sending this upstream is the best course of action for now.

gofman commented 1 year ago

Then we can get this version from registry in wbemprox instead of hardcoding the same there, but this is of course a separate change.

Samsuper12 commented 1 year ago

My MR to Wine is approved. This issue might be closed.

https://gitlab.winehq.org/wine/wine/-/merge_requests/3104

Thanks all!

kisak-valve commented 1 year ago

Hello @Samsuper12, it probably makes sense to keep this merge request open for a little while to track the request to backport the upstream commit into Proton Experimental.

Samsuper12 commented 1 year ago

@kisak-valve OK. No problem!

Samsuper12 commented 1 year ago

@kisak-valve Hi! My fix is already in Wine. Can someone push it to Proton/Wine?

ivyl commented 1 year ago

I've just cherry-picked it to experimental. It should show up in bleeding-edge soon and in the very next experimental release. Thanks!