On 10/8/21 11:23 AM, Henri Verbeet wrote:
On Thu, 7 Oct 2021 at 04:04, Zebediah Figura zfigura@codeweavers.com wrote:
Partially undoes 61024aa12f.
We want to be able to allocate a new upload BO for a given map, but it's a bit structurally awkward to ask for a specific memory layout when parepare_upload_bo() might return different ones depending on the flags and other factors.
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
This is kind of janky, though. It might not be all that unreasonable to ask prepare_upload_bo() for a specific layout.
Right; it's not spelled out above, but the issue is that in some cases we may want to return an existing allocation, in which case we wouldn't have control over its layout. The options are essentially to either ignore the requested layout in those cases, or to simply always let map_upload_bo() pick an appropriate layout. This patch (in combination with the following in the series) effectively does the latter. I suppose the question is, are there ever cases where we'd need a different layout than would be returned by map_upload_bo() by default?
Right, that. I failed to correctly articulate the actual requirements.
I don't believe there are actually any cases where we need a different layout that map_upload_bo() returns by default, although that's partly because we can control what map_upload_bo() returns by default. In a sense that's how it used to work.
Fundamentally that means the map pitch of the entire resource for maps, and "anything, really" for update_sub_resource(). Since that's the fundamental difference, I wanted to deal with the problem of map pitch in those specific functions, rather than letting it be inferred from what essentially amounts to the flags we pass to prepare_upload_bo() [i.e. whether they include either DISCARD or NOOVERWRITE]. Though this is also why I even now am not particularly happy about the existence of that function as a combined entity :-/
But then on the other hand, it's kind of ugly to ask for a specific layout when map_upload_bo() might return an existing allocation.
I guess one reason why it's better to let map_upload_bo() choose the pitch is that in the case of update_sub_resource we could potentially pick a staging texture of any size [not that I'm sure we have any reason to do that.] Or return a prior discard map if the resource hadn't been touched in the meantime [not that there's any reason for an application to do that?]
In terms of the commit message, note that prepare_upload_bo() no longer exists; that's map_upload_bo() now.
While reviewing this patch, I noticed the wined3d_not_from_cs() call is below the map_upload_bo() block now; that seems undesirable.
@@ -2519,6 +2524,12 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, wined3d_device_context_submit(context, WINED3D_CS_QUEUE_MAP); wined3d_device_context_finish(context, WINED3D_CS_QUEUE_MAP);
- if (SUCCEEDED(hr))
- {
map_desc->data = map_ptr;map_desc->row_pitch = row_pitch;map_desc->slice_pitch = slice_pitch;- } return hr; }
We might as well return on failure, and save some indentation.