[PATCH 0/5] MR657: wined3d: Managed texture patches.
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/texture.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 15d87e02183..10a8dcb06f4 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -1670,7 +1670,6 @@ void wined3d_texture_load(struct wined3d_texture *texture, || (texture->async.flags & WINED3D_TEXTURE_ASYNC_COLOR_KEY && !color_key_equal(&texture->async.gl_color_key, &texture->async.src_blt_color_key)))) { - unsigned int sub_count = texture->level_count * texture->layer_count; unsigned int i; TRACE("Reloading because of color key value change.\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/657
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/device.c | 12 ++----- dlls/wined3d/texture.c | 61 +++++++++++----------------------- dlls/wined3d/wined3d_private.h | 2 ++ 3 files changed, 24 insertions(+), 51 deletions(-) diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index f215da6d310..2cd558051d1 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -4905,17 +4905,11 @@ HRESULT CDECL wined3d_device_context_copy_sub_resource_region(struct wined3d_dev unsigned int src_level = src_sub_resource_idx % src_texture->level_count; unsigned int src_row_block_count, src_row_count; - if (dst_sub_resource_idx >= dst_texture->level_count * dst_texture->layer_count) - { - WARN("Invalid destination sub-resource %u.\n", dst_sub_resource_idx); + if (!wined3d_texture_validate_sub_resource_idx(dst_texture, dst_sub_resource_idx)) return WINED3DERR_INVALIDCALL; - } - if (src_sub_resource_idx >= src_texture->level_count * src_texture->layer_count) - { - WARN("Invalid source sub-resource %u.\n", src_sub_resource_idx); + if (!wined3d_texture_validate_sub_resource_idx(src_texture, src_sub_resource_idx)) return WINED3DERR_INVALIDCALL; - } if (dst_texture->sub_resources[dst_sub_resource_idx].map_count) { @@ -5369,7 +5363,7 @@ HRESULT CDECL wined3d_device_set_cursor_properties(struct wined3d_device *device TRACE("device %p, x_hotspot %u, y_hotspot %u, texture %p, sub_resource_idx %u.\n", device, x_hotspot, y_hotspot, texture, sub_resource_idx); - if (sub_resource_idx >= texture->level_count * texture->layer_count + if (!wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx) || texture->resource.type != WINED3D_RTYPE_TEXTURE_2D) return WINED3DERR_INVALIDCALL; diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 10a8dcb06f4..09b26933353 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -45,6 +45,15 @@ struct wined3d_rect_f float b; }; +bool wined3d_texture_validate_sub_resource_idx(const struct wined3d_texture *texture, unsigned int sub_resource_idx) +{ + if (sub_resource_idx < texture->level_count * texture->layer_count) + return true; + + WARN("Invalid sub-resource index %u.\n", sub_resource_idx); + return false; +} + BOOL wined3d_texture_can_use_pbo(const struct wined3d_texture *texture, const struct wined3d_d3d_info *d3d_info) { if (!d3d_info->pbo || texture->resource.format->conv_byte_count || texture->resource.pin_sysmem @@ -2212,16 +2221,10 @@ BOOL wined3d_texture_prepare_location(struct wined3d_texture *texture, static struct wined3d_texture_sub_resource *wined3d_texture_get_sub_resource(struct wined3d_texture *texture, unsigned int sub_resource_idx) { - UINT sub_count = texture->level_count * texture->layer_count; - TRACE("texture %p, sub_resource_idx %u.\n", texture, sub_resource_idx); - if (sub_resource_idx >= sub_count) - { - WARN("sub_resource_idx %u >= sub_count %u.\n", sub_resource_idx, sub_count); + if (!wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return NULL; - } - return &texture->sub_resources[sub_resource_idx]; } @@ -4127,11 +4130,11 @@ HRESULT CDECL wined3d_device_context_blt(struct wined3d_device_context *context, context, dst_texture, dst_sub_resource_idx, wine_dbgstr_rect(dst_rect), src_texture, src_sub_resource_idx, wine_dbgstr_rect(src_rect), flags, fx, debug_d3dtexturefiltertype(filter)); - if (dst_sub_resource_idx >= dst_texture->level_count * dst_texture->layer_count + if (!wined3d_texture_validate_sub_resource_idx(dst_texture, dst_sub_resource_idx) || dst_texture->resource.type != WINED3D_RTYPE_TEXTURE_2D) return WINED3DERR_INVALIDCALL; - if (src_sub_resource_idx >= src_texture->level_count * src_texture->layer_count + if (!wined3d_texture_validate_sub_resource_idx(src_texture, src_sub_resource_idx) || src_texture->resource.type != WINED3D_RTYPE_TEXTURE_2D) return WINED3DERR_INVALIDCALL; @@ -4179,11 +4182,8 @@ HRESULT CDECL wined3d_texture_get_overlay_position(const struct wined3d_texture TRACE("texture %p, sub_resource_idx %u, x %p, y %p.\n", texture, sub_resource_idx, x, y); if (!(texture->resource.usage & WINED3DUSAGE_OVERLAY) - || sub_resource_idx >= texture->level_count * texture->layer_count) - { - WARN("Invalid sub-resource specified.\n"); + || !wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return WINEDDERR_NOTAOVERLAYSURFACE; - } overlay = &texture->overlay_info[sub_resource_idx]; if (!overlay->dst_texture) @@ -4211,11 +4211,8 @@ HRESULT CDECL wined3d_texture_set_overlay_position(struct wined3d_texture *textu TRACE("texture %p, sub_resource_idx %u, x %d, y %d.\n", texture, sub_resource_idx, x, y); if (!(texture->resource.usage & WINED3DUSAGE_OVERLAY) - || sub_resource_idx >= texture->level_count * texture->layer_count) - { - WARN("Invalid sub-resource specified.\n"); + || !wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return WINEDDERR_NOTAOVERLAYSURFACE; - } overlay = &texture->overlay_info[sub_resource_idx]; w = overlay->dst_rect.right - overlay->dst_rect.left; @@ -4238,18 +4235,12 @@ HRESULT CDECL wined3d_texture_update_overlay(struct wined3d_texture *texture, un dst_sub_resource_idx, wine_dbgstr_rect(dst_rect), flags); if (!(texture->resource.usage & WINED3DUSAGE_OVERLAY) || texture->resource.type != WINED3D_RTYPE_TEXTURE_2D - || sub_resource_idx >= texture->level_count * texture->layer_count) - { - WARN("Invalid sub-resource specified.\n"); + || !wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return WINEDDERR_NOTAOVERLAYSURFACE; - } if (!dst_texture || dst_texture->resource.type != WINED3D_RTYPE_TEXTURE_2D - || dst_sub_resource_idx >= dst_texture->level_count * dst_texture->layer_count) - { - WARN("Invalid destination sub-resource specified.\n"); + || wined3d_texture_validate_sub_resource_idx(dst_texture, dst_sub_resource_idx)) return WINED3DERR_INVALIDCALL; - } overlay = &texture->overlay_info[sub_resource_idx]; @@ -4298,15 +4289,10 @@ HRESULT CDECL wined3d_texture_update_overlay(struct wined3d_texture *texture, un void * CDECL wined3d_texture_get_sub_resource_parent(struct wined3d_texture *texture, unsigned int sub_resource_idx) { - unsigned int sub_count = texture->level_count * texture->layer_count; - TRACE("texture %p, sub_resource_idx %u.\n", texture, sub_resource_idx); - if (sub_resource_idx >= sub_count) - { - WARN("sub_resource_idx %u >= sub_count %u.\n", sub_resource_idx, sub_count); + if (!wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return NULL; - } return texture->sub_resources[sub_resource_idx].parent; } @@ -4314,15 +4300,10 @@ void * CDECL wined3d_texture_get_sub_resource_parent(struct wined3d_texture *tex void CDECL wined3d_texture_set_sub_resource_parent(struct wined3d_texture *texture, unsigned int sub_resource_idx, void *parent) { - unsigned int sub_count = texture->level_count * texture->layer_count; - TRACE("texture %p, sub_resource_idx %u, parent %p.\n", texture, sub_resource_idx, parent); - if (sub_resource_idx >= sub_count) - { - WARN("sub_resource_idx %u >= sub_count %u.\n", sub_resource_idx, sub_count); + if (!wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return; - } texture->sub_resources[sub_resource_idx].parent = parent; } @@ -4330,17 +4311,13 @@ void CDECL wined3d_texture_set_sub_resource_parent(struct wined3d_texture *textu HRESULT CDECL wined3d_texture_get_sub_resource_desc(const struct wined3d_texture *texture, unsigned int sub_resource_idx, struct wined3d_sub_resource_desc *desc) { - unsigned int sub_count = texture->level_count * texture->layer_count; const struct wined3d_resource *resource; unsigned int level_idx; TRACE("texture %p, sub_resource_idx %u, desc %p.\n", texture, sub_resource_idx, desc); - if (sub_resource_idx >= sub_count) - { - WARN("sub_resource_idx %u >= sub_count %u.\n", sub_resource_idx, sub_count); + if (!wined3d_texture_validate_sub_resource_idx(texture, sub_resource_idx)) return WINED3DERR_INVALIDCALL; - } resource = &texture->resource; desc->format = resource->format->id; diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 1dbd90067f5..a87471fd5d1 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -4690,6 +4690,8 @@ void wined3d_texture_upload_from_texture(struct wined3d_texture *dst_texture, un unsigned int src_sub_resource_idx, const struct wined3d_box *src_box) DECLSPEC_HIDDEN; void wined3d_texture_validate_location(struct wined3d_texture *texture, unsigned int sub_resource_idx, DWORD location) DECLSPEC_HIDDEN; +bool wined3d_texture_validate_sub_resource_idx(const struct wined3d_texture *texture, + unsigned int sub_resource_idx) DECLSPEC_HIDDEN; void wined3d_texture_clear_dirty_regions(struct wined3d_texture *texture) DECLSPEC_HIDDEN; HRESULT wined3d_texture_no3d_init(struct wined3d_texture *texture_no3d, struct wined3d_device *device, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/657
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/device.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index 2cd558051d1..e214c47f071 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -5521,8 +5521,23 @@ void CDECL wined3d_device_evict_managed_resources(struct wined3d_device *device) if ((resource->usage & WINED3DUSAGE_MANAGED) && !resource->map_count) { - TRACE("Evicting %p.\n", resource); - wined3d_cs_emit_unload_resource(device->cs, resource); + if (resource->access & WINED3D_RESOURCE_ACCESS_GPU) + { + TRACE("Evicting %p.\n", resource); + wined3d_cs_emit_unload_resource(device->cs, resource); + } + + if (resource->type != WINED3D_RTYPE_BUFFER) + { + struct wined3d_texture *texture = texture_from_resource(resource); + unsigned int i; + + if (texture->dirty_regions) + { + for (i = 0; i < texture->layer_count; ++i) + wined3d_texture_add_dirty_region(texture, i, NULL); + } + } } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/657
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=121370 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply
From: Zebediah Figura <zfigura(a)codeweavers.com> This was probably added on the assumption that IDirect3DDevice8::CopyRects() behaves like the similar IDirect3DDevice9::UpdateSurface(), but it does not. --- dlls/d3d8/device.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/dlls/d3d8/device.c b/dlls/d3d8/device.c index b8193a83cbc..5113e11fb33 100644 --- a/dlls/d3d8/device.c +++ b/dlls/d3d8/device.c @@ -1346,9 +1346,6 @@ static HRESULT WINAPI d3d8_device_CopyRects(IDirect3DDevice8 *iface, TRACE("iface %p, src_surface %p, src_rects %p, rect_count %u, dst_surface %p, dst_points %p.\n", iface, src_surface, src_rects, rect_count, dst_surface, dst_points); - /* Check that the source texture is in WINED3D_POOL_SYSTEM_MEM and the - * destination texture is in WINED3D_POOL_DEFAULT. */ - wined3d_mutex_lock(); wined3d_texture_get_sub_resource_desc(src->wined3d_texture, src->sub_resource_idx, &wined3d_desc); if (wined3d_desc.bind_flags & WINED3D_BIND_DEPTH_STENCIL) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/657
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=121371 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/d3d8/tests/visual.c | 2 +- dlls/wined3d/texture.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/d3d8/tests/visual.c b/dlls/d3d8/tests/visual.c index b51760544c4..1f6e91609db 100644 --- a/dlls/d3d8/tests/visual.c +++ b/dlls/d3d8/tests/visual.c @@ -5726,7 +5726,7 @@ static void add_dirty_rect_test(void) ok(hr == S_OK, "Failed to update texture, hr %#lx.\n", hr); add_dirty_rect_test_draw(device); color = getPixelColor(device, 320, 240); - todo_wine ok(color_match(color, 0x00ff00ff, 1), "Got unexpected color 0x%08x.\n", color); + ok(color_match(color, 0x00ff00ff, 1), "Got unexpected color 0x%08x.\n", color); /* Tests with managed textures. */ fill_surface(surface_managed0, 0x00ff0000, 0); diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 09b26933353..88404c9f8a8 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -4171,6 +4171,9 @@ HRESULT CDECL wined3d_device_context_blt(struct wined3d_device_context *context, wined3d_device_context_emit_blt_sub_resource(&dst_texture->resource.device->cs->c, &dst_texture->resource, dst_sub_resource_idx, &dst_box, &src_texture->resource, src_sub_resource_idx, &src_box, flags, fx, filter); + if (dst_texture->dirty_regions) + wined3d_texture_add_dirty_region(dst_texture, dst_sub_resource_idx, &dst_box); + return WINED3D_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/657
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=121372 Your paranoid android. === build (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/wined3d/device.c:5521 Task: Patch failed to apply
From patch 2/5: ```diff +bool wined3d_texture_validate_sub_resource_idx(const struct wined3d_texture *texture, unsigned int sub_resource_idx) +{ + if (sub_resource_idx < texture->level_count * texture->layer_count) + return true; + + WARN("Invalid sub-resource index %u.\n", sub_resource_idx); + return false; +} ```
It's fairly inconsequential, but `wined3d_texture_validate_sub_resource_idx()` sounds like something that modifies the texture. (Compare e.g. `wined3d_texture_validate_location()`.)
From patch 3/5: ```diff @@ -5521,8 +5521,23 @@ void CDECL wined3d_device_evict_managed_resources(struct wined3d_device *device)
if ((resource->usage & WINED3DUSAGE_MANAGED) && !resource->map_count) { - TRACE("Evicting %p.\n", resource); - wined3d_cs_emit_unload_resource(device->cs, resource); + if (resource->access & WINED3D_RESOURCE_ACCESS_GPU) + { + TRACE("Evicting %p.\n", resource); + wined3d_cs_emit_unload_resource(device->cs, resource); + } ```
Doesn't `WINED3DUSAGE_MANAGED` imply `WINED3D_RESOURCE_ACCESS_GPU`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/657#note_6408
On 8/17/22 07:34, Henri Verbeet (@hverbeet) wrote:
From patch 2/5:
```diff +bool wined3d_texture_validate_sub_resource_idx(const struct wined3d_texture *texture, unsigned int sub_resource_idx) +{ + if (sub_resource_idx < texture->level_count * texture->layer_count) + return true; + + WARN("Invalid sub-resource index %u.\n", sub_resource_idx); + return false; +} ```
It's fairly inconsequential, but `wined3d_texture_validate_sub_resource_idx()` sounds like something that modifies the texture. (Compare e.g. `wined3d_texture_validate_location()`.)
Perhaps, although I'm not sure how to distinguish "make valid" from "verify validity", and in the absence of other code I would typically assume the latter. Perhaps "verify" rather than "validate"?
From patch 3/5:
```diff @@ -5521,8 +5521,23 @@ void CDECL wined3d_device_evict_managed_resources(struct wined3d_device *device)
if ((resource->usage & WINED3DUSAGE_MANAGED) && !resource->map_count) { - TRACE("Evicting %p.\n", resource); - wined3d_cs_emit_unload_resource(device->cs, resource); + if (resource->access & WINED3D_RESOURCE_ACCESS_GPU) + { + TRACE("Evicting %p.\n", resource); + wined3d_cs_emit_unload_resource(device->cs, resource); + } ```
Doesn't `WINED3DUSAGE_MANAGED` imply `WINED3D_RESOURCE_ACCESS_GPU`?
At the moment, but the plan is to separate the texture into CPU and GPU copies, and at the moment at least it's more convenient to mark both copies with the MANAGED flag. [In particular: d3d8_surface currently holds a reference to the sysmem texture, and this lets GetDesc() work without any extra work. Also, we currently check the MANAGED flag to determine whether to pin sysmem, but we should probably be pinning sysmem for all CPU-accessible textures.] The above diff probably shouldn't have been part of this patch, though, and probably was combined with it by mistake.
It's fairly inconsequential, but `wined3d_texture_validate_sub_resource_idx()` sounds like something that modifies the texture. (Compare e.g. `wined3d_texture_validate_location()`.)
Perhaps, although I'm not sure how to distinguish "make valid" from "verify validity", and in the absence of other code I would typically assume the latter. Perhaps "verify" rather than "validate"?
We'd typically use e.g. `wined3d_texture_is_valid_sub_resource_idx()` or some variant on that. Perhaps `wined3d_texture_sub_resource_idx_valid()` would also work, although it's a bit more ambiguous.
Doesn't `WINED3DUSAGE_MANAGED` imply `WINED3D_RESOURCE_ACCESS_GPU`?
At the moment, but the plan is to separate the texture into CPU and GPU copies, and at the moment at least it's more convenient to mark both copies with the MANAGED flag.
If that implication is going away this probably makes sense, but yes, it would have been cleaner separate from this patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/657#note_6421
This merge request was approved by Jan Sikorski. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/657
participants (6)
-
Henri Verbeet (＠hverbeet) -
Jan Sikorski (＠jsikorski)
-
Marvin -
Zebediah Figura
-
Zebediah Figura (＠zfigura)
-
Zebediah Figura (she/her)