[PATCH 1/3] wined3d: Return the map pitch from wined3d_device_context_emit_map().

[Zebediah Figura]

On 10/8/21 11:23 AM, Henri Verbeet wrote:
> On Thu, 7 Oct 2021 at 04:04, Zebediah Figura <zfigura at 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 at 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.
>