Re: [PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.
On 6 July 2015 at 22:20, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
if (context_debug_output_enabled(gl_info)) + ctx_flags = WGL_CONTEXT_DEBUG_BIT_ARB; + ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MAJOR_VERSION_ARB; + ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version >> 16; + ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MINOR_VERSION_ARB; + ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version & 0xffff; + if (gl_info->selected_gl_version >= MAKEDWORD_VERSION(3, 2)) + ctx_flags |= WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB; + if (ctx_flags) { ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_FLAGS_ARB; - ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_DEBUG_BIT_ARB; + ctx_attribs[ctx_attrib_idx++] = ctx_flags; } ctx_attribs[ctx_attrib_idx] = 0;
if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs))) { - ERR("Failed to create a WGL context.\n"); - context_release(ret); - goto out; + if (ctx_flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB) + { + ctx_attribs[ctx_attrib_idx - 1] &= ~WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB; + if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs))) + { + ERR("Failed to create a WGL context.\n"); + context_release(ret); + goto out; + } + } } }
I think this is starting to become complex enough that we'd probably benefit from a helper function to be able to share some of this with wined3d_adapter_init().
+ glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile); + if (glGetError() != GL_NO_ERROR) + context_profile = 0; Should this perhaps just check for gl_info->p_wglCreateContextAttribsARB instead of eating errors?
+ for (i = 0; i < ARRAY_SIZE(supported_gl_versions); ++i) + { + if (supported_gl_versions[i] <= wined3d_settings.max_gl_version) + { + if (supported_gl_versions[i] != wined3d_settings.max_gl_version) + ERR_(winediag)("Requested GL version %u.%u which is unsupported, trying version %u.%u instead.\n", + wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff, + supported_gl_versions[i] >> 16, supported_gl_versions[i] & 0xffff); + break; + } + } + if (i == ARRAY_SIZE(supported_gl_versions)) + { + ERR_(winediag)("Requested invalid GL version %u.%u.\n", + wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff); + i = ARRAY_SIZE(supported_gl_versions) - 1; + } This is a bit different than what I had in mind. I think max_gl_version should just be a maximum, and e.g. specifying 4.4 should just result in 3.2 at the moment. I think the winediag message should just happen if the registry setting is set, regardless of its actual value. (Except possibly if it's equal to the default value.) I.e. we also want to know if someone sets e.g. 3.2 while the default is currently 1.0 when reporting a bug.
+ if (!get_config_key(hkey, appkey, "MaxGLVersion", buffer, size)) + { + unsigned int major, minor; + + if (sscanf(buffer, "%u.%u", &major, &minor) == 2) + wined3d_settings.max_gl_version = MAKEDWORD_VERSION(major, minor); + else + ERR_(winediag)("Couldn't parse the requested maximum OpenGL version.\n"); + } I'd make this a DWORD, if nothing else for consistency with MaxShaderModelVS etc. Perhaps "MaxVersionGL" would also more consistent for the naming, I'm not sure.
2015-07-07 16:58 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 6 July 2015 at 22:20, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
if (context_debug_output_enabled(gl_info)) + ctx_flags = WGL_CONTEXT_DEBUG_BIT_ARB; + ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MAJOR_VERSION_ARB; + ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version >> 16; + ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MINOR_VERSION_ARB; + ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version & 0xffff; + if (gl_info->selected_gl_version >= MAKEDWORD_VERSION(3, 2)) + ctx_flags |= WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB; + if (ctx_flags) { ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_FLAGS_ARB; - ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_DEBUG_BIT_ARB; + ctx_attribs[ctx_attrib_idx++] = ctx_flags; } ctx_attribs[ctx_attrib_idx] = 0;
if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs))) { - ERR("Failed to create a WGL context.\n"); - context_release(ret); - goto out; + if (ctx_flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB) + { + ctx_attribs[ctx_attrib_idx - 1] &= ~WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB; + if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs))) + { + ERR("Failed to create a WGL context.\n"); + context_release(ret); + goto out; + } + } } }
I think this is starting to become complex enough that we'd probably benefit from a helper function to be able to share some of this with wined3d_adapter_init().
+ glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile); + if (glGetError() != GL_NO_ERROR) + context_profile = 0; Should this perhaps just check for gl_info->p_wglCreateContextAttribsARB instead of eating errors?
You mean just check whether wglCreateContextAttribsARB succeeded for a GL version >= 3.2? That should work, although it might be a bit awkward (but not necessarily more than the current code).
+ for (i = 0; i < ARRAY_SIZE(supported_gl_versions); ++i) + { + if (supported_gl_versions[i] <= wined3d_settings.max_gl_version) + { + if (supported_gl_versions[i] != wined3d_settings.max_gl_version) + ERR_(winediag)("Requested GL version %u.%u which is unsupported, trying version %u.%u instead.\n", + wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff, + supported_gl_versions[i] >> 16, supported_gl_versions[i] & 0xffff); + break; + } + } + if (i == ARRAY_SIZE(supported_gl_versions)) + { + ERR_(winediag)("Requested invalid GL version %u.%u.\n", + wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff); + i = ARRAY_SIZE(supported_gl_versions) - 1; + } This is a bit different than what I had in mind. I think max_gl_version should just be a maximum, and e.g. specifying 4.4 should just result in 3.2 at the moment. I think the winediag message should just happen if the registry setting is set, regardless of its actual value. (Except possibly if it's equal to the default value.) I.e. we also want to know if someone sets e.g. 3.2 while the default is currently 1.0 when reporting a bug.
Yeah, I was unsure here and I like your suggestion.
+ if (!get_config_key(hkey, appkey, "MaxGLVersion", buffer, size)) + { + unsigned int major, minor; + + if (sscanf(buffer, "%u.%u", &major, &minor) == 2) + wined3d_settings.max_gl_version = MAKEDWORD_VERSION(major, minor); + else + ERR_(winediag)("Couldn't parse the requested maximum OpenGL version.\n"); + } I'd make this a DWORD, if nothing else for consistency with MaxShaderModelVS etc.
Hmm, how would you expect the (major, minor) version to be encoded in the registry DWORD? Same as MAKEDWORD_VERSION()?
Perhaps "MaxVersionGL" would also more consistent for the naming, I'm not sure.
I'm not sure that's better overall but I'm not attached to "MaxGLVersion" either. I'll go with "MaxVersionGL" unless I'll find it to particularly hurt my eyes ;)
On 7 July 2015 at 18:37, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2015-07-07 16:58 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
+ glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile); + if (glGetError() != GL_NO_ERROR) + context_profile = 0; Should this perhaps just check for gl_info->p_wglCreateContextAttribsARB instead of eating errors?
You mean just check whether wglCreateContextAttribsARB succeeded for a GL version >= 3.2? That should work, although it might be a bit awkward (but not necessarily more than the current code).
I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but that doesn't work because the query is only supported on 3.2 or later contexts. But we could probably just check "gl_version" against 3.2 instead.
I'd make this a DWORD, if nothing else for consistency with MaxShaderModelVS etc.
Hmm, how would you expect the (major, minor) version to be encoded in the registry DWORD? Same as MAKEDWORD_VERSION()?
We could, but there are of course other options as well. (E.g. 0x00030002, 0x0302, 0x32, 302, 32.) We could also have separate entries for the major and minor numbers, although I suppose that's awkward if one is set but not the other.
2015-07-07 19:12 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 July 2015 at 18:37, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2015-07-07 16:58 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
+ glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile); + if (glGetError() != GL_NO_ERROR) + context_profile = 0; Should this perhaps just check for gl_info->p_wglCreateContextAttribsARB instead of eating errors?
You mean just check whether wglCreateContextAttribsARB succeeded for a GL version >= 3.2? That should work, although it might be a bit awkward (but not necessarily more than the current code).
I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but that doesn't work because the query is only supported on 3.2 or later contexts. But we could probably just check "gl_version" against 3.2 instead.
Yeah, that's probably easier.
I'd make this a DWORD, if nothing else for consistency with MaxShaderModelVS etc.
Hmm, how would you expect the (major, minor) version to be encoded in the registry DWORD? Same as MAKEDWORD_VERSION()?
We could, but there are of course other options as well. (E.g. 0x00030002, 0x0302, 0x32, 302, 32.) We could also have separate entries for the major and minor numbers, although I suppose that's awkward if one is set but not the other.
It's a bit ugly in any way. I'll go with the MAKEDWORD_VERSION() style first and see how it looks.
2015-07-07 19:43 GMT+02:00 Matteo Bruni <matteo.mystral(a)gmail.com>:
2015-07-07 19:12 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 July 2015 at 18:37, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2015-07-07 16:58 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
+ glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile); + if (glGetError() != GL_NO_ERROR) + context_profile = 0; Should this perhaps just check for gl_info->p_wglCreateContextAttribsARB instead of eating errors?
You mean just check whether wglCreateContextAttribsARB succeeded for a GL version >= 3.2? That should work, although it might be a bit awkward (but not necessarily more than the current code).
I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but that doesn't work because the query is only supported on 3.2 or later contexts. But we could probably just check "gl_version" against 3.2 instead.
Yeah, that's probably easier.
Actually no, we can't, I just remembered. Linux binary drivers offer compatibility contexts and will return the latest supported GL version even if you create a 1.0 context. We don't want to enable the wined3d 3.2 codepath there.
On 7 July 2015 at 23:43, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Actually no, we can't, I just remembered. Linux binary drivers offer compatibility contexts and will return the latest supported GL version even if you create a 1.0 context. We don't want to enable the wined3d 3.2 codepath there. I don't think that should make a difference, they still shouldn't set the core profile bit in that case. (But if they do, the current patch would be just as broken.)
2015-07-08 9:17 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 July 2015 at 23:43, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Actually no, we can't, I just remembered. Linux binary drivers offer compatibility contexts and will return the latest supported GL version even if you create a 1.0 context. We don't want to enable the wined3d 3.2 codepath there. I don't think that should make a difference, they still shouldn't set the core profile bit in that case. (But if they do, the current patch would be just as broken.)
They don't. I thought you were suggesting to remove the GL_CONTEXT_PROFILE_MASK query entirely but now I see you were only talking about the glGetError() part. Updated patch coming in a bit.
participants (2)
-
Henri Verbeet -
Matteo Bruni