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

[Zebediah Figura]

On 10/11/21 8:43 AM, Henri Verbeet wrote:
> 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().

Sure, that makes sense.

> 
>>>> @@ -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()?
> 

Right, that one's also fine with me.