[PATCH 3/3] wined3d: Handle NOOVERWRITE maps on persistently mapped Vulkan buffers from the client thread.

[Henri Verbeet]

On Mon, 11 Oct 2021 at 05:53, Zebediah Figura <zfigura at codeweavers.com> wrote:
> >> +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.
> >
>
> I suppose your expectation is that it invalidates the map contents,
> which it does not, but rather it invalidates the map pointer. I don't
> know how better to name it to clarify that difference. (Shove a _ptr
> onto the end, maybe?)
>
> Essentially it means "the CS thread might reallocate the BO behind our
> back, thereby invalidating bo->map_ptr, which means we can't treat this
> one as canonical anymore". It's definitely awkward, but I don't see a
> better way to achieve this...
>
Part of the issue is that it operates on a wined3d_resource, I think.
You can tell it actually modifies the client sub-resource by looking
at the implementation, but ideally you wouldn't have to. On the other
hand, the fact that it's a persistent map is somewhat immaterial. I
guess I would have expected something like
invalidate_client_address(), or perhaps
wined3d_client_sub_resource_invalidate_address().

> >> @@ -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()?
> >
>
> As it stands this patch never has both FLUSH_ON_UNMAP and
> UPLOAD_ON_UNMAP set at the same time. I essentially was thinking of them
> as different things to do on an unmap. I guess in that sense "UPLOAD"
> should really have been "COPY" or "BLIT" or something.
>
That probably works. Alternatively, could we just drop FLUSH_ON_UNMAP
and set UPLOAD_ON_UNMAP any time we want to call
wined3d_device_context_upload_bo()?