On 22 August 2013 00:30, Stefan Dösinger stefan@codeweavers.com wrote:
+const char *wined3d_debug_location(DWORD location) +{
- char buf[238];
That number looks a bit arbitrary, although I suppose it's at least large enough.
- if (volume->resource.pool == WINED3D_POOL_SYSTEM_MEM
|| volume->resource.pool == WINED3D_POOL_SCRATCH)- {
if (location & ~WINED3D_LOCATION_SYSMEM){ERR("Trying to load a sysmem or scratch volume into %s.\n",wined3d_debug_location(location));return;}- }
In principle we have resource_access_from_location() and the access_flags field in struct wined3d_resource for this. It's only a warning in surface_load_location() because we load e.g. default pool surfaces into sysmem for CPU fallbacks, but that doesn't apply to volumes. The right way for surfaces would probably be to use a staging resource for CPU fallbacks instead, or something along those lines.
- if (count_bits(location) > 1)
FIXME("Implement loading two locations with one call\n");
I don't think we really want that.
+static void wined3d_volume_update_location(struct wined3d_volume *volume, DWORD location, BOOL keep) +{
- if (keep)
- {
TRACE("Volume %p, invalidating %s.\n", volume,wined3d_debug_location(volume->locations & ~location));volume->locations = location;- }
- else
- {
TRACE("Volume %p, invalidating %s.\n", volume, wined3d_debug_location(location));volume->locations &= ~location;- }
+}
I'm a bit unhappy with the "persistent" flag to surface_modify_location(), and this is essentially the same thing. I think it would be better to just have two separate functions, wined3d_volume_invalidate_location() and wined3d_volume_validate_location(). Perhaps wined3d_volume_update_location() can work too, but in that case I think it should take two location masks, one to set and one to clear.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 09:11, schrieb Henri Verbeet:
On 22 August 2013 00:30, Stefan Dösinger stefan@codeweavers.com wrote:
+const char *wined3d_debug_location(DWORD location) +{ + char buf[238];
That number looks a bit arbitrary, although I suppose it's at least large enough.
It's taken from one of the other debug functions, so yeah, it is arbitrary. The amount of location flags will grow with future patches.
- if (volume->resource.pool == WINED3D_POOL_SYSTEM_MEM +
|| volume->resource.pool == WINED3D_POOL_SCRATCH) + { + if (location & ~WINED3D_LOCATION_SYSMEM) + { + ERR("Trying to load a sysmem or scratch volume into %s.\n", + wined3d_debug_location(location)); + return; + } + }
In principle we have resource_access_from_location() and the access_flags field in struct wined3d_resource for this.
I didn't know about that, I'll change this check and the mappable check to use resource.access_flags. I can't reuse resource_access_from_location until surfaces are migrated to the new location flags though.
What I'm not quite happy with is WINED3D_RESOURCE_ACCESS_SCRATCH. Scratch resources are mappable. Having the extra location just complicates things and I see no gain from it.
Surfaces are much more difficult, fixing volumes is just the warm-up :-. The rough plan for surfaces is to enforce mapping restrictions in the external API, but ignore them internally, so we can do things like update_surface on a non-dynamic default pool surface surface with a converted or color keyed format. Maybe staging memory can work for surface-to-surface copies of converted surfaces.
- if (count_bits(location) > 1) + FIXME("Implement
loading two locations with one call\n"); +
I don't think we really want that.
You mean just drop those two lines?
I'm a bit unhappy with the "persistent" flag to surface_modify_location(), and this is essentially the same thing. I think it would be better to just have two separate functions, wined3d_volume_invalidate_location() and wined3d_volume_validate_location(). Perhaps wined3d_volume_update_location() can work too, but in that case I think it should take two location masks, one to set and one to clear.
Ok.
On 22 August 2013 09:58, Stefan Dösinger stefan@codeweavers.com wrote:
What I'm not quite happy with is WINED3D_RESOURCE_ACCESS_SCRATCH. Scratch resources are mappable. Having the extra location just complicates things and I see no gain from it.
Yeah, I'm not particularly happy about it either. The issue is that scratch resources allow some extra formats over sysmem resources, but can't be used for resource uploads. The idea is to eventually push the concept of pools out into d3d8 and d3d9, and just have access flags in wined3d, but we'd still need to be able to distinguish scratch resources from sysmem resources somehow. At the moment I'm thinking the better option would be to pass that through the creation flags instead, i.e. have a flag like WINED3D_SURFACE_SCRATCH, although ideally at the resource level.
- if (count_bits(location) > 1) + FIXME("Implement
loading two locations with one call\n"); +
I don't think we really want that.
You mean just drop those two lines?
Yeah. I think trying to load two locations at once should always be an error in the caller, so in principle it could also be an ERR instead, but just letting it get handled by the default case in the switch should be fine.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 10:25, schrieb Henri Verbeet:
Yeah, I'm not particularly happy about it either. The issue is that scratch resources allow some extra formats over sysmem resources, but can't be used for resource uploads. The idea is to eventually push the concept of pools out into d3d8 and d3d9, and just have access flags in wined3d, but we'd still need to be able to distinguish scratch resources from sysmem resources somehow. At the moment I'm thinking the better option would be to pass that through the creation flags instead, i.e. have a flag like WINED3D_SURFACE_SCRATCH, although ideally at the resource level.
I think it's best to check the pool in operations that accept a resource as parameter. We already do that in most places. Update_texture on volumes doesn't, but a later patch in my series fixes that.
And yeah, long-term we want pool and usage to go away from wined3d and stay in d3d8/9.
Wrt wined3d_volume_validate_location and wined3d_volume_invalidate_location: Should validate_location(LOCATION_X) just add LOCATION_X to the valid locations, or invalidate all others? (i.e., should it do a logical or or an assignment)? I currently implement it as logical or, so in some cases the caller is responsible for calling invalidate_location on the other locations.
On 22 August 2013 11:53, Stefan Dösinger stefan@codeweavers.com wrote:
I think it's best to check the pool in operations that accept a resource as parameter. We already do that in most places.
Yes, but if "pool" goes away we need some other way to check for scratch resources. It's just that in retrospect the access flags probably aren't the right place.
Wrt wined3d_volume_validate_location and wined3d_volume_invalidate_location: Should validate_location(LOCATION_X) just add LOCATION_X to the valid locations, or invalidate all others? (i.e., should it do a logical or or an assignment)? I currently implement it as logical or, so in some cases the caller is responsible for calling invalidate_location on the other locations.
Yes, logical or.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 12:58, schrieb Henri Verbeet:
Yes, but if "pool" goes away we need some other way to check for scratch resources. It's just that in retrospect the access flags probably aren't the right place.
If pool goes to d3d8/9, the checks should go to d3d8/9 as well.
There's an argument to be made that we shouldn't create wined3d resources for scratch resources at all, but that'd mean cloning a big amount of logic in those libs.
On 22 August 2013 13:07, Stefan Dösinger stefan@codeweavers.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 12:58, schrieb Henri Verbeet:
Yes, but if "pool" goes away we need some other way to check for scratch resources. It's just that in retrospect the access flags probably aren't the right place.
If pool goes to d3d8/9, the checks should go to d3d8/9 as well.
You can't do that properly. The basic issue is that some formats are valid for scratch resources, but aren't for sysmem resources. (There are a bunch of other restrictions we don't currently check on resource creation either, but that's not the point here.) So we need some way to determine if a resource is a scratch resource or not in wined3d, at the very least on resource creation.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 13:14, schrieb Henri Verbeet:
On 22 August 2013 13:07, Stefan Dösinger stefan@codeweavers.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2013-08-22 12:58, schrieb Henri Verbeet:
Yes, but if "pool" goes away we need some other way to check for scratch resources. It's just that in retrospect the access flags probably aren't the right place.
If pool goes to d3d8/9, the checks should go to d3d8/9 as well.
You can't do that properly. The basic issue is that some formats are valid for scratch resources, but aren't for sysmem resources.
Right, it needs at least some sort of way to bypass the resource limitation checks.
Is it ok to worry about this later and just remove WINED3D_RESOURCE_ACCESS_SCRATCH for now?
On 22 August 2013 13:32, Stefan Dösinger stefan@codeweavers.com wrote:
Is it ok to worry about this later and just remove WINED3D_RESOURCE_ACCESS_SCRATCH for now?
Sure.
-
Henri Verbeet -
Stefan Dösinger