On Thu, 7 Oct 2021 at 04:22, Zebediah Figura zfigura@codeweavers.com wrote:
+static void adapter_vk_flush_bo_address(struct wined3d_context *context,
const struct wined3d_const_bo_address *data, size_t size)+{
- struct wined3d_context_vk *context_vk = wined3d_context_vk(context);
- const struct wined3d_vk_info *vk_info;
- struct wined3d_device_vk *device_vk;
- VkMappedMemoryRange range;
- struct wined3d_bo_vk *bo;
- if (!(bo = (struct wined3d_bo_vk *)data->buffer_object))
return;- vk_info = context_vk->vk_info;
- device_vk = wined3d_device_vk(context->device);
- range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE;
- range.pNext = NULL;
- range.memory = bo->vk_memory;
- range.offset = bo->memory_offset + (uintptr_t)data->addr;
- range.size = size;
- VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range));
+}
That has a fair amount of overlap with adapter_vk_unmap_bo_address(). Would a common helper make sense?
+static void *adapter_vk_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource,
unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch,unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo)+{
- wined3d_not_from_cs(device->cs);
- /* Limit NOOVERWRITE maps to buffers for now; there are too many ways that
* a texture can be invalidated to even count. */- if (wined3d_map_persistent() && resource->type == WINED3D_RTYPE_BUFFER && (flags & WINED3D_MAP_NOOVERWRITE))
- {
struct wined3d_client_sub_resource *sub_resource;const struct wined3d_bo_vk *bo_vk;uint8_t *map_ptr;sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx);bo_vk = (const struct wined3d_bo_vk *)sub_resource->addr.buffer_object;map_ptr = bo_vk ? bo_vk->map_ptr : NULL;map_ptr += (uintptr_t)sub_resource->addr.addr;if (!map_ptr){TRACE("Sub-resource is not persistently mapped.\n");return NULL;}wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, row_pitch, slice_pitch);upload_bo->addr = *wined3d_const_bo_address(&sub_resource->addr);upload_bo->flags = 0;if (bo_vk)map_ptr += bo_vk->memory_offset;map_ptr = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box);if (!(bo_vk->memory_type & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT))upload_bo->flags |= UPLOAD_BO_FLUSH_ON_UNMAP;return map_ptr;- }
- return NULL;
+}
Do we need adapter_vk_alloc_upload_bo() as such? I suppose it would make sense to expose something like wined3d_context_vk_create_bo() through the adapter ops, but the "upload bo" mechanism seems more generic than that. In its current form, this also doesn't allocate all that much, and doesn't seem that all that specific to the Vulkan backend. We use the wined3d_bo_vk structure, but the "map_ptr" and "memory_offset" would apply just as well to a GL equivalent. I.e., if we introduced a common wined3d_bo structure, this could just be in wined3d_cs_map_upload_bo().
+static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{
- struct wined3d_client_sub_resource *sub_resource;
- sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx);
- memset(&sub_resource->addr, 0, sizeof(sub_resource->addr));
+}
Is this function correctly named? It doesn't quite do what I'd expect something called invalidate_persistent_map() to do.
@@ -2511,6 +2539,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context,
wined3d_resource_wait_idle(resource);
- /* We might end up loading the resource into sysmem. */
- invalidate_persistent_map(resource, sub_resource_idx);
Sure, we might end up loading the resource into sysmem. That doesn't necessarily explain why we would need to call invalidate_persistent_map() though. (I.e., the issue is with invalidating the BUFFER location, not so much with loading into the SYSMEM location.)
@@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch;
wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch);
if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP)
if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK;
Uploading when FLUSH_ON_UNMAP is set seems odd. I.e., should there ever be a case where FLUSH_ON_UNMAP is set, UPLOAD_ON_UNMAP is not set, and we still want to call wined3d_device_context_upload_bo()?
@@ -2581,8 +2612,7 @@ static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void * if (op->dst_resource->type == WINED3D_RTYPE_BUFFER) { wined3d_buffer_copy(buffer_from_resource(op->dst_resource), op->dst_box.left,
buffer_from_resource(op->src_resource), op->src_box.left,op->src_box.right - op->src_box.left);
buffer_from_resource(op->src_resource), op->src_box.left, op->src_box.right - op->src_box.left);
That's a whitespace change. The change is not particularly wrong, but it doesn't seem particularly relevant to the rest of this patch either.
- /* This might result in an implicit discard. */
- if (dst_resource->type == WINED3D_RTYPE_BUFFER && dst_box->right - dst_box->left == dst_resource->size)
invalidate_persistent_map(dst_resource, 0);
Somewhat like the other invalidate_persistent_map() comment above, it would require someone to already be fairly familiar with the code in question to understand the comment. (I.e., the issue here is that if the CS thread does end up doing a DISCARD for this operation, any subsequent operations would need to reference the new bo instead of the current one.)
@@ -2727,8 +2761,22 @@ static void wined3d_cs_exec_update_sub_resource(struct wined3d_cs *cs, const voi if (resource->type == WINED3D_RTYPE_BUFFER) { struct wined3d_buffer *buffer = buffer_from_resource(resource);
size_t size = box->right - box->left;
wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, box->right - box->left);
if (op->bo.flags & UPLOAD_BO_FLUSH_ON_UNMAP){wined3d_context_flush_bo_address(context, &op->bo.addr, size);}else{wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, size);}goto done;- }
It seems like we're mostly using FLUSH_ON_UNMAP here to determine whether "op->bo.addr" refers to the same GPU memory as "buffer->buffer_object". Could we perhaps just compare them?
static bool wined3d_cs_unmap_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_box *box, struct upload_bo *bo) {
- struct wined3d_client_sub_resource *sub_resource;
- sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx);
- if (sub_resource->upload_bo.addr.buffer_object || sub_resource->upload_bo.addr.addr)
- {
I guess that works; could perhaps do with a helper.
*bo = sub_resource->upload_bo;*box = sub_resource->upload_box;memset(&sub_resource->upload_bo, 0, sizeof(sub_resource->upload_bo));memset(&sub_resource->upload_box, 0, sizeof(sub_resource->upload_box));return true;- }
- return false;
}
We might as well just return if the condition is false, and save some indentation.
+struct wined3d_client_sub_resource +{
- struct wined3d_bo_address addr;
- struct upload_bo upload_bo;
- struct wined3d_box upload_box;
+};
I'm tempted to think we want to track this per resource instead of per sub-resource. For buffers it doesn't really matter though, and for textures we don't implement it yet. Perhaps we should just leave textures alone for now? That would also avoid wined3d_resource_get_client_sub_resource() for the moment.