Re: [PATCH 5/5] wined3d: Implement basic volume location management
On 22 August 2013 00:30, Stefan Dösinger <stefan(a)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(a)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.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSFcSTAAoJEN0/YqbEcdMwx/0P/361Sz0YfrOKDwjtex5mddmf L4IVlDb+a3U+Qwg1H7ecEMTaYAdxSy2WEsUn9MSZU6co24ZZcylSWG1ptaOJ63t+ QYVbsjtEGeLbEimUmywvjCfRxsvVbUBex48+Mc5yx20dNywbpJLuX2OfCUBOfpOH Oh1AjmBmsKpEP4f06AAlUnEvKq7NmNO5ieTrklYehPUQAU8Fn94h5TgteEpoLJeW N90nV0GoIrJi9S2O76MvVG7OkTZn2kqaUO74TyJUqz/IDRJrYsqtftcjHgibR3OX jwPZu8BClCS523VWQppIfsQj2O6Q0k95VXGRMfHqasdBz79SRG4Ix7lDaSoqFb6B r/tS8iA6ppTeWxWfhJYC81griA5RSWTWpUXuD76Bl6qBjpnX+kDFQNqV+5ls+1UB IercXePRE964TtZO+Liihv2ygWiKPC9K0HLH/P7bC3znoqgVT2fyRLOdHPAAnnY9 w2PWqtb4sjSnisMsL6+6s3Q/FdKU6/fmS85Zau5AreNs3wX7UZgOZSA2wstBKknW bjgAz+pFwkQWgeS2KkDC5Uv8SjAew+Ed8fU7LQl551Hwj+JY77cHBiPGaLrKOuO6 t3dN6Q6kLbz4qmW8ZWZeCQIk3V7c2fFsLdTQDc+CcVUzPe/kxXKf1A8+jt3N4LBy j7dOZKslfvnvUUDh1zYo =TKW/ -----END PGP SIGNATURE-----
On 22 August 2013 09:58, Stefan Dösinger <stefan(a)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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSFd+sAAoJEN0/YqbEcdMwkJQP/30+RdD61lcUYmTOm8S2vPqR nvcX6DTdcuoKV0I7W/9dg+lOx4tKgll37pSCyp7B0nsXKcHS3keXlvc9gTnNSgu5 LDJRsIc4DYbHU9BR5nbsG7gx6yjWg6LcS63DxIaIZLk0MbhKJaPKs2enXOVPJyVd e8UW2bf855MACPFIeBRI8o5vNH5L8R6jFsXb0P5qHIu1c/L4Du0VAYa3EaFtGWYj q2pAsAfKnjxedERIYIQ33FGHPucXhJ8254+silvgKwkfGLZTF40sKxFLnjxz/XTW eM7kL7LJOfcqcgmpb+wgjfQMNXeNiu3YERYoIwE5+jFB+uPZy0u+WCxpDa8xI8HF K4hpMIW0F01NbG6p/rSJWS75YMjlCJT4w/La0UTbSQ14ML8s+1OtrZBEvksTikFl KmiXhzTNNwDLUL2E9EEboQHsxJZ9AXjOxEm3xlfB2afay5+gPg6Kox2tiG/wyN2M ReGmvpOC6Ddrdf7Bvror1AOVni1h4GTsF3yh/507ea1gU4oaKBz33nz4DPVvvVzD DvHlMQUGenxANYzskqAJT9KygpNuK2bgsBawGxqYKo7DyEF8wO1suu3pzb0vPwiT SRr/VWtD7e2HksrhTjlE5ToLr9nFFO5Xk/dhOzOCetAayBJ5jbil//gtD1fparw6 5ZYIdkWufcxTALuA6ZE8 =Wym2 -----END PGP SIGNATURE-----
On 22 August 2013 11:53, Stefan Dösinger <stefan(a)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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSFfDvAAoJEN0/YqbEcdMwAJgP/j2Mm/gqxQWLpLJg4xSWDBkB jqVndRtd1R8u2ScRYWuzFjuCgBegjCr27WXU4BJdxOJGOvREWVW14lL6fLSY+KCH Z+4XFIQJk4h0lQziTIJ37hvS82IY9gYChZWS3YuePtVbKlbpZBl9RbTrZxgLvbSn Gwr7tud9XjEaKEFchIKXczcxLXR3GcRnfqb6/aYqM58PnMByH42dPESXW9KnjCS1 62dq4HOzvDNwyKxVsn9fHjf51ltTuBph8gATHbFvjRPp2YR/kcsNIHDy9Yc7S+ww gpCjK4ptRtvtNYv2Kv7xzR9OX/7F6V9a4J9LUdd1r9bUQJIbJMga7zPUrQU9m+o2 cl7f6hH+u+VpDL7R2eAXsX+ebCd4rJU3EWM6RN50cc+2nRwK1ghLzu6tFOV16Po1 zzUH57n3/GYEH0AolTgDi4F3K4EIq1Nt9cdQTU51dokzSQqOR56Gfro4wEMxwb+C XsOisLQje2RvjbX+PYqHYRJPzxAZAPLxEbl1iNg9d8MpmOO8wwC1IQhrJ6YYseDk yaXkTgzttq0XUmBMNbLbFmC/SN7N9UaaZFqWARfPYWsbQBpsoxlS49Vg0EAJsrJ/ cznSxDmKPkx+L8P5g+U60oTuchtJTRAUXHG13jYEghMCqCafDUDyNKBtvJEMNlY9 lz+DuMbYfVFJHDsh93Oo =trd4 -----END PGP SIGNATURE-----
On 22 August 2013 13:07, Stefan Dösinger <stefan(a)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(a)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? -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSFfbOAAoJEN0/YqbEcdMwqvwP/RlJtU0IhJ4gVaDRsIY9Jv5P 6+r+kL2phq6LuBcO/mhid/RqVl1yiB2gc5Mh0PHiAst7Rsor5Ozhzve0KI/Acgwb yYwZzNecSGtIIID+YhVErfoDZGhknUw4luXSIcTH6NTHv9WGg75GErClF4SejfPy N2I0RUElfXDLxQXHcWuftjZcwX7RBkPrdNUbqNAoRQBnXxqkWhbCNTdH4O6hv1lz 4g4nF831JPpznkWURZpdqEmPvDqMdP8j63wVp39TLIqtXMQR0B/SPy+M9YPaLfEL BeeDOSDkY5mvp4OtGIQ1vd//M9TNS4sz9RHUjrTqzc3k4DrlxdPRY6l5ImQnWKEc noNeELLdYUcGUPgwkNLndOuX2jACdWF3R3Y82RXSIEOpuqgmZ+ifAsRqsjmLoj/p l+SyMnDcgMMDhnqxFKrkeZuyDFeJu8h8esEoEuSTjKuWwjv3JuxFE8DSlraJwDVw 6nwToBHMnD0kRZuCrdDocHOHMCCExT8OZJVmUh+JXHufr7IF2g9gf0SkWzr3mpW/ v6PxAZHcD7NbBPg5rCCl3F6goNGtosw8cuvEJzp176ifYONCibpsG3qQuG6n0/wQ vRYcexRWmK8TgU7fxCa4xzCykGkBPeNR5ukIM4iuwjHhx1nLsuk+45v15vyMEbRj DqkYAnjFTWlYNYqSvCv8 =N3Hp -----END PGP SIGNATURE-----
participants (2)
-
Henri Verbeet -
Stefan Dösinger