[PATCH 0/5] MR4624: wined3d: More old-GL fixes.
From: Matteo Bruni <mbruni(a)codeweavers.com> Which is really what it means nowadays: resources created by the CS. It has mostly implications WRT thread safety. In particular, the flag doesn't mean that the resource doesn't participate in memory accounting (that was split into WINED3DUSAGE_VIDMEM_ACCOUNTING by 723cd0a4ae76ec45320290893c7af7f90b668556), nor that it isn't checked for Reset purposes (we use parent == NULL for that). --- dlls/wined3d/arb_program_shader.c | 2 +- dlls/wined3d/glsl_shader.c | 2 +- dlls/wined3d/resource.c | 6 +++--- dlls/wined3d/surface.c | 2 +- dlls/wined3d/texture.c | 6 +++--- include/wine/wined3d.h | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c index ee693924a24..01785166cba 100644 --- a/dlls/wined3d/arb_program_shader.c +++ b/dlls/wined3d/arb_program_shader.c @@ -7883,7 +7883,7 @@ static DWORD arbfp_blitter_blit(struct wined3d_blitter *blitter, enum wined3d_bl desc.format = src_texture->resource.format->id; desc.multisample_type = src_texture->resource.multisample_type; desc.multisample_quality = src_texture->resource.multisample_quality; - desc.usage = WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_GPU; desc.width = wined3d_texture_get_level_width(src_texture, src_level); diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c index 5dfcbb689b7..0c7bd49f686 100644 --- a/dlls/wined3d/glsl_shader.c +++ b/dlls/wined3d/glsl_shader.c @@ -13167,7 +13167,7 @@ static DWORD glsl_blitter_blit(struct wined3d_blitter *blitter, enum wined3d_bli desc.format = src_texture->resource.format->id; desc.multisample_type = src_texture->resource.multisample_type; desc.multisample_quality = src_texture->resource.multisample_quality; - desc.usage = WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_GPU; desc.width = wined3d_texture_get_level_width(src_texture, src_level); diff --git a/dlls/wined3d/resource.c b/dlls/wined3d/resource.c index 9d3fd0a426d..85c55e566b2 100644 --- a/dlls/wined3d/resource.c +++ b/dlls/wined3d/resource.c @@ -35,7 +35,7 @@ static void resource_check_usage(uint32_t usage, unsigned int access) | WINED3DUSAGE_OVERLAY | WINED3DUSAGE_SCRATCH | WINED3DUSAGE_MANAGED - | WINED3DUSAGE_PRIVATE + | WINED3DUSAGE_CS | WINED3DUSAGE_LEGACY_CUBEMAP | ~WINED3DUSAGE_MASK; @@ -221,7 +221,7 @@ HRESULT resource_init(struct wined3d_resource *resource, struct wined3d_device * adapter_adjust_memory(device->adapter, size); } - if (!(usage & WINED3DUSAGE_PRIVATE)) + if (!(usage & WINED3DUSAGE_CS)) device_resource_add(device, resource); return WINED3D_OK; @@ -247,7 +247,7 @@ void resource_cleanup(struct wined3d_resource *resource) adapter_adjust_memory(resource->device->adapter, (INT64)0 - resource->size); } - if (!(resource->usage & WINED3DUSAGE_PRIVATE)) + if (!(resource->usage & WINED3DUSAGE_CS)) device_resource_released(resource->device, resource); wined3d_resource_reference(resource); diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c index 87b3d3c38a4..a1ae63a0f78 100644 --- a/dlls/wined3d/surface.c +++ b/dlls/wined3d/surface.c @@ -271,7 +271,7 @@ static struct wined3d_texture *surface_convert_format(struct wined3d_texture *sr desc.format = dst_format->id; desc.multisample_type = WINED3D_MULTISAMPLE_NONE; desc.multisample_quality = 0; - desc.usage = WINED3DUSAGE_SCRATCH | WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_SCRATCH | WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_CPU | WINED3D_RESOURCE_ACCESS_MAP_R | WINED3D_RESOURCE_ACCESS_MAP_W; desc.width = wined3d_texture_get_level_width(src_texture, texture_level); diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 8a7f9455bb0..88647a9cdb7 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -418,7 +418,7 @@ static void texture2d_blt_fbo(struct wined3d_device *device, struct wined3d_cont desc.format = resolve_format_id; desc.multisample_type = src_texture->resource.multisample_type; desc.multisample_quality = src_texture->resource.multisample_quality; - desc.usage = WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_GPU; desc.width = wined3d_texture_get_level_width(src_texture, src_level); @@ -457,7 +457,7 @@ static void texture2d_blt_fbo(struct wined3d_device *device, struct wined3d_cont desc.format = resolve_format_id; desc.multisample_type = dst_texture->resource.multisample_type; desc.multisample_quality = dst_texture->resource.multisample_quality; - desc.usage = WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_GPU; desc.width = wined3d_texture_get_level_width(dst_texture, dst_level); @@ -6268,7 +6268,7 @@ static DWORD ffp_blitter_blit(struct wined3d_blitter *blitter, enum wined3d_blit desc.format = src_texture->resource.format->id; desc.multisample_type = src_texture->resource.multisample_type; desc.multisample_quality = src_texture->resource.multisample_quality; - desc.usage = WINED3DUSAGE_PRIVATE; + desc.usage = WINED3DUSAGE_CS; desc.bind_flags = 0; desc.access = WINED3D_RESOURCE_ACCESS_GPU; desc.width = wined3d_texture_get_level_width(src_texture, src_level); diff --git a/include/wine/wined3d.h b/include/wine/wined3d.h index fcbe094e30b..c7c50cc2f0d 100644 --- a/include/wine/wined3d.h +++ b/include/wine/wined3d.h @@ -930,7 +930,7 @@ enum wined3d_memory_segment_group #define WINED3DUSAGE_MASK 0x10007bf0 #define WINED3DUSAGE_SCRATCH 0x00400000 -#define WINED3DUSAGE_PRIVATE 0x00800000 +#define WINED3DUSAGE_CS 0x00800000 #define WINED3DUSAGE_LEGACY_CUBEMAP 0x01000000 #define WINED3DUSAGE_OWNDC 0x02000000 #define WINED3DUSAGE_STATICDECL 0x04000000 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4624
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/wined3d/context_gl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/wined3d/context_gl.c b/dlls/wined3d/context_gl.c index 55546130d35..c4c4d07decb 100644 --- a/dlls/wined3d/context_gl.c +++ b/dlls/wined3d/context_gl.c @@ -4895,7 +4895,7 @@ static void draw_primitive_immediate_mode(struct wined3d_context_gl *context_gl, if (!idx_size && idx_data) ERR("Non-NULL idx_data with 0 idx_size, this should never happen.\n"); - if (instance_count) + if (instance_count && instance_count != 1) FIXME("Instancing not implemented.\n"); /* Immediate mode drawing can't make use of indices in a VBO - get the -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4624
From: Matteo Bruni <mbruni(a)codeweavers.com> More fallout from 2ddb6b66a7cda0bf6aaddc0c6899e35cc92ceee9. --- dlls/wined3d/state.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c index 2fb75785daf..6dfbc0400bc 100644 --- a/dlls/wined3d/state.c +++ b/dlls/wined3d/state.c @@ -3639,6 +3639,8 @@ static void transform_projection(struct wined3d_context *context, const struct w const struct wined3d_gl_info *gl_info = wined3d_context_gl(context)->gl_info; struct wined3d_matrix projection; + TRACE("context %p, state %p, state_id %lu.\n", context, state, state_id); + gl_info->gl_ops.gl.p_glMatrixMode(GL_PROJECTION); checkGLcall("glMatrixMode(GL_PROJECTION)"); @@ -3669,6 +3671,8 @@ static void vertexdeclaration(struct wined3d_context *context, const struct wine BOOL wasrhw = context->last_was_rhw; unsigned int i; + TRACE("context %p, state %p, state_id %lu.\n", context, state, state_id); + transformed = context->stream_info.position_transformed; if (transformed != context->last_was_rhw && !useVertexShaderFunction) updateFog = TRUE; @@ -3685,25 +3689,12 @@ static void vertexdeclaration(struct wined3d_context *context, const struct wine * make sure they're properly set. */ if (!useVertexShaderFunction) { - /* TODO: Move this mainly to the viewport state and only apply when - * the vp has changed or transformed / untransformed was switched. */ if (wasrhw != context->last_was_rhw - && !isStateDirty(context, STATE_TRANSFORM(WINED3D_TS_PROJECTION)) && !isStateDirty(context, STATE_VIEWPORT)) transform_projection(context, state, STATE_TRANSFORM(WINED3D_TS_PROJECTION)); - /* World matrix needs reapplication here only if we're switching between rhw and non-rhw - * mode. - * - * If a vertex shader is used, the world matrix changed and then vertex shader unbound - * this check will fail and the matrix not applied again. This is OK because a simple - * world matrix change reapplies the matrix - These checks here are only to satisfy the - * needs of the vertex declaration. - * - * World and view matrix go into the same gl matrix, so only apply them when neither is - * dirty - */ - if (transformed != wasrhw && !isStateDirty(context, STATE_TRANSFORM(WINED3D_TS_WORLD_MATRIX(0))) - && !isStateDirty(context, STATE_TRANSFORM(WINED3D_TS_VIEW))) + /* World matrix needs reapplication here only if we're switching + * between rhw and non-rhw mode. */ + if (transformed != wasrhw) transform_world(context, state, STATE_TRANSFORM(WINED3D_TS_WORLD_MATRIX(0))); if (!isStateDirty(context, STATE_RENDER(WINED3D_RS_COLORVERTEX))) context_apply_state(context, state, STATE_RENDER(WINED3D_RS_COLORVERTEX)); @@ -3941,8 +3932,9 @@ static void viewport_miscpart_cc(struct wined3d_context *context, static void viewport_vertexpart(struct wined3d_context *context, const struct wined3d_state *state, DWORD state_id) { - if (!isStateDirty(context, STATE_TRANSFORM(WINED3D_TS_PROJECTION))) - transform_projection(context, state, STATE_TRANSFORM(WINED3D_TS_PROJECTION)); + TRACE("context %p, state %p, state_id %lu.\n", context, state, state_id); + + transform_projection(context, state, STATE_TRANSFORM(WINED3D_TS_PROJECTION)); if (!isStateDirty(context, STATE_RENDER(WINED3D_RS_POINTSCALEENABLE)) && state->render_states[WINED3D_RS_POINTSCALEENABLE]) state_pscale(context, state, STATE_RENDER(WINED3D_RS_POINTSCALEENABLE)); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4624
From: Matteo Bruni <mbruni(a)codeweavers.com> Those parameters are not supported on multisample textures in the first place (basically, GL mandates ARB_sampler_objects for those). The combination COND_NP2 + ARB_texture_multisample is not expected to happen without wined3d configuration overrides. --- dlls/wined3d/texture.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 88647a9cdb7..96f77806420 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -1380,7 +1380,8 @@ void wined3d_texture_gl_bind(struct wined3d_texture_gl *texture_gl, gl_info->gl_ops.gl.p_glTexParameteri(target, GL_TEXTURE_WRAP_R, GL_CLAMP_TO_EDGE); } - if (texture_gl->t.flags & WINED3D_TEXTURE_COND_NP2) + if (texture_gl->t.flags & WINED3D_TEXTURE_COND_NP2 && target != GL_TEXTURE_2D_MULTISAMPLE + && target != GL_TEXTURE_2D_MULTISAMPLE_ARRAY) { /* Conditional non power of two textures use a different clamping * default. If we're using the GL_WINE_normalized_texrect partial -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4624
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/d3d9/tests/visual.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dlls/d3d9/tests/visual.c b/dlls/d3d9/tests/visual.c index 2d54920ddba..705e3d8ff9c 100644 --- a/dlls/d3d9/tests/visual.c +++ b/dlls/d3d9/tests/visual.c @@ -26855,6 +26855,7 @@ static void test_sample_attached_rendertarget(void) unsigned int color, i; IDirect3D9 *d3d; ULONG refcount; + D3DCAPS9 caps; BOOL is_warp; HWND window; HRESULT hr; @@ -26904,6 +26905,17 @@ static void test_sample_attached_rendertarget(void) return; } + hr = IDirect3DDevice9_GetDeviceCaps(device, &caps); + ok(hr == D3D_OK, "Got unexpected hr %#lx.\n", hr); + if (caps.PixelShaderVersion < D3DPS_VERSION(2, 0)) + { + skip("No shader model 2 support, skipping tests.\n"); + IDirect3DDevice9_Release(device); + IDirect3D9_Release(d3d); + DestroyWindow(window); + return; + } + hr = IDirect3DDevice9_CreateQuery(device, D3DQUERYTYPE_EVENT, NULL); if (hr == D3DERR_NOTAVAILABLE) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4624
```diff - if (instance_count) + if (instance_count && instance_count != 1) ```
I.e., "if (instance_count > 1)", right? Is drawing a single instance completely equivalent to a non-instanced draw?
``` Subject: [PATCH 3/5] wined3d: Don't skip FFP projection transform update.
More fallout from 2ddb6b66a7cda0bf6aaddc0c6899e35cc92ceee9. ```
I can roughly guess what the issue here is, but ideally someone reading the patch wouldn't have to; the helpful thing to do would be to describe the scenario that would cause the transformation update to be incorrectly skipped. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55330
```diff - if (instance_count) + if (instance_count && instance_count != 1) ```
I.e., "if (instance_count > 1)", right? Is drawing a single instance completely equivalent to a non-instanced draw?
Err, yes, that condition can definitely be simplified. Good point about the difference between the two. I guess in general that can make a difference for instanced vertex attributes, so probably this patch is no good. I should probably explain what prompted this patch in the first place, which is d3d9's DrawPrimitive(). It calls wined3d_device_context_draw() with an instance count of 1 and it has a comment explicitly mentioning how instancing is not a thing for non-indexed draws. FWIW, instance count seems to be specified as 0 for non-indexed draws in all the other d3d versions. That means that every DrawPrimitive() call in d3d9 potentially prints this FIXME(). Is that proper, or should d3d9 pass 0 as instance_count for non-indexed draws? I'm not sure if the tests check this case (FWIW the d3d9 tests still pass with 0 instance_count). I can look further into it and add an appropriate testcase if needed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55421
I should probably explain what prompted this patch in the first place, which is d3d9's DrawPrimitive(). It calls wined3d_device_context_draw() with an instance count of 1 and it has a comment explicitly mentioning how instancing is not a thing for non-indexed draws. FWIW, instance count seems to be specified as 0 for non-indexed draws in all the other d3d versions.
Actually, I remember that, that's from !2282. I don't think it was ever clarified whether 0 or 1 would be the right instance count here, but I suspect it should 0. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55424
``` Subject: [PATCH 3/5] wined3d: Don't skip FFP projection transform update.
More fallout from 2ddb6b66a7cda0bf6aaddc0c6899e35cc92ceee9. ```
I can roughly guess what the issue here is, but ideally someone reading the patch wouldn't have to; the helpful thing to do would be to describe the scenario that would cause the transformation update to be incorrectly skipped.
I sent a few patches like that in the past. Would simply referencing the first commit in the "series" (e106bbdd39a5f27bce028ed992033eb0a5635f60) be good enough or do you think I should rehash that commit message for this particular new case? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55428
On Thu Dec 7 20:11:57 2023 +0000, Henri Verbeet wrote:
I should probably explain what prompted this patch in the first place, which is d3d9's DrawPrimitive(). It calls wined3d_device_context_draw() with an instance count of 1 and it has a comment explicitly mentioning how instancing is not a thing for non-indexed draws. FWIW, instance count seems to be specified as 0 for non-indexed draws in all the other d3d versions. Actually, I remember that, that's from !2282. I don't think it was ever clarified whether 0 or 1 would be the right instance count here, but I suspect it should 0. Ah, right! I'll try to write a test.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55433
I sent a few patches like that in the past. Would simply referencing the first commit in the "series" (e106bbdd39a5f27bce028ed992033eb0a5635f60) be good enough or do you think I should rehash that commit message for this particular new case?
Referencing e106bbdd39a5f27bce028ed992033eb0a5635f60 would be fine with me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55437
Fine with me. I'm planning to destroy the state table in the medium term anyway ;-) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4624#note_55438
participants (4)
-
Henri Verbeet (＠hverbeet) -
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Zebediah Figura (＠zfigura)