[PATCH 1/2] wined3d: Implement wined3d_deferred_context_update_sub_resource().

[Matteo Bruni]

On Fri, Jun 11, 2021 at 5:40 AM Zebediah Figura (she/her)
<zfigura at codeweavers.com> wrote:
>
> On 5/31/21 6:43 PM, Henri Verbeet wrote:
> > On Mon, 31 May 2021 at 23:45, Zebediah Figura (she/her)
> > <zfigura at codeweavers.com> wrote:
> >> On 5/31/21 4:53 AM, Henri Verbeet wrote:
> >>> My (very rough) proposal for this would be to replace the "data" field
> >>> in the wined3d_cs_update_sub_resource structure with a
> >>> wined3d_bo_address, and then introduce something like the following:
> >>>
> >>>       static bool wined3d_device_context_get_upload_bo(struct
> >>> wined3d_device_context *context,
> >>>               struct wined3d_resource *resource, unsigned int
> >>> sub_resource_idx, const struct wined3d_box *box,
> >>>               uint32_t flags, struct wined3d_bo_address *address, void **map_ptr)
> >>>       {
> >>>           ...
> >>>       }
> >>
> >> This function strikes me as more than a little confusing, party because
> >> it seems to be returning two partially overlapping things, with mostly
> >> different usages...
> >>
> >> i.e. I'd presume the idea is for
> >> wined3d_device_context_update_sub_resource() to retrieve this and then
> >> call something like wined3d_context_copy_bo_address() to copy from
> >> sysmem, which makes sense, but then what are we doing with map_ptr?
> >>
> > Well, wined3d_context_copy_bo_address() needs a wined3d_context, which
> > you typically don't have on application threads.
> >
> > The basic idea is that you'd pass "address" to
> > WINED3D_CS_OP_UPDATE_SUB_RESOURCE, and "map_ptr " to the application.
> > (In case of map/unmap; for update_sub_resource() you'd just copy the
> > data into "map_ptr".)
> >
> >> Or, if we use this in wined3d_device_context_map(), couldn't we just
> >> call wined3d_context_map_bo_address() on the returned wined3d_bo_address?
> >>
> > Conceptually, sure, you could split this into allocate/map/unmap. In
> > practice though, you'll always want the upload buffer to be mapped, in
> > order to copy data into it from system memory. And in fact, if we're
> > going to return a GPU buffer here, it should generally already be
> > mapped. (For various reasons; for 64-bit applications, we use
> > persistent maps if we can; for OpenGL, we want to avoid making GL
> > calls from application threads; if we have a dedicated upload buffer
> > belonging to the device context, it would have been mapped during
> > creation.) When needed, unmap can happen when the upload buffer is
> > consumed.
>
> Thanks for explaining the rationale there, that does help a lot.
>
> It still feels awkward to me to pass the same information maybe twice,
> but I guess the wined3d_bo_address part should basically be considered
> opaque above the CS thread.

Yes, as I understand it that's to be used by wined3d to keep track of
the lifetime of the memory block.

> >>> For deferred contexts you should then be able to do something
> >>> (conceptually) very similar to patch 2/2, and in the end we may not
> >>> even need map/unmap/update_sub_resource in the
> >>> wined3d_device_context_ops structure.
> >>>
> >> We'd still presumably need an unmap, though, unless the idea above is to
> >> *always* allocate new (GPU or CPU) memory, which seems like a much
> >> bigger step.
> >>
> > Not necessarily, but wined3d_cs_exec_update_sub_resource() can unmap if needed.
> >
> >> Unless I'm misreading things, this also seems to imply that
> >> *_resource_sub_resource_map() shouldn't exist anymore, right?
> >>
> > Non-discard maps still exist, and
> > wined3d_device_context_get_upload_bo() wouldn't be used for those.
> >
>
> So the idea goes like this:
>
> (1) Pass a whole wined3d_bo_address as part of struct
> wined3d_cs_update_sub_resource.
>
> (2) Introduce wined3d_device_context_get_upload_bo() to allocate memory.
> At first, only use it for deferred contexts.
>
> (3) Use wined3d_device_context_get_upload_bo() for immediate contexts as
> well, for both DISCARD and NOOVERWRITE maps and for update_sub_resource.
> This implies always allocating new CPU/GPU memory, going through the
> DEFAULT queue, and not waiting for the upload to complete.

wined3d_device_context_get_upload_bo() should probably avoid the
queues entirely, at least in the normal case. More below.

> The idea that map/unmap/update_sub_resource can go away seems to imply
> (3) to me, but I'm more than a little curious as to whether it's
> actually desirable in all cases. Even with persistent mapping you still
> need to allocate new memory, which isn't exactly cheap, especially with
> large resources.

You don't need to actually allocate new memory every time, just return
memory that you know is available at the moment. The specific
mechanism used by wined3d_device_get_upload_bo() isn't important to
the callers but yes, the API we want would in fact return a new,
independent memory area for each DISCARD map. It's the only way to
make things fast with constant buffer updates, to name the most
critical example. For some of those you'll easily have thousands of
DISCARD maps per frame and if you don't want to block on buffer maps
and e.g. you have one frame latency to play with, that means having
thousands of "instances" of each buffer moving around at any given
time.
WRT the mechanism, it might be allocating a large chunk of system
memory and suballocating from it, keeping track of the lifetime of
each chunk as it is returned to the application and then back to
wined3d. Or suballocating from GPU memory. It could even be creating a
ton of GL buffers and flipping them around as needed, it doesn't
matter; the idea is that each memory chunk can be reused, once we know
the memory is available again because it's not used by wined3d (e.g.
if it's a block of system memory that's been uploaded to the GPU) or
the GPU (e.g. if we directly returned the memory where the GPU
resource is stored) anymore. Ideally, when the application is in a
steady state, there should be next to no actual memory allocation or
GL / Vulkan resource creation. But, in principle, everything would
work just as well (if not as fast) if we allocated system memory every
time wined3d_device_context_get_upload_bo() is called and freed it
once wined3d is done with uploading the data that was stored into it
by the application.

FWIW, the DYNAMIC d3d usage is supposed to flag the resources that are
going to be updated frequently and are performance sensitive. In
principle we could reserve wined3d_device_context_get_upload_bo() to
DYNAMIC resources only.

> And then there's the aforementioned non-discard maps—don't we still need
> an explicit WINED3D_CS_OP_UNMAP for those? Or any case (are there any?)
> where we don't want to persistently map a resource?

We might want to keep the existing CS operations for these cases,
sure. I suspect it will become clearer further down the road.