Was looking at bug 160757, and noticed that nVidia's bundled 'vulkaninfo' implementation, and Reltech-Vr's GLview 'openglex.exe' return a different Vulkan driver string than what our own skia.log reports. This is a change from previously, and the Vulkan driver we record seems improbable--vulkaninfo and openglex both report driver 552.12.0.0 but our skia.log records 552.48.0.0 Vulcan community tracking [1] shows latest driver to be 552.22.0.0, and latest for this nVidia GPU to be 552.12.0.0 Since the Vulkan driver string is used in our deny list for Vulkan GPUs seems like we've got an issue? =-ref-= [1] Vulkan hardware database at https://vulkan.gpuinfo.org/ =-skia.log-= RenderMethod: vulkan Vendor: 0x10de Device: 0x1380 API: 1.3.277 Driver: 552.48.0 DeviceType: discrete DeviceName: NVIDIA GeForce GTX 750 Ti Denylisted: no
We've not changed the way we read the driver. Checked and I get the same "Driver: 552.48.0" Vulkan driver string with each of these builds: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: 7c2ed9919d6d9d286d9062b91577d6bb2b7de8aa CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded Version: 24.2.2.2 (X86_64) / LibreOffice Community Build ID: d56cc158d8a96260b836f100ef4b4ef25d6f1a01 CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded Version: 7.6.6.3 (X86_64) / LibreOffice Community Build ID: d97b2716a9a4a2ce1391dee1765565ea469b0ae7 CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded Version: 7.5.9.2 (X86_64) / LibreOffice Community Build ID: cdeefe45c17511d326101eed8008ac4092f278a9 CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL threaded Version: 7.4.7.2 (x64) / LibreOffice Community Build ID: 723314e595e8007d3cf785c16538505a1c878ca5 CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win Locale: en-US (en_US); UI: en-US Calc: CL
I know nothing about Skia, so just took a look. Skia info are retrieved here: 255 denylisted 256 = isVulkanDenylisted(sk_app::VulkanWindowContext::getPhysDeviceProperties()); 257 SAL_INFO("vcl.skia", "Vulkan denylisted: " << denylisted); (see https://opengrok.libreoffice.org/xref/core/vcl/skia/SkiaHelper.cxx?r=3c8af232#256) then used here: 158 static bool isVulkanDenylisted(const VkPhysicalDeviceProperties& props) 159 { ... 162 driverVersion = props.driverVersion; ... 166 OUString driverVersionString = versionAsString(driverVersion); ... 173 CrashReporter::addKeyValue("VulkanDriver", driverVersionString, CrashReporter::AddItem); ... 184 writeToLog(logFile, "Driver", driverVersionString); (See https://opengrok.libreoffice.org/xref/core/vcl/skia/SkiaHelper.cxx?r=3c8af232#isVulkanDenylisted) Quote from https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkPhysicalDeviceProperties.html " // Provided by VK_VERSION_1_0 typedef struct VkPhysicalDeviceProperties { uint32_t apiVersion; uint32_t driverVersion; uint32_t vendorID; uint32_t deviceID; VkPhysicalDeviceType deviceType; char deviceName[VK_MAX_PHYSICAL_DEVICE_NAME_SIZE]; uint8_t pipelineCacheUUID[VK_UUID_SIZE]; VkPhysicalDeviceLimits limits; VkPhysicalDeviceSparseProperties sparseProperties; } VkPhysicalDeviceProperties; ... driverVersion is the vendor-specified version of the driver. " and finally the method which converts uint32_t to OUString: 146 static OUString versionAsString(uint32_t version) 147 { 148 return OUString::number(version >> 22) + "." + OUString::number((version >> 12) & 0x3ff) + "." 149 + OUString::number(version & 0xfff); 150 } See https://opengrok.libreoffice.org/xref/core/vcl/skia/SkiaHelper.cxx?r=3c8af232#146
GLview and vulkaninfo utils both return the uint32_t of "2315452416" as driverVersion for this nVidia GPU. And both convert its ID string to "552.12.0.0" vulkaninfo.exe run GPU0: VkPhysicalDeviceProperties: --------------------------- apiVersion = 1.3.277 (4206869) driverVersion = 552.12.0.0 (2315452416) vendorID = 0x10de deviceID = 0x1380 deviceType = PHYSICAL_DEVICE_TYPE_DISCRETE_GPU deviceName = NVIDIA GeForce GTX 750 Ti pipelineCacheUUID = e1b3e8ed-3cf0-cbc3-9cd7-33f3274361af But we end up with "552.48.0", and following along in SkiaHelper.cxx at 166 we convert to use the 3 part versionAsString() against the uint32_t of the driverVersion. Hunted around at Khronos, and reddit [1] and it looks like the driver version is supposed to be vendor controlled--and nVidia makes the uint32_t driver version a 4-part string. While Khronos and generic get a 3-part driver string. I couldn't figure out the masking by hand, but does 3-part "552.48.0" match the 4-part "552.12.0.0" conversion of the driverVersion "2315452416"? If so, guess it would not be an issue that they don't match. Or might be better for to just use the uint32_t "driverVersion" as returned, and not convert it? =-ref-= [1] https://www.reddit.com/r/vulkan/comments/fmift4/how_to_decode_driverversion_field_of/
I kept digging and noticed this commit (2020-02-12) 2b702f7436acf6883b41508277441e5ea0a53d51 which includes this change: static bool isVulkanBlacklisted(const VkPhysicalDeviceProperties& props) { static const char* const types[] = { "other", "integrated", "discrete", "virtual", "cpu", "??" }; // VkPhysicalDeviceType + OUString driverVersion = OUString::number(props.driverVersion >> 22) + "." + + OUString::number((props.driverVersion >> 12) & 0x3ff) + "." + + OUString::number(props.driverVersion & 0xfff); + OUString vendorId = "0x" + OUString::number(props.vendorID, 16); + OUString deviceId = "0x" + OUString::number(props.deviceID, 16); SAL_INFO("vcl.skia", "Vulkan API version: " << (props.apiVersion >> 22) << "." << ((props.apiVersion >> 12) & 0x3ff) << "." - << (props.apiVersion & 0xfff) << ", driver version: " - << (props.driverVersion >> 22) << "." << ((props.driverVersion >> 12) & 0x3ff) - << "." << (props.driverVersion & 0xfff) << std::hex << ", vendor: 0x" - << props.vendorID << ", device: 0x" << props.deviceID << std::dec - << ", type: " << types[std::min<unsigned>(props.deviceType, SAL_N_ELEMENTS(types))] + << (props.apiVersion & 0xfff) << ", driver version: " << driverVersion + << ", vendor: " << vendorId << ", device: " << deviceId << ", type: " + << types[std::min<unsigned>(props.deviceType, SAL_N_ELEMENTS(types) - 1)] << ", name: " << props.deviceName); - return false; + return DriverBlocklist::IsDeviceBlocked(getBlacklistFile(), driverVersion, vendorId, deviceId); } so before this commit, we used apiVersion, afterwards, we started to use driverVersion. For api version, Skia uses this macro: #define VK_MAKE_VERSION(major, minor, patch) \ (((major) << 22) | ((minor) << 12) | (patch)) in addition with these: #define VK_VERSION_MAJOR(version) ((uint32_t)(version) >> 22) #define VK_VERSION_MINOR(version) (((uint32_t)(version) >> 12) & 0x3ff) #define VK_VERSION_PATCH(version) ((uint32_t)(version) & 0xfff) it corresponds well with the LO part: 146 static OUString versionAsString(uint32_t version) 147 { 148 return OUString::number(version >> 22) + "." + OUString::number((version >> 12) & 0x3ff) + "." 149 + OUString::number(version & 0xfff); 150 } but driverVersion uses this one: #define GR_GL_DRIVER_VER(major, minor, point) ((static_cast<uint64_t>(major) << 32) | \ (static_cast<uint64_t>(minor) << 16) | \ static_cast<uint64_t>(point)) So I suppose LO should be adapted. But a last thing, in Skia we got: typedef struct VkPhysicalDeviceProperties { uint32_t apiVersion; uint32_t driverVersion; uint32_t vendorID; uint32_t deviceID; VkPhysicalDeviceType deviceType; char deviceName[VK_MAX_PHYSICAL_DEVICE_NAME_SIZE]; uint8_t pipelineCacheUUID[VK_UUID_SIZE]; VkPhysicalDeviceLimits limits; VkPhysicalDeviceSparseProperties sparseProperties; } VkPhysicalDeviceProperties; apiVersion is uint32_t ok, but driverVersion is uint64_t when reading the macro. So even if we change LO part, it seems there's a bug in Skia, doesn't it? Noel/Mike: perhaps one of you might be interested in this one?
The Vulkan API documents VkPhysicalDeviceProperties and its driverVersion this way [1]: > uint32_t driverVersion; > * driverVersion is the vendor-specified version of the driver. > Note > The encoding of driverVersion is implementation-defined. It may not use the same > encoding as apiVersion. Applications should follow information from the vendor > on how to extract the version information from driverVersion. This explicitly tells that the encoding might be different in different vendors. And discussion at [2] confirms that, showing that AMD seems to follow Vilkan's apiVersion encoding scheme ("10|10|12"), while Intel and NVidia use some own schemes (NVidia is said to use "10|8|8|6"). The GR_GL_DRIVER_VER macro can't be relevant here: it encodes three numbers into a 64-bit number, with major part occupying bits in the higher 32 bits, and only minor and point are in the lower 32 bits. Note that driverVersion in VkPhysicalDeviceProperties is explicitly 32-bit. I suppose, that the current code is OK for the task - considering only the check part; all encodings seem to stick to the most important agreement, that higher bits encode higher level of version number hierarchy. But if needed, there could be a code that would decode the version into strings, taking the vendor into account (just as a human-readable info, to match expectations). [1] https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkPhysicalDeviceProperties.html [2] https://www.reddit.com/r/vulkan/comments/fmift4/how_to_decode_driverversion_field_of/
Or maybe avoid trying to decode the version number, and just show the number as 32-bit hex like "0x12345678". This way, there wouldn't be a similarity to the vendor's "552.22.0.0" string, indicating clearly that the code uses internal encoding.
Thank you Mike for your feedback, I've submitted https://gerrit.libreoffice.org/c/core/+/166736 as you m
(In reply to Julien Nabet from comment #7) > Thank you Mike for your feedback, I've submitted > https://gerrit.libreoffice.org/c/core/+/166736 as you m "...as you may have seen"
Regards nVidia driver versioning: this from Sascha Willems, the Vulkan community's maintainer of the vulkan.gpuinfo.org repository--as sourced from nVidia [1] <snip> This is the information I got from NVIDIA : Starting with MSB : 10 bits = major version (up to r1023) 8 bits = minor version (up to 255) 8 bits = secondary branch version/build version (up to 255) 6 bits = tertiary branch/build version (up to 63) </snip> So nVidia intends encoding of a 4-part driver version string into uint32_t of props.driverVersion. I recall Lubos being particular about difference between vendor's "marketing" driver version and the actual Vulkan driver version that we needed for the denylist, I guess some of that is at play here since we've got to convert/extract the version string to test. =-ref-= [1] https://github.com/SaschaWillems/VulkanCapsViewer/issues/2
As a sidebar, worth noting... Poking around the gpuinfo.org web portal, they offer handy "capability" checkers for Vulkan, OpenGL, and OpenCL--each linked to optionally upload report to the appropriate repository. Also host db and Android app for OpenGL ES (Embedded Systems). All seem a useful resource to recommend during QA for us to obtain reliable GPU device and driver info. The gpuinfo.org Vulkan utility is here, simple unzip or appimage. https://vulkan.gpuinfo.org/download.php They offer for macOS but not too much use as Apple Metal API are not Khronos Vulkan API based, and few macOS installs would switch to Vulkan. Also, Patrick L. has been handling Metal devices directly in quartz/cgutils.mm, so no separate denylist implementation for macOS.
(In reply to V Stuart Foote from comment #10) > As a sidebar, worth noting... > > Poking around the gpuinfo.org web portal, they offer handy "capability" > checkers for Vulkan, OpenGL, and OpenCL--each linked to optionally upload > report to the appropriate repository. Also host db and Android app for > OpenGL ES (Embedded Systems). > > All seem a useful resource to recommend during QA for us to obtain reliable > GPU device and driver info. > > The gpuinfo.org Vulkan utility is here, simple unzip or appimage. > > https://vulkan.gpuinfo.org/download.php > ... I had submitted https://gerrit.libreoffice.org/c/core/+/166736 but your idea seems better so I abandoned my patch/ However, I suppose using this requires more work than a simple patch (dealing with gbuild, make the app work for all OSes, removing parts in LO, adding other ones which call the external code, etc.) I can't help here => uncc myself.
(In reply to Julien Nabet from comment #11) > I had submitted https://gerrit.libreoffice.org/c/core/+/166736 but your idea > seems better so I abandoned my patch/ No, your patch of skiahelper.cxx is the way to go. I don't think we should integrate the gpuinfo.org project stuff, its just out there to get a better handle on the driver strings and device IDs we need. > However, I suppose using this requires more work than a simple patch > (dealing with gbuild, make the app work for all OSes, removing parts in LO, > adding other ones which call the external code, etc.) Think we just need to decide if we want to test the uint32_t returned by prop.driverVersion, or the suggested Vendor masking to render to string as in comment 6. > I can't help here => uncc myself. With so much effort as you've invested in the Vulkan denylist handling, you deserve a say in how it could be refactored--if at all. =-=-= another sidebar, regards the bitwise mask for extracting the Vulkan driverVersion for Intel GPUs. So I guess we're doing the correct thing with them? On a Win11 laptop with Intel Iris Plus graphics that needed a driver update. Ran the Vulkan Caps viewer, and vulkaninfo.exe, and compared to the skia.log (of our handling of the Vulkan driver conversion For the 27.20.100.8853 Intel driver *Vulkan HW Capability 1.2.151 and 0.402.661 (and in DB as 100.8853) *vulkaninfo.exe Device Properties and Extensions: ================================= GPU0: VkPhysicalDeviceProperties: --------------------------- apiVersion = 4202647 (1.2.151) driverVersion = 1647253 (0x192295) vendorID = 0x8086 deviceID = 0x8a52 deviceType = PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU deviceName = Intel(R) Iris(R) Plus Graphics skia.log RenderMethod: vulkan Vendor: 0x8086 Device: 0x8a52 API: 1.2.151 Driver: 0.402.661 DeviceType: integrated DeviceName: Intel(R) Iris(R) Plus Graphics Denylisted: no =-=-=-= For the 31.0.101.2115 Intel driver *Vulkan HW Capability 1.3.215 and 0.404.2115 (and in DB as 101.2115) *vulkaninfo.exe GPU0: VkPhysicalDeviceProperties: --------------------------- apiVersion = 4206807 (1.3.215) driverVersion = 1656899 (0x194843) vendorID = 0x8086 deviceID = 0x8a52 deviceType = PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU deviceName = Intel(R) Iris(R) Plus Graphics pipelineCacheUUID = 7232703e-7731-4532-7a3f-79317e357c03 skia.log RenderMethod: vulkan Vendor: 0x8086 Device: 0x8a52 API: 1.3.215 Driver: 0.404.2115 DeviceType: integrated DeviceName: Intel(R) Iris(R) Plus Graphics Denylisted: noion).
(In reply to Julien Nabet from comment #7) > I've submitted https://gerrit.libreoffice.org/c/core/+/166736 Continuing the topic that we touched there: I believe, that we need to use the formatted string for user, and for use in the denylist file (as human readable); but for comparison, we should convert the formatted string back into the uint32_t. This would automatically resolve the problem of Intel having different encodings on Windows - the conversion of the string will do different things on Windows vs. Linux, so the same version string will turn into respective proper numbers. Also, this conversion from the string into uint32_t will allow to easily convert current strings into their numbers (using Vulkan encoding), and then turn back into the proper new numbers (using NVidia or Intel encoding).
(In reply to Mike Kaganski from comment #13) > (In reply to Julien Nabet from comment #7) > > I've submitted https://gerrit.libreoffice.org/c/core/+/166736 > > Continuing the topic that we touched there: I believe, that we need to use > the formatted string for user, and for use in the denylist file (as human > readable); but for comparison, we should convert the formatted string back > into the uint32_t. This would automatically resolve the problem of Intel > having different encodings on Windows - the conversion of the string will do > different things on Windows vs. Linux, so the same version string will turn > into respective proper numbers. > > Also, this conversion from the string into uint32_t will allow to easily > convert current strings into their numbers (using Vulkan encoding), and then > turn back into the proper new numbers (using NVidia or Intel encoding). Sorry, it's confused for me. If you got a clear idea of what to do, please go ahead. Abandoning my patch again.
(In reply to Mike Kaganski from comment #5) > The Vulkan API documents VkPhysicalDeviceProperties and its driverVersion > this way [1]: > > > uint32_t driverVersion; > ... BTW, I had read this too but when looking at source code in workdir/UnpackedTarball/skia, by just typing: fgrep -nR driverVersion * here what I got: include/third_party/vulkan/vulkan/vulkan_core.h:2775: uint32_t driverVersion; src/gpu/ganesh/gl/GrGLContext.h.orig:65: GrGLDriverVersion driverVersion() const { return fDriverInfo.fDriverVersion; } src/gpu/ganesh/gl/GrGLUtil.cpp:416: GrGLDriverVersion driverVersion = GR_GL_DRIVER_UNKNOWN_VER; src/gpu/ganesh/gl/GrGLUtil.cpp:434: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:453: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:467: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:480: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:491: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:512: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, driverPoint); src/gpu/ganesh/gl/GrGLUtil.cpp:523: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:537: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:556: driverVersion = GR_GL_DRIVER_VER(driverMajor, driverMinor, 0); src/gpu/ganesh/gl/GrGLUtil.cpp:565: return {driver, driverVersion}; src/gpu/ganesh/gl/GrGLContext.h:65: GrGLDriverVersion driverVersion() const { return fDriverInfo.fDriverVersion; } src/gpu/ganesh/gl/GrGLCaps.cpp:3747: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(367, 57, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:3801: ctxInfo.driverVersion() > GR_GL_DRIVER_VER(127, 0, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:3889: (ctxInfo.driverVersion() < GR_GL_DRIVER_VER(10, 30, 12) || src/gpu/ganesh/gl/GrGLCaps.cpp:3912: if (ctxInfo.driverVersion() <= GR_GL_DRIVER_VER(219, 0, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:3919: if (ctxInfo.driverVersion() < GR_GL_DRIVER_VER(145, 0, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:3951: ctxInfo.driverVersion() > GR_GL_DRIVER_VER(53, 0, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4209: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(337, 00, 0) && src/gpu/ganesh/gl/GrGLCaps.cpp:4222: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(355, 00, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4291: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(1, 15, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4455: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(571, 0, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4465: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(1, 16, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4517: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(1, 26, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4547: ctxInfo.driverVersion() < GR_GL_DRIVER_VER(1, 19, 0)) { src/gpu/ganesh/gl/GrGLCaps.cpp:4578: ctxInfo.driverVersion() >= GR_GL_DRIVER_VER(2, 1, 19900)) { why driverVersion appears only with GR_GL_DRIVER_VER except for include/third_party/vulkan/vulkan/vulkan_core.h ?
(In reply to Julien Nabet from comment #15) >... > why driverVersion appears only with GR_GL_DRIVER_VER except for > include/third_party/vulkan/vulkan/vulkan_core.h ? I think those are mostly leftovers from migration from OpenGL to Vulkan that Luboš had handled. OpenGL removed from VCL, but retained some of the OpenGL driver version used in the DENYLIST handling? https://gerrit.libreoffice.org/c/core/+/107290 https://gerrit.libreoffice.org/c/core/+/107362 https://gerrit.libreoffice.org/c/core/+/107398 https://gerrit.libreoffice.org/c/core/+/107399 And, I'm not sure but seems like the version and os handling for the DENYLIST will need some attention. No Windows 11 and especially if we implement testing as uint32_t against the returned VkPhysicalDeviceProperties prop.driverVersion vcl/source/helper/driverblocklist.cxx vcl/inc/driverblocklist.hxx For Vulkan, Luboš had tweaked the version string conversion there with https://gerrit.libreoffice.org/c/core/+/103118
(In reply to V Stuart Foote from comment #16) > (In reply to Julien Nabet from comment #15) > > >... > > why driverVersion appears only with GR_GL_DRIVER_VER except for > > include/third_party/vulkan/vulkan/vulkan_core.h ? > > I think those are mostly leftovers from migration from OpenGL to Vulkan that > Luboš had handled. OpenGL removed from VCL, but retained some of the OpenGL > driver version used in the DENYLIST handling? > >... the code I quoted is in Skia lib (workdir/UnpackedTarball/skia), not in LO. IMHO, in addition to our pb of int/string driver conversion there's a pb in Skia lib too.
(In reply to Julien Nabet from comment #15) > include/third_party/vulkan/vulkan/vulkan_core.h:2775: uint32_t driverVersion; > src/gpu/ganesh/gl/GrGLUtil.cpp:416: GrGLDriverVersion driverVersion = GR_GL_DRIVER_UNKNOWN_VER; (In reply to Julien Nabet from comment #17) > IMHO, in addition to our pb of int/string driver conversion there's a pb in > Skia lib too. No. The name 'driverVersion', where used together with GR_GL_DRIVER_VER, is absolutely unrelated, an instance of "GrGLDriverVersion", which is a typedef for uint64_t.